fix(rerun): emit partial stdout on TimeoutError in single-FE rerun --wait#219
fix(rerun): emit partial stdout on TimeoutError in single-FE rerun --wait#219Awad-de wants to merge 28 commits into
Conversation
…wait
Apply the fix in src/commands/test.ts. When the overall --timeout polling
deadline is exceeded on a single FE rerun, emit {runId, status:"running"}
to stdout before exit 7.
Co-authored-by: Cursor <cursoragent@cursor.com>
|
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:
WalkthroughUpdates ChangesRerun wait timeout handling
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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.ts (1)
6102-6113: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate partial-timeout-print logic between
TimeoutErrorandRequestTimeoutErrorbranches.The new
TimeoutErrorblock (Lines 6102-6113) and the existingRequestTimeoutErrorblock (Lines 6126-6142) build near-identical{ runId, status: 'running' }objects and near-identical text formatters, differing only in the wording of the timeout reason and hint line. Consider extracting a small shared helper (e.g.printTimeoutPartial(out, runId, reasonText)) to keep both branches — and any future timeout branches — in sync.♻️ Proposed refactor sketch
+function printRerunTimeoutPartial( + out: ReturnType<typeof makeOutput>, + runId: string, + reasonText: string, +): void { + const partial = { runId, status: 'running' as const }; + out.print(partial, data => { + const p = data as typeof partial; + return [ + `runId ${p.runId}`, + `status ${p.status} (${reasonText})`, + `hint Re-attach with: testsprite test wait ${p.runId}`, + ].join('\n'); + }); +}Then call it from both catch branches with the appropriate
reasonText.Also applies to: 6126-6142
🤖 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 6102 - 6113, The TimeoutError and RequestTimeoutError branches in test.ts duplicate the same partial-run printing logic. Extract the shared `{ runId, status: 'running' }` construction and `out.print` formatter into a small helper (for example, a timeout-partial printer) and call it from both catch paths, passing only the branch-specific timeout reason/hint text so the two cases stay consistent.src/commands/test.rerun.spec.ts (1)
4699-4703: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider also asserting
nextActionon the thrown error.Per DOCUMENTATION.md,
--wait --timeoutexceedance is expected to include anextActionpointing callers totest wait <run-id>. Assertingerr.details?.nextAction(or equivalent) here would guard that documented contract, not just the exit code and stdout partial.As per path instructions: "Exit-code mapping: error paths must map to the documented exit code (see DOCUMENTATION.md / the exit-code table)".
🤖 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.rerun.spec.ts` around lines 4699 - 4703, The rerun timeout assertion currently checks only the exit code and stdout, but it should also verify the documented `nextAction` on the thrown error. Update the `test.rerun.spec.ts` timeout case to assert `err.details?.nextAction` (or the equivalent error field) in the same block where `err` is matched, using the rerun/wait flow around the `rerunResp.runId` scenario, so the contract that callers are directed to `test wait <run-id>` is covered.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.rerun.spec.ts`:
- Around line 4699-4703: The rerun timeout assertion currently checks only the
exit code and stdout, but it should also verify the documented `nextAction` on
the thrown error. Update the `test.rerun.spec.ts` timeout case to assert
`err.details?.nextAction` (or the equivalent error field) in the same block
where `err` is matched, using the rerun/wait flow around the `rerunResp.runId`
scenario, so the contract that callers are directed to `test wait <run-id>` is
covered.
In `@src/commands/test.ts`:
- Around line 6102-6113: The TimeoutError and RequestTimeoutError branches in
test.ts duplicate the same partial-run printing logic. Extract the shared `{
runId, status: 'running' }` construction and `out.print` formatter into a small
helper (for example, a timeout-partial printer) and call it from both catch
paths, passing only the branch-specific timeout reason/hint text so the two
cases stay consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: a45e95fc-68a8-4092-b342-b64f27518f03
📒 Files selected for processing (2)
src/commands/test.rerun.spec.tssrc/commands/test.ts
|
Discord Username / User ID |
|
@jangjos-128 @zeshi-du Hello. Is everything okay? This is the previous PR #155. Is there a problem? |
|
Hi @Awad-de — yes, everything's okay! There's no problem with your work at all. We'll review it through the normal process from here. You don't need to do anything else. Apologies for the confusion, and thanks again for contributing to TestSprite! |
|
Hi @jangjos-128 thanks but I'm participating in the hackathon-s3 competition. Do I have to win every PR that's merged? |
|
Not at all @Awad-de! In our community, it's definitely quality over quantity. It really comes down to how impactful and useful your PR is for the folks managing the CLI. Even a single, solid merge that brings real value can absolutely get rewarded! 🙌 |
zeshi-du
left a comment
zeshi-du
left a comment
There was a problem hiding this comment.
The production fix is correct — the TimeoutError branch now mirrors its RequestTimeoutError sibling (partial {runId, status} to stdout, hint to stderr, exit 7 preserved), and that's exactly the right shape.
One blocking issue in the test diff: it replaces the existing [finding-5] regression test (batch rerun --wait: RequestTimeoutError during fan-out poll writes JSON stdout + exit 7) in place with the new [finding-4] single-FE test. The batch fan-out classification path it guarded is still live production code, so merging as-is silently drops its only regression coverage — CI stays green because the new test fills the slot.
Ask: restore the [finding-5] batch test and add your new [finding-4] test alongside it, so both code paths keep independent coverage. No changes needed to the production code. Thanks @Awad-de!
|
✅ This PR is linked to an issue assigned to @Awad-de — thanks! The |
github-actions
Bot
added
the
needs-issue
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/commands/test.rerun.spec.ts (1)
4855-4857: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove duplicated properties in the type assertion.
The
acceptedproperty is duplicated four times in the type definition.♻️ Proposed fix
const parsed = JSON.parse(stdoutLines.join('\n')) as { accepted: Array<{ testId: string; runId: string; status: string }>; - accepted: Array<{ testId: string; runId: string; status: string }>; - accepted: Array<{ testId: string; runId: string; status: string }>; - accepted: Array<{ testId: string; runId: string; status: string }>; };🤖 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.rerun.spec.ts` around lines 4855 - 4857, Update the JSON.parse type assertion in the test rerun parsing code so the accepted property is declared only once, preserving its existing array element shape and all surrounding parsing behavior.
🤖 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 `@src/commands/test.rerun.spec.ts`:
- Around line 4847-4852: Update the runTestRerun invocation to pass only the
options object and a deps object containing fetchImpl and the output capture
callback. Wrap the call in error handling for RequestTimeoutError, then assert
the thrown error’s expected properties instead of treating the call result as a
returned error.
- Around line 4847-4853: Update the runTestRerun invocation in the affected test
to include output: 'json' in its options object, ensuring stdoutLines contains
JSON for the existing parsing logic.
---
Nitpick comments:
In `@src/commands/test.rerun.spec.ts`:
- Around line 4855-4857: Update the JSON.parse type assertion in the test rerun
parsing code so the accepted property is declared only once, preserving its
existing array element shape and all surrounding parsing behavior.
🪄 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: 8641cf0c-d1be-4228-92df-ce18f962213d
📒 Files selected for processing (1)
src/commands/test.rerun.spec.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@src/commands/test.rerun.spec.ts`:
- Line 4827: Remove the unused creds binding in the test setup, or pass creds
into the relevant setup call if credentials are required there; ensure no
unreferenced assignment remains.
🪄 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: 4ebd16c3-168c-4fd0-abe7-6a5317fd8aee
📒 Files selected for processing (1)
src/commands/test.rerun.spec.ts
Fix missing newline at end of file.
Fix missing newline at end of file in test.rerun.spec.ts
Fix missing newline at end of file in test.rerun.spec.ts
Fix missing newline at end of file in test.rerun.spec.ts
github-actions
Bot
removed
the
needs-issue
Fixed formatting issues by adding a missing comma and clarifying comments.
|
Hi @zeshi-du
|
Removed InterruptError and ShutdownController imports.
3 participants
What does this PR do?
Fixes an asymmetry in
testsprite test rerun --wait(single FE rerun path):when the overall
--timeoutpolling deadline is exceeded, the CLI now emits apartial
{ runId, status: "running" }object to stdout before throwingexit 7 — matching the behavior already present for
RequestTimeoutErrorandthe recently fixed
test run --wait/test waitpaths.Before:
TimeoutError→ throwApiErrorimmediately → stdout empty in--output jsonmode → AI agents must scrape stderr to recover the runId.After:
TimeoutError→out.print({ runId, status: "running" })→ throw→ caller can programmatically chain into
testsprite test wait <runId>.Related issue
Closes #157
Hackathon CLI Improvement — agent recovery on polling timeout (complements
the
test run --wait/test waitTimeoutError fix; this covers theremaining single-FE
test rerun --waitpath).Type of change
Checklist
mainbranch.feat(...),fix(...),docs(...), …).npm run lintandnpm run format:checkpass.npm run typecheckpasses.npm testpasses and coverage stays at or above the 80% gate.README.md/DOCUMENTATION.mdwhere relevant. (N/A — internal error-handling parity fix, no CLI surface change.)Notes for reviewers
Root cause
In
runTestRerun(single FE rerun, no BE closure), theTimeoutErrorcatchblock at ~line 6132 called
ticker.finalize()then threwApiErrorwithoutany
out.print(). The adjacentRequestTimeoutErrorbranch already emitteda partial run to stdout — this was simply a missed mirror.
Scope
test rerun --waitTimeoutErrorpath only (~12 lines).into the batch result object on timeout;
test run --wait/test waitwere fixed in a prior PR.
Test added
[finding-4]intest.rerun.spec.ts:TimeoutErrorviatimeoutSeconds: 0(immediate deadline){ runId, status: "running" }Verification
npm test → 1571 passed
npm run typecheck → clean
npm run lint:fix → clean
Summary by CodeRabbit
test rerun --waittimeout handling to emit a parseable partial JSON progress payload to stdout before exiting with the expected timeout code.runIds in the partial output withtimeout/runningstatus.