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
Allow to run tests in the main process #51548
Comments
sounds like a legit feature. @nodejs/test_runner WDYT? |
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 withastro/astro#9758 Here's the result between # 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
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 Another thing that would be interesting to test is to use Worker threads, not sure this has been experimented with yet. |
that can be done by omitting the
I plan on experimenting with both worker threads and simply importing test files later today |
By skipping pnpm test 10.53s user 1.37s system 115% cpu 10.294 total Interestingly it's similar to using |
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. |
It hardly matters what we think about that choice, changing it would be a breaking change, so probably not worth it. |
Sounds like a legitimate feature to me |
Isn't it duplicate of #48871 ? |
What is the problem this feature will solve?
At the moment, transitioning to
node:test
frommocha
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: withastro/astro#9758 (comment)
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.The text was updated successfully, but these errors were encountered: