test_runner: support running tests in process#53927

cjihrig · 2024-07-18T17:01:06Z

Opening as a draft. Some of this logic is from #53921. I also still need to finish docs and tests. I've added @MoLow as a co-author because the work in #51579 was very helpful.

This commit introduces a new --experimental-test-isolation flag that, when set to 'none', causes the test runner to execute all tests in the same process. By default, this is the main test runner process, but if watch mode is enabled, it spawns a separate process that runs all of the tests.

The default value of the new flag is 'process', which uses the existing behavior of running each test file in its own child process.

It is worth noting that when the isolation mode is 'none', globals and all other top level logic (such as top level hooks) is shared among all files.

Fixes: #51548

nodejs-github-bot · 2024-07-18T17:01:10Z

Review requested:

@nodejs/test_runner

matthewp · 2024-07-19T16:00:07Z

Has there been discussion about changing the default in a future major or is that off the table?

cjihrig · 2024-07-19T16:08:03Z

There has not been any discussion and it is not off the table.

jsumners · 2024-07-20T19:24:57Z

Please never make this the default.

nodejs-github-bot · 2024-08-18T02:25:22Z

CI: https://ci.nodejs.org/job/node-test-pull-request/61191/

cjihrig · 2024-08-18T20:40:56Z

Marking as ready for review. I still want to rebase this on #54423 when it lands (hopefully tomorrow). From some quick testing, the performance appears to be on par with node:test (no --test) flag.

codecov · 2024-08-18T21:59:25Z

Codecov Report

Attention: Patch coverage is 76.88679% with 49 lines in your changes missing coverage. Please review.

Project coverage is 87.34%. Comparing base (5376e69) to head (c96455c).
Report is 335 commits behind head on main.

Files with missing lines	Patch %	Lines
lib/internal/test_runner/runner.js	73.33%	40 Missing ⚠️
lib/internal/test_runner/utils.js	76.00%	6 Missing ⚠️
src/node_options.cc	57.14%	2 Missing and 1 partial ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #53927 +/- ## ========================================== + Coverage 87.31% 87.34% +0.02%  ========================================== Files 648 649 +1 Lines 182365 182544 +179 Branches 34985 35033 +48 ========================================== + Hits 159232 159438 +206 + Misses 16399 16371 -28 - Partials 6734 6735 +1

Files with missing lines	Coverage Δ
lib/internal/main/test_runner.js	`100.00% <100.00%> (ø)`
lib/internal/test_runner/harness.js	`92.30% <100.00%> (+2.39%)`	⬆️
lib/internal/test_runner/test.js	`98.36% <100.00%> (+0.01%)`	⬆️
src/env-inl.h	`96.55% <100.00%> (-0.24%)`	⬇️
src/node_options.h	`98.18% <100.00%> (+0.01%)`	⬆️
src/node_options.cc	`87.82% <57.14%> (-0.32%)`	⬇️
lib/internal/test_runner/utils.js	`62.77% <76.00%> (+1.66%)`	⬆️
lib/internal/test_runner/runner.js	`88.41% <73.33%> (+3.97%)`	⬆️

... and 28 files with indirect coverage changes

doc/api/cli.md

doc/api/test.md

lib/internal/test_runner/runner.js

mcollina

lgtm

This commit updates the way the test runner computes inherited hooks. Instead of computing them when the Test/Suite is constructed, they are now computed just prior to running the Test/Suite. The reason is because when multiple test files are run in the same process, it is possible for the inherited hooks to change as more files are loaded.

cjihrig · 2024-08-21T12:56:21Z

CI is green. This just needs a reapproval.

mcollina

lgtm

nodejs-github-bot · 2024-08-21T13:39:21Z

Landed in 821ffab...cc26951

This commit updates the way the test runner computes inherited hooks. Instead of computing them when the Test/Suite is constructed, they are now computed just prior to running the Test/Suite. The reason is because when multiple test files are run in the same process, it is possible for the inherited hooks to change as more files are loaded. PR-URL: #53927Fixes: #51548 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Matteo Collina <[email protected]>

