node
node copied to clipboard
nodejs
test_runner: allow running test files in single process
Fixes: https://github.com/nodejs/node/issues/51548
this PR is still missing tests & documentation
The https://github.com/nodejs/node/labels/notable-change label has been added by @MoLow.
Please suggest a text for the release notes if you'd like to include a more detailed summary, then proceed to update the PR description with the text or a link to the notable change suggested text comment. Otherwise, the commit will be placed in the Other Notable Changes section.
![github-actions[bot] avatar](https://avatars.githubusercontent.com/in/15368?v=4 "Published by a pub.dev verified publisher"){kind=small}
Could you please add a test for before/after all hooks and make sure they are not executed multiple times for each file?
Couple of thoughts:
I think in case of no isolation we should set the --test-concurrency default value to be 1
If there is concurrency, should it run in different process or same one?
I'v ended up adding a single process as an entry point for importing all files - this way we support features such as coverage, watch mode, timeout etc. @nodejs/test_runner please take a look. @rluvaton are there still tests you think should be added?
@rluvaton are there still tests you think should be added?
The tests that I wrote in my prev comment, the implementation detail should not affect the test cases
Also, there are some thoughts regarding concurrency that seem like you missed
Also, there are some thoughts regarding concurrency that seem like you missed
can you elaborate?
I think in case of no isolation we should set the
--test-concurrencydefault value to be 1
Having concurrent test can lead to flaky tests when mutating some global state
If there is concurrency, should it run in different process or same one?
Because it can cause flakiness, I would assume concurrency will run in different processes each process won't isolation
I think it makes sense to document that --test-concurrency will be 1 when running everything in the same process.
@MoLow once this lands, do you think it will be feasible to make --test-only only necessary when running multiple processes? It seems like it to me, but I haven't reviewed this code yet (there are conflicts). If we could manage that, I think it would be a huge win.
@MoLow once this lands, do you think it will be feasible to make --test-only only necessary when running multiple processes? It seems like it to me, but I haven't reviewed this code yet (there are conflicts). If we could manage that, I think it would be a huge win.
Yes, I think that will be possible. Il try getting back to this PR this week
Having concurrent test can lead to flaky tests when mutating some global state
I strongly feel this is the responsibility of the test author to ensure they do not create dependencies between tests, and properly manage variables in the global context. I don't think node should have to baby-sit anyone.
Jest allows for certain globals, and they make the same very clear in their docs.
@MoLow are you still planning to work on this? If not, would you mind if I took a shot at it. I'd really like to see this functionality land for the possibilities of a performance boost and avoiding the --test-only flag when isolation is disabled.