test_runner: make enqueue explicit

nodejs · MoLow · Aug 3, 2023 · Jul 26, 2023 · Jul 26, 2023 · Sep 23, 2023
commit 37e13737cab5c1868aac8fc66d8aa6b7714ad07b
diff --git a/lib/internal/test_runner/harness.js b/lib/internal/test_runner/harness.js
@@ -2,7 +2,6 @@
 const {
   ArrayPrototypeForEach,
   FunctionPrototypeBind,
-  PromiseResolve,
   SafeMap,
 } = primordials;
 const { getCallerLocation } = internalBinding('util');
@@ -210,20 +209,21 @@ function getGlobalRoot() {
   return globalRoot;
 }
 
-async function startSubtest(subtest) {
+async function startSubtest(subtest, parent) {
   await reportersSetup;
   getGlobalRoot().harness.bootstrapComplete = true;
-  await subtest.start();
+  if (parent instanceof Suite) {
+    subtest.enqueue();
+  } else {
+    await subtest.start();
+  }
 }
 
 function runInParentContext(Factory) {
   function run(name, options, fn, overrides) {
     const parent = testResources.get(executionAsyncId()) || getGlobalRoot();
     const subtest = parent.createSubtest(Factory, name, options, fn, overrides);
-    if (!(parent instanceof Suite)) {
-      return startSubtest(subtest);
-    }
-    return PromiseResolve();
+    return startSubtest(subtest, parent);
   }
 
   const test = (name, options, fn) => {

diff --git a/lib/internal/test_runner/test.js b/lib/internal/test_runner/test.js
@@ -318,6 +318,7 @@ class Test extends AsyncResource {
     this.fn = fn;
     this.harness = null; // Configured on the root test by the test harness.
     this.mock = null;
+    this.enqueued = null;
     this.cancelled = false;
     this.skipped = skip !== undefined && skip !== false;
     this.isTodo = todo !== undefined && todo !== false;
@@ -529,18 +530,25 @@ class Test extends AsyncResource {
     return this.run();
   }
 
+  enqueue() {
+    if (this.enqueued) {
+      return this.enqueued.promise;
+    }
+    this.reporter.enqueue(this.nesting, kFilename, this.name);
+    this.enqueued = createDeferredPromise();
+    this.enqueued.test = this;
+    this.parent.addPendingSubtest(this.enqueued);
+    return this.enqueued.promise;
+  }
+
   start() {
     // If there is enough available concurrency to run the test now, then do
     // 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.parent.hasConcurrency()) {
-      const deferred = createDeferredPromise();
-
-      deferred.test = this;
-      this.parent.addPendingSubtest(deferred);
-      return deferred.promise;
+    if (this.enqueued || !this.parent.hasConcurrency()) {
+      return this.enqueue();
     }
+    this.reporter.enqueue(this.nesting, kFilename, this.name);
     return this.dequeue();
   }
 
@@ -938,7 +946,7 @@ class Suite extends Test {
       }
 
       await this.runHook('before', hookArgs);
-
+      this.processPendingSubtests();
       stopPromise = stopTest(this.timeout, this.signal);
       const subtests = this.skipped || this.error ? [] : this.subtests;
       const promise = SafePromiseAll(subtests, (subtests) => subtests.start());

diff --git a/test/fixtures/test-runner/output/describe_it.js b/test/fixtures/test-runner/output/describe_it.js
@@ -290,12 +290,12 @@ describe('subtest sync throw fails', () => {
 });
 
 describe('describe sync throw fails', () => {
-  it('should not run', () => {});
+  it('should run', () => {});
   throw new Error('thrown from describe');
 });
 
 describe('describe async throw fails', async () => {
-  it('should not run', () => {});
+  it('should run', () => {});
   throw new Error('thrown from describe');
 });
 

diff --git a/test/fixtures/test-runner/output/describe_it.snapshot b/test/fixtures/test-runner/output/describe_it.snapshot
@@ -211,9 +211,6 @@ ok 21 - immediate resolve pass
         *
         *
         *
-        new Promise (<anonymous>)
-        *
-        *
       ...
     # Subtest: mixing describe/it and test should work
     ok 2 - mixing describe/it and test should work
@@ -483,7 +480,7 @@ not ok 50 - custom inspect symbol that throws fail
         *
         *
         *
-        new Promise (<anonymous>)
+        *
         *
         *
       ...
@@ -518,14 +515,10 @@ not ok 51 - subtest sync throw fails
   code: 'ERR_TEST_FAILURE'
   ...
 # Subtest: describe sync throw fails
-    # Subtest: should not run
-    not ok 1 - should not run
+    # Subtest: should run
+    ok 1 - should run
       ---
-      duration_ms: ZERO
-      location: '/test/fixtures/test-runner/output/describe_it.js:(LINE):3'
-      failureType: 'cancelledByParent'
-      error: 'test did not finish before its parent and was cancelled'
-      code: 'ERR_TEST_FAILURE'
+      duration_ms: *
       ...
     1..1
 not ok 52 - describe sync throw fails
@@ -549,14 +542,10 @@ not ok 52 - describe sync throw fails
     *
   ...
 # Subtest: describe async throw fails
-    # Subtest: should not run
-    not ok 1 - should not run
+    # Subtest: should run
+    ok 1 - should run
       ---
-      duration_ms: ZERO
-      location: '/test/fixtures/test-runner/output/describe_it.js:(LINE):3'
-      failureType: 'cancelledByParent'
-      error: 'test did not finish before its parent and was cancelled'
-      code: 'ERR_TEST_FAILURE'
+      duration_ms: *
       ...
     1..1
 not ok 53 - describe async throw fails
@@ -711,9 +700,9 @@ not ok 58 - invalid subtest fail
 # Warning: Test "callback async throw after done" generated asynchronous activity after the test ended. This activity created the error "Error: thrown from callback async throw after done" and would have caused the test to fail, but instead triggered an uncaughtException event.
 # tests 67
 # suites 11
-# pass 31
+# pass 33
 # fail 19
-# cancelled 4
+# cancelled 2
 # skipped 9
 # todo 4
 # duration_ms *
diff --git a/test/fixtures/test-runner/output/hooks.snapshot b/test/fixtures/test-runner/output/hooks.snapshot
@@ -222,9 +222,6 @@ not ok 5 - afterEach throws
         *
         *
         *
-        new Promise (<anonymous>)
-        *
-        *
       ...
     # Subtest: 2
     ok 2 - 2
@@ -258,9 +255,6 @@ not ok 6 - afterEach when test fails
         *
         *
         *
-        new Promise (<anonymous>)
-        *
-        *
       ...
     # Subtest: 2
     not ok 2 - 2

diff --git a/test/fixtures/test-runner/output/junit_reporter.snapshot b/test/fixtures/test-runner/output/junit_reporter.snapshot
@@ -19,7 +19,7 @@
       *
       *
       *
-      at async startSubtest (node:internal/test_runner/harness:208:3),
+      at async startSubtest (node:internal/test_runner/harness:210:5),
   code: 'ERR_TEST_FAILURE'
 }
 		</failure>
@@ -37,7 +37,7 @@
       *
       *
       *
-      at async startSubtest (node:internal/test_runner/harness:208:3),
+      at async startSubtest (node:internal/test_runner/harness:210:5),
   code: 'ERR_TEST_FAILURE'
 }
 		</failure>
@@ -62,7 +62,7 @@
       *
       *
       *
-      at async startSubtest (node:internal/test_runner/harness:208:3),
+      at async startSubtest (node:internal/test_runner/harness:210:5),
   code: 'ERR_TEST_FAILURE'
 }
 		</failure>
@@ -83,7 +83,7 @@
       *
       *
       *
-      at async startSubtest (node:internal/test_runner/harness:208:3),
+      at async startSubtest (node:internal/test_runner/harness:210:5),
   code: 'ERR_TEST_FAILURE'
 }
 		</failure>
