test_runner, cli: add --test-concurrency flag by cjihrig · Pull Request #49996 · nodejs/node

cjihrig · 2023-10-01T15:49:05Z

This commit adds a new --test-concurrency CLI flag that controls the parallelism of the test runner CLI.

Fixes: #49487

nodejs-github-bot · 2023-10-01T15:49:10Z

Review requested:

@nodejs/test_runner

mcollina

lgtm

nodejs-github-bot · 2023-10-01T16:54:51Z

CI: https://ci.nodejs.org/job/node-test-pull-request/54447/

nodejs-github-bot · 2023-10-01T17:48:01Z

CI: https://ci.nodejs.org/job/node-test-pull-request/54448/

MoLow · 2023-10-01T17:56:04Z

how does this test if the files really run concurrently? We have fixtures that create a mutex, can we use them?
node/test/parallel/test-runner-concurrency.js
Lines 89 to 98 in 966e3d3
test('--test multiple files',{skip: os.availableParallelism()<3},async()=>{
awaitfs.writeFile(tmpdir.resolve('test-runner-concurrency'),'');
const{ code, stderr }=awaitcommon.spawnPromisified(process.execPath,[
'--test',
fixtures.path('test-runner','concurrency','a.mjs'),
fixtures.path('test-runner','concurrency','b.mjs'),
]);
assert.strictEqual(stderr,'');
assert.strictEqual(code,0);
});

also, better to test the --test-concurrency within https://github.com/nodejs/node/blob/966e3d34936b28bfb31f00a6f862226baa3bf9d0/test/parallel/test-runner-concurrency.js to avoid the two tests using this "mutex" in parallel (and it is related to this file)

I now relaized this assersion. deepStrictEqual(readdirSync(tmpdir.path), [])
anyway we can still move the test into that file but not a blocker.
also, probably skip second test if os.availableParallelism() < 3

how does this test if the files really run concurrently?
We know that they are not running sequentially because each file never finishes. Since we know that the tests aren't running sequentially, we just need to verify that the batch size is respected. Plus, this change is not introducing any new concurrency functionality, it's just assigning a different value using the existing functionality.

nodejs-github-bot · 2023-10-08T17:30:37Z

CI: https://ci.nodejs.org/job/node-test-pull-request/54622/ 🟡

nodejs-github-bot · 2023-10-08T20:09:30Z

Commit Queue failed

