test_runner: address flaky retry review nits

Han5991 · Han5991 · commit f96a8797a7bf · 2026-06-18T07:09:05.000+09:00
Reflect reviewer feedback on the flaky retry change: add JSDoc block comments for IntelliSense, wrap long conditionals and calls across multiple lines, clarify comment wording, and simplify the report directive merge since spreading undefined is a no-op. Alphabetize the diagnostics filter list and de-mangle the TAP and JUnit reporter calls. No behavior change.

Signed-off-by: sangwook &lt;rewq5991@gmail.com&gt;
diff --git a/lib/internal/test_runner/harness.js b/lib/internal/test_runner/harness.js
@@ -412,7 +412,8 @@ function runInParentContext(Factory) {
       const overrides = {
         __proto__: null,
         [keyword]: keyword === 'flaky' && typeof options?.[keyword] === 'number' ?
-          options[keyword] : true,
+          options[keyword] :
+          true,
         loc: getCallerLocation(),
       };
 
diff --git a/lib/internal/test_runner/reporter/junit.js b/lib/internal/test_runner/reporter/junit.js
@@ -133,11 +133,19 @@ module.exports = async function* junitReporter(source) {
           }
           if (event.data.retryCount > 0) {
             ArrayPrototypePush(currentTest.children, {
-              __proto__: null, nesting: event.data.nesting + 1, tag: 'properties',
+              __proto__: null,
+              nesting: event.data.nesting + 1,
+              tag: 'properties',
               attrs: { __proto__: null },
               children: [{
-                __proto__: null, nesting: event.data.nesting + 2, tag: 'property',
-                attrs: { __proto__: null, name: 'flaky', value: event.data.retryCount },
+                __proto__: null,
+                nesting: event.data.nesting + 2,
+                tag: 'property',
+                attrs: {
+                  __proto__: null,
+                  name: 'flaky',
+                  value: event.data.retryCount,
+                },
               }],
             });
           }
diff --git a/lib/internal/test_runner/reporter/tap.js b/lib/internal/test_runner/reporter/tap.js
@@ -34,14 +34,30 @@ async function * tapReporter(source) {
   for await (const { type, data } of source) {
     switch (type) {
       case 'test:fail': {
-        yield reportTest(data.nesting, data.testNumber, 'not ok', data.name,
-                         data.skip, data.todo, data.expectFailure, data.retryCount);
+        yield reportTest(
+          data.nesting,
+          data.testNumber,
+          'not ok',
+          data.name,
+          data.skip,
+          data.todo,
+          data.expectFailure,
+          data.retryCount,
+        );
         const location = data.file ? `${data.file}:${data.line}:${data.column}` : null;
         yield reportDetails(data.nesting, data.details, location);
         break;
       } case 'test:pass':
-        yield reportTest(data.nesting, data.testNumber, 'ok', data.name,
-                         data.skip, data.todo, data.expectFailure, data.retryCount);
+        yield reportTest(
+          data.nesting,
+          data.testNumber,
+          'ok',
+          data.name,
+          data.skip,
+          data.todo,
+          data.expectFailure,
+          data.retryCount,
+        );
         yield reportDetails(data.nesting, data.details, null);
         break;
       case 'test:plan':
diff --git a/lib/internal/test_runner/runner.js b/lib/internal/test_runner/runner.js
@@ -123,7 +123,15 @@ const kFilterArgValues = [
   '--experimental-config-file',
 ];
 const kDiagnosticsFilterArgs = [
-  'tests', 'suites', 'pass', 'fail', 'cancelled', 'skipped', 'todo', 'flaky', 'duration_ms',
+  'cancelled',
+  'duration_ms',
+  'fail',
+  'flaky',
+  'pass',
+  'skipped',
+  'suites',
+  'tests',
+  'todo',
 ];
 
 const kCanceledTests = new SafeSet()
@@ -321,9 +329,11 @@ class FileTest extends Test {
         // Only an exhausted flaky timeout is upgraded to a failure, mirroring
         // the child-side rule; other cancellations stay cancelled.
         cancelled: kCanceledTests.has(item.data.details?.error?.failureType) &&
-          !(item.data.retryCount > 0 &&
-            item.data.details?.error?.failureType === kTestTimeoutFailure),
-        // retryCount is present (even 0) only for flaky-marked tests, so its
+          !(
+            item.data.retryCount > 0 &&
+            item.data.details?.error?.failureType === kTestTimeoutFailure
+          ),
+        // retryCount is present (even when 0) only for flaky-tagged tests, so its
         // presence is what marks this reconstructed test as flaky for the parent
         // counter across the process-isolation IPC boundary.
         isFlaky: item.data.retryCount !== undefined,
diff --git a/lib/internal/test_runner/test.js b/lib/internal/test_runner/test.js
@@ -335,7 +335,7 @@ class TestContext {
       const assert = { __proto__: null };
       // Each assertion counts against an active `t.plan()`. A flaky test re-arms
       // `test.plan` on every retry, so it must be read live (deferred to call
-      // time). A non-flaky test counts against the plan captured once here, at
+      // time). A non-flaky test counts against the plan captured here once, at
       // first `.assert` access: this preserves the historical behavior that a
       // `t.plan()` called *after* the first assertion is not retroactively
       // counted (see test-runner/flaky/assert-before-plan). The flaky-vs-captured
@@ -753,11 +753,12 @@ class Test extends AsyncResource {
     this.plan = null;
     this.expectedAssertions = plan;
     this.cancelled = false;
-    if (flaky === undefined || flaky === null) {
-      // Inherit flaky only from a suite parent: a suite propagates it to its
+    if (flaky == null) {
+      // Inherit flaky only from a suite parent: a suite propagates to its
       // test-cases, but a test must not propagate it to its own subtests.
       this.flakyRetries = this.parent?.reportedType === 'suite' ?
-        (this.parent.flakyRetries ?? 0) : 0;
+        (this.parent.flakyRetries ?? 0) :
+        0;
     } else if (typeof flaky === 'boolean') {
       this.flakyRetries = flaky ? kDefaultFlakyRetries : 0;
     } else if (typeof flaky === 'number') {
@@ -1322,12 +1323,17 @@ class Test extends AsyncResource {
     this.report = noop;
     queueMicrotask(() => this.postRun());
   }
-  // Reset per-attempt state before retrying a flaky test. Anything that would
-  // otherwise leak between attempts is cleared here so the retry starts from a
-  // clean slate. `before`/`after` (suite-once) and the accumulated parent hooks
-  // are intentionally left untouched. `baseDiagnosticsCount` is the diagnostics
-  // length captured before the first attempt, so diagnostics emitted during
-  // construction survive while a previous attempt's are discarded.
+  /**
+   * Reset per-attempt state before retrying a flaky test. Anything that would
+   * otherwise leak between attempts is cleared here so the retry starts from a
+   * clean slate. `before`/`after` (suite-once) and the accumulated parent hooks
+   * are intentionally left untouched. `baseDiagnosticsCount` is the diagnostics
+   * length captured before the first attempt, so diagnostics emitted during
+   * construction survive while a previous attempt's are discarded.
+   * @param {TestContext} ctx
+   * @param {number} baseDiagnosticsCount
+   * @param {{ beforeEach: number, ownAfterEachCount: number }} baseHookCounts
+   */
   #resetForRetry(ctx, baseDiagnosticsCount, baseHookCounts) {
     this.error = null;
     this.passed = false;
@@ -1339,11 +1345,11 @@ class Test extends AsyncResource {
     // Reset assertion bookkeeping; re-arm the plan if one was requested via the
     // `plan` option so that `t.plan(N)` does not accumulate across attempts.
     this.plan = null;
-    if (this.expectedAssertions) {
+    if (this.expectedAssertions != null) {
       ctx.plan(this.expectedAssertions);
     }
-    // Drop subtests created by the previous attempt so they are not double
-    // counted or double reported. outputSubtestCount must be reset too:
+    // Drop subtests created by the previous attempt so they are not double-
+    // counted or double-reported. `outputSubtestCount` must be reset too:
     // otherwise it accumulates across attempts while `subtests` holds only the
     // final attempt's children, inflating the plan count and - when the final
     // attempt creates no subtest - leaving outputSubtestCount > 0 with an empty
@@ -1364,8 +1370,11 @@ class Test extends AsyncResource {
     // The re-executing body re-registers its per-attempt hooks; before/after
     // are kept (runOnce, and an earlier attempt's teardown must survive).
     ArrayPrototypeSplice(this.hooks.beforeEach, baseHookCounts.beforeEach);
-    ArrayPrototypeSplice(this.hooks.afterEach, baseHookCounts.ownAfterEachCount,
-                         this.hooks.ownAfterEachCount - baseHookCounts.ownAfterEachCount);
+    ArrayPrototypeSplice(
+      this.hooks.afterEach,
+      baseHookCounts.ownAfterEachCount,
+      (this.hooks.ownAfterEachCount - baseHookCounts.ownAfterEachCount),
+    );
     this.hooks.ownAfterEachCount = baseHookCounts.ownAfterEachCount;
     // Abort the previous attempt's controller so any signal-bound work it
     // started (e.g. a timer awaiting t.signal) is torn down instead of leaking.
@@ -1377,13 +1386,20 @@ class Test extends AsyncResource {
     this.abortController = new AbortController();
     this.signal = this.abortController.signal;
   }
-  // The retry-eligibility terms shared by the catch-path `willRetry` gate and
-  // the loop's `shouldRetry` decision. Kept in one place so a future stop
-  // condition cannot be added to one site and forgotten at the other.
+  /**
+   * The retry-eligibility terms shared by the catch-path `willRetry` gate and
+   * the loop's `shouldRetry` decision. Kept in one place so a future stop
+   * condition cannot be added to one site and forgotten at the other.
+   * @param {number} attempt
+   * @param {number} maxAttempts
+   * @returns {boolean}
+   */
   #retriesRemaining(attempt, maxAttempts) {
-    return attempt < maxAttempts &&
+    return (
+      attempt < maxAttempts &&
       !this.expectFailure &&
-      !(this.signal.aborted || this.outerSignal?.aborted);
+      !(this.signal.aborted || this.outerSignal?.aborted)
+    );
   }
 
   async run() {
@@ -1436,7 +1452,11 @@ class Test extends AsyncResource {
     let afterEach = makeAfterEach();
     let stopPromise;
     const basePublishEnd = () => testChannel.end.publish(channelContext);
-    const basePublishError = (err) => testChannel.error.publish({ __proto__: null, ...channelContext, error: err });
+    const basePublishError = (err) => testChannel.error.publish({
+      __proto__: null,
+      ...channelContext,
+      error: err,
+    });
     let publishEnd = basePublishEnd;
     let publishError = basePublishError;
 
@@ -1447,11 +1467,13 @@ class Test extends AsyncResource {
     const maxAttempts = this.flakyRetries + 1;
     const baseDiagnosticsCount = this.diagnostics.length;
     // Baseline for #resetForRetry to truncate body-registered hooks back to.
-    const baseHookCounts = this.isFlaky ? {
-      __proto__: null,
-      beforeEach: this.hooks.beforeEach.length,
-      ownAfterEachCount: this.hooks.ownAfterEachCount,
-    } : null;
+    const baseHookCounts = this.isFlaky ?
+      {
+        __proto__: null,
+        beforeEach: this.hooks.beforeEach.length,
+        ownAfterEachCount: this.hooks.ownAfterEachCount,
+      } :
+      null;
 
     for (let attempt = 1; attempt <= maxAttempts; attempt++) {
       if (attempt > 1) {
@@ -1481,16 +1503,15 @@ class Test extends AsyncResource {
         // the test runs within both the runStores context (for AsyncLocalStorage/bindStore)
         // AND the AsyncResource scope. It's critical that runStores wraps the function,
         // not the runInAsyncScope call itself, to maintain AsyncLocalStorage bindings.
-        let testFn = this.fn;
-        if (channelContext !== null && testChannel.start.hasSubscribers) {
-          testFn = (...fnArgs) => testChannel.start.runStores(channelContext, () => {
+        const testFn = (channelContext !== null && testChannel.start.hasSubscribers) ?
+          (...fnArgs) => testChannel.start.runStores(channelContext, () => {
             // Rebind from the base each attempt; binding on top of the
             // previous binding would pin the first attempt's async context.
             publishEnd = AsyncResource.bind(basePublishEnd);
             publishError = AsyncResource.bind(basePublishError);
             return ReflectApply(this.fn, this, fnArgs);
-          });
-        }
+          }) :
+          this.fn;
 
         ArrayPrototypeUnshift(runArgs, testFn, ctx);
 
@@ -1541,20 +1562,26 @@ class Test extends AsyncResource {
         await afterEach();
         // Keep `after` for the final attempt: a failed subtest is about to
         // trigger a retry, which must not run against torn-down state.
-        const subtestRetryPending =
+        const subtestRetryPending = (
           this.#retriesRemaining(attempt, maxAttempts) &&
-          ArrayPrototypeSome(this.subtests,
-                             (subtest) => !subtest.passed && !subtest.isTodo);
+          ArrayPrototypeSome(
+            this.subtests,
+            (subtest) => !subtest.passed && !subtest.isTodo,
+          )
+        );
         if (!subtestRetryPending) {
           await after();
         }
       } catch (err) {
         attemptError = err;
         if (isTestFailureError(err)) {
-          if (err.failureType === kTestTimeoutFailure &&
-              (!this.isFlaky || this.expectFailure)) {
-            // expectFailure never retries, so its timeout keeps the non-flaky
-            // cancellation semantics instead of becoming an expected failure.
+          if (
+            err.failureType === kTestTimeoutFailure &&
+            (!this.isFlaky || this.expectFailure)
+          ) {
+            // expectFailure tests are intentionally not retried, so an
+            // expectFailure timeout flows through #cancel to preserve the
+            // existing non-flaky cancellation semantics.
             this.#cancel(err);
           } else {
             // A flaky timeout must not flow through #cancel: #cancel aborts the
@@ -1593,19 +1620,26 @@ class Test extends AsyncResource {
       // the parent later, in postRun) is detected here so the parent still
       // retries. An external abort and an expectFailure verdict stop retrying
       // immediately.
-      const subtestsFailed = this.isFlaky &&
-        ArrayPrototypeSome(this.subtests, (subtest) => !subtest.passed && !subtest.isTodo);
+      const subtestsFailed = (
+        this.isFlaky &&
+        ArrayPrototypeSome(this.subtests, (subtest) => !subtest.passed && !subtest.isTodo)
+      );
       if (this.passed && !subtestsFailed) {
         break;
       }
-      const shouldRetry = this.#retriesRemaining(attempt, maxAttempts) &&
-        (!this.passed || subtestsFailed);
+      const shouldRetry = (
+        this.#retriesRemaining(attempt, maxAttempts) &&
+        (!this.passed || subtestsFailed)
+      );
       if (!shouldRetry) {
         // Run any final-attempt duties deferred toward a retry that is now
         // called off (e.g. the outer signal aborted during afterEach).
         try { await after(); } catch { /* test is already failing */ }
-        if (willRetry && attemptError !== undefined &&
-            channelContext !== null && testChannel.error.hasSubscribers) {
+        if (
+          willRetry && attemptError !== undefined &&
+          channelContext !== null &&
+          testChannel.error.hasSubscribers
+        ) {
           publishError(attemptError);
         }
         break;
@@ -1828,9 +1862,7 @@ class Test extends AsyncResource {
 
     if (this.isFlaky) {
       const flaky = this.reporter.getFlaky(this.retryCount);
-      directive = directive === undefined ?
-        flaky :
-        { __proto__: null, ...directive, ...flaky };
+      directive = { __proto__: null, ...directive, ...flaky };
     }
 
     if (this.reportedType) {
@@ -1849,8 +1881,10 @@ class Test extends AsyncResource {
     return { __proto__: null, details, directive };
   }
 
-  // Counters must not see a subtree that a flaky ancestor may still discard:
-  // park the count on that ancestor until it settles (report()) or retries.
+  /**
+   * Counters must not see a subtree that a flaky ancestor may still discard:
+   * park the count on that ancestor until it settles (report()) or retries.
+   */
   #countCompleted() {
     let ancestor = this.parent;
     while (ancestor !== null) {
@@ -1878,7 +1912,8 @@ class Test extends AsyncResource {
       // outputSubtestCount was non-zero; fall back to this test's own nesting
       // rather than dereferencing subtests[0] (which would crash the run).
       const subtestNesting = this.subtests.length ?
-        this.subtests[0].nesting : this.nesting + 1;
+        this.subtests[0].nesting :
+        this.nesting + 1;
       this.reporter.plan(subtestNesting, this.loc, this.outputSubtestCount);
     } else {
       this.reportStarted();
