Repository navigation
test: reuse modules across host snapshot recovery cases - #29471
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation |
|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 1 files. |
| Linked Issues check | |
Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | |
Check skipped because no linked issues were found for this pull request. |
| Title check | |
The title clearly and concisely describes the main change: reusing modules across host snapshot recovery test cases. |
| Description check | |
The description covers the required sections, explains the change and rationale, documents testing and limitations, and identifies the visual proof as not applicable. The missing issue link is reasona… |
Full details: Description check
-
Explanation
The description covers the required sections, explains the change and rationale, documents testing and limitations, and identifies the visual proof as not applicable. The missing issue link is reasonably explained by the maintainer exception, and CI validation is documented later in the description.
✨ Finishing Touches: 💡 1
-
📝 Generate docstrings
-
- Commit to this branch
- Create a new PR
🛠️ Fix failing CI checks: 💡
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 @coderabbitai help to get the list of available commands.
…st-snapshot-module-reuse
…-module-reuse' into OrcaWin/test-audit-host-snapshot-module-reuse
ELI5
The three remote snapshot recovery tests now load their application modules once per file. Each case still gets its own runtime, transport, pane, store and terminal buffers.
What Changed
Remove the per-case module-cache reset and await the existing asynchronous terminal environment setup before starting each case. Keep the dynamic imports, fresh fixtures, cleanup and every case body/assertion unchanged. No application code changes.
Why
Reloading the full host and renderer graph for every case costs time without adding isolation to these already separate objects. Waiting for setup explicitly prevents a cached import from letting a case start before its fake browser API and startup snapshot are ready. Existing transport disposal releases the last multiplexer stream; existing teardown disposes startup snapshots and restores mocks. This uses those lifecycle boundaries rather than adding a test-only production reset.
Linked Issue
Maintainer-requested test audit; no separate issue.
Visual Proof
N/A — test setup only; no application behavior or UI changes.
Testing
Linux, Bun 1.4.3 / Node 24.21.0, one worker and background launch: three alternating full-file pairs pass the same three cases. Median module time 11.919 → 9.898 s (16.96% lower; ranges 11.822–11.949 vs 9.540–9.946 s). Median process wall 13.437 → 11.521 s (14.25% lower; ranges 13.405–13.628 vs 11.434–11.674 s). Full-file measurements include imports; no whole-PR gain claimed.
Ignoring the actual replay painter's history flag makes both original and candidate fail the same existing assertion: recovered normal history lacks MISSED-299. Production bytes restored exactly. All 59 owner/replay/history/paint sibling cases pass; shuffled owner passes all three. Web typecheck, normal and anti-slop lint, formatting, diff and all changed-code quality checks pass. Independent final review approved fixture isolation, awaited setup, unchanged case bodies and caught history regression; Hosted original exact source qualified at run 38850544014 / merge ebd94e7: all three cases passed, module 9.711 s on Blacksmith X64 four workers / Node 24.19.0 / Bun 1.4.3. Candidate hosted result is reported below; this historical unpaired original is not a controlled whole-PR baseline.
These cases exercise the real host headless model, snapshot publisher, wire codec, remote transport, pane replay and xterm buffer results through in-process boundary fixtures. They do not exercise a physical SSH connection, OS PTY or rendered Electron window. Existing baseline sometimes logs a missing fake Codex account API from a late sweep; candidate runs have no such warning after explicitly awaited setup. No warning suppression was added.
Review
Cross-platform, SSH and mixed-version contracts remain in the unchanged tests. All three test bodies and their assertions are byte-identical. Preserve live-TUI alternate-frame/covered-history parity, exited-TUI normal recovery and missed history between two TUI sessions.
Agent skill upstream boundary
Checklist
N/Awith reasonpnpm lint,pnpm typecheck,pnpm test, andpnpm buildpass (or CI will cover; local preferred)Refresh validation: Refreshed onto main 723c258 after #29499 repaired inherited CI failures. Candidate bytes remain identical to the approved patch. Refreshed target passes all three cases (official module 9,609 ms); five recovery/snapshot sibling suites pass 55 cases. Fresh exact-head run 38866749458 has passed.
Hosted validation: Exact-head full PR run 38866749458 passed, including aggregate PR checks passed and all five clean unit shards. All three target cases passed; official module cost 6,387 ms versus historical original 9,711 ms. This 34.2% lower observation is descriptive: main, dependencies, runner and scheduling differ, so no causal or whole-PR saving is claimed.