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 <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>

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();

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);

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;

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());
+});

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 *

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);

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 *

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

@@ -21,9 +21,6 @@ not ok 1 - fails
     *
     *
     *
-    *
-    *
-    *
   ...
 1..1
 # tests 1

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 },