test_runner: fix suite rerun edge case

Signed-off-by: Moshe Atlow <moshe@atlow.co.il>
PR-URL: https://github.com/nodejs/node/pull/62860
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: Jacob Smith <jacob@frende.me>

lib/internal/test_runner/test.js

@@ -777,7 +777,13 @@ class Test extends AsyncResource {
               loc: [child.line, child.column, child.file],
             }, noop);
             t.endTime = t.startTime = hrtime();
-            t.start();
+            // For suites, Suite.run() starts the subtests via SafePromiseAll.
+            // Starting them here as well would run them twice, re-invoking the
+            // synthetic children-creator against a now-incremented disambiguator
+            // and producing spurious failures.
+            if (this.reportedType !== 'suite') {
+              t.start();
+            }
           }
         };
       }

test/fixtures/test-runner/rerun.js

@@ -46,3 +46,22 @@ describe('describe rerun', { timeout: 1000, concurrency: 1000 }, () => {
   });
   test('a');
 });
+
+
+// Shared helper creates subtests at the same source location each time it's
+// called, producing ambiguous test identifiers (disambiguated with ":(N)"
+// suffixes in the rerun state file). Regression coverage for a bug where the
+// suite's synthetic rerun fn double-started its direct children, which then
+// re-invoked the synthetic descendant-creator against an already-incremented
+// disambiguator map and emitted spurious failures.
+function ambiguousHelper(t) {
+  return Promise.all([
+    t.test('shared sub A', () => {}),
+    t.test('shared sub B', () => {}),
+  ]);
+}
+
+describe('rerun with ambiguous shared helper', { timeout: 1000, concurrency: 1000 }, () => {
+  test('first caller', (t) => ambiguousHelper(t));
+  test('second caller', (t) => ambiguousHelper(t));
+});

test/parallel/test-runner-test-rerun-failures.js

