From ef4bdbfb76b92c2d8ff1febdd925eedd9dd82291 Mon Sep 17 00:00:00 2001 From: Colin Ihrig Date: Tue, 20 Aug 2024 03:14:01 -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 PR-URL: https://github.com/nodejs/node/pull/54423 Reviewed-By: Benjamin Gruenbaum Reviewed-By: Matteo Collina Reviewed-By: Moshe Atlow Reviewed-By: Jake Yuesong Li --- 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 },