Skip to content
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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

RedYetiDev
Copy link
Member

@RedYetiDev RedYetiDev commented Jun 29, 2024

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 and sequential directories.

Other testing directories haven't been changed, as enabling recursion with them ending in coverage errors.

Fixes #52901 (Failed attempt)
Fixes #53410 (Failed attempt)
Fixes #53309 (Failed attempt)


I apologize for the three failed PRs, coverage issues are very finicky, especially when the test files and coverage files are all being modified by different commits at the same time.

After narrowing down what works, this is the final and correct implementation

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. test Issues and PRs related to the tests. tools Issues and PRs related to the tools directory. labels Jun 29, 2024
@RedYetiDev
Copy link
Member Author

CC @nodejs/testing

@RedYetiDev
Copy link
Member Author

Also, this should be backported to v20 and v18.

@RedYetiDev RedYetiDev added lts-watch-v18.x PRs that may need to be released in v18.x. lts-watch-v20.x PRs that may need to be released in v20.x labels Jun 29, 2024
@targos
Copy link
Member

targos commented Jun 30, 2024

@nodejs/python

@cclauss
Copy link
Contributor

cclauss commented Jun 30, 2024

Do the proposed changes:

  • Accelerate test runs?
  • Improve test coverage?
  • Deliver clearer error messages?
  • Make it easier to add new tests?

Would yielding tests or at least creating a tuple of tests be more efficient than creating a list?

@RedYetiDev
Copy link
Member Author

RedYetiDev commented Jun 30, 2024

Make it easier to add new tests?

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.

@joyeecheung
Copy link
Member

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 tools/test.py "test/*/*inspector*" for example

@RedYetiDev
Copy link
Member Author

Do we have consensus on this being feasible?

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 test/.../<subsystem>/test-<api> allows them to be easily accessible. Subfolders also allow for easier access to specific tests. Instead of scrolling through a long list, contributors can quickly navigate to the relevant subfolder, reducing time spent searching for files. Even more so, subfolders help manage the growth of test files by compartmentalizing them. As the project expands, new tests can be added to the appropriate subfolder without cluttering the main test directory (which is currently cluttered IMO).

@joyeecheung
Copy link
Member

joyeecheung commented Jul 2, 2024

Instead of scrolling through a long list, contributors can quickly navigate to the relevant subfolder, reducing time spent searching for files.

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.

@joyeecheung
Copy link
Member

As the project expands, new tests can be added to the appropriate subfolder without cluttering the main test directory

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.

@RedYetiDev
Copy link
Member Author

RedYetiDev commented Jul 2, 2024

the organization could be very random and subjective.

FWIW The folder conventions would be based on the current test names, so test-http-file would be http/test-file. I believe that test/parallel shouldn't have 3500 files alone, and it should be broken up.

@Trott
Copy link
Member

Trott commented Jul 2, 2024

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.

We had a similar massive move of everything from test/simple to test/parallel and test/sequential way back in 0e19476 almost 10 years ago. That had a big performance win behind it, but caused headaches for years, making git blame (and git log and probably others I'm forgetting) basically useless on every test file that commit moved.

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 git blame -C would still work, but I'm not sure and I don't think the benefit here is worth even the relatively small headache of making everyone use -C for several years.)

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.

@RedYetiDev
Copy link
Member Author

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 whatwg-url, but I'd need to verify that)

@cclauss
Copy link
Contributor

cclauss commented Jul 3, 2024

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 pytest discovery without the custom code of testpy.

Tools like pytest-xdist and friends would automatically run the discovered Python tests in parallel across all CPUs and the removal of Python's Global Interpreter Lock (GIL) should simplify and accelerate that trend.


Edit:

Q: What percentage of the current Python tests would automatically be found by pytest discovery...

A: pytest only discovered 248 tests and those were in:

  • deps/v8/
  • test/tools/
  • tools/gyp/pylib/gyp/
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lts-watch-v18.x PRs that may need to be released in v18.x. lts-watch-v20.x PRs that may need to be released in v20.x needs-ci PRs that need a full CI run. test Issues and PRs related to the tests. tools Issues and PRs related to the tools directory.
6 participants