This commit introduces a new --experimental-test-isolation flag that, when set to 'none', causes the test runner to execute all tests in the same process. By default, this is the main test runner process, but if watch mode is enabled, it spawns a separate process that runs all of the tests. The default value of the new flag is 'process', which uses the existing behavior of running each test file in its own child process. It is worth noting that when the isolation mode is 'none', globals and all other top level logic (such as top level before() and after() hooks) is shared among all files. Co-authored-by: Moshe Atlow <[email protected]> PR-URL: #53927Fixes: #51548 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Matteo Collina <[email protected]>

This commit updates the way the test runner computes inherited hooks. Instead of computing them when the Test/Suite is constructed, they are now computed just prior to running the Test/Suite. The reason is because when multiple test files are run in the same process, it is possible for the inherited hooks to change as more files are loaded. PR-URL: #53927Fixes: #51548 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Matteo Collina <[email protected]>

This commit introduces a new --experimental-test-isolation flag that, when set to 'none', causes the test runner to execute all tests in the same process. By default, this is the main test runner process, but if watch mode is enabled, it spawns a separate process that runs all of the tests. The default value of the new flag is 'process', which uses the existing behavior of running each test file in its own child process. It is worth noting that when the isolation mode is 'none', globals and all other top level logic (such as top level before() and after() hooks) is shared among all files. Co-authored-by: Moshe Atlow <[email protected]> PR-URL: #53927Fixes: #51548 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Matteo Collina <[email protected]>

Notable changes: net: * (SEMVER-MINOR) exclude ipv6 loopback addresses from server.listen (Giovanni Bucci) #54264 test_runner: * (SEMVER-MINOR) add support for coverage thresholds (Aviv Keller) #54429 * (SEMVER-MINOR) support running tests in process (Colin Ihrig) #53927 * (SEMVER-MINOR) defer inheriting hooks until run() (Colin Ihrig) #53927 PR-URL: TODO

Notable changes: net: * (SEMVER-MINOR) exclude ipv6 loopback addresses from server.listen (Giovanni Bucci) #54264 test_runner: * (SEMVER-MINOR) add support for coverage thresholds (Aviv Keller) #54429 * (SEMVER-MINOR) support running tests in process (Colin Ihrig) #53927 * (SEMVER-MINOR) defer inheriting hooks until run() (Colin Ihrig) #53927 PR-URL: #54560

This commit introduces a new --experimental-test-isolation flag that, when set to 'none', causes the test runner to execute all tests in the same process. By default, this is the main test runner process, but if watch mode is enabled, it spawns a separate process that runs all of the tests. The default value of the new flag is 'process', which uses the existing behavior of running each test file in its own child process. It is worth noting that when the isolation mode is 'none', globals and all other top level logic (such as top level before() and after() hooks) is shared among all files. Co-authored-by: Moshe Atlow <[email protected]> PR-URL: #53927Fixes: #51548 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Matteo Collina <[email protected]>

Notable changes: net: * (SEMVER-MINOR) exclude ipv6 loopback addresses from server.listen (Giovanni Bucci) #54264 src: * (SEMVER-MINOR) add JS APIs for compile cache and NODE_DISABLE_COMPILE_CACHE (Joyee Cheung) #54501 src,lib: * (SEMVER-MINOR) add performance.uvMetricsInfo (Rafael Gonzaga) #54413 test_runner: * (SEMVER-MINOR) add support for coverage thresholds (Aviv Keller) #54429 * (SEMVER-MINOR) support running tests in process (Colin Ihrig) #53927 * (SEMVER-MINOR) defer inheriting hooks until run() (Colin Ihrig) #53927 vm: * (SEMVER-MINOR) introduce vanilla contexts via vm.constants.DONT_CONTEXTIFY (Joyee Cheung) #54394 PR-URL: #54560

