fix(test): prevent cross-file mock leak in SDK executor tests - #1075
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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.
Code Review
This pull request introduces a centralized resetAllMocks function in the shared test mocks file to ensure a clean state between tests and prevent cross-file leaks in the Bun test environment. It replaces manual mock clearing with this unified reset and removes afterAll(mock.restore()) calls in favor of per-test resets. Feedback focuses on ensuring all shared mocks, specifically directoryResolveMock, are included in the reset logic and moving the reset call to a top-level hook in claude-sdk.test.ts to guarantee isolation for all test cases.
| terminateExecutorMock.mockReset(); | ||
| terminateExecutorMock.mockImplementation(async () => undefined); | ||
|
|
||
| resetQueryMock(); |
There was a problem hiding this comment.
The resetAllMocks function is missing a reset for directoryResolveMock. Since this mock is shared across test files and can be overridden (e.g., in claude-sdk.test.ts), it should be restored to its default implementation to ensure test isolation and prevent cross-file leaks.
resetQueryMock();
directoryResolveMock.mockReset();
directoryResolveMock.mockImplementation(async (name: string) => ({
entry: {
name,
dir: '/tmp/test',
promptMode: 'system' as const,
model: 'sonnet',
registeredAt: new Date().toISOString(),
permissions: { preset: 'full' },
},
builtin: false,
}));| createAndLinkExecutorMock.mockClear(); | ||
| updateExecutorStateMock.mockClear(); | ||
| terminateExecutorMock.mockClear(); | ||
| resetAllMocks(); |
There was a problem hiding this comment.
In this file, resetAllMocks() is only called within the World A registry integration block. To fully prevent cross-test leaks as intended by this PR, it should be moved to the top-level beforeEach (around line 35). This ensures that earlier tests (like spawn and deliver) also start with a clean slate, which is important since they rely on shared mocks like directory.resolve and queryMock.
Root cause: three interrelated issues caused CI-only failures: 1. mock.restore() in afterAll clears all process-global mock.module registrations, breaking whichever test file runs second 2. mockClear() only clears call counts but not mockImplementation() overrides set by other test files' nested describe blocks 3. Concurrent delivery afterEach set queryMock via dynamic import without session_id in result events Fix: - Add resetAllMocks() to _sdk-mocks.ts that does mockReset() + mockImplementation() for every shared mock function - Use resetAllMocks() in both test files' beforeEach blocks - Remove mock.restore() from both files' afterAll - Use shared queryMock directly instead of dynamic imports
da9ba74 to
cafa880
Compare
Summary
mock.restore()fromafterAllin both SDK test files -- it clears all process-global mock.module registrations, breaking whichever file runs second in CIresetAllMocks()to shared_sdk-mocks.tsthat doesmockReset()+mockImplementation()for every shared mock (stronger thanmockClear()which only clears call counts)resetAllMocks()in both test files'beforeEachto guarantee clean slate regardless of file execution orderqueryMockdirectly in concurrent delivery tests instead of dynamic importsRoot cause
Three interrelated issues caused CI-only failures (11 tests):
mock.restore()inafterAllclears ALL process-globalmock.moduleregistrations, so whichever test file runs second loses its mocksmockClear()only clears call counts but does NOT resetmockImplementation()overrides set by other files' nesteddescribeblocksafterEachrestored queryMock via dynamic import withoutsession_idin result eventsTest plan