@@ -27,6 +27,13 @@ const expectedStateFile = [
     'test/fixtures/test-runner/rerun.js:44:3': { passed_on_attempt: 0, name: 'passed on first attempt' },
     'test/fixtures/test-runner/rerun.js:47:3': { passed_on_attempt: 0, name: 'a' },
     'test/fixtures/test-runner/rerun.js:43:1': { passed_on_attempt: 0, name: 'describe rerun' },
+    'test/fixtures/test-runner/rerun.js:59:7': { passed_on_attempt: 0, name: 'shared sub A' },
+    'test/fixtures/test-runner/rerun.js:60:7': { passed_on_attempt: 0, name: 'shared sub B' },
+    'test/fixtures/test-runner/rerun.js:65:3': { passed_on_attempt: 0, name: 'first caller' },
+    'test/fixtures/test-runner/rerun.js:59:7:(1)': { passed_on_attempt: 0, name: 'shared sub A' },
+    'test/fixtures/test-runner/rerun.js:60:7:(1)': { passed_on_attempt: 0, name: 'shared sub B' },
+    'test/fixtures/test-runner/rerun.js:66:3': { passed_on_attempt: 0, name: 'second caller' },
+    'test/fixtures/test-runner/rerun.js:64:1': { passed_on_attempt: 0, name: 'rerun with ambiguous shared helper' },
   },
   {
     'test/fixtures/test-runner/rerun.js:9:1': { passed_on_attempt: 0, name: 'ok' },
@@ -38,11 +45,17 @@ const expectedStateFile = [
     'test/fixtures/test-runner/rerun.js:39:1': { passed_on_attempt: 0, name: 'nested ambiguous (expectedAttempts=0)' },
     'test/fixtures/test-runner/rerun.js:30:16:(1)': { passed_on_attempt: 0, name: '2 levels deep' },
     'test/fixtures/test-runner/rerun.js:35:13:(1)': { passed_on_attempt: 0, name: 'ok' },
-    'test/fixtures/test-runner/rerun.js:43:1': { passed_on_attempt: 0, name: 'describe rerun' },
-    'test/fixtures/test-runner/rerun.js:44:3': { passed_on_attempt: 0, name: 'passed on first attempt' },
     'test/fixtures/test-runner/rerun.js:45:13': { passed_on_attempt: 0, name: 'nested' },
-    'test/fixtures/test-runner/rerun.js:45:13:(1)': { passed_on_attempt: 1, name: 'nested' },
+    'test/fixtures/test-runner/rerun.js:44:3': { passed_on_attempt: 0, name: 'passed on first attempt' },
     'test/fixtures/test-runner/rerun.js:47:3': { passed_on_attempt: 0, name: 'a' },
+    'test/fixtures/test-runner/rerun.js:43:1': { passed_on_attempt: 0, name: 'describe rerun' },
+    'test/fixtures/test-runner/rerun.js:59:7': { passed_on_attempt: 0, name: 'shared sub A' },
+    'test/fixtures/test-runner/rerun.js:60:7': { passed_on_attempt: 0, name: 'shared sub B' },
+    'test/fixtures/test-runner/rerun.js:65:3': { passed_on_attempt: 0, name: 'first caller' },
+    'test/fixtures/test-runner/rerun.js:59:7:(1)': { passed_on_attempt: 0, name: 'shared sub A' },
+    'test/fixtures/test-runner/rerun.js:60:7:(1)': { passed_on_attempt: 0, name: 'shared sub B' },
+    'test/fixtures/test-runner/rerun.js:66:3': { passed_on_attempt: 0, name: 'second caller' },
+    'test/fixtures/test-runner/rerun.js:64:1': { passed_on_attempt: 0, name: 'rerun with ambiguous shared helper' },
   },
   {
     'test/fixtures/test-runner/rerun.js:3:1': { passed_on_attempt: 2, name: 'should fail on first two attempts' },
@@ -53,15 +66,21 @@ const expectedStateFile = [
     'test/fixtures/test-runner/rerun.js:29:13': { passed_on_attempt: 0, name: 'nested' },
     'test/fixtures/test-runner/rerun.js:35:13': { passed_on_attempt: 0, name: 'ok' },
     'test/fixtures/test-runner/rerun.js:39:1': { passed_on_attempt: 0, name: 'nested ambiguous (expectedAttempts=0)' },
-    'test/fixtures/test-runner/rerun.js:29:13:(1)': { passed_on_attempt: 2, name: 'nested' },
     'test/fixtures/test-runner/rerun.js:30:16:(1)': { passed_on_attempt: 0, name: '2 levels deep' },
+    'test/fixtures/test-runner/rerun.js:29:13:(1)': { passed_on_attempt: 2, name: 'nested' },
     'test/fixtures/test-runner/rerun.js:35:13:(1)': { passed_on_attempt: 0, name: 'ok' },
     'test/fixtures/test-runner/rerun.js:40:1': { passed_on_attempt: 2, name: 'nested ambiguous (expectedAttempts=1)' },
-    'test/fixtures/test-runner/rerun.js:43:1': { passed_on_attempt: 0, name: 'describe rerun' },
-    'test/fixtures/test-runner/rerun.js:44:3': { passed_on_attempt: 0, name: 'passed on first attempt' },
     'test/fixtures/test-runner/rerun.js:45:13': { passed_on_attempt: 0, name: 'nested' },
-    'test/fixtures/test-runner/rerun.js:45:13:(1)': { passed_on_attempt: 1, name: 'nested' },
+    'test/fixtures/test-runner/rerun.js:44:3': { passed_on_attempt: 0, name: 'passed on first attempt' },
     'test/fixtures/test-runner/rerun.js:47:3': { passed_on_attempt: 0, name: 'a' },
+    'test/fixtures/test-runner/rerun.js:43:1': { passed_on_attempt: 0, name: 'describe rerun' },
+    'test/fixtures/test-runner/rerun.js:59:7': { passed_on_attempt: 0, name: 'shared sub A' },
+    'test/fixtures/test-runner/rerun.js:60:7': { passed_on_attempt: 0, name: 'shared sub B' },
+    'test/fixtures/test-runner/rerun.js:65:3': { passed_on_attempt: 0, name: 'first caller' },
+    'test/fixtures/test-runner/rerun.js:59:7:(1)': { passed_on_attempt: 0, name: 'shared sub A' },
+    'test/fixtures/test-runner/rerun.js:60:7:(1)': { passed_on_attempt: 0, name: 'shared sub B' },
+    'test/fixtures/test-runner/rerun.js:66:3': { passed_on_attempt: 0, name: 'second caller' },
+    'test/fixtures/test-runner/rerun.js:64:1': { passed_on_attempt: 0, name: 'rerun with ambiguous shared helper' },
   },
 ];
 
@@ -81,26 +100,26 @@ test('test should pass on third rerun', async () => {
   let { code, stdout, signal } = await common.spawnPromisified(process.execPath, args);
   assert.strictEqual(code, 1);
   assert.strictEqual(signal, null);
-  assert.match(stdout, /pass 11/);
+  assert.match(stdout, /pass 17/);
   assert.match(stdout, /fail 4/);
-  assert.match(stdout, /suites 1/);
+  assert.match(stdout, /suites 2/);
   assert.deepStrictEqual(await getStateFile(), expectedStateFile.slice(0, 1));
 
   ({ code, stdout, signal } = await common.spawnPromisified(process.execPath, args));
   assert.strictEqual(code, 1);
   assert.strictEqual(signal, null);
-  assert.match(stdout, /pass 13/);
+  assert.match(stdout, /pass 18/);
   assert.match(stdout, /fail 3/);
-  assert.match(stdout, /suites 1/);
+  assert.match(stdout, /suites 2/);
   assert.deepStrictEqual(await getStateFile(), expectedStateFile.slice(0, 2));
 
 
   ({ code, stdout, signal } = await common.spawnPromisified(process.execPath, args));
   assert.strictEqual(code, 0);
   assert.strictEqual(signal, null);
-  assert.match(stdout, /pass 18/);
+  assert.match(stdout, /pass 21/);
   assert.match(stdout, /fail 0/);
-  assert.match(stdout, /suites 1/);
+  assert.match(stdout, /suites 2/);
   assert.deepStrictEqual(await getStateFile(), expectedStateFile);
 });
 
@@ -110,32 +129,32 @@ test('test should pass on third rerun with `--test`', async () => {
   let { code, stdout, signal } = await common.spawnPromisified(process.execPath, args);
   assert.strictEqual(code, 1);
   assert.strictEqual(signal, null);
-  assert.match(stdout, /pass 11/);
+  assert.match(stdout, /pass 17/);
   assert.match(stdout, /fail 4/);
-  assert.match(stdout, /suites 1/);
+  assert.match(stdout, /suites 2/);
   assert.deepStrictEqual(await getStateFile(), expectedStateFile.slice(0, 1));
 
   ({ code, stdout, signal } = await common.spawnPromisified(process.execPath, args));
   assert.strictEqual(code, 1);
   assert.strictEqual(signal, null);
-  assert.match(stdout, /pass 13/);
+  assert.match(stdout, /pass 18/);
   assert.match(stdout, /fail 3/);
-  assert.match(stdout, /suites 1/);
+  assert.match(stdout, /suites 2/);
   assert.deepStrictEqual(await getStateFile(), expectedStateFile.slice(0, 2));
 
 
   ({ code, stdout, signal } = await common.spawnPromisified(process.execPath, args));
   assert.strictEqual(code, 0);
   assert.strictEqual(signal, null);
-  assert.match(stdout, /pass 18/);
+  assert.match(stdout, /pass 21/);
   assert.match(stdout, /fail 0/);
-  assert.match(stdout, /suites 1/);
+  assert.match(stdout, /suites 2/);
   assert.deepStrictEqual(await getStateFile(), expectedStateFile);
 });
 
 test('using `run` api', async () => {
   let stream = run({ files: [fixture], rerunFailuresFilePath: stateFile });
-  stream.on('test:pass', common.mustCall(12));
+  stream.on('test:pass', common.mustCall(19));
   stream.on('test:fail', common.mustCall(4));
 
   // eslint-disable-next-line no-unused-vars
@@ -145,7 +164,7 @@ test('using `run` api', async () => {
 
 
   stream = run({ files: [fixture], rerunFailuresFilePath: stateFile });
-  stream.on('test:pass', common.mustCall(14));
+  stream.on('test:pass', common.mustCall(20));
   stream.on('test:fail', common.mustCall(3));
 
   // eslint-disable-next-line no-unused-vars
@@ -155,7 +174,7 @@ test('using `run` api', async () => {
 
 
   stream = run({ files: [fixture], rerunFailuresFilePath: stateFile });
-  stream.on('test:pass', common.mustCall(19));
+  stream.on('test:pass', common.mustCall(23));
   stream.on('test:fail', common.mustNotCall());
 
   // eslint-disable-next-line no-unused-vars