Add tests naming section to `CONTRIBUTING.md` #2816

rumyantseva · 2023-06-07T21:05:32Z

Description

Closes FerretDB/engineering#66.

Inspired by Add tests naming section to CONTRIBUTING.md #2708 and the brainstorming we had after that.
Feel free to share your thoughts about the items that look questionable.
Please note that our current naming doesn't always follow the provided guidelines, so after choosing the final guidelines we will have separate PRs or issues to refactor the tests to meet the guidelines.
This is the first version of the guidelines, we can adjust it later based on our needs.
I set the label "do not merge", so everyone can take a look.

Readiness checklist

I added/updated unit tests.
I added/updated integration/compatibility tests.
I added/updated comments and checked rendering.
I made spot refactorings.
I updated user documentation.
I ran task all, and it passed.
I ensured that PR title is good enough for the changelog.
(for maintainers only) I set Reviewers (@FerretDB/core), Labels, Project and project's Sprint fields.
I marked all done items in this checklist.

codecov · 2023-06-07T21:16:39Z

Codecov Report

Merging #2816 (4699ab7) into main (1b7ff7e) will not change coverage.
The diff coverage is n/a.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #2816   +/-   ##
=======================================
  Coverage   63.81%   63.81%           
=======================================
  Files         442      442           
  Lines       22519    22519           
=======================================
  Hits        14370    14370           
  Misses       7210     7210           
  Partials      939      939

