Conversation
Summary of ChangesHello @fi3ework, 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 significantly enhances the reliability and isolation of browser-mode snapshot tests. By transitioning to temporary test fixtures and implementing robust process termination, it ensures that test runs do not interfere with each other or leave behind residual processes or modified files, particularly addressing issues with file locking on Windows. 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. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request significantly improves the e2e snapshot tests by refactoring them to use temporary fixtures. This makes the tests more robust and isolated, which is a great improvement. The use of prepareFixtures and cleaning up in afterEach is a solid pattern. I've added a couple of suggestions to further enhance the test cleanup logic for better reliability and type safety. Overall, this is a high-quality contribution.
| treeKill(pid, 'SIGKILL'); | ||
| await new Promise((resolve) => setTimeout(resolve, 2000)); |
There was a problem hiding this comment.
Using setTimeout with a fixed duration can make tests slower and potentially flaky. The tree-kill library provides a callback that is executed once the process tree is killed. Using this callback is more reliable and avoids the arbitrary wait.
await new Promise<void>((resolve) => {
treeKill(pid, 'SIGKILL', () => {
// The callback is called when the process tree has been killed or if an error occurred.
// We resolve here to ensure cleanup continues.
resolve();
});
});There was a problem hiding this comment.
Pull request overview
This PR refactors the browser mode snapshot tests to use temporary fixtures, ensuring better test isolation and preventing file conflicts on Windows.
Changes:
- Refactors snapshot tests to use
prepareFixturesfor creating temporary, isolated test fixtures - Implements process cleanup with
treeKillinafterEachto prevent file lock issues on Windows - Replaces direct
runBrowserClicalls withrunBrowserCliWithCwdto execute tests in temporary directories
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Refactor `e2e/browser-mode/snapshot.test.ts` to use temporary fixtures - Use `prepareFixtures` to copy test files to a non-tracked directory before execution - Implement `treeKill` in `afterEach` to ensure clean process termination - Use `runBrowserCliWithCwd` to execute tests in the isolated temporary directory - Remove manual restoration of tracked test files in `afterEach`
Summary
e2e/browser-mode/snapshot.test.tsto use temporary fixturesprepareFixturesto copy test files to a non-tracked directory before executiontreeKillinafterEachto ensure clean process terminationrunBrowserCliWithCwdto execute tests in the isolated temporary directoryafterEachRelated Links
Checklist