test(storage): give the policy suite an explicit harness budget - #732
Merged
Conversation
seedHome() writes real files and a sqlite fixture per case. On a slow
windows-latest runner those cases take 6-8s against bun's 5s default and fail
deterministically (run 30507689929 failed the same five twice, the second time
after a --failed rerun) while a faster runner passes the identical code —
locally the file is 22 pass in ~100ms.
The sibling storage-cleanup suite already carries { timeout: 20_000 } /
{ timeout: 30_000 } on its seeding-heavy cases with a comment naming the same
cause. This file had no budget at all, so one file-level setDefaultTimeout(30_000)
covers all fifteen seedHome sites. setDefaultTimeout is the existing convention
here (grok-management-api, codex-history-provider, claude-management-api,
server-combo-failover-e2e, subagent-fallback-handle-responses all use 30_000).
Test-harness only: no src/storage change, since nothing indicates the selection
logic is wrong.
Closes #727
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe storage policy test suite now imports ChangesStorage policy test harness
Estimated code review effort: 1 (Trivial) | ~2 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #727.
What breaks
tests/storage-policy.test.tsinherits bun's 5s default harness timeout. ItsseedHome()helper writes real files plus a sqlite fixture for every case itbacks — 15 call sites — and on a slow
windows-latestrunner those cases land at6-8s and fail. Run 30507689929
failed five of them, then failed the same set again after a
--failedrerun, soit reproduces rather than flakes. Locally the whole file is 22 pass in ~100ms.
Every failure sat between 6s and 8s against a 5s ceiling: these are timeouts,
not assertion failures, and nothing indicates the selection logic is wrong.
What changes
One file,
setDefaultTimeout(30_000)plus a comment naming the cause and therun id. That is the same budget the sibling
tests/storage-cleanup.test.tsalready gives its seeding-heavy cases —
{ timeout: 20_000 }at 14 sites and{ timeout: 30_000 }at 5, with a comment saying seeding "trips bun's default5s harness timeout". A file-level call covers all 15
seedHomesites herewithout touching each case, and
setDefaultTimeout(30_000)is already theconvention in
grok-management-api,codex-history-provider,claude-management-api,server-combo-failover-e2eandsubagent-fallback-handle-responses.No
src/change. No workflow change — the job-level ceiling was already raisedto 20 minutes in #717, and this is the per-test budget underneath it.
Verification
Mechanism proven in isolation first, under CI's
--isolatemode: a 6s case withno budget fails with
this test timed out after 5000ms; the identical case withsetDefaultTimeoutpasses at 6005ms. Same failure shape as #727.bun x tsc --noEmit— exit 0bun test --isolate tests/storage-policy.test.ts— 22 pass, 0 fail, 91 expect() callsWindows behaviour is what CI has to confirm; the local run only proves the
change is inert on a fast filesystem.
Follow-up, not in scope
Windows pays roughly 2.5x for this suite versus ubuntu. Raising budgets hides
the margin, it does not close the gap. The suspect is
bun test --isolateinci.yml(per-test process isolation is expensive on Windows), which is areliability tradeoff and an owner call, not something to fold into a timeout fix.
Making
seedHomecheaper is the other candidate.Summary by CodeRabbit