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?

[JakobJingleheimer]

where tests pass in isolation but would fail when ran together.

Not to derail this PR: that sounds an XY problem: either the test is bad or the technical design of the implementation being tested is flawed. In 20 years of engineering, the root-cause of these kinds of problems has always been one of those.

[lpinca]

Even Node.js tools/test.py allows to specify the number of parallel tasks to run via the -j option and it make sense to me. I want control over this.

[MoLow]

I think it should be opt-out: tests should be isolated. If parallelism causes a problem, you have a different problem that parallelism is merely revealing.

I just want to point out that the scope of this discussion should also cover test and not only it - this PR aligns it to behave the same as test, and their defaults probably should be the same.

[JakobJingleheimer]

Even Node.js tools/test.py allows to specify the number of parallel tasks to run via the -j option and it make sense to me. I want control over this.

I'll open a separate PR to add a true option. We don't need to choose one or the other.

Sorry, thank you for the fix @MoLow.

[lpinca]

I'll open a separate PR to add a true option. We don't need to choose one or the other.

We are going off-topic but I don't see the need for it. -j 0 or -j omitted is equivalent to your true, that is use all cores, but again I want to be able to specify 1,2,...n, not all or nothing.

[MoLow]

Even Node.js tools/test.py allows to specify the number of parallel tasks to run via the -j option and it makes sense to me. I want control over this.

I think everyone wants control over concurrency. the question is whether it should be opt-in or opt-out.
the question is if the node test runner favors speed or favors stability,
it is true that good tests should be isolated - but the fact is many (probably most) authors write some tests that are not (just take a look at the CI of this repo :/ ), so I think "test should be isolated" is not a valid argument here

[cjihrig]

My personal opinion is that within a single test file, things should happen serially by default. The CLI runner should execute files in parallel by default. Additional parallelism should be opt-in.

[JakobJingleheimer]

but again I want to be able to specify 1,2,...n, not all or nothing.

I think you misread/misunderstood my comment. I didn't propose removing the integer option. I proposed adding a true option. If you want to hassle with some number, fine, set concurrency: 4. I don't, so I set concurrency: true. Or are you saying I can currently set concurrency: 0 and that will make it as concurrent as possible?

the question is if the node test runner favors speed or favors stability,

It doesn't have to be a binary choice ;)

it is true that good tests should be isolated - but the fact is many (probably most) authors write some tests that are not (just take a look at the CI of this repo :/ )

I'm working on it #43784 I just need this PR's fix ;)

[ljharb]

A true option that automatically chooses "number of CPUs minus one" sounds awesome to me, regardless of the default.

[lpinca]

Ok I probably misunderstood. It was not clear that you wanted to add true. What I understood was that you wanted to turn the integer option into a boolean only one.

[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