test_runner: introduce NODE_TEST_WORKER_ID for improved concurrent te… #56091

cu8code · 2024-11-30T19:36:22Z

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 · 2024-11-30T19:36:27Z

Review requested:

@nodejs/test_runner

mcollina

lgtm

pmarchini

Hey @cu8code, thanks for the PR.
I'm requesting changes since we're still missing tests.
Regarding the implementation, LGTM.

Another important thing: to make this new feature available to users, we also need to update the documentation with this behavior 🚀

JakobJingleheimer · 2024-12-01T16:29:12Z

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

JakobJingleheimer · 2024-12-01T17:49:17Z

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).

cu8code · 2024-12-05T15:34:43Z

@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.

cjihrig

Left a few comments. Have you verified that this works with watch mode (particularly watch mode + process isolation)?

cjihrig · 2024-12-05T15:38:46Z

doc/api/test.md

@@ -466,6 +466,15 @@ each other in ways that are not possible when isolation is enabled. For example,
 if a test relies on global state, it is possible for that state to be modified
 by a test originating from another file.

+## Environment Variables
+
+### `NODE_TEST_WORKER_ID`


This should be documented in doc/api/cli.md instead.

cjihrig · 2024-12-05T15:40:35Z

test/fixtures/test-runner/NODE_TEST_WORKER_ID/1.test.js

@@ -0,0 +1,7 @@
+// This file tests the `NODE_TEST_WORKER_ID` feature.
+// The test logic is implemented in `test/parallel/test-runner-cli.js`.


Can you remove this line. If things get moved around, this will almost certainly be forgotten about.

cjihrig · 2024-12-05T15:40:45Z

test/fixtures/test-runner/NODE_TEST_WORKER_ID/1.test.js

+
+test('test1', t => {
+    console.log('NODE_TEST_WORKER_ID', process.env.NODE_TEST_WORKER_ID)
+});


Please add a newline at the end of the file.

cjihrig · 2024-12-05T15:41:20Z

test/fixtures/test-runner/NODE_TEST_WORKER_ID/1.test.js

@@ -0,0 +1,7 @@
+// This file tests the `NODE_TEST_WORKER_ID` feature.


Please rename the directory from NODE_TEST_WORKER_ID to worker-id.

@cu8code this still needs to be addressed 😁

cjihrig · 2024-12-05T15:41:43Z

test/fixtures/test-runner/NODE_TEST_WORKER_ID/2.test.js

@@ -0,0 +1,7 @@
+// This file tests the `NODE_TEST_WORKER_ID` feature.


The same comments apply to this file.

cjihrig · 2024-12-05T15:41:53Z

test/fixtures/test-runner/NODE_TEST_WORKER_ID/3.test.js

@@ -0,0 +1,7 @@
+// This file tests the `NODE_TEST_WORKER_ID` feature.


The same comments apply to this file.

cjihrig · 2024-12-05T15:42:07Z

test/fixtures/test-runner/NODE_TEST_WORKER_ID/4.test.js

@@ -0,0 +1,7 @@
+// This file tests the `NODE_TEST_WORKER_ID` feature.


I think three files is enough for this. I would drop this file.

cjihrig · 2024-12-05T15:42:59Z

test/parallel/test-runner-cli.js

@@ -428,3 +428,31 @@ for (const isolation of ['none', 'process']) {
  assert.strictEqual(child.status, 0);
  assert.strictEqual(child.signal, null);
 }