Notable changes: net: * (SEMVER-MINOR) exclude ipv6 loopback addresses from server.listen (Giovanni Bucci) nodejs#54264 src: * (SEMVER-MINOR) add JS APIs for compile cache and NODE_DISABLE_COMPILE_CACHE (Joyee Cheung) nodejs#54501 src,lib: * (SEMVER-MINOR) add performance.uvMetricsInfo (Rafael Gonzaga) nodejs#54413 test_runner: * (SEMVER-MINOR) add support for coverage thresholds (Aviv Keller) nodejs#54429 * (SEMVER-MINOR) support running tests in process (Colin Ihrig) nodejs#53927 * (SEMVER-MINOR) defer inheriting hooks until run() (Colin Ihrig) nodejs#53927 vm: * (SEMVER-MINOR) introduce vanilla contexts via vm.constants.DONT_CONTEXTIFY (Joyee Cheung) nodejs#54394 PR-URL: nodejs#54560

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 Jul 18, 2024

cjihrig mentioned this pull request Jul 18, 2024
Allow to run tests in the main process #51548
Closed

cjihrig force-pushed the isolation branch from b555287 to 5282a6eCompare July 19, 2024 04:32

cjihrig force-pushed the isolation branch from 5282a6e to 44d348eCompare July 31, 2024 22:18

cjihrig force-pushed the isolation branch from 44d348e to 25ef2d7Compare August 18, 2024 02:05

cjihrig added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 18, 2024

github-actionsbot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 18, 2024

cjihrig force-pushed the isolation branch 5 times, most recently from 395ddf1 to 6fa5bf3Compare August 18, 2024 20:36

cjihrig requested review from MoLow, atlowChemi, benjamingr and mcollina August 18, 2024 20:38

cjihrig marked this pull request as ready for review August 18, 2024 20:39

benjamingr approved these changes Aug 18, 2024
View reviewed changes

avivkeller reviewed Aug 18, 2024
View reviewed changes

doc/api/cli.md Outdated Show resolvedHide resolved
doc/api/test.md Outdated Show resolvedHide resolved
doc/api/test.md Outdated Show resolvedHide resolved
lib/internal/test_runner/runner.jsShow resolvedHide resolved

mcollina approved these changes Aug 19, 2024
View reviewed changes

cjihrig force-pushed the isolation branch from 6fa5bf3 to aecf0edCompare August 20, 2024 12:26

cjihrig added the commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. label Aug 21, 2024

mcollina approved these changes Aug 21, 2024
View reviewed changes

cjihrig added the commit-queue Add this label to land a pull request using GitHub Actions. label Aug 21, 2024

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Aug 21, 2024

nodejs-github-bot closed this Aug 21, 2024

cjihrig deleted the isolation branch August 21, 2024 13:39

RafaelGSS mentioned this pull request Aug 25, 2024
v22.8.0 proposal #54560
Merged

RafaelGSS mentioned this pull request Aug 30, 2024
tools: remove readability/fn_size rule #54663
Merged

mcollina mentioned this pull request Sep 1, 2024
Mocha-like root hooks for test runner #54675
Closed

targos added the dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. label Sep 21, 2024

blimmer mentioned this pull request Nov 20, 2024
Support concurrency when --experimental-test-isolation is set to 'none'#55939
Open

Uh oh!

test_runner: support running tests in process#53927

test_runner: support running tests in process #53927

Uh oh!

Conversation

cjihrig commented Jul 18, 2024

Uh oh!

nodejs-github-bot commented Jul 18, 2024

Uh oh!

matthewp commented Jul 19, 2024

Uh oh!

cjihrig commented Jul 19, 2024

Uh oh!

jsumners commented Jul 20, 2024

Uh oh!

nodejs-github-bot commented Aug 18, 2024

Uh oh!

cjihrig commented Aug 18, 2024

Uh oh!

codecovbot commented Aug 18, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

cjihrig commented Aug 21, 2024

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Aug 21, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

codecovbot commented Aug 18, 2024•
edited
Loading