test(file-suggestions): stop cross-spawn mock leaking into later suites#1667
Conversation
fileSuggestions.test.ts installs a cross-spawn mock via mock.module, which bun does NOT undo on mock.restore() — it persists process-wide. The mock's interception was gated on a closure captured at install time, so after this suite the persisted mock kept returning a fake child (with no kill()) for any git command. Test files run sequentially in one process, so a later suite's real `git ls-files` (e.g. /lsp recommend's filesystem-scan fallback) hit the fake child, hung to its 5s timeout, and crashed on child.kill() — an order-dependent failure in smoke-and-tests. Gate the interception on a module-level activeSpawnScenario that is set only while one of this suite's spawn-scenario tests runs and cleared in afterEach, so the persisted mock falls through to the real spawn afterward. Also give the fake child a kill() that emits close, so any stray caller terminates cleanly instead of throwing. Verified the full src suite is green across repeated runs (was intermittently red on the /lsp filesystem-scan test).
|
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)
📜 Recent review details⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🧰 Additional context used📓 Path-based instructions (4)**/*.{ts,tsx,js,jsx,py}📄 CodeRabbit inference engine (CONTRIBUTING.md)
Files:
**/*⚙️ CodeRabbit configuration file
Files:
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}⚙️ CodeRabbit configuration file
Files:
**⚙️ CodeRabbit configuration file
Files:
🔇 Additional comments (1)
📝 WalkthroughWalkthroughThe Changescross-spawn mock scoping and fake child process fix
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 6 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (6 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
Thanks for the contribution. I do not see any actionable issues from my review.
@kevincodex1 LGTM
3 participants
Problem
src/commands/lsp/lsp.test.ts→/lsp recommend > falls back to filesystem scanning when git cannot enumerate workspace filesfails intermittently insmoke-and-testswith:It reproduces locally on a full
bun test src/run (and onmain), but passes in isolation — a classic order-dependent test-isolation bug.Root cause
src/hooks/fileSuggestions.test.tsmockscross-spawnviamock.module(...). bun'smock.restore()resets spies but does not undomock.module— the module mock persists process-wide for the rest of the run. Its interception was gated onoptions.spawnScenario, captured in the factory closure at install time, so after this suite finished the persisted mock kept returning a fake child (a bareEventEmitter, nokill()) for anygitcommand.bun runs test files sequentially in one process, so a later suite's real
git ls-files(the/lsp recommendfilesystem-scan fallback) hit the leaked fake child, which never emitsclose.execFileNoThrowwaited out its 5s timeout, then calledchild.kill()→TypeError, and the 5s wait also blew the test's own timeout.Fix
activeSpawnScenario, set only while one of this suite's spawn-scenario tests runs and cleared inafterEach. Once cleared, the persisted mock falls through to the realspawn, so later suites' git commands are untouched.kill()that emitsclose, so any stray caller terminates cleanly instead of throwing.Verification
fileSuggestions.test.tsin isolation: 9/9 pass.bun test src/green across repeated runs (previously intermittently red on the/lsptest).tsc --noEmitclean.Summary by CodeRabbit
No user-facing changes in this release.