test(cleanup): fix temporary directory leaks in test suites#26217
test(cleanup): fix temporary directory leaks in test suites#26217
Conversation
Add proper afterEach cleanup logic to mcp-client.test.ts, extension-manager-scope.test.ts, and configure.test.ts to ensure temporary directories are removed after test completion. Fixes #22032
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses persistent issues where test suites were failing to clean up temporary directories, resulting in significant disk space usage and clutter. By standardizing the teardown process and ensuring proper resource release, this change improves the reliability and cleanliness of the test environment across different operating systems. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces cleanup logic in the test suites to address temporary directory leaks. The reviewer identified unrelated changes in package-lock.json that should be reverted to maintain focus. Additionally, the reviewer recommended refactoring the manual platform-specific timeouts in the cleanup code to use the built-in maxRetries and retryDelay options of fs.rmSync for improved robustness.
I am having trouble creating individual review comments. Click here to see my feedback.
package-lock.json (453)
The package-lock.json contains numerous unrelated changes adding "peer": true to various dependencies. These changes appear to be a side effect of using a different npm version or environment and are unrelated to the PR's goal of fixing temporary directory leaks. Please revert these changes to keep the PR focused and avoid potential issues with dependency resolution in other environments.
References
- Pull Requests should be kept small and focused. (link)
packages/cli/src/commands/extensions/configure.test.ts (101-111)
Instead of a manual setTimeout and platform check, you can use the built-in maxRetries and retryDelay options in fs.rmSync. This is more robust, cleaner, and avoids a mandatory delay on every test run. It also allows the afterEach to be synchronous, which is preferred in test environments for simplicity.
afterEach(() => {
try {
if (tempWorkspaceDir) {
fs.rmSync(tempWorkspaceDir, {
recursive: true,
force: true,
maxRetries: 10,
retryDelay: 100,
});
}
} catch {
// ignore
}
});
References
- For test environment setup, it is acceptable to use synchronous operations (e.g., fs.*Sync, execSync) within an async function. This approach is preferred for its simplicity and readability when the setup is sequential and not a performance bottleneck.
packages/cli/src/config/extension-manager-scope.test.ts (90-102)
Instead of a manual setTimeout and platform check, you can use the built-in maxRetries and retryDelay options in fs.rmSync. This is more robust, cleaner, and avoids a mandatory delay on every test run. It also allows the afterEach to be synchronous, which is preferred in test environments for simplicity.
afterEach(() => {
for (const dir of [currentTempHome, tempWorkspace]) {
try {
if (dir) {
fs.rmSync(dir, {
recursive: true,
force: true,
maxRetries: 10,
retryDelay: 100,
});
}
} catch {
// ignore
}
}
});
References
- For test environment setup, it is acceptable to use synchronous operations (e.g., fs.*Sync, execSync) within an async function. This approach is preferred for its simplicity and readability when the setup is sequential and not a performance bottleneck.
packages/core/src/tools/mcp-client.test.ts (108-118)
Instead of a manual setTimeout and platform check, you can use the built-in maxRetries and retryDelay options in fs.rmSync. This is more robust, cleaner, and avoids a mandatory delay on every test run. It also allows the afterEach to be synchronous, which is preferred in test environments for simplicity.
afterEach(() => {
try {
if (testWorkspace) {
fs.rmSync(testWorkspace, {
recursive: true,
force: true,
maxRetries: 10,
retryDelay: 100,
});
}
} catch {
// ignore
}
});References
- For test environment setup, it is acceptable to use synchronous operations (e.g., fs.*Sync, execSync) within an async function. This approach is preferred for its simplicity and readability when the setup is sequential and not a performance bottleneck.
packages/core/src/tools/mcp-client.test.ts (2424-2434)
Instead of a manual setTimeout and platform check, you can use the built-in maxRetries and retryDelay options in fs.rmSync. This is more robust, cleaner, and avoids a mandatory delay on every test run. It also allows the afterEach to be synchronous, which is preferred in test environments for simplicity.
afterEach(() => {
try {
if (testWorkspace) {
fs.rmSync(testWorkspace, {
recursive: true,
force: true,
maxRetries: 10,
retryDelay: 100,
});
}
} catch {
// ignore
}
});
References
- For test environment setup, it is acceptable to use synchronous operations (e.g., fs.*Sync, execSync) within an async function. This approach is preferred for its simplicity and readability when the setup is sequential and not a performance bottleneck.
packages/core/src/tools/mcp-client.test.ts (2642-2652)
Instead of a manual setTimeout and platform check, you can use the built-in maxRetries and retryDelay options in fs.rmSync. This is more robust, cleaner, and avoids a mandatory delay on every test run. It also allows the afterEach to be synchronous, which is preferred in test environments for simplicity.
afterEach(() => {
try {
if (testWorkspace) {
fs.rmSync(testWorkspace, {
recursive: true,
force: true,
maxRetries: 10,
retryDelay: 100,
});
}
} catch {
// ignore
}
});
References
- For test environment setup, it is acceptable to use synchronous operations (e.g., fs.*Sync, execSync) within an async function. This approach is preferred for its simplicity and readability when the setup is sequential and not a performance bottleneck.
packages/core/src/tools/mcp-client.test.ts (2816-2826)
Instead of a manual setTimeout and platform check, you can use the built-in maxRetries and retryDelay options in fs.rmSync. This is more robust, cleaner, and avoids a mandatory delay on every test run. It also allows the afterEach to be synchronous, which is preferred in test environments for simplicity.
afterEach(() => {
try {
if (testWorkspace) {
fs.rmSync(testWorkspace, {
recursive: true,
force: true,
maxRetries: 10,
retryDelay: 100,
});
}
} catch {
// ignore
}
});References
- For test environment setup, it is acceptable to use synchronous operations (e.g., fs.*Sync, execSync) within an async function. This approach is preferred for its simplicity and readability when the setup is sequential and not a performance bottleneck.
|
Size Change: -4 B (0%) Total Size: 33.9 MB
ℹ️ View Unchanged
|
Moving vi.useRealTimers() to the top of afterEach hooks ensures that any async operations (like microtasks or the Windows file handle delay) don't hang when fake timers were active in the test.
devr0306
left a comment
There was a problem hiding this comment.
Code Review
Summary
The PR correctly addresses the temporary folder leak by explicitly routing fs.mkdtempSync to os.tmpdir() and adding explicit teardown logic in the afterEach hooks. The inclusion of vi.useRealTimers() prior to the async teardown is an excellent catch to prevent tests from hanging on the Windows setTimeout delay when fake timers are active.
Findings
-
Improvement (Code Duplication):
You have duplicated the exact same robust cleanup block (the Windows platform check, the 100mssetTimeout, and thefs.rmSyncinside atry/catch) roughly 6 times acrossconfigure.test.ts,extension-manager-scope.test.ts, andmcp-client.test.ts.The codebase already has a centralized test utility for this:
cleanupTmpDirinpackages/test-utils/src/file-system-test-helpers.ts. Currently, it only does:export async function cleanupTmpDir(dir: string) { await fs.rm(dir, { recursive: true, force: true }); }
I highly recommend updating
cleanupTmpDirin thetest-utilspackage to include your new Windows delay and try/catch resilience, and then simply importing and callingawait cleanupTmpDir(testWorkspace)in yourafterEachblocks. This will keep the test files much cleaner and ensure the Windows fix is applied universally whenever anyone cleans up a temp directory. -
Nitpick:
You are usingfs.rmSyncinside anasyncfunction (afterEach(async () => { ... })). If you extract this to the asynccleanupTmpDirhelper, you can safely use the asyncfs.rm(fromnode:fs/promises) instead to avoid blocking the event loop during teardowns.
Conclusion
The core logic is correct and effectively fixes the bug and test hangs. However, before merging, consider consolidating the duplicate cleanup logic into the existing cleanupTmpDir utility in packages/test-utils for better maintainability.
Centralized the temporary directory cleanup logic into test-utils/cleanupTmpDir. This utility now includes a 100ms delay for Windows platform and handles errors silently. Updated all affected test files to use this new utility.
Summary
This PR fixes an issue where several test suites were leaving behind temporary directories in the OS temp folder, leading to disk space waste and clutter.
Details
packages/core/src/tools/mcp-client.test.ts(4 locations),packages/cli/src/config/extension-manager-scope.test.ts, andpackages/cli/src/commands/extensions/configure.test.tsto include properafterEachcleanup logic.fs.rmSync(..., { recursive: true, force: true })and includes a 100ms delay for Windows to ensure file handles are released.workspaceContextreferences are cleared to prevent potential memory leaks.configure.test.tswas updated to usepath.join(os.tmpdir(), ...)for consistency.Related Issues
Fixes #22032
How to Validate
npm test -w @google/gemini-cli-core -- src/tools/mcp-client.test.tsnpm test -w @google/gemini-cli -- src/config/extension-manager-scope.test.tsnpm test -w @google/gemini-cli -- src/commands/extensions/configure.test.ts/tmpon MacOS/Linux) and verify that nogemini-agent-test-*orgemini-cli-test-*directories remain after the tests complete.Pre-Merge Checklist