Skip to content

Commit cbe30a0

Browse files
cjihrigRafaelGSS
cjihrig
authored and
RafaelGSS
committed
test_runner: finish build phase before running tests
This commit updates the test runner to wait for suites to finish building before starting any tests. This is necessary when test filtering is enabled, as suites may transition from filtered to not filtered depending on what is inside of them. Fixes: #54084 Fixes: #54154 PR-URL: #54423 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: Jake Yuesong Li <jake.yuesong@gmail.com>
1 parent 909c532 commit cbe30a0

File tree

9 files changed

+302
-14
lines changed

9 files changed

+302
-14
lines changed

‎lib/internal/test_runner/harness.js

Lines changed: 33 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,11 @@
11
'use strict';
22
const {
33
ArrayPrototypeForEach,
4+
ArrayPrototypePush,
45
FunctionPrototypeBind,
56
PromiseResolve,
67
SafeMap,
8+
SafePromiseAllReturnVoid,
79
} = primordials;
810
const { getCallerLocation } = internalBinding('util');
911
const {
@@ -24,6 +26,7 @@ const {
2426
shouldColorizeTestFiles,
2527
} = require('internal/test_runner/utils');
2628
const { queueMicrotask } = require('internal/process/task_queues');
29+
const { createDeferredPromise } = require('internal/util');
2730
const { bigint: hrtime } = process.hrtime;
2831
const resolvedPromise = PromiseResolve();
2932
const testResources = new SafeMap();
@@ -32,9 +35,12 @@ let globalRoot;
3235
testResources.set(reporterScope.asyncId(), reporterScope);
3336

3437
function createTestTree(rootTestOptions, globalOptions) {
38+
const buildPhaseDeferred = createDeferredPromise();
3539
const harness = {
3640
__proto__: null,
37-
allowTestsToRun: false,
41+
buildPromise: buildPhaseDeferred.promise,
42+
buildSuites: [],
43+
isWaitingForBuildPhase: false,
3844
bootstrapPromise: resolvedPromise,
3945
watching: false,
4046
config: globalOptions,
@@ -56,6 +62,13 @@ function createTestTree(rootTestOptions, globalOptions) {
5662
shouldColorizeTestFiles: shouldColorizeTestFiles(globalOptions.destinations),
5763
teardown: null,
5864
snapshotManager: null,
65+
async waitForBuildPhase() {
66+
if (harness.buildSuites.length > 0) {
67+
await SafePromiseAllReturnVoid(harness.buildSuites);
68+
}
69+
70+
buildPhaseDeferred.resolve();
71+
},
5972
};
6073

6174
harness.resetCounters();
@@ -243,14 +256,25 @@ function lazyBootstrapRoot() {
243256
}
244257

245258
async function startSubtestAfterBootstrap(subtest) {
246-
if (subtest.root.harness.bootstrapPromise) {
247-
// Only incur the overhead of awaiting the Promise once.
248-
await subtest.root.harness.bootstrapPromise;
249-
subtest.root.harness.bootstrapPromise = null;
250-
queueMicrotask(() => {
251-
subtest.root.harness.allowTestsToRun = true;
252-
subtest.root.processPendingSubtests();
253-
});
259+
if (subtest.root.harness.buildPromise) {
260+
if (subtest.root.harness.bootstrapPromise) {
261+
await subtest.root.harness.bootstrapPromise;
262+
subtest.root.harness.bootstrapPromise = null;
263+
}
264+
265+
if (subtest.buildSuite) {
266+
ArrayPrototypePush(subtest.root.harness.buildSuites, subtest.buildSuite);
267+
}
268+
269+
if (!subtest.root.harness.isWaitingForBuildPhase) {
270+
subtest.root.harness.isWaitingForBuildPhase = true;
271+
queueMicrotask(() => {
272+
subtest.root.harness.waitForBuildPhase();
273+
});
274+
}
275+
276+
await subtest.root.harness.buildPromise;
277+
subtest.root.harness.buildPromise = null;
254278
}
255279

256280
await subtest.start();

‎lib/internal/test_runner/runner.js

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -610,7 +610,7 @@ function run(options = kEmptyObject) {
610610
}
611611
const runFiles = () => {
612612
root.harness.bootstrapPromise = null;
613-
root.harness.allowTestsToRun = true;
613+
root.harness.buildPromise = null;
614614
return SafePromiseAllSettledReturnVoid(testFiles, (path) => {
615615
const subtest = runTestFile(path, filesWatcher, opts);
616616
filesWatcher?.runningSubtests.set(path, subtest);

‎lib/internal/test_runner/test.js

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -766,7 +766,7 @@ class Test extends AsyncResource {
766766
// it. Otherwise, return a Promise to the caller and mark the test as
767767
// pending for later execution.
768768
this.reporter.enqueue(this.nesting, this.loc, this.name);
769-
if (!this.root.harness.allowTestsToRun || !this.parent.hasConcurrency()) {
769+
if (this.root.harness.buildPromise || !this.parent.hasConcurrency()) {
770770
const deferred = createDeferredPromise();
771771

772772
deferred.test = this;

‎test/fixtures/test-runner/output/filtered-suite-delayed-build.js

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,16 @@
1+
// Flags: --test-name-pattern=enabled
2+
'use strict';
3+
const common = require('../../../common');
4+
const { suite, test } = require('node:test');
5+
6+
suite('async suite', async () => {
7+
await 1;
8+
test('enabled 1', common.mustCall());
9+
await 1;
10+
test('not run', common.mustNotCall());
11+
await 1;
12+
});
13+
14+
suite('sync suite', () => {
15+
test('enabled 2', common.mustCall());
16+
});

‎test/fixtures/test-runner/output/filtered-suite-delayed-build.snapshot

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,34 @@
1+
TAP version 13
2+
# Subtest: async suite
3+
# Subtest: enabled 1
4+
ok 1 - enabled 1
5+
---
6+
duration_ms: *
7+
...
8+
1..1
9+
ok 1 - async suite
10+
---
11+
duration_ms: *
12+
type: 'suite'
13+
...
14+
# Subtest: sync suite
15+
# Subtest: enabled 2
16+
ok 1 - enabled 2
17+
---
18+
duration_ms: *
19+
...
20+
1..1
21+
ok 2 - sync suite
22+
---
23+
duration_ms: *
24+
type: 'suite'
25+
...
26+
1..2
27+
# tests 2
28+
# suites 2
29+
# pass 2
30+
# fail 0
31+
# cancelled 0
32+
# skipped 0
33+
# todo 0
34+
# duration_ms *

‎test/fixtures/test-runner/output/filtered-suite-order.mjs

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,49 @@
1+
// Flags: --test-only
2+
import { describe, test, after } from 'node:test';
3+
4+
after(() => { console.log('with global after()'); });
5+
await Promise.resolve();
6+
7+
console.log('Execution order was:');
8+
const ll = (t) => { console.log(` * ${t.fullName}`) };
9+
10+
describe('A', () => {
11+
test.only('A', ll);
12+
test('B', ll);
13+
describe.only('C', () => {
14+
test.only('A', ll);
15+
test('B', ll);
16+
});
17+
describe('D', () => {
18+
test.only('A', ll);
19+
test('B', ll);
20+
});
21+
});
22+
describe.only('B', () => {
23+
test('A', ll);
24+
test('B', ll);
25+
describe('C', () => {
26+
test('A', ll);
27+
});
28+
});
29+
describe('C', () => {
30+
test.only('A', ll);
31+
test('B', ll);
32+
describe.only('C', () => {
33+
test('A', ll);
34+
test('B', ll);
35+
});
36+
describe('D', () => {
37+
test('A', ll);
38+
test.only('B', ll);
39+
});
40+
});
41+
describe('D', () => {
42+
test('A', ll);
43+
test.only('B', ll);
44+
});
45+
describe.only('E', () => {
46+
test('A', ll);
47+
test('B', ll);
48+
});
49+
test.only('F', ll);

‎test/fixtures/test-runner/output/filtered-suite-order.snapshot

Lines changed: 166 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,166 @@
1+
Execution order was:
2+
* A > A
3+
* A > C > A
4+
* A > D > A
5+
* B > A
6+
* B > B
7+
* B > C > A
8+
* C > A
9+
* C > C > A
10+
* C > C > B
11+
* C > D > B
12+
* D > B
13+
* E > A
14+
* E > B
15+
* F
16+
with global after()
17+
TAP version 13
18+
# Subtest: A
19+
# Subtest: A
20+
ok 1 - A
21+
---
22+
duration_ms: *
23+
...
24+
# Subtest: C
25+
# Subtest: A
26+
ok 1 - A
27+
---
28+
duration_ms: *
29+
...
30+
1..1
31+
ok 2 - C
32+
---
33+
duration_ms: *
34+
type: 'suite'
35+
...
36+
# Subtest: D
37+
# Subtest: A
38+
ok 1 - A
39+
---
40+
duration_ms: *
41+
...
42+
1..1
43+
ok 3 - D
44+
---
45+
duration_ms: *
46+
type: 'suite'
47+
...
48+
1..3
49+
ok 1 - A
50+
---
51+
duration_ms: *
52+
type: 'suite'
53+
...
54+
# Subtest: B
55+
# Subtest: A
56+
ok 1 - A
57+
---
58+
duration_ms: *
59+
...
60+
# Subtest: B
61+
ok 2 - B
62+
---
63+
duration_ms: *
64+
...
65+
# Subtest: C
66+
# Subtest: A
67+
ok 1 - A
68+
---
69+
duration_ms: *
70+
...
71+
1..1
72+
ok 3 - C
73+
---
74+
duration_ms: *
75+
type: 'suite'
76+
...
77+
1..3
78+
ok 2 - B
79+
---
80+
duration_ms: *
81+
type: 'suite'
82+
...
83+
# Subtest: C
84+
# Subtest: A
85+
ok 1 - A
86+
---
87+
duration_ms: *
88+
...
89+
# Subtest: C
90+
# Subtest: A
91+
ok 1 - A
92+
---
93+
duration_ms: *
94+
...
95+
# Subtest: B
96+
ok 2 - B
97+
---
98+
duration_ms: *
99+
...
100+
1..2
101+
ok 2 - C
102+
---
103+
duration_ms: *
104+
type: 'suite'
105+
...
106+
# Subtest: D
107+
# Subtest: B
108+
ok 1 - B
109+
---
110+
duration_ms: *
111+
...
112+
1..1
113+
ok 3 - D
114+
---
115+
duration_ms: *
116+
type: 'suite'
117+
...
118+
1..3
119+
ok 3 - C
120+
---
121+
duration_ms: *
122+
type: 'suite'
123+
...
124+
# Subtest: D
125+
# Subtest: B
126+
ok 1 - B
127+
---
128+
duration_ms: *
129+
...
130+
1..1
131+
ok 4 - D
132+
---
133+
duration_ms: *
134+
type: 'suite'
135+
...
136+
# Subtest: E
137+
# Subtest: A
138+
ok 1 - A
139+
---
140+
duration_ms: *
141+
...
142+
# Subtest: B
143+
ok 2 - B
144+
---
145+
duration_ms: *
146+
...
147+
1..2
148+
ok 5 - E
149+
---
150+
duration_ms: *
151+
type: 'suite'
152+
...
153+
# Subtest: F
154+
ok 6 - F
155+
---
156+
duration_ms: *
157+
...
158+
1..6
159+
# tests 14
160+
# suites 10
161+
# pass 14
162+
# fail 0
163+
# cancelled 0
164+
# skipped 0
165+
# todo 0
166+
# duration_ms *

‎test/fixtures/test-runner/output/source_mapped_locations.snapshot

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -21,9 +21,6 @@ not ok 1 - fails
2121
*
2222
*
2323
*
24-
*
25-
*
26-
*
2724
...
2825
1..1
2926
# tests 1

‎test/parallel/test-runner-output.mjs

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -101,6 +101,8 @@ const tests = [
101101
{ name: 'test-runner/output/eval_dot.js', transform: specTransform },
102102
{ name: 'test-runner/output/eval_spec.js', transform: specTransform },
103103
{ name: 'test-runner/output/eval_tap.js' },
104+
{ name: 'test-runner/output/filtered-suite-delayed-build.js' },
105+
{ name: 'test-runner/output/filtered-suite-order.mjs' },
104106
{ name: 'test-runner/output/filtered-suite-throws.js' },
105107
{ name: 'test-runner/output/hooks.js' },
106108
{ name: 'test-runner/output/hooks_spec_reporter.js', transform: specTransform },

0 commit comments

Comments
 (0)