feat(cli): add 'test flaky' repeat-run flaky-test detector#132
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
WalkthroughThis PR adds a ChangesFlaky Test Detection Feature
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant testCommand
participant runFlaky
participant TestSprite API
User->>testCommand: testsprite test flaky <test-id>
testCommand->>runFlaky: invoke(opts)
loop each attempt
runFlaky->>TestSprite API: POST /tests/{id}/runs/rerun
TestSprite API-->>runFlaky: runId / error
runFlaky->>TestSprite API: poll run status
TestSprite API-->>runFlaky: terminal outcome
end
runFlaky->>runFlaky: summarizeFlaky(attempts)
runFlaky-->>testCommand: report or CLIError
testCommand-->>User: text/JSON output
Related issues: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
lxcario
force-pushed
the
feat/test-flaky-command
branch
from
d9ce26d to
6734c92
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/commands/test.flaky.spec.ts (2)
236-255: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMissing exit-code assertion for FAILING verdict test.
Unlike the sibling FLAKY test (lines 231-233), which asserts
exitCode === 1, this test only checks the message contains'failing'. Per the documented contract, exit 1 applies to both flaky and failing verdicts — worth asserting here too for parity and to guard the exit-code contract.As per path instructions, "Exit-code mapping: error paths must map to the documented exit code ... don't introduce ad-hoc codes."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/commands/test.flaky.spec.ts` around lines 236 - 255, The FAILING verdict test is missing an assertion that the thrown CLIError uses exit code 1, unlike the nearby FLAKY test. Update the test in test.flaky.spec.ts within the runFlaky/CLIError assertion block to verify the error’s exit code is 1 in addition to checking the message contains failing, keeping parity with the documented exit-code contract for failing verdicts.Source: Path instructions
327-346: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winTest doesn't verify the exit code 4 it claims to test.
The test title says "throws exit 4 when the test has no replayable run," but only asserts
err instanceof ApiErrorand.code === 'NOT_FOUND'— it never checks.exitCode. Given the documented exit-code contract for this command (exit 4 for no replayable run), this test should assert that mapping explicitly, similar to how the validation tests check.exitCodefor exit 5.As per path instructions, "Exit-code mapping: error paths must map to the documented exit code (see DOCUMENTATION.md / the exit-code table); don't introduce ad-hoc codes." A test named after this specific exit code should verify it.
✅ Suggested fix
expect(err).toBeInstanceOf(ApiError); expect((err as ApiError).code).toBe('NOT_FOUND'); + expect((err as ApiError).exitCode).toBe(4);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/commands/test.flaky.spec.ts` around lines 327 - 346, The flaky test for no replayable run only checks the ApiError code and does not verify the documented exit-code mapping. Update the test in test.flaky.spec.ts around runFlaky to assert the returned error’s exitCode is 4 in addition to checking ApiError and NOT_FOUND, matching the existing exit-code assertions used in the validation tests and the command’s documented contract.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/commands/test.flaky.spec.ts`:
- Around line 236-255: The FAILING verdict test is missing an assertion that the
thrown CLIError uses exit code 1, unlike the nearby FLAKY test. Update the test
in test.flaky.spec.ts within the runFlaky/CLIError assertion block to verify the
error’s exit code is 1 in addition to checking the message contains failing,
keeping parity with the documented exit-code contract for failing verdicts.
- Around line 327-346: The flaky test for no replayable run only checks the
ApiError code and does not verify the documented exit-code mapping. Update the
test in test.flaky.spec.ts around runFlaky to assert the returned error’s
exitCode is 4 in addition to checking ApiError and NOT_FOUND, matching the
existing exit-code assertions used in the validation tests and the command’s
documented contract.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: c6bb73ac-e0ca-4831-bceb-e706bb13605e
⛔ Files ignored due to path filters (1)
test/__snapshots__/help.snapshot.test.ts.snapis excluded by!**/*.snap,!**/*.snap
📒 Files selected for processing (8)
CHANGELOG.mdDOCUMENTATION.mdsrc/commands/test.flaky.spec.tssrc/commands/test.test.tssrc/commands/test.tssrc/lib/flaky.test.tssrc/lib/flaky.tstest/help.snapshot.test.ts
✅ Files skipped from review due to trivial changes (3)
- test/help.snapshot.test.ts
- DOCUMENTATION.md
- CHANGELOG.md
🚧 Files skipped from review as they are similar to previous changes (4)
- src/commands/test.test.ts
- src/lib/flaky.test.ts
- src/commands/test.ts
- src/lib/flaky.ts
|
Rescoped per the #115 triage: --runs is now capped at 10 (validated 1-10) - the constant, the --runs help text, validation, docs (DOCUMENTATION.md / CHANGELOG.md) and the bound tests are all updated. Auto-heal stays OFF and the backend-credit advisory is kept. Also rebased onto current main (resolved a CHANGELOG conflict and refreshed the help snapshot for the new --request-timeout global option); all 8 checks are green now. Ready for the continued review. |
|
This is approved — the rework matches the issue #115 decision exactly ( |
Rescope the flaky detector's --runs bound from 1-100 to 1-10 as requested in the TestSprite#115 triage: uncapped FE replays amplify free executions. Updates the MAX_FLAKY_RUNS constant (which drives the validation, error message, and --runs help text), docs, changelog, and the runs-bound tests. Regenerates the help snapshot, which also adds the previously-missing 'test flaky' entry.
Rebasing onto current main (which added the global --request-timeout option, TestSprite#17) changes the 'Global options' line rendered in the test flaky --help output. Regenerate the snapshot so the help snapshot test stays green on CI.
lxcario
force-pushed
the
feat/test-flaky-command
branch
from
6734c92 to
ab46f14
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CHANGELOG.md`:
- Around line 9-14: Resolve the merge conflict in CHANGELOG.md by removing the
conflict markers and keeping only the flaky-test release note. Preserve the
`testsprite test flaky` entry with the `--runs 1–10` limit, and drop the
unrelated JUnit report bullet so the changelog matches the current CLI contract.
Use the existing `testsprite test flaky <test-id>` entry as the anchor when
editing.
In `@src/commands/test.ts`:
- Around line 8239-8251: The `test flaky` dry-run branch exits before
`makeClient()` and therefore skips the stderr dry-run banner, unlike other
dry-run flows. Update the `opts.dryRun` path in `test` command handling to
explicitly emit the same dry-run banner used by `runDiff` and `runTestRunAll`
before returning, keeping the dry-run output on stderr and preserving
consistency across CLI commands.
- Around line 8287-8308: The rerun error handling in the `triggerRerun` catch
block currently treats all non-NOT_FOUND failures as retryable attempts, which
hides auth/permission failures and the intended exit-3 path. Update the `catch
(err)` logic in `test.ts` to detect non-retryable API errors (especially
authorization/permission cases) and rethrow them instead of pushing to
`attempts` or updating the ticker, while keeping only transient errors as
recorded attempts. Also ensure the `--dry-run` path still emits the usual
dry-run banner before rerun handling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: c9e2ab50-9482-4b4f-a787-f2a7aee6bc4e
⛔ Files ignored due to path filters (1)
test/__snapshots__/help.snapshot.test.ts.snapis excluded by!**/*.snap,!**/*.snap
📒 Files selected for processing (8)
CHANGELOG.mdDOCUMENTATION.mdsrc/commands/test.flaky.spec.tssrc/commands/test.test.tssrc/commands/test.tssrc/lib/flaky.test.tssrc/lib/flaky.tstest/help.snapshot.test.ts
✅ Files skipped from review due to trivial changes (1)
- src/commands/test.test.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- test/help.snapshot.test.ts
- DOCUMENTATION.md
- src/lib/flaky.ts
- src/lib/flaky.test.ts
| if (opts.dryRun) { | ||
| out.print({ | ||
| dryRun: true, | ||
| command: 'test flaky', | ||
| testId: opts.testId, | ||
| runs: opts.runs, | ||
| untilFail: opts.untilFail, | ||
| method: 'POST', | ||
| path: `/api/cli/v1/tests/${opts.testId}/runs/rerun`, | ||
| note: `Would replay the test up to ${opts.runs}x with auto-heal OFF and report a stability score.`, | ||
| }); | ||
| return undefined; | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Dry-run path omits the dry-run banner.
This branch returns before makeClient(), so the stderr dry-run banner never fires. Every other early-return dry-run path emits it explicitly (e.g. runDiff Line 3729, runTestRunAll Line 5437 — "this path returns before makeClient() fires the banner, so emit it here"). Without it, test flaky --dry-run is inconsistent with the rest of the CLI.
🛠️ Proposed fix
if (opts.dryRun) {
+ emitDryRunBanner(stderrFn);
out.print({
dryRun: true,
command: 'test flaky',As per path instructions: "human chatter, hints, and advisories go to stderr."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (opts.dryRun) { | |
| out.print({ | |
| dryRun: true, | |
| command: 'test flaky', | |
| testId: opts.testId, | |
| runs: opts.runs, | |
| untilFail: opts.untilFail, | |
| method: 'POST', | |
| path: `/api/cli/v1/tests/${opts.testId}/runs/rerun`, | |
| note: `Would replay the test up to ${opts.runs}x with auto-heal OFF and report a stability score.`, | |
| }); | |
| return undefined; | |
| } | |
| if (opts.dryRun) { | |
| emitDryRunBanner(stderrFn); | |
| out.print({ | |
| dryRun: true, | |
| command: 'test flaky', | |
| testId: opts.testId, | |
| runs: opts.runs, | |
| untilFail: opts.untilFail, | |
| method: 'POST', | |
| path: `/api/cli/v1/tests/${opts.testId}/runs/rerun`, | |
| note: `Would replay the test up to ${opts.runs}x with auto-heal OFF and report a stability score.`, | |
| }); | |
| return undefined; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/commands/test.ts` around lines 8239 - 8251, The `test flaky` dry-run
branch exits before `makeClient()` and therefore skips the stderr dry-run
banner, unlike other dry-run flows. Update the `opts.dryRun` path in `test`
command handling to explicitly emit the same dry-run banner used by `runDiff`
and `runTestRunAll` before returning, keeping the dry-run output on stderr and
preserving consistency across CLI commands.
Source: Path instructions
| } catch (err) { | ||
| // A missing replayable run is fatal for the whole command (mirror rerun): | ||
| // there is nothing to repeat, so point the user at a fresh `test run`. | ||
| if (err instanceof ApiError && err.code === 'NOT_FOUND') { | ||
| throw ApiError.fromEnvelope({ | ||
| error: { | ||
| code: 'NOT_FOUND', | ||
| message: `Test ${opts.testId} has no replayable run (unknown/cross-tenant id, or it has never completed a clean run).`, | ||
| nextAction: `Trigger a fresh run first: testsprite test run ${opts.testId}`, | ||
| requestId: err.requestId ?? 'local', | ||
| details: { testId: opts.testId, reason: 'no_replayable_run' }, | ||
| }, | ||
| }); | ||
| } | ||
| // Any other trigger error is recorded as an errored attempt so a single | ||
| // transient blip doesn't abort a long stability probe. | ||
| const code = err instanceof ApiError ? err.code : 'ERROR'; | ||
| attempts.push({ attempt: i, runId: null, outcome: 'error', failureKind: code }); | ||
| ticker.update(`Attempt ${i}/${opts.runs} — error (${code})`); | ||
| if (opts.untilFail) break; | ||
| continue; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the flaky command implementation and nearby help text.
rg -n "runFlaky|flaky|emitDryRunBanner|AUTH_REQUIRED|AUTH_INVALID|NOT_FOUND|untilFail|runs" src/commands/test.ts
# Show the relevant slices around the flaky command and dry-run handling.
sed -n '8120,8365p' src/commands/test.tsRepository: TestSprite/testsprite-cli
Length of output: 22872
Propagate non-retryable rerun errors instead of counting them as attempts
triggerRerun should rethrow auth/permission failures instead of recording them as error attempts; otherwise the command returns exit 1 and masks the documented exit 3 path. --dry-run also skips the usual dry-run banner here.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/commands/test.ts` around lines 8287 - 8308, The rerun error handling in
the `triggerRerun` catch block currently treats all non-NOT_FOUND failures as
retryable attempts, which hides auth/permission failures and the intended exit-3
path. Update the `catch (err)` logic in `test.ts` to detect non-retryable API
errors (especially authorization/permission cases) and rethrow them instead of
pushing to `attempts` or updating the ticker, while keeping only transient
errors as recorded attempts. Also ensure the `--dry-run` path still emits the
usual dry-run banner before rerun handling.
Source: Path instructions
|
Rebased onto latest Ready for merge whenever you are — thanks for the review! 🙏 |
2 participants
Closes #115
What
Adds a new command, testsprite test flaky , that replays a single test N times, aggregates the outcomes, and reports a stability verdict: stable / flaky / failing.
Why
A test that passes once can still be nondeterministic. There is currently no first-class way to surface that from the CLI. flaky lets a human (or CI) quantify stability before trusting -- or gating a merge on -- a result.
Behavior
: per-attempt timeout.Tests
19 tests across src/lib/flaky.test.ts (aggregation/verdict logic, exit codes, until-fail, runs bounds) and src/commands/test.flaky.spec.ts (command wiring, flag parsing, JSON output). All pass. typecheck + prettier clean. CHANGELOG and DOCUMENTATION updated; help snapshot refreshed for the new subcommand.
Summary by CodeRabbit
testsprite test flaky <test-id>to replay a test multiple times and classify stability as stable/flaky/failing using strict verbatim replays (auto-heal disabled).--runs(capped),--timeout,--until-fail,--output json, and--dry-run.runId,failureKind).test flaky --help.