Conversation
|
Review requested:
|
nodejs-github-bot
added
lib / src
JakobJingleheimer
left a comment
JakobJingleheimer
left a comment
There was a problem hiding this comment.
Looking good so far. I expected to see a loop somewhere; did you merely not get to that yet (since this is a draft) or did I miss some magic somewhere?
PS Sorry this took me a while to get to.
| @@ -87,6 +87,9 @@ function formatTestReport(type, data, showErrorDetails = true, prefix = '', inde | |||
| } | |||
| } else if (expectFailure !== undefined) { | |||
| title += ` # EXPECTED FAILURE`; | |||
| } else if (flaky !== undefined && flaky > 0) { | |||
| const retryText = flaky === 1 ? 're-try' : 're-tries'; | |||
| title += ` # FLAKY ${flaky} ${retryText}`; | |||
| let { fn, name, parent } = options; | ||
| const { concurrency, entryFile, expectFailure, loc, only, timeout, todo, skip, signal, plan } = options; | ||
|
|
||
| const { concurrency, entryFile, expectFailure, flaky, loc, only, timeout, todo, skip, signal, plan } = options; |
There was a problem hiding this comment.
nit: this is getting really long
| const { concurrency, entryFile, expectFailure, flaky, loc, only, timeout, todo, skip, signal, plan } = options; | |
| const { | |
| entryFile, | |
| expectFailure, | |
| flaky, | |
| loc, | |
| only, | |
| plan, | |
| signal, | |
| skip, | |
| timeout, | |
| todo, | |
| concurrency, | |
| } = options; |
There was a problem hiding this comment.
I think this should not be committed?
| @@ -78,6 +78,10 @@ function reportTest(nesting, testNumber, status, name, skip, todo, expectFailure | |||
| line += ` # TODO${typeof todo === 'string' && todo.length ? ` ${tapEscape(todo)}` : ''}`; | |||
| } else if (expectFailure !== undefined) { | |||
| line += ' # EXPECTED FAILURE'; | |||
| //should we use flaky >=0 here? for always printing 0 retries | |||
| } else if (flaky !== undefined && flaky > 0) { | |||
| const retryText = flaky === 1 ? 're-try' : 're-tries'; | |||
| line += ` # FLAKY ${flaky} ${retryText}`; | |||
There was a problem hiding this comment.
In the feature proposal, the output cites the number of re-tries it took to succeed. So I think the name for flaky should be changed to something like flakyRetriedCount to disambiguate it with the maximum number of re-tries to attempt.
# FLAKY 42 re-tries
|
Hi @vespa7, are you still planning to continue this PR? If not, I’d like to pick this up and open a follow-up PR based on the same idea. If that works for you, I’ll make sure to credit your original work. |
Yes I will publish the pr tomorrow |
JakobJingleheimer
left a comment
JakobJingleheimer
left a comment
There was a problem hiding this comment.
Thanks for this!
I agree the formatting changes are improvements, but they're not germane to this PR, and they make it rather difficult to see the real changes, so please undo them 😰
| * `testNumber` {number} The ordinal number of the test. | ||
| * `todo` {string|boolean|undefined} Present if [`context.todo`][] is called | ||
| * `skip` {string|boolean|undefined} Present if [`context.skip`][] is called | ||
| * `flakyRetriedCount` {number|undefined} The number of retries taken for a |
There was a problem hiding this comment.
This feels a little wordy to me. Maybe just retryCount?
|
@vespa7 Are you still working on it? If you don't mind, I'd like to work on it! |
Yes, still working on it |
There was a problem hiding this comment.
Pull request overview
Adds initial support for “flaky” tests in the Node.js test runner, enabling automatic retries and exposing retry info in reporter output and docs.
Changes:
- Implement
flakyoption parsing/inheritance and retry execution ininternal/test_runner/test.js. - Propagate/display flaky retry metadata (
retryCount) acrossTestsStreamand multiple reporters (TAP, dot, JUnit, human-readable utils). - Add TAP output fixtures/snapshots and document the new API surface (
*.flaky()shorthands + options/events updates).
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| test/test-runner/test-output-flaky.mjs | Adds snapshot-based test to validate flaky reporter output. |
| test/fixtures/test-runner/output/flaky.js | Fixture exercising flaky retries, suite inheritance, and invalid inputs. |
| test/fixtures/test-runner/output/flaky.snapshot | Expected TAP output including flaky directives and summary counter. |
| lib/internal/test_runner/test.js | Core implementation: parse flaky, run retry loop, report retry directive, add flaky summary line. |
| lib/internal/test_runner/tests_stream.js | Adds getFlaky() directive payload (retryCount). |
| lib/internal/test_runner/utils.js | Increments new harness.counters.flaky counter for flaky tests. |
| lib/internal/test_runner/harness.js | Initializes/resets counters.flaky; exposes .flaky shorthand on test/suite factories. |
| lib/internal/test_runner/reporter/utils.js | Adds # FLAKY … annotation in formatted output when retryCount > 0. |
| lib/internal/test_runner/reporter/tap.js | Emits # FLAKY … in TAP lines when retryCount > 0. |
| lib/internal/test_runner/reporter/junit.js | Adds a <property name="flaky" …> when retryCount > 0. |
| lib/internal/test_runner/reporter/dot.js | Renders a distinct marker for flaky-pass tests (based on retryCount). |
| doc/api/test.md | Documents flaky option + shorthands + retryCount event field. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
|
|
||
| this.pass(); | ||
| await afterEach(); | ||
| await after(); |
There was a problem hiding this comment.
Retries currently re-run only the test body; beforeEach is executed once before the loop and afterEach is executed once after the loop (and is additionally wrapped in runOnce). If the intended semantics of “re-run the test” include per-attempt hooks/cleanup, the hooks need to run inside the retry loop (and afterEach should not be runOnce), otherwise state can leak across attempts.
| * `todo` {string|boolean|undefined} Present if [`context.todo`][] is called | ||
| * `skip` {string|boolean|undefined} Present if [`context.skip`][] is called | ||
| * `retryCount` {number|undefined} The number of retries taken for a | ||
| flaky test. Present when a test is marked as flaky. |
There was a problem hiding this comment.
The docs say retryCount is present on 'test:fail' whenever a test is marked as flaky, but the current implementation only adds retryCount via the getFlaky() directive when a test passes after retries (see Test#getReportDetails(), which only sets the flaky directive when this.passed). Either update the docs to match the actual emitted data, or include retryCount in fail events as well (likely with the number of retries attempted).
| flaky test. Present when a test is marked as flaky. | |
| flaky test. Present only when flaky metadata is included in the event. |
| ArrayPrototypePush(promises, stopPromise); | ||
| try { | ||
| const runArgs = ArrayPrototypeSlice(args); | ||
| ArrayPrototypeUnshift(runArgs, this.fn, ctx); |
There was a problem hiding this comment.
Within the flaky retry loop the test is invoked via ArrayPrototypeUnshift(runArgs, this.fn, ctx), which bypasses the testFn wrapper created earlier (used to preserve testChannel.start.runStores(...) / AsyncLocalStorage context). Retried executions should call the same wrapped function that non-flaky execution would, otherwise diagnostics_channel subscribers see different behavior and context propagation can break.
| ArrayPrototypeUnshift(runArgs, this.fn, ctx); | |
| ArrayPrototypeUnshift(runArgs, testFn, ctx); |
| this.pass(); | ||
| break; | ||
| } catch (attemptErr) { | ||
| if (attempt < maxAttempts) { | ||
| this.retriesTaken = attempt; |
There was a problem hiding this comment.
The retry loop only retries when the test body throws/rejects (caught by catch (attemptErr)), but many failures are recorded via this.fail(...) without throwing (e.g. t.fail(), callback+promise misuse, plan failures, etc.). In those cases this.pass() returns early with this.error !== null and the code still breaks, so flaky tests won't actually retry. After running an attempt, the loop should detect !this.passed/this.error !== null and continue (or throw) when attempt < maxAttempts.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #61746 +/- ##
==========================================
- Coverage 89.60% 89.60% -0.01%
==========================================
Files 706 706
Lines 219179 219262 +83
Branches 41993 42005 +12
==========================================
+ Hits 196400 196468 +68
- Misses 14675 14678 +3
- Partials 8104 8116 +12
🚀 New features to boost your workflow:
|
JakobJingleheimer
added
the
test_runner
|
@vespa7 heads up: the first commit must follow a specific format, something like |
| const retryText = flaky === 1 ? 're-try' : 're-tries'; | ||
| line += ` # FLAKY ${flaky} ${retryText}`; | ||
| line += ` # EXPECTED FAILURE${typeof expectFailure === 'string' ? ` ${tapEscape(expectFailure)}` : ''}`; | ||
| } else if (retryCount !== undefined && retryCount > 0) { |
There was a problem hiding this comment.
nit: I think you don't need the retryCount !== undefined because undefined > 0 → false
| } else if (retryCount !== undefined && retryCount > 0) { | |
| } else if (retryCount > 0) { |
| } else if (flaky !== undefined && flaky > 0) { | ||
| const retryText = flaky === 1 ? 're-try' : 're-tries'; | ||
| title += ` # FLAKY ${flaky} ${retryText}`; | ||
| } else if (retryCount !== undefined && retryCount > 0) { |
There was a problem hiding this comment.
same
| } else if (retryCount !== undefined && retryCount > 0) { | |
| } else if (retryCount > 0) { |
|
@vespa7 Can you update your commit + PR title to actually explain the change? Additionally, please add a proper description |
nodejs/test-runner - Feature proposal:
flaky