test_runner: allow running test files in single process#51579

MoLow · 2024-01-27T19:51:38Z

Fixes: #51548

this PR is still missing tests & documentation

nodejs-github-bot · 2024-01-27T19:51:43Z

Review requested:

@nodejs/test_runner

lib/internal/test_runner/runner.js

benjamingr

WIP LGTM

aduh95 · 2024-01-29T18:09:47Z

doc/api/cli.md

+### `--experimental-test-isolation`
+
+<!-- YAML
+added:
+ - REPLACEME
+-->
+
+When running tests with the `node:test` module,
+each test file is run in its own process.
+running `--experimental-test-isolation=none` will run all
+files in the same process.


Could we maybe make it a feature reserved to run()? We can always add the flag later, but it seems to me better if we can avoid adding the flag until we know we want to keep the feature along.

I prefer to keep the flag as well since I believe this feature makes a lot of sense. I have seen a lot of use cases justifying this:
typescript, which needs to re-compile per each swpaned process
setup, for example initializing a server that has a penalty for running again and again per process
and many other variations of this tradeoff

test/fixtures/test-runner/no-isolation/a.test.js

test/fixtures/test-runner/no-isolation/b.test.js

github-actions · 2024-01-30T07:31:10Z

The notable-changePRs with changes that should be highlighted in changelogs. label has been added by @MoLow.

Please suggest a text for the release notes if you'd like to include a more detailed summary, then proceed to update the PR description with the text or a link to the notable change suggested text comment. Otherwise, the commit will be placed in the Other Notable Changes section.

doc/api/cli.md

rluvaton · 2024-02-05T10:33:43Z

Could you please add a test for before/after all hooks and make sure they are not executed multiple times for each file?

Couple of thoughts:

I think in case of no isolation we should set the --test-concurrency default value to be 1

If there is concurrency, should it run in different process or same one?

src/node_options.cc

rluvaton · 2024-02-21T20:17:14Z

Can you add a test that we still capture the stdout and sterr correctly?

MoLow · 2024-02-27T18:12:06Z

I'v ended up adding a single process as an entry point for importing all files - this way we support features such as coverage, watch mode, timeout etc. @nodejs/test_runner please take a look.
@rluvaton are there still tests you think should be added?

rluvaton · 2024-02-27T18:33:01Z

doc/api/test.md

 number. If a nullish value is provided, each process gets its own port,
 incremented from the primary's `process.debugPort`.
 **Default:**`undefined`.
+*`isolation`{string} If `'process'`, each test file is run in


Suggested change
*`isolation`{string} If `'process'`, each test file is run in
*`isolation`{'process' | 'none'} If `'process'`, each test file is run in

rluvaton · 2024-02-27T18:33:12Z

@rluvaton are there still tests you think should be added?

The tests that I wrote in my prev comment, the implementation detail should not affect the test cases

Also, there are some thoughts regarding concurrency that seem like you missed

MoLow · 2024-02-27T18:35:25Z

Also, there are some thoughts regarding concurrency that seem like you missed

can you elaborate?

rluvaton · 2024-02-27T18:56:19Z

I think in case of no isolation we should set the --test-concurrency default value to be 1

Having concurrent test can lead to flaky tests when mutating some global state

If there is concurrency, should it run in different process or same one?

Because it can cause flakiness, I would assume concurrency will run in different processes each process won't isolation

cjihrig · 2024-03-26T16:17:07Z

I think it makes sense to document that --test-concurrency will be 1 when running everything in the same process.

@MoLow once this lands, do you think it will be feasible to make --test-only only necessary when running multiple processes? It seems like it to me, but I haven't reviewed this code yet (there are conflicts). If we could manage that, I think it would be a huge win.

MoLow · 2024-03-27T12:29:23Z

@MoLow once this lands, do you think it will be feasible to make --test-only only necessary when running multiple processes? It seems like it to me, but I haven't reviewed this code yet (there are conflicts). If we could manage that, I think it would be a huge win.

Yes, I think that will be possible. Il try getting back to this PR this week

adrian-85 · 2024-05-02T13:38:39Z

Having concurrent test can lead to flaky tests when mutating some global state

I strongly feel this is the responsibility of the test author to ensure they do not create dependencies between tests, and properly manage variables in the global context. I don't think node should have to baby-sit anyone.

Jest allows for certain globals, and they make the same very clear in their docs.

