test_runner: support running tests in process by cjihrig · Pull Request #53927 · nodejs/node

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: #53927 Fixes: #51548 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

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 <moshe@atlow.co.il> PR-URL: #53927 Fixes: #51548 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

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: #53927 Fixes: #51548 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

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 <moshe@atlow.co.il> PR-URL: #53927 Fixes: #51548 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

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 <moshe@atlow.co.il> PR-URL: #53927 Fixes: #51548 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

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 5282a6e Compare July 19, 2024 04:32

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

cjihrig force-pushed the isolation branch from 44d348e to 25ef2d7 Compare 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-actions bot 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 6fa5bf3 Compare 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 resolved Hide resolved

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

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

lib/internal/test_runner/runner.js Show resolved Hide resolved

mcollina approved these changes Aug 19, 2024

View reviewed changes

cjihrig force-pushed the isolation branch from 6fa5bf3 to aecf0ed Compare 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!

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!

codecov bot 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

codecov bot commented Aug 18, 2024 •

edited

Loading