test_runner: fix it concurrency

[MoLow]

No description provided.

[MoLow]

CC @aduh95

[aduh95]

/cc @nodejs/test_runner

[nodejs-github-bot]

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

[aduh95]

Hum it looks like it makes message/test_runner_desctibe_it flaky. @MoLow do you know where this might be coming from?

=== release test_runner_desctibe_it.js ===
Path: message/test_runner_desctibe_it.js
Error: --- stderr ---
node:internal/test_runner/harness:29
      throw err;
      ^
TypeError: Cannot mix BigInt and other types, use explicit conversions
    at ItTest.report (node:internal/test_runner/test:425:42)
    at ItTest.finalize (node:internal/test_runner/test:418:10)
    at Test.processReadySubtestRange (node:internal/test_runner/test:195:15)
    at ItTest.postRun (node:internal/test_runner/test:388:19)
    at Test.postRun (node:internal/test_runner/test:370:17)
    at process.exitHandler (node:internal/test_runner/harness:79:10)
    at process.emit (node:events:513:28)
Node.js v19.0.0-pre

[MoLow]

Hum it looks like it makes message/test_runner_desctibe_it flaky. @MoLow do you know where this might be coming from?

@aduh95
yes, it happens when a test is canceled and was not yet assigned a start time. you can see I already fixed that here
adding concurrency introduced a timing condition that made it more likely to happen since previously when a suite ran serially - either a child test did not run or it did, it was never queued.

I suggest waiting for the fix to land, then rebase and make sure test is stable in this branch

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[juliangruber]

I'm surprised by this, is it actually expected to run it tests in parallel?

[ljharb]

I don't think it is - jest runs files in parallel (as does mocha, i believe), but i'm not aware of anything that runs individual its in parallel.

[MoLow]

perhaps this should be a feature that is off by default?

[MoLow]

Since this PR conflicts main and needs to be updated anyway - I will add the conclusion gathered in this discussion and re-request code reviews

[MoLow]

so the current implementation in this PR works this way:

• in the level of a test/it the default concurrency is 1
• when running --test, the default will be the number of CPUs, see:

node/lib/internal/test_runner/test.js

Lines 43 to 44 in 5eccc6b

	// TODO(cjihrig): Use uv_available_parallelism() once it lands.
	const rootConcurrency = isTestRunner ? cpus().length : 1;

• I want to leave support for boolean value for the concurrency setting out of the scope of this PR since the number of CPUs as a default only makes sense to me at the file level since each file spawns its own process. inside a file this isn't necessarily a good default - I will open a separate issue to discuss this

[nodejs-github-bot]

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

[juliangruber]

so the current implementation in this PR works this way:

• in the level of a test/it the default concurrency is 1

The code still looks like it runs all tests in parallel with unlimited concurrency, also the testTimeout has been removed

[MoLow]

The code still looks like it runs all tests in parallel with unlimited concurrency, also the testTimeout has been removed

the default is currently 1, see #43757 (comment)

[MoLow]

also the testTimeout has been removed

I am not sure if timeout is needed in describe and if it should support an asynchronous function, anyway - I will sort that out in #43554

[nodejs-github-bot]

Landed in a3766bc