cjihrig · 2024-05-10T22:49:18Z

@MoLow are you still planning to work on this? If not, would you mind if I took a shot at it. I'd really like to see this functionality land for the possibilities of a performance boost and avoiding the --test-only flag when isolation is disabled.

MoLow · 2024-05-11T18:25:59Z

@cjihrig please go ahead 👑

nodejs-github-bot added lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Jan 27, 2024

MoLow force-pushed the experimental-test-isolation branch from 88d0d08 to 3c85f9aCompare January 27, 2024 19:56

MoLow added the test_runner Issues and PRs related to the test runner subsystem. label Jan 27, 2024

aduh95 reviewed Jan 27, 2024
View reviewed changes

lib/internal/test_runner/runner.js Outdated Show resolvedHide resolved

aduh95 reviewed Jan 27, 2024
View reviewed changes

lib/internal/test_runner/runner.js Outdated Show resolvedHide resolved

benjamingr reviewed Jan 28, 2024
View reviewed changes

MoLow force-pushed the experimental-test-isolation branch 3 times, most recently from 1fa0c69 to 244aa35Compare January 29, 2024 17:07

aduh95 reviewed Jan 29, 2024
View reviewed changes

test/fixtures/test-runner/no-isolation/a.test.js Outdated Show resolvedHide resolved

aduh95 reviewed Jan 29, 2024
View reviewed changes

test/fixtures/test-runner/no-isolation/b.test.js Outdated Show resolvedHide resolved

richardlau added the semver-minor PRs that contain new features and should be released in the next minor version. label Jan 29, 2024

MoLow force-pushed the experimental-test-isolation branch from 244aa35 to d9fbc3aCompare January 30, 2024 07:20

MoLow added the notable-change PRs with changes that should be highlighted in changelogs. label Jan 30, 2024

aduh95 reviewed Jan 30, 2024
View reviewed changes

doc/api/cli.md Outdated Show resolvedHide resolved

MoLow force-pushed the experimental-test-isolation branch from d9fbc3a to e18b7b9Compare January 31, 2024 15:12

rluvaton reviewed Feb 5, 2024
View reviewed changes

src/node_options.cc Outdated Show resolvedHide resolved

test_runner: allow running test files in single process
3ee9d01

MoLow force-pushed the experimental-test-isolation branch from e18b7b9 to 3ee9d01Compare February 27, 2024 18:10

rluvaton reviewed Feb 27, 2024
View reviewed changes

cjihrig mentioned this pull request May 6, 2024
having --test and --test-only flags increase prototyping time #47945
Closed

MoLow closed this May 24, 2024

MoLow deleted the experimental-test-isolation branch May 24, 2024 09:01

MoLow restored the experimental-test-isolation branch May 24, 2024 09:01

cjihrig mentioned this pull request Jul 18, 2024
test_runner: support running tests in process #53927
Closed

	*`isolation`{string} If `'process'`, each test file is run in
	*`isolation`{'process' \| 'none'} If `'process'`, each test file is run in

Uh oh!

test_runner: allow running test files in single process#51579

test_runner: allow running test files in single process #51579

Uh oh!

Conversation

MoLow commented Jan 27, 2024

Uh oh!

nodejs-github-bot commented Jan 27, 2024

Uh oh!

Uh oh!

Uh oh!

benjamingr left a comment

Choose a reason for hiding this comment

Uh oh!

aduh95Jan 29, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MoLowJan 30, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

github-actionsbot commented Jan 30, 2024

Uh oh!

Uh oh!

rluvaton commented Feb 5, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

rluvaton commented Feb 21, 2024

Uh oh!

MoLow commented Feb 27, 2024

Uh oh!

rluvatonFeb 27, 2024

Choose a reason for hiding this comment

Uh oh!

rluvaton commented Feb 27, 2024

Uh oh!

MoLow commented Feb 27, 2024

Uh oh!

rluvaton commented Feb 27, 2024

Uh oh!

cjihrig commented Mar 26, 2024

Uh oh!

MoLow commented Mar 27, 2024

Uh oh!

adrian-85 commented May 2, 2024

Uh oh!

cjihrig commented May 10, 2024

Uh oh!

MoLow commented May 11, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

aduh95Jan 29, 2024•
edited
Loading

rluvaton commented Feb 5, 2024•
edited
Loading