node
node copied to clipboard
nodejs
Allow to run tests in the main process
What is the problem this feature will solve?
At the moment, transitioning to node:test from mocha isn't always possible because of perf regressions in the execution of the test suite. This feature will allow us to break these perf executions.
Here are some perf times that I run in our Astro repository: https://github.com/withastro/astro/pull/9758#issuecomment-1905598216
What is the feature you are proposing to solve the problem?
I would propose to accept an option to change the default behaviour. I don't want to bikeshed the name of the option, I leave to people that are better than me.
What alternatives have you considered?
Unfortunately, there isn't an alternative. We really want to transition to node:test, although at Astro we have a lot of tests, and we want to make sure that our CI doesn't become way slower than it already is.
Did you try to implement that in userland to validate the perf difference with Mocha is gone?
@matthewp suggested a workaround to use a file that imports all the test files so to node it looks like a single test file. It seems to work fine and I had implemented that in this branch, which is based on https://github.com/withastro/astro/pull/9758
Here's the result between node:test and mocha, running time pnpm test in the repo's /packages/integrations/node directory:
# node:test
pnpm test 10.24s user 1.24s system 115% cpu 9.914 total
# mocha
pnpm test 8.74s user 1.07s system 158% cpu 6.198 total
Mocha is perhaps slightly faster because it didn't have to spin up a child process at all. NOTE: in the branch linked above, I made a 2nd commit to fix some flaky tests. I also copied that to the Mocha tests so it's fairly compared.
The tradeoff of launching all the test from the same process is that if one test mutates a global, all tests are affected, so it certainly makes sense to not provide that option by default. It should not be too complicated to offer it as an option for the run() function.
Mocha is perhaps slightly faster because it didn't have to spin up a child process at all.
It would be interesting to test that, either by spawning a child process to start mocha, or by not spawning a child process to start node:test implementation.
Another thing that would be interesting to test is to use Worker threads, not sure this has been experimented with yet.
It would be interesting to test that, either by spawning a child process to start mocha, or by not spawning a child process to start node:test implementation.
that can be done by omitting the --test flag
Another thing that would be interesting to test is to use Worker threads, not sure this has been experimented with yet.
I plan on experimenting with both worker threads and simply importing test files later today
By skipping node:test's run() and importing the test files directly, I was able to test skipping the child process (can confirm that mutated globals are shared). Here's the result for time pnpm test like above:
pnpm test 10.53s user 1.37s system 115% cpu 10.294 total
Interestingly it's similar to using run() (that has isolation). So maybe starting one child process is fine, but too many it might take a toll on some projects. I've not looked into what else could have Mocha running faster though. NOTE: we have a custom astro-scripts test CLI to execute node:test, but compared to Mocha's, ours should be lighter-weight.
@aduh95
The tradeoff of launching all the test from the same process is that if one test mutates a global, all tests are affected, so it certainly makes sense to not provide that option by default.
I don't know how much code on npm is depending on globals, I assume the majority of it is simple utility input/output functions that don't need such a feature.
I also think file-based isolation is a weird choice in that it creates a refactor hazard. You decide that test1.js and test2.js are very similar so you combine them into 1 file and now the tests fail. That seems like a failure in your code you should be fixing.
Given that people use files for code organization it seems like a strange choice to me to make it also be about test isolation.
I also think file-based isolation is a weird choice in that it creates a refactor hazard.
It hardly matters what we think about that choice, changing it would be a breaking change, so probably not worth it.
Implementation work on this has moved to https://github.com/nodejs/node/pull/53927
