Repository navigation
[CI] test: remove idle polling from compaction contracts - #26373
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (1)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 6 remain after this review. 📝 WalkthroughWalkthroughThe compaction tests now pass Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to This change only adjusts polling frequency in the compaction tests; no specific merge risk is evident. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
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 |
nwparker
changed the title
1 participant
ELI5
The compaction tests spend repeated 50ms gaps waiting for positive results that are already available. Check those same results every 1ms so successful tests finish sooner.
What Changed
Add
{ interval: 1 }to 24 existing synchronousvi.waitForassertions instructured-conversation-compaction.test.ts. All 21 cases, callbacks, assertions and default 1,000ms deadlines remain. Asynchronous journal waits, child-exit checks, fake clocks and the real 20/50ms quiet windows retain their original behavior.Why
Use the existing polling API to remove idle time while retaining real asynchronous work and failure detection. Shorter deadlines, fewer cases or replacing the real clock would weaken these contracts. More frequent polling can use additional CPU when the expected result does not arrive.
Linked Issue
Related measured test-suite improvements: #26327 and #26360.
Visual Proof
N/A — test polling only; no visual or interaction change.
Testing
Correctness run 37699229724 passed all eight original/candidate commands under Bun 1.4.2 and actual Node 24.21.0 with shuffle seeds 104729 and 130363. Every command retained all 21 original case identities and passed them without skips. Original source, runtime routes, four workers, isolation, six setup files, errors and normal process exit were checked independently.
Timing run 37705183920 retained every whole
pnpm testcommand in before/after, after/before, before/after order:All three pairs improve, including the first commands. This is 1.18× faster for this fixture with 21 cases. There was no warmup, retry or discarded sample. All six commands passed the same original cases with four workers, isolation, six setup files and disabled Vitest filesystem module cache. The held Linux ARM host used Vitest 5.0.3 and the pinned runtimes above. Shared OS and Bun transpiler caches can still warm between counted commands.
The measurements use frozen source 31a5b42. The product fixture before/after bytes match that source exactly on base 5a1009f. Root and an independent reviewer replayed the collector, all six raw case/status maps, settings, process evidence and arithmetic. This single-file result does not establish the overall 2× full-suite goal.
Earlier timing attempt 37702809683 failed before launching any timed command because its admission record used a string run ID where the validator required a number. That failed attempt and its raw artifact remain retained; the corrected definition changes only the admission ID type. No failed timing sample was replaced or discarded.
oxfmt --check, changed-code checks and Node typecheck passed; no native or Electron test launched locally.Review
Independent source, all-eight correctness and all-six raw timing reviews passed. Root removed the 24 new polling options and recovered the entire original module byte for byte. All current-head checks passed or were explicitly skipped by scope. CodeRabbit reviewed this head and reported no actionable comments; there are no unresolved review threads.
Agent skill upstream boundary
Notes
No production behavior, process termination, SSH execution, remote protocol or workspace/platform routing changes. Native/runtime contracts remain under actual Node where configured and covered by CI.
Checklist
Final merge boundary
Current-head CI qualifies
eeaf0cafd6edc21b6292aebc2489fb6b7a98a97b. Main was65a0e9620ee05143642332e10bfc3a43c6b212f0at the final source check, which is later than the candidate base5a1009f82fa1e1ae5e33e5ff965885fadc00415e; the compaction fixture still matches the measured before bytes exactly. Other upstream changes were not covered by the candidate CI. Merged at369b202b0168bc772e9405c0108e062d5e137712with actual parent65a0e9620ee05143642332e10bfc3a43c6b212f0. The actual squash changes only this test file, and both parent/candidate fixture hashes match the measured before/after bytes exactly. The actual parent differs from the candidate base, so the candidate CI does not qualify the additional upstream changes.