test(windows): stabilize advisory loop and bootstrap cases#579
Conversation
There was a problem hiding this comment.
Suggested priority: P2 (includes user-path files (packages/app/src/context/global-sync/bootstrap.test.ts)).
P1/P0 are reserved for maintainer confirmation. Please relabel manually if this is a release blocker, security issue, data-loss risk, or updater/runtime failure.
|
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 (2)
📝 WalkthroughWalkthroughThis PR addresses Windows test flakiness with two changes: session status error synchronization that waits for the correct state assertion, and platform-aware timeout constants for session loop tests that replace hardcoded millisecond values. ChangesWindows Test Stability Fixes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
There was a problem hiding this comment.
Code Review
This pull request updates test logic to wait for an explicit error state in bootstrap.test.ts and replaces hardcoded timeouts with slowIOTimeout in prompt-effect.test.ts to improve cross-platform stability. Feedback suggests further replacing the default timeout in the waitFor call with slowIOTimeout to avoid potential flakiness on Windows CI and identifies several other locations in prompt-effect.test.ts where hardcoded timeouts should be updated for consistency.
…656) Why windows-advisory has been flaking at ~55% on dev push (27 failures / 50 runs in the latest sample). bun's default 3s it.live timeout is consistently tight on the Windows runner for Effect-fiber + SQLite + tmpdir-server tests in packages/opencode/test/session/prompt-effect.test.ts. Four prior PRs (#543, a3b8e54, cf6d1cd, #579) each bumped one or two tests at a time to a slowIOTimeout constant without converging — today's failure on "cancel records MessageAbortedError on interrupted process" was the fifth instance of the same root cause. What Wrap testEffect's live runner via withDefaultLiveTimeout so every it.live in prompt-effect.test.ts picks up a Windows-aware default (10s Windows / 3s elsewhere). Remove 12 third-arg timeout literals (5 x 3_000, 5 x slowIOTimeout, 2 x shellQueueTimeout) and the two duplicated `const ... Timeout` definitions. The wrapper also covers .only and .skip for symmetry. Explicit non-default timeouts (5_000, 10_000, 30_000) still override. Out of scope bun 1.3.13 watcher.node segfault on Windows process exit, and transient actions/cache failures, are upstream / infra and intentionally left to fail "normally" per d6fa1e6. The advisory workflow is not in branch protection and existing if: always() artifact and summary uploads preserve the diagnostic signal. Verification - tsgo --noEmit: clean - bun test test/session/prompt-effect.test.ts --timeout 30000: 54 pass / 0 fail / 28.69s (macOS) - bun test test/github/ci-workflow.test.ts --timeout 30000: 9 pass / 0 fail - PR CI on 4d4792c: all green (typecheck, lint, unit-app, unit-desktop, unit-opencode, e2e-artifacts, smoke-macos-arm64, analyze-js-ts, CodeQL) Review follow-ups Gemini suggested also wrapping .only / .skip; applied and thread resolved. An external review flagged a P1 about .only not being wrapped; audited against HEAD and the claim referenced the pre-amend version (8ad2c1a), not the current 4d4792c — no change required. Risk Detection of a true hang on Windows live tests now takes up to 10s rather than 3s. Acceptable for an advisory signal. No production code paths touched. windows-advisory will still show occasional red runs from upstream / infra causes by design.
Summary
slowIOTimeoutbudgetprovider_readybranchWhy
Two chronic Windows advisory failures were open at the same time:
packages/opencodesession loop tests still used a hardcoded3000mstimeout even though the file already had the establishedslowIOTimeoutconstant for Windows-sensitive casesbootstrapDirectory > marks session status as error when status hydration failstest waited onprovider_readybefore assertingsession_status_state === "error", but provider refresh and session-status hydration run in parallel, so Windows timing could satisfy the wait before the error branch settledThese are different failure modes, but both close cleanly in the test layer.
Related Issue
Closes #555.
Closes #546.
Human Review Status
Pending. A human should make the final merge decision after reviewing the final diff and verification evidence.
Review Focus
packages/opencode/test/session/prompt-effect.test.ts: only the two named advisory loop tests should move from3000toslowIOTimeoutpackages/app/src/context/global-sync/bootstrap.test.ts: the error-path test should now wait forsession_status_state === "error", matching the behavior it actually verifiesRisk Notes
Low. Test-only changes. No runtime or product behavior changes.
How To Verify
Screenshots or Recordings
Not needed. No visible UI changes.
Checklist
dev, and my PR title and commit messages use Conventional Commits in EnglishSummary by CodeRabbit