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

Support concurrency when --experimental-test-isolation is set to 'none' #55939

Open
blimmer opened this issue Nov 20, 2024 · 5 comments
Open
Labels
feature request Issues that request new features to be added to Node.js. test_runner Issues and PRs related to the test runner subsystem.

Comments

@blimmer
Copy link

blimmer commented Nov 20, 2024

What is the problem this feature will solve?

It would be nice to be able to run multiple "workers" when using the none test isolation mode.

Today, according to the docs, when --experimental-test-isolation is set to 'none', it implies 1 concurrency:

node/doc/api/cli.md

Lines 2252 to 2264 in f270462

### `--test-concurrency`
<!-- YAML
added:
- v21.0.0
- v20.10.0
- v18.19.0
-->
The maximum number of test files that the test runner CLI will execute
concurrently. If `--experimental-test-isolation` is set to `'none'`, this flag
is ignored and concurrency is one. Otherwise, concurrency defaults to
`os.availableParallelism() - 1`.

However, mocha another test runner that does not isolate tests, does accept a concurrency flag: https://mochajs.org/#parallel-tests

What is the feature you are proposing to solve the problem?

We're looking to move off of jest because its test module isolation is extremely slow. We love the idea of using the node-native test runner with isolation disabled, instead of adopting another third-party framework like mocha.

However, we'd need to write some custom code (e.g., using parallel) to spin up n concurrent, isolation-disabled tests to effectively utilize all the cores available on our CI machine.

The docs and other recent comments all indicate that when --experimental-test-isolation is set to 'none', concurrency must be 1. However, I couldn't find the reasoning in the original PR or issue.

There's probably a good reason for this but, as someone not intimately familiar with the implementation, it's not obvious to me why we wouldn't be able to run non-isolated tests concurrently, like in mocha.

What alternatives have you considered?

I could probably use a tool like parallel to spin up multiple calls to node --test. However, this would require me to also write code to split up all the test files between the parallel runs, etc.

@blimmer blimmer added the feature request Issues that request new features to be added to Node.js. label Nov 20, 2024
@github-project-automation github-project-automation bot moved this to Awaiting Triage in Node.js feature requests Nov 20, 2024
@cjihrig
Copy link
Contributor

cjihrig commented Nov 20, 2024

It would be nice to be able to run multiple "workers" when using the none test isolation mode.

This wouldn't make sense. A "worker" here is a child process. When isolation=none, there are no child processes and everything runs in the same process.

However, mocha another test runner that does not isolate tests, does accept a concurrency flag

That flag essentially does isolation=process. Also note all of the caveats that come with that flag in Mocha.

I think you are essentially conflating parallelism of the CLI (the concurrency and isolation flags) with parallelism within a single file. There is no CLI flag to set concurrency within a single file, but there are APIs such as the concurrency option to suite() and test().

@RedYetiDev RedYetiDev added the test_runner Issues and PRs related to the test runner subsystem. label Nov 20, 2024
@stephenh
Copy link

Hi @cjihrig ! Thanks for the quick reply... I work with @blimmer 👋, so can chime in on our use case...

We understand isolation=none currently means "no child processes", and that it seems like "asking for concurrency means we're asking for child processes"...

Stepping back, what we're after is running ~8 test files simultaneously (across a suite of ~1000s of test files), but without each test file getting its own dedicated short-lived child process.

I.e. we'd be fine with ~8 child processes, as long as those child processes were long-lived, and did not have any "isolation" (aka new process) between each test files.

What we're facing with "1 test file => 1 short-lived process" (isolation) is that each test file has to pay a "require the node_modules world" tax, that in our large codebase is unfortunately very expensive (3-5 seconds, depending on the machine).

We want to "require the node_modules world" as few times as possible, ideally just once (or maybe once per child process), as long as each child process is long-lived, and could then run ~100s of test files each, as that amortizes each process's "require the world" cost across many tests.

Afaiu this is what Mocha's concurrency flag does--you're right, it boots up multiple child process (so we get concurrency), but that doesn't mean each test file gets it's own child process (which means isolation).

@stephenh
Copy link

stephenh commented Nov 20, 2024

Fwiw I think we'd be fine with "all tests run in the main process" (isolation=none), as long as, out of our ~1000s of test files, there are ~5-10 test files running "at the same time".

Because our tests make I/O calls to a database *, they are not CPU bound, so we want to be executing ~5-10 of then at once (we currently do 8), instead of having them run serially, one test file after the other.

Our assumption was that concurrency was how to enable this "8 test files running at once", but I think we're ambivalent whether that would be 1 main process (with 8 test files executing simultaneously) or 8 long-lived child processes (with each child process doing 1 file at a time).

* Technically we give each executing-in-parallel test its own database, so they don't stomp on each other.

@cjihrig
Copy link
Contributor

cjihrig commented Nov 21, 2024

Thanks for explaining. So this is a feature request for one or both of these things:

  • Support a pool of worker processes where each process is able to run multiple test files based on completion of previous work.
  • Support running multiple test files concurrently in the main test runner process.

@stephenh
Copy link

@cjihrig yep, that's right!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Issues that request new features to be added to Node.js. test_runner Issues and PRs related to the test runner subsystem.
Projects
Status: Awaiting Triage
Development

No branches or pull requests

4 participants