[functional test] Re-organize functionnal tests directories

[Pierre-Sassoulas]

Type of Changes

	Type
✓	🔨 Refactoring

Description

By using the message name as directory name when applicable.

Inspired by #5396

[coveralls]

Pull Request Test Coverage Report for Build 1541483344

• 0 of 0 changed or added relevant lines in 0 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage remained the same at 93.696%

---

Totals
Change from base Build 1541216646:	0.0%
Covered Lines:	14164
Relevant Lines:	15117

---

💛 - Coveralls

[Pierre-Sassoulas]

@cdce8p @DanielNoord this is only a first step as there is still a lot of tidying up to do. I'm using the following code. I think we can merge #5474 before #5475 because moving directories will be easier than moving files anyway and applying the norm all at once is time consuming.

(It's easier to review this by looking at the functional directory locally, suggestion welcome.)

[cdce8p]

Tbh I'm not really a big fan of constantly moving tests around without much benefits. It's already nearly impossible to use Github blame to see the correct PR and this certainly doesn't help. Furthermore, what really is the point of putting just two files into a directory?

If it were up to me, I would suggest some last changes and from then on leave them alone.

• Have a basic folder structure by the first letter.
• Every folder after that should only be on a need / makes sense basis. I isn't really an issue to have 100-200 files in a folder. Most IDEs support quick file lookup anyway.
• Keep extension tests separate, like they are now (with the ext folder).

--
That's also a reason why I suggested to use a separate structure for tests that should be used as examples. #5396 (comment)

--
I do know that you spend quite some time do this, but those are my honest thoughts.

[Pierre-Sassoulas]

It's already nearly impossible to use Github blame to see the correct PR

git still permit to blame and use log properly ? For example git log --pretty=short -u -L 1,10:tests/functional/a/anomalous-backslash-in-string/anomalous_backslash_escape.py or git blame tests/functional/a/anomalous-backslash-in-string/anomalous_backslash_escape.py permit to see when the file was added or modified even if it was moved.

what really is the point of putting just two files into a directory?

We're starting to have a lot of functional tests. Especially since Daniel took on the work of moving the unittest to functional. It's going to become a nightmare to maintain if we can just put functional tests wherever we want with whatever name we want. I don't say we should not be able to, just that there should be an expected location to put a functional tests without having to do a full search on the whole pylint repositories to discover the name of the functional test files. Enforcing that a functional test file exists directly in pytest will prevent us from having to ask for it, the implementer to think about where to put it and comment asking for a better name/location in reviews. It's also possible to have more than one file in the directory corresponding to a message (deprecated-method does for example).

I isn't really an issue to have 100-200 files in a folder. Most IDEs support quick file lookup anyway.

It's an issue to not be able to see all the file in the explorer, not all contributor are super efficient with an IDE. In fact the same argument could be made about very big files. The IDE permits to jump between class definitions and find usage very easily, why bother creating a file per class and packages ? black still burst their big __init__.py recently :)

Keep extension tests separate, like they are now (with the ext folder).

I agree and I like where the extension are at right now. Wouldn't it be nice if we had a folder for each checkers and then a folder for each message of this checker too ? We could keep a "free for all" section with letter for mixed functional tests alongside it ?

[cdce8p]

git still permit to blame and use log properly ?

Technically yes, but that's quite complicated. (Compared to Github)

I don't say we should not be able to, just that there should be an expected location to put a functional tests without having to do a full search on the whole pylint repositories to discover the name of the functional test files.

I basically never navigate the file tree myself. As long as there are tests, the fastest way to find them is to use search, e.g. [invalid-name], and look at the results. That's what I do for every unknown error. It also has the benefit that with a search for invalid-name I can quickly find the right checker file.

Wouldn't it be nice if we had a folder for each checkers and then a folder for each message of this checker too ?

That still requires me to know the checker a message belongs to. From my experience, nearly impossible.

Enforcing that a functional test file exists directly in pytest will prevent us from having to ask for it, the implementer to think about where to put it and comment asking for a better name/location in reviews. It's also possible to have more than one file in the directory corresponding to a message (deprecated-method does for example).

That will always happen. I would suggest to update the contributing documentation. (Although I know it isn't always read.) Not strictly requiring a test file to open a PR also lowers the barrier for new contributors. It's much better if we (as maintainers) can help with the tests than not having a patch at all. => I do not believe we should enforce requiring tests!

It's an issue to not be able to see all the file in the explorer, not all contributor are super efficient with an IDE. In fact the same argument could be made about very big files. The IDE permits to jump between class definitions and find usage very easily, why bother creating a file per class and packages ?

A good middle ground is necessary. Folder structures that are nested multiple levels deep aren't helpful either. Especially if those only contains one or two files.

We could keep a "free for all" section with letter for mixed functional tests alongside it ?

I find it much more helpful if the test file itself makes sense. I.e. related issues contained in one file. Enforcing too strict requirements might lead to worse tests overall just to satisfy the pytest check.

I've seen this recently were devs wrote worse code just so pylint wouldn't complain. I plan to open an issue for it soon.

[Pierre-Sassoulas]

That still requires me to know the checker a message belongs to. From my experience, nearly impossible.

Wouldn't you know which checker the message you're working on come from ? You're modifying it or even creating it.

Not strictly requiring a test file to open a PR also lowers the barrier for new contributors.

I think inexperienced contributor will provide a patch, will not launch test locally and then realize that the CI is their MR is red (where the test message could tell them what to do, our ourselves if they don't read why the test fails). Happened to me in black, their CI checks that the changelog is properly formatted for example.

A good middle ground is necessary. Folder structures that are nested multiple levels deep aren't helpful either. Especially if those only contains one or two files.

Do you have something in mind ? My concern is that dividing by starting letter is not enough anymore and the sub-folder that start to appear are not well organized as there is no clear rules.

I've seen this recently were devs wrote worse code just so pylint wouldn't complain. I plan to open an issue for it soon.

I agree with that opened #5496 to start the discussion.

[DanielNoord]

Wouldn't you know which checker the message you're working on come from ? You're modifying it or even creating it.

I often look for test files to see when messages are supposed to trigger when I come across them in other tests. I agree that the source-checker is not often known.

That said, I often use the search function of my IDE to look for these files instead of going through the folder structure.

Not strictly requiring a test file to open a PR also lowers the barrier for new contributors.

I think inexperienced contributor will provide a patch, will not launch test locally and then realize that the CI is their MR is red (where the test message could tell them what to do, our ourselves if they don't read why the test fails). Happened to me in black, their CI checks that the changelog is properly formatted for example.

I'm not sure how this would be implemented, but I can see this becoming problematic with refactoring and smaller bug fixes. For example, with my recent change to the docstring style checker I didn't necessarily have to add a new test although the change was useful. I fear this might become too strict quickly and working with contributors to make them add tests is the better option here (although more time-consuming).

[Pierre-Sassoulas]

I fear this might become too strict quickly and working with contributors to make them add tests is the better option here (although more time-consuming).

OK let's not enforce it then.

What would be your preferred organization?

Currently we have:

a/
  a/abstract/
  a/anomalous/
  a/...
...
ext/$checker_name/$checker_message
...
z/

We could add a directory for each checker on top of that (so we extend the extension organization to all checkers):

a/
  a/abstract/
  a/anomalous/
  a/...
...
$checker_name/$checker_message
ext/$checker_name/$checker_message
...
z/

Or a directory for each message in what we currently have (the current MR does that)

a/
  a/abstract/
  a/anomalous/
  a/$checker_message
  a/...
...
ext/$checker_name/$checker_message
...
z/

[cdce8p]

What would be your preferred organization?

I would still suggest to leave the existing tests as is. For new tests, I would say it's enough if it loosely makes sense. When in doubt, I would create a new test file named after the check itself. For example the test for trailing-comma-tuple should be in t/trailing_comma_tuple.py and so on (that's also where they are currently). Another one, it should still be possible to put regression tests into r/regression/.

I would also recommend to bump the max number of test files allowed in a folder.

[DanielNoord]

As this is on the 2.13 milestone let's try to reach a consensus here.

Personally I'm not a big fan of multiple sub-directories in tests/functional. One of the issues is that tests might be put in the wrong place if there are too many place where they might belong. I think we have all missed correctly checking the file name of a change in a PR once, GitHub's interface makes it easy to forget to check that. This has resulted in tests/functional/u/use/using_constant_test.txt for example. To me it makes little sense that using is in the use subdirectory.
Furthermore, when I open the u directory looking for use-a-generator tests I'll scroll down to the us... files only to then find out that there is a specific subdirectory for use... files. To me that example illustrates that if we don't enforce sub-directories for (for example) every first word then it's actually less user-friendly: for which test names should I first look at the sub-directories and for which should I scroll straight down?
This can be solved by enforcing more sub-directories, but as I said I think that creates the risk of misplacing fails and missing that when reviewing PRs.

So, I think the current status quo is somewhat "optimal" as it finds a balance between both problems.

The question would then be how to formulate the current status quo in the contributing documentation? I'd suggest something like this:

---

Functional tests files should be put in the test/functional directory. For existing checkers, new test cases should preferably be appended to the existing test file. For new checkers, a new file new_checker_message.py should be created (Note the use of underscores). This file should then be placed in the test/functional/n sub-directory.

Some additional notes:

• If the checker is part of an extension the test should go in test/functional/ext/extension_name
• If the test is a regression test it should go in test/r/regression. The file name should start with regression_.
• For some sub-directories, such as test/functional/u, there are additional sub-directories (test/functional/u/use). Please check if your test file should be placed in any of these directories. It should be placed there if the sub-directory name matches the first letters of your test file name.

---

I know you put a lot of time into this @Pierre-Sassoulas but perhaps streamlining the current status quo and guiding users into using the current structure could be a good compromise for all here.

[Pierre-Sassoulas]

OK, what about an enforced rule that if a directory exists inside a first level one letter name, all file name must start by the directory name and all file that start with this word must be in this directory ?

[DanielNoord]

OK, what about an enforced rule that if a directory exists inside a first level one letter name, all file name must start by the directory name and all file that start with this word must be in this directory ?

I'm not sure what you mean with "all file name must start by the directory name"?

[Pierre-Sassoulas]

Say you have a file called use_grammar.py in tests/functional/u/using then the file need to be renamed to using_grammar.py or be moved to tests/functional/u/use or tests/functional/u/

[DanielNoord]

Say you have a file called use_grammar.py in tests/functional/u/using then the file need to be renamed to using_grammar.py or be moved to tests/functional/u/use or tests/functional/u/

Agreed! I think this is a change/fix that we can indeed do, as those files are indeed incorrectly placed.

I think the biggest problem with what I proposed is deciding when to add that use or using subdirectory, but I think doing so for every first word increases complexity unnecessarily while never doing it makes the directories too large. I'd settle for the very unsatisfactory "sometimes".. 😓

[Pierre-Sassoulas]

deciding when to add that use or using subdirectory,

When there are 5 files or more starting by ↨useor using ?

[DanielNoord]

Makes sense. That's easily translatable to a directive for the contributing guidelines.

[cdce8p]

I like your suggestion @DanielNoord!

The script to enforce the current structure would be a nice to have, but certainly not a release blocker for `2.13´. Everything else is probably best done in a new PR / issue. Should we close the PR then?

[DanielNoord]

I have written a little test to enforce the structure in #5753. We should also add the text to the documentation, which I will do after #5753 lands.

[DanielNoord]

Closing as the original goal of this PR was achieved with #5753 and #5792 introduces the documentation changes that are discussed here.