test_runner: add global setup and teardown functionality #57438
Conversation
|
Review requested:
|
nodejs-github-bot
added
c++
|
Although it's still a draft, would anyone be willing to spend some time providing feedback / ideas? @nodejs/test_runner Thaaaanks 🚀 |
|
I looked at this very quickly, so I may have missed something. There are some additional things that I think need to be tested:
|
|
Hey @cjihrig, thanks for the feedback! Point by point:
|
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 Regarding interop with |
|
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! 🚀 |
|
hey @cjihrig, @atlowChemi I've added the following tests :
My main concern at the moment is this change in behaviour: it('should include test type in enqueue, dequeue events', async (t) => {
const stream = await run({
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 3I'd really appreciate any feedback or suggestions regarding how I've implemented the feature 😁 |
lib/internal/test_runner/harness.js
Outdated
|
|
||
| testResources.set(reporterScope.asyncId(), reporterScope); | ||
|
|
||
| async function setupGlobalSetupTeardownFunctions(globalSetupPath, cwd) { |
|
Looking after the failing test! |
lib/internal/test_runner/harness.js
Outdated
| const esmLoader = require('internal/modules/esm/loader'); | ||
| const { kEmptyObject } = require('internal/util'); | ||
| const { validateFunction } = require('internal/validators'); | ||
| const { getOptionValue } = require('internal/options'); |
There was a problem hiding this comment.
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.
lib/internal/test_runner/harness.js
Outdated
|
|
||
| testResources.set(reporterScope.asyncId(), reporterScope); | ||
|
|
||
| async function setupGlobalSetupTeardownFunctions(globalSetupPath, cwd) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
FWIW, it may need to be cwd aware since it already deals with loading of reporters.
Codecov ReportAttention: Patch coverage is
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 36412 +25
==========================================
+ Hits 167378 167639 +261
+ Misses 11037 10995 -42
+ Partials 7109 7103 -6
🚀 New features to boost your workflow:
|
doc/api/test.md
Outdated
| 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: |
There was a problem hiding this comment.
"either" seems like the user can only specify one or the other.
test/fixtures/test-runner/global-setup-teardown/basic-setup-teardown.ts
Outdated
Show resolved
Hide resolved
test/fixtures/test-runner/global-setup-teardown/basic-setup-teardown.js
Outdated
Show resolved
Hide resolved
test/fixtures/test-runner/global-setup-teardown/async-setup-teardown.js
Outdated
Show resolved
Hide resolved
lib/internal/test_runner/harness.js
Outdated
| @@ -94,7 +126,7 @@ function createTestTree(rootTestOptions, globalOptions) { | |||
|
|
|||
| function createProcessEventHandler(eventName, rootTest) { | |||
| return (err) => { | |||
| if (rootTest.harness.bootstrapPromise) { | |||
| if (rootTest.harness.bootstrapPromise || rootTest.harness.globalSetupPromise) { | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
node/lib/internal/test_runner/runner.js
Line 738 in 657f818
node/lib/internal/test_runner/runner.js
Line 753 in 657f818
node/lib/internal/test_runner/harness.js
Line 281 in 657f818
node/lib/internal/test_runner/harness.js
Line 290 in 657f818
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
pmarchini
force-pushed
the
test_runner/49732-global-setup-teardown
branch
from
3ddcf93 to
46a2340
Compare
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
runfunction, preceded and followed bysetup/teardownwhen it's necessary to launch containers or other resources.At the moment, I've added support for a possible shared
contextobject 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
runnew option documentation and test to cover the feature viarunfunction.