nodejs / node

Node.js JavaScript runtime ✨🐢🚀✨
https://nodejs.org
Other
107.97k stars 29.79k forks source link

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

Open blimmer opened 5 days ago

blimmer commented 5 days ago

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: https://github.com/nodejs/node/blob/f270462c09ddfd770291a7c8a2cd204b2c63d730/doc/api/cli.md#L2252-L2264

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.

cjihrig commented 5 days ago

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().

stephenh commented 5 days ago

Hi @cjihrig ! Thanks for the quick reply... I work with @blimmer :wave:, 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 commented 5 days ago

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 commented 5 days ago

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

stephenh commented 5 days ago

@cjihrig yep, that's right!