@@ -101,7 +101,7 @@
       *
       *
       *
-      at async startSubtest (node:internal/test_runner/harness:208:3),
+      at async startSubtest (node:internal/test_runner/harness:210:5),
   code: 'ERR_TEST_FAILURE'
 }
 		</failure>
@@ -148,7 +148,7 @@ true !== false
       *
       *
       *
-      at async startSubtest (node:internal/test_runner/harness:208:3),
+      at async startSubtest (node:internal/test_runner/harness:210:5),
   code: 'ERR_TEST_FAILURE'
 }
 		</failure>
@@ -174,7 +174,7 @@ Error [ERR_TEST_FAILURE]: thrown from subtest sync throw fail
       *
       *
       *
-      at Test.dequeue (node:internal/test_runner/test:529:17),
+      at Test.dequeue (node:internal/test_runner/test:530:17),
   code: 'ERR_TEST_FAILURE'
 }
 			</failure>
@@ -221,7 +221,7 @@ Error [ERR_TEST_FAILURE]: thrown from subtest sync throw fail
       *
       *
       *
-      at async startSubtest (node:internal/test_runner/harness:208:3),
+      at async startSubtest (node:internal/test_runner/harness:210:5),
   code: 'ERR_TEST_FAILURE'
 }
 		</failure>
@@ -272,7 +272,7 @@ Error [ERR_TEST_FAILURE]: thrown from subtest sync throw fail
       *
       *
       *
-      at async startSubtest (node:internal/test_runner/harness:208:3),
+      at async startSubtest (node:internal/test_runner/harness:210:5),
   code: 'ERR_TEST_FAILURE'
 }
 		</failure>
@@ -359,7 +359,7 @@ Error [ERR_TEST_FAILURE]: thrown from subtest sync throw fails at first
       *
       *
       *
-      at Test.dequeue (node:internal/test_runner/test:529:17),
+      at Test.dequeue (node:internal/test_runner/test:530:17),
   code: 'ERR_TEST_FAILURE'
 }
 			</failure>