test: refactor tests for easier isolated execution

[bcoe]

Problem Statement

I've been eyeballing test coverage reports, as a way of finding subsystems where I can pitch in and help write more tests. In this process I believe I've identified a few aspects of the testing feedback loop that can be improved:

1. running tests with coverage takes forever (there isn't an easy way to drill down to
   run coverage for just one submodule, e.g., http2.).
2. it's confusing which folder new tests should be put in; some newer features place
   tests in a submodule-specific folder, e.g, @bmeck's tests for es-modules can be
   found in ./test/es-modules, other tests are placed in the catch-all ./test/parallel folder.
3. there are currently thousands of tests in the ./test/parallel folder, which makes it hard
   to easily work with in some text editors.
4. several variables need to be updated when new test folders are added, e.g., CI_JS_SUITES.

Proposed Solution

This pull requests introduces several mostly backwards-compatible changes to the test runner, in an effort to streamline the testing feedback loop:

1. CI_NATIVE_SUITES and CI_JS_SUITES have been made configurable, so that make coverage can be run against a subset of suites, e.g., CI_JS_SUITES=child-process* CI_NATIVE_SUITES= make coverage.

2. tools/test.py has been refactored, such that it now performs a recursive walk of folders and supports nested suites, making it easier to group tests by subsystem:
   

3. ./test/parallel can be split into subsystem specific suites, using the refactored tools/test.py.

4. the special variable DEFAULT_JS_SUITES has been introduced. When this is observed test.py will use a programmatically generated list of reasonable-default test suites.

   it's worth noting, that in this process I removed known_issues from the default list of tests. I'm concerned that this would drive up coverage metrics for tests that we know are failing; make test-known-issues can be run periodically in its stead.

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• documentation is changed or added
• commit message follows commit guidelines

Affected core subsystem(s)

test, build

CC: @addaleax, @MylesBorins, @jasnell

[bcoe]

It's admittedly ugly that we would need to touch all the test files to enforce this new structure. I am however already seeing benefits, it's nice to be able to run coverage for just child-process. It's worth noting it's still possible to eyeball the history using:

git log --follow test/child-process/parallel/test-child-process-bad-stdio.js

[bcoe]

this is a magic variable which test.py understands, my thinking is that this would prevent us from accidentally forgetting to update this list in the future.

[Trott]

If this removes known_issues, then we should explicitly add known_issues to test-ci. Alternatively, there can be a JS_SUITES variable and CI_JS_SUITES variable that is JS_SUITES + known_issues.

EDIT: Uh, that is, unless test-ci is used to generate coverage tests. I don't know if coverage reports are based on make test or make test-ci or something else...

[bcoe]

@Trott I think this would be smart, main concern is I think we should remove known_issues from the coverage run, so split the configuration a bit between test-ci and test-coverage.

EDIT: I think coverage actually defaults to make test, so I'd advocate introducing test-cov that basically just drops known_issues from the suites run so that they don't reflect in coverage (CC: @addaleax).

[bcoe]

it seemed reasonable to me that test-known-issues should not be part of the default test run, my main concern being that this could lead to inaccurate coverage results.

[bnoordhuis]

it's nice to be able to run coverage for just child-process

That already works: tools/test.py parallel/test-child-process-*

I'm not a fan of the huge amount of churn. I still get annoyed on a weekly basis by the test/simple split into test/parallel and test/sequential and that at least introduced the benefit of running tests in parallel.

[addaleax]

It's admittedly ugly that we would need to touch all the test files to enforce this new structure.

If this happens, you should be prepare to make backports for all active release lines ;)

Also, for a bit of history and previous discussion, see #6217

it's nice to be able to run coverage for just child-process

That already works: tools/test.py parallel/test-child-process-*

We might just need to advertise that more, yes …

[jasnell]

@bcoe ... Thank you for working on this. I'll have to dig into the details a bit later but I'm a big fan of this and have wanted to see this kind of refactoring for quite some time.

And yes, if this goes through we'll definitely need to backport.

[Trott]

This section has great info. I'm not sure if CONTRIBUTING.md is the place for it. Can we move it to it's own document in doc/guides and link to it from CONTRIBUTING.md? CONTRIBUTING.md is the file people get a link asking them to read when they are in the process of opening a pull request.

[bcoe]

Yeah, wasn't quite sure where to put it ... I like the idea of a adding it to writing-tests.md or creating its own guide depending on what others are feeling.

[Trott]

+1 to putting it in its own guide.

[bcoe]

@bnoordhuis @addaleax

That already works: tools/test.py parallel/test-child-process-*

Was thinking about this myself, without introducing all the churn in test names we could make CI_JS_SUITES and CI_NATIVE_SUITES configurable and use:

CI_JS_SUITES=parallel/test-child-process-* make coverage.

Unless I'm missing something, it's the Makefile that combines the coverage reports and outputs reports, so I think at a minimum this would be a nice change (making these variables configurable) so that the feedback loop when testing with coverage is lovely.

[mscdex]

I have a feeling this structure will cause confusion for the tests we have that involve multiple modules (e.g. cluster and dgram). How would you know where to put those tests? If you put them in the wrong directory, you run the risk of missing them when running the other group of tests.

[bcoe]

I'm not a fan of the huge amount of churn. I still get annoyed on a weekly basis by the test/simple split into test/parallel and test/sequential and that at least introduced the benefit of running tests in parallel.

Once I moved a couple sub-systems into their own top-level folder, I definitely got somewhat spooked by the large amount of files that would change it git history. The main benefit I see is that you would now be able to more easily able to run pummel, parallel, and sequential tests for a given subsystem, e.g.,

python tools/test.py http2*

vs.,

python tools/test.py "parallel/test-http2*" "sequential/test-http2*" "pummel/test-http2*"'

