refactor(opencode): default it.live tests to platform-aware timeout - #656
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
✨ 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 |
There was a problem hiding this comment.
Suggested priority: P3 (only low-risk paths changed (.github/workflows/windows-advisory.yml, packages/opencode/test/session/prompt-effect.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.
There was a problem hiding this comment.
Code Review
This pull request introduces a withDefaultLiveTimeout utility to automatically apply platform-specific timeouts for test effects, addressing slow execution on Windows runners and removing redundant timeout arguments across the test suite. A review comment suggests improving the wrapper to also apply these default timeouts to the .only and .skip modifiers, ensuring consistent behavior during targeted test execution.
Wrap testEffect's live runner in prompt-effect.test.ts so every live test picks up a Windows-aware default timeout. Previously each new live test needed a hand-tuned third-arg timeout to avoid flaking on the slow Windows runner, and the file accumulated two duplicate slowIOTimeout/shellQueueTimeout constants plus 12 per-test overrides. Removing the overrides keeps the same effective coverage while making the default the right thing. Explicit third-arg timeouts (5_000 / 10_000 / 30_000) still override. Verified locally: typecheck clean, 54/54 pass in 25s on macOS.
6819a4f to
4d4792c
Compare
Summary
Replace hand-tuned per-test timeouts in
packages/opencode/test/session/prompt-effect.test.tswith a single Windows-aware default applied through anit.livewrapper. Removes 12 third-arg timeout literals (5 ×3_000, 5 ×slowIOTimeout, 2 ×shellQueueTimeout) plus the two duplicatedconst ... Timeoutdefinitions. Explicit non-default timeouts (5_000,10_000,30_000) still override. The wrapper also covers.onlyand.skipso debug and skip variants do not fall back to bun's 3-second default.Why
windows-advisory has been flaking at ~55% on dev push (27 failures / 50 runs in the latest sample), driven by three independent failure modes, only one of which is fixable in our test code. H1: bun's default 3-second
it.livetimeout is consistently tight on the Windows runner for Effect-fiber + SQLite + tmpdir-server tests in this file. Over the past 11 days four prior PRs (#543, a3b8e54, cf6d1cd, #579) each bumped one or two tests at a time to aslowIOTimeoutconstant without converging — today's failure oncancel records MessageAbortedError on interrupted processwas the fifth instance of the same root cause. This change makes the slow-runner default the right thing once and ends the per-test bandaging pattern. H2 (bun 1.3.13watcher.nodesegfault on Windows process exit) and H3 (transientactions/cache) are upstream / infra and intentionally left to fail "normally" per the design decision in d6fa1e6: the advisory workflow is already not in branch protection, and existingif: always()artifact and summary uploads preserve the diagnostic signal without masking the red run.Related Issue
No issue. Surfaced from a direct investigation of dev-push CI noise.
Human Review Status
Pending. A human should make the final merge decision after reviewing the final diff and verification evidence.
Review Focus
The
withDefaultLiveTimeoutwrapper's type signature inprompt-effect.test.ts:276intentionally uses structuralany[]parameters to avoid widening thetestEffectruntime layer's generic tounknown(an earlierReturnType<typeof testEffect>attempt failed typecheck with a layer-incompatibility cascade). Confirm this trade-off is acceptable. The wrapper now also covers.onlyand.skipafter a Gemini suggestion — there is currently no.onlyusage in the file, but applying defaults consistently avoids a future Windows-debug surprise. Every previously hand-tuned site (3_000/slowIOTimeout/shellQueueTimeout) now relies on the 10-second Windows default — confirm none of those sites used a tight timeout as a deliberate budget assertion rather than "slowest we'll tolerate".Risk Notes
If a future bug makes a live test actually hang, detection on Windows now takes up to 10 seconds rather than 3. Acceptable for an advisory signal. No production code paths touched. windows-advisory will still show occasional red runs from H2 / H3 by design.
How To Verify
Windows-side validation will come from windows-advisory runs on the post-merge dev push: the
cancel records ...test and the other four hard-coded3_000sites should stop timing out. The bun-watcher and cache failure modes are deferred and will continue to show as occasional red runs by design.Screenshots or Recordings
N/A — no UI change.
Checklist
dev, and my PR title and commit messages use Conventional Commits in English