test_runner: add global setup and teardown functionality by pmarchini · Pull Request #57438 · nodejs/node

pmarchini · 2025-03-13T10:29:58Z

I'm opening this PR as a draft to gather some feedback regarding global setup/teardown.

The idea is to leverage the new configuration file to allow users to manage this scenario.

Personally, I'm currently using a "wrapper" file that directly runs the run function, preceded and followed by setup/teardown when it's necessary to launch containers or other resources.

At the moment, I've added support for a possible shared context object between the two hooks.

However, I'm not fully convinced about this approach.

I think some crucial improvements might include adding events into the test stream to notify about the success or failure of global setup and teardown.

Note: this PR is still missing run new option documentation and test to cover the feature via run function.

nodejs-github-bot · 2025-03-13T10:30:06Z

Review requested:

@nodejs/config
@nodejs/test_runner

pmarchini · 2025-03-13T12:03:35Z

Although it's still a draft, would anyone be willing to spend some time providing feedback / ideas? @nodejs/test_runner

Thaaaanks 🚀

cjihrig · 2025-03-13T12:54:48Z

I looked at this very quickly, so I may have missed something. There are some additional things that I think need to be tested:

Running without --test. If someone wants to run a single file that contains tests like node foo.js, that should work like the other test runner flags do.
This should verify that no user code can run before these new hooks. This is especially important for describe(), which builds the suites before the test runner starts executing things.
How does this interact with --import and --require since this is duplicating functionality that they provide.
How does this interact with watch mode.

pmarchini · 2025-03-13T14:30:02Z

Hey @cjihrig, thanks for the feedback!

Point by point:

I think it would make sense to limit the functionality to --test only or direct run invocation.
I agree, I will cover that case.
Currently, --require and --import clearly act at startup, and in the case of isolation process, they cause a rerun for each individual test file.
I wonder if, from a developer experience perspective, it would make sense to evaluate whether overlapping multiple flags could lead to unintended side effects for the user.
At the same time, having potential overlap seems reasonable to me...what do you think?
Regarding watch, I'm torn. Initially, I was thinking of not supporting it, but reconsidering, especially in scenarios involving container creation/teardown or similar cases, it seems important to provide this functionality.
My only concern is whether it would be possible to break down the implementation incrementally (also to ease the review process).

cjihrig · 2025-03-13T14:52:40Z

I think it would make sense to limit the functionality to --test only or direct run invocation.

Regarding watch, I'm torn.

I feel pretty strongly that these need to be supported. For people working on the test runner, I understand that it can be annoying to support the various modes of operation, but that isn't a good reason not to do it. IMO, it only makes sense to omit something if there is a good reason from the end user's perspective (for example --test-force-exit not being compatible with watch mode).

Regarding interop with --require/--import, I don't really have strong feelings, but I think we should nail down what the behavior will be. If those always run before this new functionality, that's totally fine. I think we should have a test for the happy path. We probably should test the case where someone uses one of these flags and invokes the test runner from there while also having a global setup.

pmarchini · 2025-03-13T15:21:37Z

Sounds good to me, I see your reasons and you obviously have more vision and experience than me so I'm more than glad to follow your suggestions! 🚀
I'll update the PR ASAP

pmarchini · 2025-03-27T12:31:27Z

hey @cjihrig, @atlowChemi

I've added the following tests :

Basic setup and teardown functionality
Setup-only modules
Teardown-only modules
Multiple test files with shared setup/teardown
Context sharing between setup and teardown
Async setup and teardown
Error handling in setup
TypeScript support for setup/teardown functions
ESM module support for setup/teardown functions
Ensuring setup runs only once per test run
Ensuring teardown runs only once per test run
Interop with --require flag (required module before globalSetup)
Interop with --import flag (behavior differs between run modes)
Tests in imported modules with setup/teardown
Watch mode interaction with global setup/teardown
Direct invocation vs. --test flag behavior
Edge cases when combining multiple flags

My main concern at the moment is this change in behaviour:
https://github.com/nodejs/node/pull/57438/files#diff-196bc4ec67eaf2b311f953b5282702f42d7c55a5e6ab5599fdf83f51fdf3c23cR197-R210