or perhaps:

CI_JS_SUITE="parallel/test-http2* sequential/test-http2* pummel/test-http2*" make coverage'

I also do think it could put us in a position where answering the question where should I add a test gets a bit easier. Test suites could default to being structured like es-module, running in parallel, with their tests in a top-level folder corresponding to their subsystem.

I think what I have found most confusing about the test structure currently is that some tests have snuck into their own top-level submodule specific folders, e.g., es-module, doctool, while some are in the catch-all categories like sequential and parallel.

Perhaps an alternative refactor of the test structure would be:

Step 1

• make CI_JS_SUITE, etc., configurable so that it's easier to generate coverage reports in isolation.
• add docs about test coverage and how to generate coverage reports for specific modules.
• introduce behavior like the idea of DEFAULT_JS_SUITES, so that we don't accidentally miss any
  new top-level folders that are introduced.

Step 2

Double down on the current directory structure:

• introduce a top-level native/ folder, which addons and addons-napi could be moved to.
• move any top-level folders that have snuck into the test folder into native/, parallel/, sequential/, pummel/, or internet/. It might take some forensics to figure out why some of the top-level folders exist:
  • why is timers/ its own folder?
  • why is tick-processor/ its own folder?
  • maybe there just needs to be a top-level folder like flaky/ for suites we occasionally want to run, but don't want to be part of a regular CI run?

[bcoe]

looping in @Fishrock123, given their original interest in a similar refactor.

I have a feeling this structure will cause confusion for the tests we have that involve multiple modules (e.g. cluster and dgram). How would you know where to put those tests? I

I don't immediately have a great answer to this; I pictured that we'd probably keep the top-level parallel/ and sequential/ folders for some tests -- but this would be a bit confusing that almost all tests would be in a subsystem specific folder, with stragglers in catch-all folders.

[MylesBorins]

Is there a way to potentially add meta data to the test itself to introduce subsystems rather than relying on the file system? I think this is great work fwiw, just trying to spitball some alternative ideas that could involve less churn / be more flexible

[bcoe]

@MylesBorins @bnoordhuis what if we did start to collapse tests back into a single folder, but introduced the following:

// Type: sequential
// Flags: --experimental-modules

where type might be something like, sequential, parallel, internet ... this would still require quite a bit of churn mind you, since I don't think we'd want to continue using a parallel and sequential folder.

@MylesBorins you're picturing something more along the lines of continuing to have sequential/, parallel/, but introducing a way to execute tests for a submodule across these folders. I think we could do this without introducing additional meta-information, by having something like:

test.py --subsystem=http which would walk all files, but only run files with a filename that matches the pattern test-<subsystem>-foo.

If there isn't consensus around isolating tests, I think introducing a helper for running a specific subsystem would be a good move that introduces less churn.

[mscdex]

I don't think putting the 'type' in each file is going to be very efficient. Most people are just going to do make test and so having to open each and every test file (twice unless specifically optimized to only read once) is a waste and will slow things down even further.

Having 'type' as another metadata comment can still have the same problem as the original separate directory per module proposal.

[TimothyGu]

having to open each and every test file (twice unless specifically optimized to only read once) is a waste and will slow things down even further.

We could look for the type metadata at the same time as when searching for Flags:.

[mscdex]

We could look for the type metadata at the same time as when searching for Flags:.

That wouldn't help in the common make test case. It would make sense to do that though if someone was specifying a subsystem on the command line.

[gibfahn]

The main benefit I see is that you would now be able to more easily able to run pummel, parallel, and sequential tests for a given subsystem, e.g.,

python tools/test.py http2*

vs.,

python tools/test.py "parallel/test-http2*" "sequential/test-http2*" "pummel/test-http2*"'

At least for me, the natural way to solve this would be to allow tools/test.py */test-http2-*.

[addaleax]

@gibfahn I am pretty sure that already works, I’ve run */*http2* a couple times already.

[jasnell]

Yes, that works but it's fairly obscure and does not solve the general It's-Difficult-To-Manage-Thousands-Of-Tests-All-In-One-Directory problem.

[gibfahn]

Yes, that works but it's fairly obscure

I don't know, using file globbing is the standard command-line way to specify filenames, and you could actually just have the shell expand the filenames directly, like:

tools/test.py test/*/test-http2*

It's certainly the first thing I thought of.

does not solve the general It's-Difficult-To-Manage-Thousands-Of-Tests-All-In-One-Directory problem.

So what are the specific issues with having large numbers of files in each directory? I know there's the "Github can't render more than 1000 files in one dir" issue, but what else are people running into?

I don't think the current situation is ideal, but I also think @bnoordhuis and @mscdex make valid points about issues with this approach. 99% of test runs are make test (or make test-ci), so we should optimise for that case.

[bcoe]

@gibfahn @jasnell given the feedback on this pull-request, it feels like it would be difficult to reach consensus about splitting tests into subsystem-specific folders. I'm currently working on an alternative pull-request that I think might be a good compromise ... and give me a lot of what I was itching for when running tests:

• an easy to remember shorthand for running a given subsystem, I'm thinking: test.py --subsystem=http.
• a way to run coverage for a given set of tests and still generate reports (this just requires making a few variables configurable in the Makefile).
• updated documentation that encourages people to improve coverage, and talks in detail about how to run tests for subsystems or coverage in isolation.
• switch to a black-list for the test folders that are run by default, rather than a whitelist (so that we don't forget to add folders in the future like we did with esm-modules).
• make it so it is possible to nest suites (so that, as an example, es-modules could have a parallel and pummel folder).

I think the work described above would improve the feedback loop testing, without requiring that we move thousands of test files into a new structure out of the gate.

[jasnell]

@bcoe ... Yep. I look forward to whatever you come up with :)