- Loading data for nodejs/node/pull/49996
✔ Done loading data for nodejs/node/pull/49996
----------------------------------- PR info ------------------------------------
Title test_runner, cli: add --test-concurrency flag (#49996)
Author Colin Ihrig (@cjihrig)
Branch cjihrig:test-runner-concurrency -> nodejs:main
Labels c++, semver-minor
Commits 1
- test_runner, cli: add --test-concurrency flag
Committers 1
- cjihrig PR-URL: https://github.com/nodejs/node/pull/49996
Fixes: https://github.com/nodejs/node/issues/49487
Reviewed-By: Matteo Collina Reviewed-By: Moshe Atlow Reviewed-By: Benjamin Gruenbaum Reviewed-By: Chemi Atlow ------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/49996
Fixes: https://github.com/nodejs/node/issues/49487
Reviewed-By: Matteo Collina Reviewed-By: Moshe Atlow Reviewed-By: Benjamin Gruenbaum Reviewed-By: Chemi Atlow --------------------------------------------------------------------------------
⚠ Commits were pushed since the last approving review:
⚠ - test_runner, cli: add --test-concurrency flag
ℹ This PR was created on Sun, 01 Oct 2023 15:49:05 GMT
✔ Approvals: 4
✔ - Matteo Collina (@mcollina) (TSC): https://github.com/nodejs/node/pull/49996#pullrequestreview-1651929277
✔ - Moshe Atlow (@MoLow) (TSC): https://github.com/nodejs/node/pull/49996#pullrequestreview-1651933179
✔ - Benjamin Gruenbaum (@benjamingr) (TSC): https://github.com/nodejs/node/pull/49996#pullrequestreview-1651942294
✔ - Chemi Atlow (@atlowChemi): https://github.com/nodejs/node/pull/49996#pullrequestreview-1652215745
✔ Last GitHub CI successful
ℹ Last Full PR CI on 2023-10-08T17:30:37Z: https://ci.nodejs.org/job/node-test-pull-request/54622/
- Querying data for job/node-test-pull-request/54622/
✔ Last Jenkins CI successful
--------------------------------------------------------------------------------
✔ Aborted `git node land` session in /home/runner/work/node/node/.ncu

https://github.com/nodejs/node/actions/runs/6449712653

This commit adds a new --test-concurrency CLI flag that controls the parallelism of the test runner CLI. PR-URL: nodejs#49996Fixes: nodejs#49487 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Chemi Atlow <chemi@atlow.co.il>

targos · 2023-10-10T09:06:12Z

The test added by this PR failed 4 times in one of the CI runs: https://ci.nodejs.org/job/node-test-pull-request/54448/
Please don't introduce flaky tests when you know they are flaky!

This commit adds a new --test-concurrency CLI flag that controls the parallelism of the test runner CLI. PR-URL: #49996Fixes: #49487 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Chemi Atlow <chemi@atlow.co.il>

Notable changes: deps: * (SEMVER-MINOR) update uvwasi to 0.0.19 (Node.js GitHub Bot) #49908 doc: * add H4ad to collaborators (Vinícius Lourenço) #50217 esm: * (SEMVER-MINOR) detect ESM syntax in ambiguous JavaScript (Geoffrey Booth) #50096 * use import attributes instead of import assertions (Antoine du Hamel) #50140 * --experimental-default-type flag to flip module defaults (Geoffrey Booth) #49869 fs: * (SEMVER-MINOR) add flush option to appendFile() functions (Colin Ihrig) #50095 * add flush option to writeFile() functions (Colin Ihrig) #50009 lib: * (SEMVER-MINOR) add WebSocket client (Matthew Aitken) #49830 stream: * (SEMVER-MINOR) allow pass stream class to `stream.compose` (Alex Yang) #50187 * call helper function from push and unshift (Raz Luvaton) #50173 * optimize Writable (Robert Nagy) #50012 test_runner, cli: * (SEMVER-MINOR) add --test-concurrency flag (Colin Ihrig) #49996 vm: * (SEMVER-MINOR) use import attributes instead of import assertions (Antoine du Hamel) #50141 * use default HDO when importModuleDynamically is not set (Joyee Cheung) #49950 wasi: PR-URL: #50682

Notable changes: deps: * (SEMVER-MINOR) update uvwasi to 0.0.19 (Node.js GitHub Bot) nodejs#49908 doc: * add H4ad to collaborators (Vinícius Lourenço) nodejs#50217 esm: * (SEMVER-MINOR) detect ESM syntax in ambiguous JavaScript (Geoffrey Booth) nodejs#50096 * use import attributes instead of import assertions (Antoine du Hamel) nodejs#50140 * --experimental-default-type flag to flip module defaults (Geoffrey Booth) nodejs#49869 fs: * (SEMVER-MINOR) add flush option to appendFile() functions (Colin Ihrig) nodejs#50095 * add flush option to writeFile() functions (Colin Ihrig) nodejs#50009 lib: * (SEMVER-MINOR) add WebSocket client (Matthew Aitken) nodejs#49830 stream: * (SEMVER-MINOR) allow pass stream class to `stream.compose` (Alex Yang) nodejs#50187 * call helper function from push and unshift (Raz Luvaton) nodejs#50173 * optimize Writable (Robert Nagy) nodejs#50012 test_runner, cli: * (SEMVER-MINOR) add --test-concurrency flag (Colin Ihrig) nodejs#49996 vm: * (SEMVER-MINOR) use import attributes instead of import assertions (Antoine du Hamel) nodejs#50141 * use default HDO when importModuleDynamically is not set (Joyee Cheung) nodejs#49950 wasi: PR-URL: nodejs#50682

This commit adds a new --test-concurrency CLI flag that controls the parallelism of the test runner CLI. PR-URL: #49996Fixes: #49487 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Chemi Atlow <chemi@atlow.co.il>

Notable changes: deps: * (SEMVER-MINOR) update uvwasi to 0.0.19 (Node.js GitHub Bot) #49908 * (SEMVER-MINOR) upgrade npm to 10.0.0 (npm team) #49423 doc: * add new TSC members (Michael Dawson) #48841 * move and rename loaders section (Geoffrey Booth) #49261 esm: * use import attributes instead of import assertions (Antoine du Hamel) #50140 * --experimental-default-type flag to flip module defaults (Geoffrey Booth) #49869 * unflag import.meta.resolve (Guy Bedford) #49028 * move hook execution to separate thread (Jacob Smith) #44710 * leverage loaders when resolving subsequent loaders (Maël Nison) #43772 lib: * (SEMVER-MINOR) add api to detect whether source-maps are enabled (翠 / green) #46391 * (SEMVER-MINOR) add tracing channel to diagnostics_channel (Stephen Belanger) #44943 src: * (SEMVER-MINOR) add cjs_module_lexer_version base64_version (Jithil P Ponnan) #45629 stream: * use bitmap in readable state (Benjamin Gruenbaum) #49745 test_runner: * (SEMVER-MINOR) accept `testOnly` in `run` (Moshe Atlow) #49753 * (SEMVER-MINOR) add junit reporter (Moshe Atlow) #49614 * (SEMVER-MINOR) expose location of tests (Colin Ihrig) #48975 * (SEMVER-MINOR) add shards support (Raz Luvaton) #48639 * (SEMVER-MINOR) add initial draft for fakeTimers (Erick Wendel) #47775 test_runner, cli: * (SEMVER-MINOR) add --test-concurrency flag (Colin Ihrig) #49996 tls: * (SEMVER-MINOR) add ALPNCallback server option for dynamic ALPN negotiation (Tim Perry) #45190 vm: * (SEMVER-MINOR) use import attributes instead of import assertions (Antoine du Hamel) #50141 wasi: * (SEMVER-MINOR) updates required for latest uvwasi version (Michael Dawson) #49908 PR-URL: TODO

Notable changes: deps: * (SEMVER-MINOR) update uvwasi to 0.0.19 (Node.js GitHub Bot) #49908 * (SEMVER-MINOR) upgrade npm to 10.2.3 (npm team) #50531 doc: * move and rename loaders section (Geoffrey Booth) #49261 esm: * use import attributes instead of import assertions (Antoine du Hamel) #50140 * --experimental-default-type flag to flip module defaults (Geoffrey Booth) #49869 * unflag import.meta.resolve (Guy Bedford) #49028 * move hook execution to separate thread (Jacob Smith) #44710 * leverage loaders when resolving subsequent loaders (Maël Nison) #43772 lib: * (SEMVER-MINOR) add api to detect whether source-maps are enabled (翠 / green) #46391 * (SEMVER-MINOR) add tracing channel to diagnostics_channel (Stephen Belanger) #44943 src: * (SEMVER-MINOR) add cjs_module_lexer_version base64_version (Jithil P Ponnan) #45629 stream: * use bitmap in readable state (Benjamin Gruenbaum) #49745 test_runner: * (SEMVER-MINOR) accept `testOnly` in `run` (Moshe Atlow) #49753 * (SEMVER-MINOR) add junit reporter (Moshe Atlow) #49614 * (SEMVER-MINOR) expose location of tests (Colin Ihrig) #48975 * (SEMVER-MINOR) add shards support (Raz Luvaton) #48639 * (SEMVER-MINOR) add initial draft for fakeTimers (Erick Wendel) #47775 test_runner, cli: * (SEMVER-MINOR) add --test-concurrency flag (Colin Ihrig) #49996 tls: * (SEMVER-MINOR) add ALPNCallback server option for dynamic ALPN negotiation (Tim Perry) #45190 vm: * (SEMVER-MINOR) use import attributes instead of import assertions (Antoine du Hamel) #50141 PR-URL: #50953

This commit adds a new --test-concurrency CLI flag that controls the parallelism of the test runner CLI. PR-URL: nodejs#49996Fixes: nodejs#49487 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Chemi Atlow <chemi@atlow.co.il>

This commit adds a new --test-concurrency CLI flag that controls the parallelism of the test runner CLI. PR-URL: nodejs/node#49996Fixes: nodejs/node#49487 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Chemi Atlow <chemi@atlow.co.il>

Notable changes: deps: * (SEMVER-MINOR) update uvwasi to 0.0.19 (Node.js GitHub Bot) nodejs/node#49908 * (SEMVER-MINOR) upgrade npm to 10.2.3 (npm team) nodejs/node#50531 doc: * move and rename loaders section (Geoffrey Booth) nodejs/node#49261 esm: * use import attributes instead of import assertions (Antoine du Hamel) nodejs/node#50140 * --experimental-default-type flag to flip module defaults (Geoffrey Booth) nodejs/node#49869 * unflag import.meta.resolve (Guy Bedford) nodejs/node#49028 * move hook execution to separate thread (Jacob Smith) nodejs/node#44710 * leverage loaders when resolving subsequent loaders (Maël Nison) nodejs/node#43772 lib: * (SEMVER-MINOR) add api to detect whether source-maps are enabled (翠 / green) nodejs/node#46391 * (SEMVER-MINOR) add tracing channel to diagnostics_channel (Stephen Belanger) nodejs/node#44943 src: * (SEMVER-MINOR) add cjs_module_lexer_version base64_version (Jithil P Ponnan) nodejs/node#45629 stream: * use bitmap in readable state (Benjamin Gruenbaum) nodejs/node#49745 test_runner: * (SEMVER-MINOR) accept `testOnly` in `run` (Moshe Atlow) nodejs/node#49753 * (SEMVER-MINOR) add junit reporter (Moshe Atlow) nodejs/node#49614 * (SEMVER-MINOR) expose location of tests (Colin Ihrig) nodejs/node#48975 * (SEMVER-MINOR) add shards support (Raz Luvaton) nodejs/node#48639 * (SEMVER-MINOR) add initial draft for fakeTimers (Erick Wendel) nodejs/node#47775 test_runner, cli: * (SEMVER-MINOR) add --test-concurrency flag (Colin Ihrig) nodejs/node#49996 tls: * (SEMVER-MINOR) add ALPNCallback server option for dynamic ALPN negotiation (Tim Perry) nodejs/node#45190 vm: * (SEMVER-MINOR) use import attributes instead of import assertions (Antoine du Hamel) nodejs/node#50141 PR-URL: nodejs/node#50953

This commit adds a new --test-concurrency CLI flag that controls the parallelism of the test runner CLI. PR-URL: nodejs/node#49996Fixes: nodejs/node#49487 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Chemi Atlow <chemi@atlow.co.il>

Notable changes: deps: * (SEMVER-MINOR) update uvwasi to 0.0.19 (Node.js GitHub Bot) nodejs/node#49908 * (SEMVER-MINOR) upgrade npm to 10.2.3 (npm team) nodejs/node#50531 doc: * move and rename loaders section (Geoffrey Booth) nodejs/node#49261 esm: * use import attributes instead of import assertions (Antoine du Hamel) nodejs/node#50140 * --experimental-default-type flag to flip module defaults (Geoffrey Booth) nodejs/node#49869 * unflag import.meta.resolve (Guy Bedford) nodejs/node#49028 * move hook execution to separate thread (Jacob Smith) nodejs/node#44710 * leverage loaders when resolving subsequent loaders (Maël Nison) nodejs/node#43772 lib: * (SEMVER-MINOR) add api to detect whether source-maps are enabled (翠 / green) nodejs/node#46391 * (SEMVER-MINOR) add tracing channel to diagnostics_channel (Stephen Belanger) nodejs/node#44943 src: * (SEMVER-MINOR) add cjs_module_lexer_version base64_version (Jithil P Ponnan) nodejs/node#45629 stream: * use bitmap in readable state (Benjamin Gruenbaum) nodejs/node#49745 test_runner: * (SEMVER-MINOR) accept `testOnly` in `run` (Moshe Atlow) nodejs/node#49753 * (SEMVER-MINOR) add junit reporter (Moshe Atlow) nodejs/node#49614 * (SEMVER-MINOR) expose location of tests (Colin Ihrig) nodejs/node#48975 * (SEMVER-MINOR) add shards support (Raz Luvaton) nodejs/node#48639 * (SEMVER-MINOR) add initial draft for fakeTimers (Erick Wendel) nodejs/node#47775 test_runner, cli: * (SEMVER-MINOR) add --test-concurrency flag (Colin Ihrig) nodejs/node#49996 tls: * (SEMVER-MINOR) add ALPNCallback server option for dynamic ALPN negotiation (Tim Perry) nodejs/node#45190 vm: * (SEMVER-MINOR) use import attributes instead of import assertions (Antoine du Hamel) nodejs/node#50141 PR-URL: nodejs/node#50953

cjihrig requested review from MoLow and mcollina October 1, 2023 15:49

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. labels Oct 1, 2023

cjihrig mentioned this pull request Oct 1, 2023
Add support for running all tests serially #49487
Closed

cjihrig force-pushed the test-runner-concurrency branch from 7f629c0 to abdb63dCompare October 1, 2023 15:53

mcollina approved these changes Oct 1, 2023
View reviewed changes

cjihrig added the semver-minor PRs that contain new features and should be released in the next minor version. label Oct 1, 2023

MoLow approved these changes Oct 1, 2023
View reviewed changes

MoLow added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 1, 2023

github-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 1, 2023

MoLow reviewed Oct 1, 2023
View reviewed changes

benjamingr approved these changes Oct 1, 2023
View reviewed changes

github-actionsBot mentioned this pull request Oct 2, 2023
CI Reliability 2023-10-02 nodejs/reliability#681
Open
12 tasks

atlowChemi approved these changes Oct 2, 2023
View reviewed changes

This was referenced Oct 3, 2023
CI Reliability 2023-10-03 nodejs/reliability#682
Open
CI Reliability 2023-10-04 nodejs/reliability#683
Open
CI Reliability 2023-10-05 nodejs/reliability#684
Open

cjihrig force-pushed the test-runner-concurrency branch from 5a36454 to 9e8c18bCompare October 8, 2023 17:29

cjihrig added commit-queue Add this label to land a pull request using GitHub Actions. and removed needs-ci PRs that need a full CI run. labels Oct 8, 2023

nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Oct 8, 2023

cjihrig force-pushed the test-runner-concurrency branch from 9e8c18b to 9f9c582Compare October 8, 2023 20:11

cjihrig merged commit 9f9c582 into nodejs:mainOct 8, 2023

cjihrig deleted the test-runner-concurrency branch October 8, 2023 20:11

richardlau mentioned this pull request Oct 9, 2023
parallel/test-runner-cli-concurrency fails with EBUSY: resource busy or locked#50101
Closed

RafaelGSS mentioned this pull request Oct 9, 2023
2023-10-17, Version 21.0.0 (Current) #49870
Merged

targos mentioned this pull request Nov 12, 2023
v20.10.0 release proposal #50682
Merged

mateonunez mentioned this pull request Nov 19, 2023
refactor(test): remove tap from db-authorization tests platformatic/platformatic#1814
Merged
16 tasks

targos mentioned this pull request Nov 28, 2023
v18.19.0 release proposal #50953
Merged

juanarbol mentioned this pull request Dec 6, 2023
2023-12-06, N|Solid v5.0.1 Iron Release nodesource/nsolid#41
Merged

Navigation Menu

Uh oh!

Uh oh!

test_runner, cli: add --test-concurrency flag - #49996

test_runner, cli: add --test-concurrency flag#49996
cjihrig merged 1 commit into
nodejs:mainfrom
cjihrig:test-runner-concurrency

cjihrig commented Oct 1, 2023

Uh oh!

nodejs-github-bot commented Oct 1, 2023

Uh oh!

mcollina left a comment

Uh oh!

nodejs-github-bot commented Oct 1, 2023

Uh oh!

nodejs-github-bot commented Oct 1, 2023

Uh oh!

MoLowOct 1, 2023

Uh oh!

MoLowOct 1, 2023

Uh oh!

MoLowOct 1, 2023

Uh oh!

cjihrigOct 1, 2023

Uh oh!

nodejs-github-bot commented Oct 8, 2023•
edited by cjihrig
Loading

Uh oh!

nodejs-github-bot commented Oct 8, 2023

Uh oh!

targos commented Oct 10, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

	test('--test multiple files',{skip: os.availableParallelism()<3},async()=>{
	awaitfs.writeFile(tmpdir.resolve('test-runner-concurrency'),'');
	const{ code, stderr }=awaitcommon.spawnPromisified(process.execPath,[
	'--test',
	fixtures.path('test-runner','concurrency','a.mjs'),
	fixtures.path('test-runner','concurrency','b.mjs'),
	]);
	assert.strictEqual(stderr,'');
	assert.strictEqual(code,0);
	});

Uh oh!

Uh oh!

Conversation

cjihrig commented Oct 1, 2023

Uh oh!

nodejs-github-bot commented Oct 1, 2023

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Oct 1, 2023

Uh oh!

nodejs-github-bot commented Oct 1, 2023

Uh oh!

MoLowOct 1, 2023

Choose a reason for hiding this comment

Uh oh!

MoLowOct 1, 2023

Choose a reason for hiding this comment

Uh oh!

MoLowOct 1, 2023

Choose a reason for hiding this comment

Uh oh!

cjihrigOct 1, 2023

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Oct 8, 2023• edited by cjihrig Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented Oct 8, 2023

Uh oh!

targos commented Oct 10, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

nodejs-github-bot commented Oct 8, 2023•
edited by cjihrig
Loading