test: widen timing margins on two Windows-flaky CI tests#359
Conversation
Both failed on windows-latest today, on merges of otherwise-unrelated PRs (#349, #356) - neither PR touched internal/specialist or internal/agenteval. Windows GitHub Actions runners have coarser default timer granularity and more scheduling contention than Linux/macOS, so tight timing margins that hold on the other two platforms aren't reliable there. - TestOutputToolBlocksUntilTaskCompletes: a 100ms poll timeout against a 5ms background write left only 20x margin. Widened the timeout to 2000ms (a ceiling, not a target - PollInterval keeps the test fast in the success case; it only changes how much CI jitter it tolerates before failing). - TestHarnessTimeoutCancelsBlockedAgent: confirmed there's no actual data race (harness.Agent.Run is a direct synchronous call, not a spawned goroutine racing the assertions) - the failure was workspace materialization alone consuming the full 10s budget on a contended Windows runner, so the agent was never reached. Widened to 30s. This test's own design blocks for the full timeout to observe cancellation, so unlike the other fix this directly costs ~20s more wall-clock time, but that's inherent to what it's testing.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughTwo test files had their timeout values increased to reduce flakiness under CI contention: a benchmark harness timeout raised from 10s to 30s, and an output tool test timeout raised from 100 to 2000, both with added explanatory comments. ChangesTest Timeout Adjustments
Estimated code review effort: 1 (Trivial) | ~3 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
Comment |
Zero automated PR reviewVerdict: No blockers found Blockers
Validation
ScopeHead: This deterministic review checks validation status and basic diff hygiene. A human reviewer still owns product judgment and design quality. |
Summary
Investigated two
windows-latestsmoke-test failures from today's merges. Both are pre-existing test-timing issues, unrelated to the content of the PRs whose merge-to-main runs they failed on (#349, #356 — neither touchesinternal/specialistorinternal/agenteval).TestOutputToolBlocksUntilTaskCompletes: a 100ms poll timeout against a 5ms background write left only 20x margin — not enough headroom against Windows' coarser default timer granularity and heavier CI scheduling contention than Linux/macOS. Widened the timeout to 2000ms. This is a ceiling, not a target:PollIntervalkeeps the test fast in the success case (it still returns as soon as output lands), it just tolerates more jitter before failing.TestHarnessTimeoutCancelsBlockedAgent: confirmed there's no actual data race first (harness.Agent.Runis a direct synchronous call on the test's own goroutine, not a spawned goroutine racing the assertions). The failure was workspace materialization alone consuming the full 10s budget on a contended Windows runner, so the agent was never reached before the deadline fired. Widened to 30s. Unlike the other fix, this test's own design blocks for the full timeout to observe cancellation, so this directly costs ~20s more wall-clock time — inherent to what it's testing, not avoidable without restructuring the harness contract.Test plan
go build ./...go test ./internal/specialist/ -count=1 -run TestOutputToolBlocksUntilTaskCompletes -v(0.01s)go test ./internal/agenteval/ -count=1 -run TestHarnessTimeoutCancelsBlockedAgent -v(30.03s, as expected)go test ./internal/specialist/ ./internal/agenteval/ -count=1(full packages)go test ./... -race -count=1(full repo)Summary by CodeRabbit