Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Commit cbe30a0

Browse filesBrowse 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
Copy full SHA for cbe30a0

File tree

Expand file treeCollapse file tree

9 files changed

+302
-14
lines changed
Open diff view settings
Filter options
Expand file treeCollapse file tree

9 files changed

+302
-14
lines changed
Open diff view settings
Collapse file

‎lib/internal/test_runner/harness.js‎

Copy file name to clipboardExpand all lines: lib/internal/test_runner/harness.js
+33-9Lines 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();
Collapse file

‎lib/internal/test_runner/runner.js‎

Copy file name to clipboardExpand all lines: lib/internal/test_runner/runner.js
+1-1Lines 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);
Collapse file

‎lib/internal/test_runner/test.js‎

Copy file name to clipboardExpand all lines: lib/internal/test_runner/test.js
+1-1Lines 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;
Collapse file

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

Copy file name to clipboard
+16Lines 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+
});
Collapse file

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

Copy file name to clipboard
+34Lines 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 *
Collapse file

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

Copy file name to clipboard
+49Lines 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);
Collapse file

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

Copy file name to clipboard
+166Lines 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 *
Collapse file

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

Copy file name to clipboardExpand all lines: test/fixtures/test-runner/output/source_mapped_locations.snapshot
-3Lines 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
Collapse file

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

Copy file name to clipboardExpand all lines: test/parallel/test-runner-output.mjs
+2Lines 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 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.