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
test: do not set concurrency on parallelized runs #52177
Conversation
Our CI already run test files in parallel, having `node:test` spawns child processes concurrently could lead to oversubscribing the CI machine. This commit sets the `concurrency` depending on the presence of `TEST_PARALLEL` in the env, so running the test file individually still spawns child processes concurrently, and running the whole test suite does not oversubscribe the machine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Non blocking comment: maybe exporting a function like shouldEnableConcurrency()
on common
could be better than duplicate the same verification every time.
describe('--experimental-detect-module', { concurrency: true }, () => { | ||
describe('string input', { concurrency: true }, () => { | ||
describe('--experimental-detect-module', { concurrency: !process.env.TEST_PARALLEL }, () => { | ||
describe('string input', { concurrency: !process.env.TEST_PARALLEL }, () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since concurrency
is inherited from the parent, do we need to enable it twice here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll be honest, I only did a quick search and replace, I didn't look closely at the code. I'm tempted to not bother, even if it's redundant, it doesn't harm.
Landed in bae14b7 |
Our CI already run test files in parallel, having `node:test` spawns child processes concurrently could lead to oversubscribing the CI machine. This commit sets the `concurrency` depending on the presence of `TEST_PARALLEL` in the env, so running the test file individually still spawns child processes concurrently, and running the whole test suite does not oversubscribe the machine. PR-URL: nodejs#52177 Reviewed-By: Vinícius Lourenço Claro Cardoso <contact@viniciusl.com.br> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Our CI already run test files in parallel, having `node:test` spawns child processes concurrently could lead to oversubscribing the CI machine. This commit sets the `concurrency` depending on the presence of `TEST_PARALLEL` in the env, so running the test file individually still spawns child processes concurrently, and running the whole test suite does not oversubscribe the machine. PR-URL: nodejs#52177 Reviewed-By: Vinícius Lourenço Claro Cardoso <contact@viniciusl.com.br> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Our CI already run test files in parallel, having
node:test
spawns child processes concurrently could lead to oversubscribing the CI machine. This commit sets theconcurrency
depending on the presence ofTEST_PARALLEL
in the env, so running the test file individually still spawns child processes concurrently, and running the whole test suite does not oversubscribe the machine.