From 4cb8fba6551505ecbb8a5b88ba38a7c92ee71c19 Mon Sep 17 00:00:00 2001 From: cjihrig Date: Sat, 17 Aug 2024 11:38:07 -0400 Subject: [PATCH] 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: https://github.com/nodejs/node/issues/54084 Fixes: https://github.com/nodejs/node/issues/54154 --- lib/internal/test_runner/harness.js | 42 ++++- lib/internal/test_runner/runner.js | 2 +- lib/internal/test_runner/test.js | 2 +- .../output/filtered-suite-delayed-build.js | 16 ++ .../filtered-suite-delayed-build.snapshot | 34 ++++ .../output/filtered-suite-order.mjs | 49 ++++++ .../output/filtered-suite-order.snapshot | 166 ++++++++++++++++++ .../output/source_mapped_locations.snapshot | 3 - test/parallel/test-runner-output.mjs | 2 + 9 files changed, 302 insertions(+), 14 deletions(-) create mode 100644 test/fixtures/test-runner/output/filtered-suite-delayed-build.js create mode 100644 test/fixtures/test-runner/output/filtered-suite-delayed-build.snapshot create mode 100644 test/fixtures/test-runner/output/filtered-suite-order.mjs create mode 100644 test/fixtures/test-runner/output/filtered-suite-order.snapshot diff --git a/lib/internal/test_runner/harness.js b/lib/internal/test_runner/harness.js index 9c372c115e90f2..3c56820cf4e247 100644 --- a/lib/internal/test_runner/harness.js +++ b/lib/internal/test_runner/harness.js @@ -1,9 +1,11 @@ 'use strict'; const { ArrayPrototypeForEach, + ArrayPrototypePush, FunctionPrototypeBind, PromiseResolve, SafeMap, + SafePromiseAllReturnVoid, } = primordials; const { getCallerLocation } = internalBinding('util'); const { @@ -24,6 +26,7 @@ const { shouldColorizeTestFiles, } = require('internal/test_runner/utils'); const { queueMicrotask } = require('internal/process/task_queues'); +const { createDeferredPromise } = require('internal/util'); const { bigint: hrtime } = process.hrtime; const resolvedPromise = PromiseResolve(); const testResources = new SafeMap(); @@ -32,9 +35,12 @@ let globalRoot; testResources.set(reporterScope.asyncId(), reporterScope); function createTestTree(rootTestOptions, globalOptions) { + const buildPhaseDeferred = createDeferredPromise(); const harness = { __proto__: null, - allowTestsToRun: false, + buildPromise: buildPhaseDeferred.promise, + buildSuites: [], + isWaitingForBuildPhase: false, bootstrapPromise: resolvedPromise, watching: false, config: globalOptions, @@ -56,6 +62,13 @@ function createTestTree(rootTestOptions, globalOptions) { shouldColorizeTestFiles: shouldColorizeTestFiles(globalOptions.destinations), teardown: null, snapshotManager: null, + async waitForBuildPhase() { + if (harness.buildSuites.length > 0) { + await SafePromiseAllReturnVoid(harness.buildSuites); + } + + buildPhaseDeferred.resolve(); + }, }; harness.resetCounters(); @@ -243,14 +256,25 @@ function lazyBootstrapRoot() { } async function startSubtestAfterBootstrap(subtest) { - if (subtest.root.harness.bootstrapPromise) { - // Only incur the overhead of awaiting the Promise once. - await subtest.root.harness.bootstrapPromise; - subtest.root.harness.bootstrapPromise = null; - queueMicrotask(() => { - subtest.root.harness.allowTestsToRun = true; - subtest.root.processPendingSubtests(); - }); + if (subtest.root.harness.buildPromise) { + if (subtest.root.harness.bootstrapPromise) { + await subtest.root.harness.bootstrapPromise; + subtest.root.harness.bootstrapPromise = null; + } + + if (subtest.buildSuite) { + ArrayPrototypePush(subtest.root.harness.buildSuites, subtest.buildSuite); + } + + if (!subtest.root.harness.isWaitingForBuildPhase) { + subtest.root.harness.isWaitingForBuildPhase = true; + queueMicrotask(() => { + subtest.root.harness.waitForBuildPhase(); + }); + } + + await subtest.root.harness.buildPromise; + subtest.root.harness.buildPromise = null; } await subtest.start(); diff --git a/lib/internal/test_runner/runner.js b/lib/internal/test_runner/runner.js index e994b1aa40ecab..a4874d5caead91 100644 --- a/lib/internal/test_runner/runner.js +++ b/lib/internal/test_runner/runner.js @@ -610,7 +610,7 @@ function run(options = kEmptyObject) { } const runFiles = () => { root.harness.bootstrapPromise = null; - root.harness.allowTestsToRun = true; + root.harness.buildPromise = null; return SafePromiseAllSettledReturnVoid(testFiles, (path) => { const subtest = runTestFile(path, filesWatcher, opts); filesWatcher?.runningSubtests.set(path, subtest); diff --git a/lib/internal/test_runner/test.js b/lib/internal/test_runner/test.js index e4ebbb2ee9238b..b79ff7a049ea6c 100644 --- a/lib/internal/test_runner/test.js +++ b/lib/internal/test_runner/test.js @@ -766,7 +766,7 @@ class Test extends AsyncResource { // it. Otherwise, return a Promise to the caller and mark the test as // pending for later execution. this.reporter.enqueue(this.nesting, this.loc, this.name); - if (!this.root.harness.allowTestsToRun || !this.parent.hasConcurrency()) { + if (this.root.harness.buildPromise || !this.parent.hasConcurrency()) { const deferred = createDeferredPromise(); deferred.test = this; diff --git a/test/fixtures/test-runner/output/filtered-suite-delayed-build.js b/test/fixtures/test-runner/output/filtered-suite-delayed-build.js new file mode 100644 index 00000000000000..c6b7060c2b88b2 --- /dev/null +++ b/test/fixtures/test-runner/output/filtered-suite-delayed-build.js @@ -0,0 +1,16 @@ +// Flags: --test-name-pattern=enabled +'use strict'; +const common = require('../../../common'); +const { suite, test } = require('node:test'); + +suite('async suite', async () => { + await 1; + test('enabled 1', common.mustCall()); + await 1; + test('not run', common.mustNotCall()); + await 1; +}); + +suite('sync suite', () => { + test('enabled 2', common.mustCall()); +}); diff --git a/test/fixtures/test-runner/output/filtered-suite-delayed-build.snapshot b/test/fixtures/test-runner/output/filtered-suite-delayed-build.snapshot new file mode 100644 index 00000000000000..dbe3048dffdf12 --- /dev/null +++ b/test/fixtures/test-runner/output/filtered-suite-delayed-build.snapshot @@ -0,0 +1,34 @@ +TAP version 13 +# Subtest: async suite + # Subtest: enabled 1 + ok 1 - enabled 1 + --- + duration_ms: * + ... + 1..1 +ok 1 - async suite + --- + duration_ms: * + type: 'suite' + ... +# Subtest: sync suite + # Subtest: enabled 2 + ok 1 - enabled 2 + --- + duration_ms: * + ... + 1..1 +ok 2 - sync suite + --- + duration_ms: * + type: 'suite' + ... +1..2 +# tests 2 +# suites 2 +# pass 2 +# fail 0 +# cancelled 0 +# skipped 0 +# todo 0 +# duration_ms * diff --git a/test/fixtures/test-runner/output/filtered-suite-order.mjs b/test/fixtures/test-runner/output/filtered-suite-order.mjs new file mode 100644 index 00000000000000..f7df0cb8e355a7 --- /dev/null +++ b/test/fixtures/test-runner/output/filtered-suite-order.mjs @@ -0,0 +1,49 @@ +// Flags: --test-only +import { describe, test, after } from 'node:test'; + +after(() => { console.log('with global after()'); }); +await Promise.resolve(); + +console.log('Execution order was:'); +const ll = (t) => { console.log(` * ${t.fullName}`) }; + +describe('A', () => { + test.only('A', ll); + test('B', ll); + describe.only('C', () => { + test.only('A', ll); + test('B', ll); + }); + describe('D', () => { + test.only('A', ll); + test('B', ll); + }); +}); +describe.only('B', () => { + test('A', ll); + test('B', ll); + describe('C', () => { + test('A', ll); + }); +}); +describe('C', () => { + test.only('A', ll); + test('B', ll); + describe.only('C', () => { + test('A', ll); + test('B', ll); + }); + describe('D', () => { + test('A', ll); + test.only('B', ll); + }); +}); +describe('D', () => { + test('A', ll); + test.only('B', ll); +}); +describe.only('E', () => { + test('A', ll); + test('B', ll); +}); +test.only('F', ll); diff --git a/test/fixtures/test-runner/output/filtered-suite-order.snapshot b/test/fixtures/test-runner/output/filtered-suite-order.snapshot new file mode 100644 index 00000000000000..7a18df8c7d0aea --- /dev/null +++ b/test/fixtures/test-runner/output/filtered-suite-order.snapshot @@ -0,0 +1,166 @@ +Execution order was: + * A > A + * A > C > A + * A > D > A + * B > A + * B > B + * B > C > A + * C > A + * C > C > A + * C > C > B + * C > D > B + * D > B + * E > A + * E > B + * F +with global after() +TAP version 13 +# Subtest: A + # Subtest: A + ok 1 - A + --- + duration_ms: * + ... + # Subtest: C + # Subtest: A + ok 1 - A + --- + duration_ms: * + ... + 1..1 + ok 2 - C + --- + duration_ms: * + type: 'suite' + ... + # Subtest: D + # Subtest: A + ok 1 - A + --- + duration_ms: * + ... + 1..1 + ok 3 - D + --- + duration_ms: * + type: 'suite' + ... + 1..3 +ok 1 - A + --- + duration_ms: * + type: 'suite' + ... +# Subtest: B + # Subtest: A + ok 1 - A + --- + duration_ms: * + ... + # Subtest: B + ok 2 - B + --- + duration_ms: * + ... + # Subtest: C + # Subtest: A + ok 1 - A + --- + duration_ms: * + ... + 1..1 + ok 3 - C + --- + duration_ms: * + type: 'suite' + ... + 1..3 +ok 2 - B + --- + duration_ms: * + type: 'suite' + ... +# Subtest: C + # Subtest: A + ok 1 - A + --- + duration_ms: * + ... + # Subtest: C + # Subtest: A + ok 1 - A + --- + duration_ms: * + ... + # Subtest: B + ok 2 - B + --- + duration_ms: * + ... + 1..2 + ok 2 - C + --- + duration_ms: * + type: 'suite' + ... + # Subtest: D + # Subtest: B + ok 1 - B + --- + duration_ms: * + ... + 1..1 + ok 3 - D + --- + duration_ms: * + type: 'suite' + ... + 1..3 +ok 3 - C + --- + duration_ms: * + type: 'suite' + ... +# Subtest: D + # Subtest: B + ok 1 - B + --- + duration_ms: * + ... + 1..1 +ok 4 - D + --- + duration_ms: * + type: 'suite' + ... +# Subtest: E + # Subtest: A + ok 1 - A + --- + duration_ms: * + ... + # Subtest: B + ok 2 - B + --- + duration_ms: * + ... + 1..2 +ok 5 - E + --- + duration_ms: * + type: 'suite' + ... +# Subtest: F +ok 6 - F + --- + duration_ms: * + ... +1..6 +# tests 14 +# suites 10 +# pass 14 +# fail 0 +# cancelled 0 +# skipped 0 +# todo 0 +# duration_ms * diff --git a/test/fixtures/test-runner/output/source_mapped_locations.snapshot b/test/fixtures/test-runner/output/source_mapped_locations.snapshot index 29b70fd0d08378..24c3ee8d113446 100644 --- a/test/fixtures/test-runner/output/source_mapped_locations.snapshot +++ b/test/fixtures/test-runner/output/source_mapped_locations.snapshot @@ -21,9 +21,6 @@ not ok 1 - fails * * * - * - * - * ... 1..1 # tests 1 diff --git a/test/parallel/test-runner-output.mjs b/test/parallel/test-runner-output.mjs index bd2db22ee6cc36..0125a8168e4464 100644 --- a/test/parallel/test-runner-output.mjs +++ b/test/parallel/test-runner-output.mjs @@ -101,6 +101,8 @@ const tests = [ { name: 'test-runner/output/eval_dot.js', transform: specTransform }, { name: 'test-runner/output/eval_spec.js', transform: specTransform }, { name: 'test-runner/output/eval_tap.js' }, + { name: 'test-runner/output/filtered-suite-delayed-build.js' }, + { name: 'test-runner/output/filtered-suite-order.mjs' }, { name: 'test-runner/output/filtered-suite-throws.js' }, { name: 'test-runner/output/hooks.js' }, { name: 'test-runner/output/hooks_spec_reporter.js', transform: specTransform },