Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

test_runner: allow running test files in single process #51579

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

test_runner: allow running test files in single process #51579

wants to merge 1 commit into from
+164 −39

Conversation

MoLow
Copy link
Member

@MoLow MoLow commented Jan 27, 2024

Fixes: #51548

this PR is still missing tests & documentation

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/test_runner

@nodejs-github-bot 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 MoLow force-pushed the experimental-test-isolation branch from 88d0d08 to 3c85f9a Compare January 27, 2024 19:56
benjamingr
benjamingr reviewed Jan 28, 2024
View reviewed changes
Copy link
Member

@benjamingr benjamingr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WIP LGTM

@MoLow MoLow force-pushed the experimental-test-isolation branch 3 times, most recently from 1fa0c69 to 244aa35 Compare January 29, 2024 17:07
aduh95
aduh95 reviewed Jan 29, 2024
View reviewed changes
doc/api/cli.md Outdated
Comment on lines 897 to 925
### `--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.
Copy link
Contributor

@aduh95 aduh95 Jan 29, 2024 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@richardlau richardlau added the semver-minor PRs that contain new features and should be released in the next minor version. label Jan 29, 2024
@MoLow MoLow force-pushed the experimental-test-isolation branch from 244aa35 to d9fbc3a Compare January 30, 2024 07:20
@MoLow MoLow added the notable-change PRs with changes that should be highlighted in changelogs. label Jan 30, 2024
@github-actions GitHub Actions
Copy link
Contributor

The notable-change PRs 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.

aduh95
aduh95 reviewed Jan 30, 2024
View reviewed changes
doc/api/cli.md Outdated Show resolved Hide resolved
@MoLow MoLow force-pushed the experimental-test-isolation branch from d9fbc3a to e18b7b9 Compare January 31, 2024 15:12
@rluvaton
Copy link
Member

rluvaton commented Feb 5, 2024 •
edited

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?

rluvaton
rluvaton reviewed Feb 5, 2024
View reviewed changes
src/node_options.cc Outdated Show resolved Hide resolved
@rluvaton
Copy link
Member

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

@MoLow MoLow force-pushed the experimental-test-isolation branch from e18b7b9 to 3ee9d01 Compare February 27, 2024 18:10
@MoLow
Copy link
Member Author

MoLow commented Feb 27, 2024

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
rluvaton reviewed Feb 27, 2024
View reviewed changes
doc/api/test.md
@@ -1125,6 +1125,9 @@ changes:
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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@rluvaton
Copy link
Member

@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
Copy link
Member Author

MoLow commented Feb 27, 2024

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

can you elaborate?

@rluvaton
Copy link
Member

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
Copy link
Contributor

cjihrig commented Mar 26, 2024

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
Copy link
Member Author

MoLow commented Mar 27, 2024

@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
Copy link

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 cjihrig mentioned this pull request May 6, 2024
Open
@cjihrig
Copy link
Contributor

cjihrig commented May 10, 2024

@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
Copy link
Member Author

MoLow commented May 11, 2024

@cjihrig please go ahead 👑

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@benjamingr benjamingr benjamingr left review comments

@aduh95 aduh95 aduh95 left review comments

@rluvaton rluvaton rluvaton left review comments

Assignees
No one assigned
Labels
lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. notable-change PRs with changes that should be highlighted in changelogs. semver-minor PRs that contain new features and should be released in the next minor version. test_runner
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Allow to run tests in the main process
8 participants
@MoLow @nodejs-github-bot @rluvaton @cjihrig @adrian-85 @benjamingr @aduh95 @richardlau