-
Notifications
You must be signed in to change notification settings - Fork 29.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
test: allow recursion in testing #53640
Conversation
0b74b2c
to
2c9cf7e
Compare
CC @nodejs/testing |
Also, this should be backported to v20 and v18. |
@nodejs/python |
Do the proposed changes:
Would yielding tests or at least creating a tuple of tests be more efficient than creating a list? |
Yes. The changes allow the tests to be stored in sub folders, which will keep the tests uncluttered. Once this lands in all release lines, I plan to slowly organize the parallel tests into sub folders. |
Do we have consensus on this being feasible? At least I am not seeing positive signals from #52502 Currently the component name is part of the naming convention of tests, so running tests for a specific component is already possible with |
It's definitely possible, and I think that, while the initial effort to set it up may be cumbersome, the long-term improvements will be worth it: Tests that are grouped by subsystem or functionality within subfolders, make the directory structure more intuitive. For example, organizing tests as |
There are many commands one can use to get the relevant tests (ls + grep, find etc.). We have many folders apart from parallel and sequential but I don't think people actually look into them directly so often. On the other hand subfolders would disrupt git logs and make backports / bug investigation harder. I don't think the pros here outweigh the cons. |
Actually I think as the folders grow it would be even harder to find a specific test, or do any tool-aided refactoring, because A's might expect a test to be in subfolder1/subfolder2 whereas for B it could be in subfolder3/subfolder4, and the organization could be very random and subjective. |
FWIW The folder conventions would be based on the current test names, so |
We had a similar massive move of everything from If this had a similar massive benefit, then sure. But the win here seems to be mostly aesthetic. I'm unconvinced that moving thousands of files without an overwhelmingly persuasive reason is the thing to do. (Maybe If we were starting writing tests again from scratch in a greenfield project, I too would prefer organizing tests like this or in some other similar way rather than the way we have it now. But moving thousands of files at this point in time requires a very compelling reason that doesn't seem to be here. |
I agree that moving a large number of tests isn't easy, but I do think that in the end it will have been worth it. While the headache of moving a large number of files is hard to look past, I do think that if it's done correctrly, it can be done in a way that doesn't cause too much concern for contributors. FWIW I plan to look through the PRs and find the least changed testing subsection, and start with that. (IIRC it's |
I share the sentiments of the notes of caution above. My questions above (#53640 (comment)) were to try to understand why. If the only answer is to Make it easier to add new tests then I would be cautious. It would be interesting to see what percentage of the current Python tests would automatically be found by Tools like Edit:
A:
|
Sorry for the delay here, but my answer to why? is making it easier to sort tests into categories. I think keeping tests in the |
This just doesn't seem like it's been a problem. |
I feel that as tests continue to expand, the directory will get larger and larger, and it's best to split it up, but if thats not the direction to go, feel free to close this PR. |
Per this discussion in #54796, this feature may help to allow subdirectories for public vs private APIs, ignoring what I originally wanted this feature to boast. |
I have many PRs open, and it doesn't look like this is on track to be merged soon, someone else can take on the work |
This pull request introduces recursion to the test runner. Previous attempts were unsuccessful due to issues in certain directories that did not handle recursion correctly. This PR specifically enables recursion for the
parallel
andsequential
directories.Other testing directories haven't been changed, as enabling recursion with them ending in coverage errors.