test_runner: introduce NODE_TEST_WORKER_ID for improved concurrent te…

[0hmX]

ref: #55842

Added a new environment variable, NODE_TEST_WORKER_ID, which ranges from 1 to N when --experimental-test-isolation=process is enabled and defaults to 1 when --experimental-test-isolation=none is used.

Before merging, I want to add some tests but haven't come up with a good approach yet. Here's what I aim to test:

• When --experimental-test-isolation=process is enabled, verify that NODE_TEST_WORKER_ID ranges from 1 to N.
• When --experimental-test-isolation=none is used, ensure that NODE_TEST_WORKER_ID is set to 1.

Any suggestions on how to create such tests would be greatly appreciated!

[nodejs-github-bot]

Review requested:

• [ ] @nodejs/test_runner

[JakobJingleheimer]

Regarding the tests, I think you'll want to use spawnPromisified. See test-cjs-esm-warn.js

[JakobJingleheimer]

I'm concerned this has too much cross-over with threadId, for which the lack of sequentiality guarantee is likely reasoned—sequentiality likely would have been trivial to facilitate. I don't know who within our collaborators has that context though (maybe Anna?).

If there is a good reason behind making threadId non-sequential, I suspect this will be subject to that same rationale.

If there is not a particular rationale for the non-sequentiality of threadId, perhaps it would be better to guarantee that instead of creating a quasi-redundant mechanism.

The problem described in the cited issue is well handled via threadId when a complementary design is applied (which, IMO, is better than the attempted design the OP is trying to facilitate—which may well have been necessary in the environment in which it was created).

If we're merely trying to facilitate migration, I think we should first consider that point and whether there is a way to ease that migration without redundancy.

Without seeing the OP's code, I can only imagine what it looks like, but I suspect perhaps less than 20% would need to be adjusted. Likely the biggest hindrance is knowledge, which can be provided (ex a Learn article).

[0hmX]

@JakobJingleheimer Thanks for helping with the test creation!
@pmarchini I've added the test and updated the documentation—please take a look when you have a chance.

[0hmX]

(particularly watch mode + process isolation)?

yes I have tested, its working fine! both for process and none

[0hmX]

@cjihrig I’ve made the updates you requested. Please take a moment to review them!

[pmarchini]

(particularly watch mode + process isolation)?

yes I have tested, its working fine! both for process and none

Hey @cu8code, regarding this: could you please also add some tests?

[0hmX]

:love_you_gesture: @cjihrig @pmarchini review

[codecov[bot]]

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 87.99%. Comparing base (4ee87b8) to head (50ae060). Report is 211 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #56091      +/-   ##
==========================================
- Coverage   88.00%   87.99%   -0.01%     
==========================================
  Files         656      656              
  Lines      189000   189084      +84     
  Branches    35995    35991       -4     
==========================================
+ Hits       166320   166379      +59     
- Misses      15840    15869      +29     
+ Partials     6840     6836       -4     

Files with missing lines	Coverage Δ
lib/internal/test_runner/runner.js	89.06% <100.00%> (+0.04%)	:arrow_up:

... and 53 files with indirect coverage changes

[pmarchini]

Hey @cu8code, commit linter is failing, could you please fix it? ☺️

[cjihrig]

@cu8code are you still working on this? If not, we should close this PR so that other folks can pick up the work.