Add test concurrency flag

[98lenvi]

I went with the flag name test-concurrency instead of just concurrency as concurrency could be used for something else in the future.

Fixes: #43837

[aduh95]

/cc @nodejs/test_runner @MoLow

[MoLow]

I dont think we should expose this as a cli flag

[MoLow]

Why isn't this redundant? subtests.start() handles this for you

[MoLow]

@98lenvi thanks for the contribution!
we were talking about adding a boolean option for test concurrency
ie test('name', { concurrency: true}) in addition to the currently existing number option:
test('name', { concurrency: 2 })

[98lenvi]

@MoLow Thanks for the review! Could you please re-review it?

[MoLow]

LGTM

[aduh95]

Can we keep the list in alphabetical order?

[aduh95]

Suggested change

	}
	if (typeof concurrency === 'boolean') {
	} else if (typeof concurrency === 'boolean') {

[aduh95]

I think we would need a doc update and some tests as well.

[MoLow]

10ms might introduce a lot of flakiness, I am not sure that I would test concurrency with timestamps,
Probably a chain of tests (with a length of cors * 2 or anything bigger than the number of cors) that wait for each other to complete (each test only completes when next test passes?) might be less flaky

[MoLow]

also, test/message tests the output of the test runner, you probably want to write these tests in `test/parallel

[MoLow]

I would replace the term top level to running in --test mode

[aduh95]

We could simplify the test with common.mustCall and common.mustNotCall:

Suggested change

	describe('Concurrency option (boolean) = true ', { concurrency: true }, () => {
	const suiteStartDateTime = new Date().getTime();
	const extraBufferTime = 10;
	it('should be over by 3 seconds from suite start', async () => {
	const duration = 3000;
	const expectedEndDateTime = suiteStartDateTime + duration + extraBufferTime;
	await new Promise((resolve, reject) => {
	setTimeout(resolve, duration);
	});
	assert.strictEqual(expectedEndDateTime > new Date().getTime(), true);
	});
	it('should be over by 1 second from suite start', async () => {
	const duration = 1000;
	const expectedEndDateTime = suiteStartDateTime + duration + extraBufferTime;
	await new Promise((resolve, reject) => {
	setTimeout(resolve, duration);
	});
	assert.strictEqual(expectedEndDateTime > new Date().getTime(), true);
	});
	});
	describe('Concurrency option (boolean) = false ', { concurrency: false }, () => {
	const suiteStartDateTime = new Date().getTime();
	const extraBufferTime = 10;
	it('should be over by 3 seconds from suite start', async () => {
	const duration = 3000;
	const expectedEndDateTime = suiteStartDateTime + duration + extraBufferTime;
	await new Promise((resolve, reject) => {
	setTimeout(resolve, duration);
	});
	assert.strictEqual(expectedEndDateTime > new Date().getTime(), true);
	});
	it('should be over by 4 seconds from suite start', async () => {
	const duration = 1000;
	const prevTestDuration = 3000;
	const expectedEndDateTime =
	suiteStartDateTime + duration + extraBufferTime + prevTestDuration;
	await new Promise((resolve, reject) => {
	setTimeout(resolve, duration);
	});
	assert.strictEqual(expectedEndDateTime > new Date().getTime(), true);
	});
	});
	describe('Concurrency option (boolean) = true ', { concurrency: true }, () => {
	it('should never end', () => new Promise(() => {}));
	it('should start anyway', common.mustCall());
	});
	describe('Concurrency option (boolean) = false ', { concurrency: false }, () => {
	it('should never end', () => new Promise(() => {}));
	it('should never start', common.mustNotCall());
	});

[98lenvi]

@aduh95 I tried this out, but the parent test kills the long-running sub-test which would lead the entire test to fail.

[98lenvi]

@MoLow @aduh95 Thank you for your kind review & helping in making my first PR successful. Can you please re-review it?

[JakobJingleheimer]

🙌 thanks for this! LGTM

[nodejs-github-bot]

Commit Queue failed

- Loading data for nodejs/node/pull/43887
✔  Done loading data for nodejs/node/pull/43887
----------------------------------- PR info ------------------------------------
Title      Add test concurrency flag (#43887)
   ⚠  Could not retrieve the email or name of the PR author's from user's GitHub profile!
Branch     98lenvi:add-test-concurrency-flag -> nodejs:main
Labels     experimental, author ready, needs-ci, dont-land-on-v14.x, dont-land-on-v16.x, commit-queue-squash, test_runner
Commits    15
 - test_runner: introduces test_concurrency flag
 - test_runner: use flag for root concurrency
 - test_runner: fix concurrently flag documentation
 - test_runner: use exiting option instead of cli
 - test_runner: add test cases for concurrency
 - test_runner: imports in alphabetical order
 - test_runner: fix linting in tests
 - doc: update for concurrency option in test
 - doc: format test
 - test_runner: update test + update docs
 - test_runner: update test support less than 2 core
 - Revert "test_runner: update test support less than 2 core"
 - doc: update concurrency in test
 - test_runner: add comment in test case
 - test_runner: apply suggestions
Committers 1
 - Lenvin Gonsalves 
PR-URL: https://github.com/nodejs/node/pull/43887
Fixes: https://github.com/nodejs/node/issues/43837
Reviewed-By: Antoine du Hamel 
Reviewed-By: Jacob Smith 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/43887
Fixes: https://github.com/nodejs/node/issues/43837
Reviewed-By: Antoine du Hamel 
Reviewed-By: Jacob Smith 
--------------------------------------------------------------------------------
   ℹ  This PR was created on Mon, 18 Jul 2022 10:40:46 GMT
   ✔  Approvals: 2
   ✔  - Antoine du Hamel (@aduh95) (TSC): https://github.com/nodejs/node/pull/43887#pullrequestreview-1048608752
   ✔  - Jacob Smith (@JakobJingleheimer): https://github.com/nodejs/node/pull/43887#pullrequestreview-1048687338
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2022-07-24T08:25:53Z: https://ci.nodejs.org/job/node-test-pull-request/45634/
- Querying data for job/node-test-pull-request/45634/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  No git cherry-pick in progress
   ✔  No git am in progress
   ✔  No git rebase in progress
--------------------------------------------------------------------------------
- Bringing origin/main up to date...
From https://github.com/nodejs/node
 * branch                  main       -> FETCH_HEAD
✔  origin/main is now up-to-date
- Downloading patch for 43887
From https://github.com/nodejs/node
 * branch                  refs/pull/43887/merge -> FETCH_HEAD
✔  Fetched commits as d83446b4c469..03c1313f6d89
--------------------------------------------------------------------------------
Auto-merging lib/internal/test_runner/test.js
[main f76fc5427a] test_runner: introduces test_concurrency flag
 Author: Lenvin Gonsalves 
 Date: Mon Jul 18 15:24:01 2022 +0530
 4 files changed, 12 insertions(+), 1 deletion(-)
[main 6678742c87] test_runner: use flag for root concurrency
 Author: Lenvin Gonsalves 
 Date: Mon Jul 18 16:39:25 2022 +0530
 1 file changed, 1 insertion(+), 1 deletion(-)
[main c72832c90f] test_runner: fix concurrently flag documentation
 Author: Lenvin Gonsalves 
 Date: Mon Jul 18 16:41:40 2022 +0530
 1 file changed, 1 insertion(+), 1 deletion(-)
Auto-merging lib/internal/test_runner/test.js
CONFLICT (content): Merge conflict in lib/internal/test_runner/test.js
error: could not apply 0bd2eed00a... test_runner: use exiting option instead of cli
hint: After resolving the conflicts, mark them with
hint: "git add/rm ", then run
hint: "git cherry-pick --continue".
hint: You can instead skip this commit with "git cherry-pick --skip".
hint: To abort and get back to the state before "git cherry-pick",
hint: run "git cherry-pick --abort".
   ✖  Failed to apply patches

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

[aduh95]

Landed in dab492f

Thanks for the contribution 🎉