fix(test): preserve captured output after lane timeouts - #3924
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Team Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review. 📝 WalkthroughWalkthroughThe test runner now captures stdout and stderr with bounded draining, preserves partial output, forwards output through injectable writers, and marks incomplete captures as failures. Tests cover stream, timeout, cancellation, and lane-result behavior. The contributing guide documents these semantics. ChangesTest output capture
Estimated code review effort: 3 (Moderate) | ~25 minutes Merge Risk: ⚪ Minimal · up to The test runner now retains and prints partial stdout/stderr for timed-out lanes, bounds pipe draining, and treats incomplete successful captures as failures. No concrete current-head merge-blocking risk remains. Sequence Diagram(s)sequenceDiagram
participant runTestLane
participant Bun.spawn
participant captureTestOutput
participant stdoutWriter
participant stderrWriter
runTestLane->>Bun.spawn: start test lane
Bun.spawn-->>captureTestOutput: provide stdout and stderr streams
captureTestOutput-->>runTestLane: return output and complete status
runTestLane->>stdoutWriter: emit captured stdout
runTestLane->>stderrWriter: emit captured stderr
runTestLane-->>runTestLane: return exit code 0, 1, or 124
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 4 functions across 2 files. (1 skipped: 1 unsupported.)
✨ 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 |
|
✅ Deterministic PR hygiene checks passed. |
✅ READY
Review readiness checklist
✅ 4/4 boxes ticked. This pull request has been marked Ready for Review. |
|
@coderabbitai review |
✅ Action performedReview finished.
|
리뷰 · 우선순위 58 / 80이 PR은 라인 scripts/test.ts captureTestOutput - 타임아웃 전에 모은 출력을 남기고, finish(1000)로 배수(drain)를 1초로 묶어 자손 파이프에 막히지 않게 한 핵심 경로다. 메인테이너의 판단이 필요한 지점
너의 추천 이 댓글은 grok-bot이 작성했습니다 |
…or stream-health watchdog (#3940) Preserve captured stdout and stderr when a test lane times out. Bound post-exit pipe draining to one second and report incomplete capture as a failure when the child otherwise exited successfully. Scale the Cursor stream-health fixture deadlines from one load-adjusted budget and verify meaningful progress continues for the required interval after the client receives its first text. Carries #3924 and #3930 without changes to their files. The branch commits retain the original author and cherry-pick provenance from: e241632 141077f Co-authored-by: luvs01 <27862058+luvs01@users.noreply.github.com>
|
Landed on Your commit It landed as the lower layer of a two-layer stack together with #3930, so one CI run verified both changes on the cumulative tree: run 34152136978, all 16 applicable jobs green. Your own contributor run on Closing as superseded by that verified landing. Thank you — losing the output that explains a timeout is exactly the kind of failure that wastes an afternoon, and the incomplete-capture rule is the part I expect to pay for itself. |
2 participants
Summary
When a captured test lane exceeded its timeout, the runner returned exit code 124 before forwarding any stdout or stderr. A long
test:changedrun therefore lost the diagnostics needed to explain its failure.Collect output as it arrives and forward the retained stdout and stderr before returning the timeout result. After process exit, allow one second for the pipes to drain; a descendant holding a pipe open cannot cause an unlimited wait. Report incomplete capture and return a failure when the direct child exited successfully but its captured output is incomplete. Preserve the existing signal escalation, interruption exit codes, and inherited-output mode.
The contributor guide documents the timeout and incomplete-output behavior. Scope is limited to the runner, its regression tests, and that guide.
Verification
Head
e24163231edeaa09a30a99ca1746e3b573af78ae, based ondevaeefb3ab5433c69f7621695413ec1f77a05ed54f. Tree:8c7c2aae804be1eee69049751534753b6e688f12.bun test tests/ci-workflows/test-runner.test.ts --timeout 60000: 52 passed with the project-pinned Bun 1.4.0 on Windows. The subsequent rebase changes only README and sponsor content; the tested implementation, tests, dependencies, and documentation build inputs are identical.bun run typecheck,bun run privacy:scan, andgit diff --check: passed.The full local
bun run prepushcheck reached the parallel lane's 900-second limit (exit 124) before a complete suite summary. Its runner and directly owned workers were confirmed absent afterward. This incomplete run is not a pass, and the identical timed-out run has not been repeated. Full cross-platform CI passed all 26 jobs on this exact head, including Linux shards, Windows shards, macOS lanes/control, gates, and platform checks. Contributor readiness uses that full CI and the focused local checks above; the incomplete local full run remains recorded and is not treated as a pass. CodeRabbit completed with no actionable findings or unresolved review threads. Maintainer approval and upstream-required CI remain separate merge requirements.Checklist
Review readiness checklist
Summary by CodeRabbit
Bug Fixes
Documentation