it('should include test type in enqueue, dequeue events',async(t)=>{conststream=awaitrun({files: [join(testFixtures,'default-behavior/test/suite_and_test.cjs')],});t.plan(4);stream.on('test:enqueue',common.mustCall((data)=>{if(data.name==='this is a suite'){t.assert.strictEqual(data.type,'suite');}if(data.name==='this is a test'){t.assert.strictEqual(data.type,'test');}},3));// TODO(pmarchini): this should be 2 but after `await root.harness.waitForGlobalSetup()` it's 3

I'd really appreciate any feedback or suggestions regarding how I've implemented the feature 😁

marco-ippolito · 2025-03-27T13:56:36Z


 testResources.set(reporterScope.asyncId(), reporterScope);

+async function setupGlobalSetupTeardownFunctions(globalSetupPath, cwd) {


jsdoc would be nice

pmarchini · 2025-03-27T14:06:33Z

Looking after the failing test!

cjihrig

Left a round of comments. I'll look in more detail once they are addressed and the CI is passing.

cjihrig · 2025-03-27T14:45:14Z

+const esmLoader = require('internal/modules/esm/loader');
+const { kEmptyObject } = require('internal/util');
+const { validateFunction } = require('internal/validators');
+const { getOptionValue } = require('internal/options');


We shouldn't be using this function in this file. All of the getOptionValue() calls should have already happened in parseCommandLine(), and the value should already be available here.

cjihrig · 2025-03-27T14:49:16Z


 testResources.set(reporterScope.asyncId(), reporterScope);

+async function setupGlobalSetupTeardownFunctions(globalSetupPath, cwd) {


All of this might fit better in utils.js, and called from parseCommandLine(), similar to how some other flags like --test-name-pattern are processed.

Regarding having it as part of utils.js: I agree, it definitely makes more sense.
As for parseCommandLine: at the moment, it is not "cwd-aware", and I'm not sure whether it should have that dependency.

FWIW, it may need to be cwd aware since it already deals with loading of reporters.

codecov · 2025-03-27T15:14:22Z

Codecov Report

❌ Patch coverage is 98.66667% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 90.25%. Comparing base (67786c1) to head (b4c1c12).
⚠️ Report is 1145 commits behind head on main.

Files with missing lines	Patch %	Lines
lib/internal/test_runner/runner.js	92.85%	1 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #57438 +/- ##
==========================================
+ Coverage 90.21% 90.25% +0.03% 
==========================================
Files 630 630 Lines 185524 185737 +213 Branches 36387 36413 +26 ==========================================
+ Hits 167378 167629 +251 + Misses 11037 10999 -38 
Partials 7109 7109

Files with missing lines	Coverage Δ
lib/internal/test_runner/harness.js	`93.35% <100.00%> (+0.43%)`	⬆️
lib/internal/test_runner/utils.js	`60.46% <100.00%> (+1.94%)`	⬆️
src/node_options.cc	`85.23% <100.00%> (+0.01%)`	⬆️
src/node_options.h	`98.88% <ø> (ø)`
lib/internal/test_runner/runner.js	`89.19% <92.85%> (+0.03%)`	⬆️

... and 41 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

mcollina

lgtm

nodejs-github-bot · 2025-03-31T08:12:50Z

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

cjihrig · 2025-04-02T14:27:59Z

+can be used to setup global state or fixtures for tests. This is useful for preparing resources or setting up
+shared state that is required by multiple tests.
+
+This module should export either:


"either" seems like the user can only specify one or the other.

cjihrig · 2025-04-02T14:50:11Z

 function createProcessEventHandler(eventName, rootTest) {
 return (err) => {
- if (rootTest.harness.bootstrapPromise) {
+ if (rootTest.harness.bootstrapPromise || rootTest.harness.globalSetupPromise) {


It feels wrong to need to check for both of these. bootstrapPromise should be the only thing we need to check in order to know if the test runner is still bootstrapping. bootstrapPromise should always be truthy when globalSetupPromise is truthy because the global setup hook should be considered part of the bootstrap process IMO.

I agree, my biggest concern at the moment is that we're manually setting harness.bootstrapPromise to different values based on the type of run (isolation/watch/lazy bootstrap), and for this reason, I have the impression that it makes it more complex to understand and maintain the code.
node/lib/internal/test_runner/runner.js
Line 725 in 657f818
root.harness.bootstrapPromise=null;

node/lib/internal/test_runner/runner.js
Line 738 in 657f818
root.harness.bootstrapPromise=null;

node/lib/internal/test_runner/runner.js
Line 753 in 657f818
root.harness.bootstrapPromise=promise;

node/lib/internal/test_runner/harness.js
Line 281 in 657f818
globalRoot.harness.bootstrapPromise=globalOptions.setup(globalRoot.reporter);

node/lib/internal/test_runner/harness.js
Line 290 in 657f818
subtest.root.harness.bootstrapPromise=null;
Having both bootstrap and globalSetup allows, from a code perspective, the decoupling of the existing bootstrapPromise logic (shared across the different run modes), with the goal of reducing it to just two distinct flows where the global setup is handled: lazyBootstrap and run bootstrap.
This is the rationale behind the implementation.
Note: I might be missing something, like an easy way to centralise the logic in a single place

I don't think there is anything wrong with having a separate promise. I was referring to the lifetime of the promises. If I am understanding the changes, the global setup is considered part of the bootstrapping process. So, the global setup should always be finished by the time the bootstrapping is considered finished (if it isn't, then things are likely to go wrong). I don't want to get to a state where we have a bunch of different promises that we need to check all over the place.

I've just pushed a refactor to take advantage of rootTest.harness.bootstrapPromise to encapsulate the global setup while reducing the feature's footprint.
WDYT?

cjihrig

LGTM, thanks!

mcollina

lgtm

…setup

nodejs-github-bot · 2025-04-14T13:06:30Z

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

nodejs-github-bot · 2025-04-16T17:51:09Z

Landed in cb5f671

PR-URL: #57438 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com> Reviewed-By: Chemi Atlow <chemi@atlow.co.il>

richardlau · 2025-09-19T16:17:51Z

This doesn't land cleanly on v22.x-staging so a manual backport will be necessary if this is to be released there.

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. 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 Mar 13, 2025

atlowChemi reviewed Mar 17, 2025
View reviewed changes

Comment threaddoc/api/cli.md Outdated

atlowChemi reviewed Mar 17, 2025
View reviewed changes

Comment threadsrc/node_options.cc Outdated

pmarchini marked this pull request as ready for review March 27, 2025 12:31

marco-ippolito reviewed Mar 27, 2025
View reviewed changes

Comment threaddoc/api/cli.md Outdated

marco-ippolito reviewed Mar 27, 2025
View reviewed changes

Comment threaddoc/api/cli.md Outdated

marco-ippolito reviewed Mar 27, 2025
View reviewed changes

Comment threaddoc/api/cli.md Outdated

marco-ippolito reviewed Mar 27, 2025
View reviewed changes

Comment threadlib/internal/test_runner/harness.js Outdated

cjihrig reviewed Mar 27, 2025
View reviewed changes

mcollina approved these changes Mar 31, 2025
View reviewed changes

cjihrig reviewed Apr 2, 2025
View reviewed changes

Comment threadtest/fixtures/test-runner/global-setup-teardown/basic-setup-teardown.ts Outdated

cjihrig reviewed Apr 2, 2025
View reviewed changes

Comment threadtest/fixtures/test-runner/global-setup-teardown/basic-setup-teardown.js Outdated

cjihrig reviewed Apr 2, 2025
View reviewed changes

Comment threadtest/fixtures/test-runner/global-setup-teardown/async-setup-teardown.js Outdated

cjihrig reviewed Apr 2, 2025
View reviewed changes

cjihrig approved these changes Apr 4, 2025
View reviewed changes

mcollina approved these changes Apr 4, 2025
View reviewed changes

pmarchini added 2 commits April 9, 2025 20:46

fixup! test_runner: handle process exit events in harness and global …
0ae1c09
…setup

test_runner: remove exit event listeners in harness
46a2340

pmarchini force-pushed the test_runner/49732-global-setup-teardown branch from 3ddcf93 to 46a2340Compare April 9, 2025 18:47

This comment was marked as outdated.
Sign in to view

test: forward process envs
44b4eb0

This comment was marked as outdated.
Sign in to view

pmarchini added 2 commits April 13, 2025 21:59

test_runner: remove shared context between global setup and teardown
4a613be

test_runner: update documentation
19ca427

This comment was marked as outdated.
Sign in to view

pmarchini commented Apr 14, 2025
View reviewed changes

Comment threadtest/fixtures/test-runner/global-setup-teardown/context-sharing.js Outdated

test_runner: remove fixture
b4c1c12

pmarchini requested review from MoLow and atlowChemi April 16, 2025 07:54

atlowChemi approved these changes Apr 16, 2025
View reviewed changes

atlowChemi added the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 16, 2025

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 16, 2025

nodejs-github-bot merged commit cb5f671 into nodejs:mainApr 16, 2025

marco-ippolito added the semver-minor PRs that contain new features and should be released in the next minor version. label Apr 23, 2025

RafaelGSS mentioned this pull request May 1, 2025
2025-05-06, Version 24.0.0 (Current) #57609
Merged

aduh95 added the test_runner Issues and PRs related to the test runner subsystem. label Jul 21, 2025

richardlau added the backport-requested-v22.x PRs awaiting manual backport to the v22.x-staging branch. label Sep 19, 2025


		testResources.set(reporterScope.asyncId(), reporterScope);

		async function setupGlobalSetupTeardownFunctions(globalSetupPath, cwd) {

Uh oh!

Uh oh!

Conversation

pmarchini commented Mar 13, 2025

Uh oh!

nodejs-github-bot commented Mar 13, 2025• edited by pmarchini Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pmarchini commented Mar 13, 2025

Uh oh!

cjihrig commented Mar 13, 2025• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pmarchini commented Mar 13, 2025• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cjihrig commented Mar 13, 2025

Uh oh!

pmarchini commented Mar 13, 2025

Uh oh!

Uh oh!

Uh oh!

pmarchini commented Mar 27, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pmarchini commented Mar 27, 2025

Uh oh!

cjihrig left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecovBot commented Mar 27, 2025• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Mar 31, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pmarchiniApr 3, 2025• 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cjihrig left a comment

Choose a reason for hiding this comment

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

This comment was marked as outdated.

This comment was marked as outdated.

nodejs-github-bot commented Mar 13, 2025•
edited by pmarchini
Loading

cjihrig commented Mar 13, 2025•
edited
Loading

pmarchini commented Mar 13, 2025•
edited
Loading

codecovBot commented Mar 27, 2025•
edited
Loading

pmarchiniApr 3, 2025•
edited
Loading