Flag	Coverage Δ
integration	`56.85% <ø> (ø)`
mongodb	`5.00% <ø> (ø)`
pg	`56.76% <ø> (ø)`
unit	`25.19% <ø> (-0.05%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

chilagrow

Great one! Let me know if anyone has suggestion or input!

chilagrow · 2023-06-08T01:11:24Z

CONTRIBUTING.md

+6. Keep test names concise, avoiding overly cryptic names. Use abbreviations when appropriate.
+7. Avoid including test data in the name to maintain clarity and prevent excessively long names.
+8. Test case names should follow `TitleCase` capitalization style.
+9. Test and subtest names should not exceed 64 characters.


In addition, 64 characters rule also adds pg, sqlite etc suffix, so it would be a bit shorter. 🙈

I think this is about the test name segments

chilagrow · 2023-06-08T01:14:46Z

CONTRIBUTING.md

+1. Test files should be named after the command they test.
+   For example, `find_test.go` for testing the find command.
+   Test files belonging to the same commands group are organized into folders named after this group,
+   such as `aggregation`, `operations`, `diagnostic`, etc.


When we add a new folder, do we also need to modify Taskfile so that newly tests in folder are run? 🤔

I think this is a good organisation, but might add complications to adding test, and we want adding tests to be easy.

Good point! Overall, there won't be a lot of folders and new folders won't be added too often, but I agree the process of adding new tests/folders should remain simple.

This may also have impact on recent work about running integration tests in shard #2692

Anyway, I think we need to make sure everyone is onboard, if so then we need to make sure we have tooling in place to do it simply. 🤗

That should not be a problem with ./... as long as test names are unique. Envtool should be able to check that

chilagrow · 2023-06-08T01:21:26Z

CONTRIBUTING.md

+
+1. Test files should be named after the command they test.
+   For example, `find_test.go` for testing the find command.
+   Test files belonging to the same commands group are organized into folders named after this group,


This is a question to everyone, do we want a test file for each operator such as find_bitwiseanyset_compat_test.go or are we happy grouping them in a same file with similar operators like now? Not sure how we've grouped them now but it makes sense.

To be honest, I don't like the current approach of having everything in the same file because it's hard to navigate through the files that have 1000+ lines of code. If the files for different operators are separate, it's easier to go through them, scroll through the tests for a particular operator, etc.

Yeah agree. If we all agree let's document that too 🤗

Yeah, find, update, aggregate will be huge without additional level of splitting

chilagrow · 2023-06-08T06:33:14Z

CONTRIBUTING.md

+   such as `aggregation`, `operations`, `diagnostic`, etc.
+2. If the file contains compatibility tests, add the `_compat` suffix. For example, `find_compat_test.go`.
+3. Test names should include the name of the command being tested.
+   For instance, `TestDistinct` for testing the distinct command.


Finally, for RunCommand do we have a preference? 🤔

Currently we have TestFindAndModifyCommand, TestCommandsFindAndModify, TestFindAndModifyRunCommand, or just TestFindAndModify because findAndModify can only be run as RunCommand.

Yeah, let's document this one too!

But I don't know which one to use. Up to you to suggest, I'm always in for shortest name 🙈

AlekSi · 2023-06-08T09:09:39Z

CONTRIBUTING.md

+
+1. Test files should be named after the command they test.
+   For example, `find_test.go` for testing the find command.
+   Test files belonging to the same commands group are organized into folders named after this group,


Yeah, find, update, aggregate will be huge without additional level of splitting

AlekSi · 2023-06-08T09:11:32Z

CONTRIBUTING.md

+1. Test files should be named after the command they test.
+   For example, `find_test.go` for testing the find command.
+   Test files belonging to the same commands group are organized into folders named after this group,
+   such as `aggregation`, `operations`, `diagnostic`, etc.


That should not be a problem with ./... as long as test names are unique. Envtool should be able to check that

AlekSi · 2023-06-08T09:13:33Z

CONTRIBUTING.md

+   If the test is checking for a specific error scenario, include the error scenario in the name.
+6. Keep test names concise, avoiding overly cryptic names. Use abbreviations when appropriate.
+7. Avoid including test data in the name to maintain clarity and prevent excessively long names.
+8. Test case names should follow `TitleCase` capitalization style.


Some points, including that one, should clarify the difference between top-level test functions and subtests. For example, TestFind/Doubles is a valid (full) test name, but not a valid test function name

I think we should specifically mention that no spaces, dashes or underscores should be used anywhere

AlekSi · 2023-06-08T09:14:38Z

CONTRIBUTING.md

+6. Keep test names concise, avoiding overly cryptic names. Use abbreviations when appropriate.
+7. Avoid including test data in the name to maintain clarity and prevent excessively long names.
+8. Test case names should follow `TitleCase` capitalization style.
+9. Test and subtest names should not exceed 64 characters.


I think this is about the test name segments

mergify · 2023-06-09T13:10:16Z

@rumyantseva this pull request has merge conflicts.

w84thesun

Should we close this one as we already added some naming guidelines?

wip

8d207fd

mergify bot assigned rumyantseva Jun 7, 2023

wip

880e630

rumyantseva requested review from a team, w84thesun, chilagrow and noisersup June 7, 2023 21:06

wip

4699ab7

rumyantseva requested a review from b1ron June 7, 2023 21:09

rumyantseva marked this pull request as ready for review June 7, 2023 21:09

rumyantseva requested a review from AlekSi as a code owner June 7, 2023 21:09

rumyantseva enabled auto-merge (squash) June 7, 2023 21:10

chilagrow reviewed Jun 8, 2023

View reviewed changes

rumyantseva added documentation Something user-visible is badly or not documented do not merge PRs that should not be merged labels Jun 8, 2023

chilagrow reviewed Jun 8, 2023

View reviewed changes

rumyantseva disabled auto-merge June 8, 2023 09:03

rumyantseva marked this pull request as draft June 8, 2023 09:05

AlekSi reviewed Jun 8, 2023

View reviewed changes

rumyantseva mentioned this pull request Jun 8, 2023

Describe current test naming conventions in the contributing guidelines #2821

Merged

9 tasks

mergify bot added the conflict PRs that have merge conflicts label Jun 9, 2023

w84thesun reviewed Jun 16, 2023

View reviewed changes

rumyantseva closed this Aug 17, 2023

mergify bot removed the conflict PRs that have merge conflicts label Aug 17, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add tests naming section to `CONTRIBUTING.md` #2816

Add tests naming section to `CONTRIBUTING.md` #2816

rumyantseva commented Jun 7, 2023 •

edited

Loading

codecov bot commented Jun 7, 2023 •

edited

Loading

chilagrow left a comment

chilagrow Jun 8, 2023

AlekSi Jun 8, 2023

chilagrow Jun 8, 2023

rumyantseva Jun 8, 2023

chilagrow Jun 8, 2023

AlekSi Jun 8, 2023

chilagrow Jun 8, 2023

rumyantseva Jun 8, 2023

chilagrow Jun 8, 2023

AlekSi Jun 8, 2023

chilagrow Jun 8, 2023

rumyantseva Jun 8, 2023

chilagrow Jun 8, 2023

AlekSi Jun 8, 2023

AlekSi Jun 8, 2023

AlekSi Jun 8, 2023

AlekSi Jun 8, 2023

AlekSi Jun 8, 2023

mergify bot commented Jun 9, 2023

w84thesun left a comment

Add tests naming section to CONTRIBUTING.md #2816

Add tests naming section to CONTRIBUTING.md #2816

Conversation

rumyantseva commented Jun 7, 2023 • edited Loading

Description

Readiness checklist

codecov bot commented Jun 7, 2023 • edited Loading

Codecov Report

chilagrow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mergify bot commented Jun 9, 2023

w84thesun left a comment

Choose a reason for hiding this comment

Add tests naming section to `CONTRIBUTING.md` #2816

Add tests naming section to `CONTRIBUTING.md` #2816

rumyantseva commented Jun 7, 2023 •

edited

Loading

codecov bot commented Jun 7, 2023 •

edited

Loading