+
+{


Please move these new tests to a new file - test/parallel/test-runner-worker-id.js.

cu8code · 2024-12-05T15:54:46Z

(particularly watch mode + process isolation)?

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

cu8code · 2024-12-05T17:18:40Z

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

pmarchini · 2024-12-05T17:19:52Z

test/parallel/test-runner-worker-id.js

+const fixtures = require('../common/fixtures');
+const testFixtures = fixtures.path('test-runner');
+
+{


Could you please implement the tests using node:test ?
Here is an example : https://github.com/nodejs/node/blob/main/test/parallel/test-runner-run.mjs

pmarchini · 2024-12-05T17:22:57Z

(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?

test/fixtures/test-runner/NODE_TEST_WORKER_ID/1.test.js

doc/api/cli.md

…st execution 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.

cu8code · 2024-12-05T20:46:43Z

🤟 @cjihrig @pmarchini review

cjihrig · 2024-12-05T21:38:23Z

test/parallel/test-runner-worker-id.js

+});
+
+
+test('testing with isolation enabled in watch mode', async () => {


The watch mode tests added here have at least two problems:

They don't actually test watch mode beyond passing the --watch flag. To properly test watch mode, you have to trigger the restart functionality.

These tests will almost certainly be flaky in the CI. Waiting for a second, killing the child process, and expecting it to have run successfully probably works locally, but the CI machines can be very slow. I don't think we want to have a timeout at all.

I think these need to be refactored so that:

The test runs once.

Once the expected output is seen, a source file is modified (we don't want to modify the fixtures files directly either) to trigger a restart.

The test runs again. We need the output to be the same both times.

pmarchini · 2024-12-06T07:52:46Z

test/parallel/test-runner-worker-id.js

+const fixtures = require('../common/fixtures');
+const testFixtures = fixtures.path('test-runner');
+
+test('testing with isolation enabled', () => {


nit: I suggest being more expressive about what we are testing.

For example, describe('test runner worker-id'), followed by it('should set NODE_TEST_WORKER_ID in isolation x'), etc.

rluvaton

LGTM

codecov · 2024-12-14T21:45:28Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.99%. Comparing base (4ee87b8) to head (50ae060).
Report is 112 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%)`	⬆️

... and 53 files with indirect coverage changes

pmarchini · 2024-12-14T22:13:51Z

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

MoLow · 2024-12-16T22:49:42Z

doc/api/cli.md

@@ -3306,6 +3306,12 @@ If `value` equals `'child'`, test reporter options will be overridden and test
 output will be sent to stdout in the TAP format. If any other value is provided,
 Node.js makes no guarantees about the reporter format used or its stability.

+### `NODE_TEST_WORKER_ID`


I'm not sure this is the correct place for this. this is a list of env vars node accepts, not env vars it exposes

MoLow · 2024-12-16T22:51:17Z

lib/internal/test_runner/runner.js

      return SafePromiseAllSettledReturnVoid(testFiles, (path) => {
-        const subtest = runTestFile(path, filesWatcher, opts);
+        const subtest = runTestFile(path, filesWatcher, opts, workerId);


if I have 8 cpus and 16 tests, I would expect each worker to be reused twice, but this implementation will not reuse worker ids, I think that is misleading

nodejs-github-bot added needs-ci PRs that need a full CI run. test_runner Issues and PRs related to the test runner subsystem. labels Nov 30, 2024

mcollina approved these changes Dec 1, 2024

View reviewed changes

pmarchini requested changes Dec 1, 2024

View reviewed changes

blimmer mentioned this pull request Dec 1, 2024

Expose an id for concurrent test runners (like JEST_WORKER_ID) #55842

Open

cu8code force-pushed the 55842 branch from 9e1d701 to aa6e400 Compare December 5, 2024 15:26

cjihrig reviewed Dec 5, 2024

View reviewed changes

cu8code force-pushed the 55842 branch 2 times, most recently from c722258 to 391e805 Compare December 5, 2024 17:15

pmarchini reviewed Dec 5, 2024

View reviewed changes

cjihrig reviewed Dec 5, 2024

View reviewed changes

test/fixtures/test-runner/NODE_TEST_WORKER_ID/1.test.js Outdated Show resolved Hide resolved

doc/api/cli.md Outdated Show resolved Hide resolved

doc/api/cli.md Outdated Show resolved Hide resolved

cu8code force-pushed the 55842 branch from 391e805 to 392f8f7 Compare December 5, 2024 20:35

cu8code force-pushed the 55842 branch from 392f8f7 to 50ae060 Compare December 5, 2024 20:38

cjihrig reviewed Dec 5, 2024

View reviewed changes

pmarchini reviewed Dec 6, 2024

View reviewed changes

rluvaton approved these changes Dec 14, 2024

View reviewed changes

MoLow reviewed Dec 16, 2024

View reviewed changes

jasnell approved these changes Dec 28, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test_runner: introduce NODE_TEST_WORKER_ID for improved concurrent te… #56091

test_runner: introduce NODE_TEST_WORKER_ID for improved concurrent te… #56091

cu8code commented Nov 30, 2024

nodejs-github-bot commented Nov 30, 2024

mcollina left a comment

pmarchini left a comment

JakobJingleheimer commented Dec 1, 2024

JakobJingleheimer commented Dec 1, 2024 •

edited

Loading

cu8code commented Dec 5, 2024

cjihrig left a comment

cjihrig Dec 5, 2024

cjihrig Dec 5, 2024

cjihrig Dec 5, 2024

cjihrig Dec 5, 2024

pmarchini Dec 5, 2024

cjihrig Dec 5, 2024

cjihrig Dec 5, 2024

cjihrig Dec 5, 2024

cjihrig Dec 5, 2024

cu8code commented Dec 5, 2024

cu8code commented Dec 5, 2024

pmarchini Dec 5, 2024

pmarchini commented Dec 5, 2024

cu8code commented Dec 5, 2024

cjihrig Dec 5, 2024

pmarchini Dec 6, 2024

rluvaton left a comment

codecov bot commented Dec 14, 2024

pmarchini commented Dec 14, 2024

MoLow Dec 16, 2024

MoLow Dec 16, 2024

		@@ -0,0 +1,7 @@
		// This file tests the `NODE_TEST_WORKER_ID` feature.
		// The test logic is implemented in `test/parallel/test-runner-cli.js`.

		});


		test('testing with isolation enabled in watch mode', async () => {

test_runner: introduce NODE_TEST_WORKER_ID for improved concurrent te… #56091

Are you sure you want to change the base?

test_runner: introduce NODE_TEST_WORKER_ID for improved concurrent te… #56091

Conversation

cu8code commented Nov 30, 2024

nodejs-github-bot commented Nov 30, 2024

mcollina left a comment

Choose a reason for hiding this comment

pmarchini left a comment

Choose a reason for hiding this comment

JakobJingleheimer commented Dec 1, 2024

JakobJingleheimer commented Dec 1, 2024 • edited Loading

cu8code commented Dec 5, 2024

cjihrig left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cu8code commented Dec 5, 2024

cu8code commented Dec 5, 2024

Choose a reason for hiding this comment

pmarchini commented Dec 5, 2024

cu8code commented Dec 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rluvaton left a comment

Choose a reason for hiding this comment

codecov bot commented Dec 14, 2024

Codecov Report

pmarchini commented Dec 14, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JakobJingleheimer commented Dec 1, 2024 •

edited

Loading