Conversation
Summary of ChangesHello @9aoy, 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 undertakes a substantial refactoring of the project's testing infrastructure, transitioning from Jest to the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. 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.
Pull request overview
This pull request migrates the test suite from Jest to rstest and upgrades @rspack/test-tools to version 1.7.2. The migration replaces callback-based test patterns with modern async/await syntax using NEXT_HMR() for hot module replacement testing.
Changes:
- Migrated all hot reload tests from callback-based
NEXT(update(...))pattern to async/await withNEXT_HMR() - Replaced Jest with rstest testing framework and updated configuration accordingly
- Removed Jest-specific dependencies, configuration files, and utility scripts
Reviewed changes
Copilot reviewed 34 out of 35 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| package.json | Updated dependencies, removed Jest packages, added rstest and @rspack/test-tools 1.7.2 |
| rstest.config.ts | New rstest configuration file with test environment setup |
| test/hotCases/**/*.jsx | Converted tests to async/await, replaced NEXT() callback pattern with NEXT_HMR() |
| test/configCases/**/*.js | Removed done() callback, converted to synchronous tests |
| test/Hot.test.js & HotSnapshot.test.js | Added tempDir parameter to test case creation |
| jest.config.cjs, scripts/*.cjs | Removed Jest configuration and utility files |
| /snapshots//*.snap.txt | Updated snapshot files with new file sizes and paths |
| test/hotCases/update.js | Removed obsolete update utility function |
| pnpm-lock.yaml | Updated lock file with new dependencies |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request successfully migrates the test suite from Jest to rstest, updating dependencies and test configurations accordingly. The changes include removing Jest-specific configurations and helper scripts, introducing rstest.config.ts, and refactoring hot module replacement (HMR) tests to use await NEXT_HMR() instead of the update callback. Snapshot files have been updated to reflect the new test environment paths. Overall, the migration appears to be well-executed, improving the testing infrastructure.
| "test": "rstest", | ||
| "testu": "rstest --updateSnapshot", |
There was a problem hiding this comment.
The cross-env RSPACK_HOT_TEST=true prefix was removed from the test and testu scripts. While rstest.config.ts now sets RSPACK_HOT_TEST: 'true', it's important to ensure that this environment variable is consistently applied for all test runs, especially if rstest can be invoked without this specific config file or if there are other ways to run tests that might bypass it. Consider if cross-env is still needed for broader compatibility or if the rstest.config.ts setup is sufficient for all scenarios.
| const testFilter = | ||
| process.argv.includes('--test') || process.argv.includes('-t') | ||
| ? process.argv[ | ||
| (process.argv.includes('-t') | ||
| ? process.argv.indexOf('-t') | ||
| : process.argv.indexOf('--test')) + 1 | ||
| ] | ||
| : undefined; |
There was a problem hiding this comment.
The logic for parsing testFilter from process.argv is quite verbose. This pattern is repeated for updateSnapshot and DEFAULT_MAX_CONCURRENT. Consider creating a helper function to abstract this argument parsing logic, which would improve readability and reduce duplication.
const getArgValue = (argName: string) => {
const index = process.argv.indexOf(argName);
return index !== -1 ? process.argv[index + 1] : undefined;
};
const testFilter = getArgValue('--test') || getArgValue('-t');| DEFAULT_MAX_CONCURRENT: process.argv.includes('--maxConcurrency') | ||
| ? process.argv[process.argv.indexOf('--maxConcurrency') + 1] | ||
| : undefined, |
There was a problem hiding this comment.
Similar to the testFilter comment, the logic for DEFAULT_MAX_CONCURRENT can be simplified using a helper function to parse command-line arguments. This would make the configuration cleaner and easier to read.
DEFAULT_MAX_CONCURRENT: getArgValue('--maxConcurrency'),|
|
||
| describeByWalk(__filename, (name, src, dist) => { | ||
| createHotCase(name, src, dist, 'web'); | ||
| createHotCase(name, src, dist, path.join(tempDir, name), 'web'); |
There was a problem hiding this comment.
The createHotCase function now includes path.join(tempDir, name) as an additional argument. Ensure that this new argument is correctly handled by createHotCase and that its purpose is clearly documented, especially if it's a new requirement for rstest or @rspack/test-tools.
| __filename, | ||
| (name, src, dist) => { | ||
| createHotStepCase(name, src, dist, 'web'); | ||
| createHotStepCase(name, src, dist, path.join(tempDir, name), 'web'); |
There was a problem hiding this comment.
Similar to test/Hot.test.js, createHotStepCase now includes path.join(tempDir, name). Verify that this argument is correctly utilized and documented within the createHotStepCase function.
| it("should exclude selected file when compiling", () => { | ||
| expect(__webpack_modules__[require.resolve('./file.js')].toString()) | ||
| .not.toContain('__prefresh_utils__'); | ||
| done(); | ||
| }); |
There was a problem hiding this comment.
The done callback and its invocation have been removed from the it block. This indicates a shift from callback-based asynchronous testing to promise-based or async/await testing, which is common with modern test runners like rstest. Ensure that all asynchronous tests are correctly migrated to the new pattern to prevent flaky tests or unexpected behavior.
| it("should include selected file when compiling", () => { | ||
| expect(__webpack_modules__[require.resolve('foo')].toString()) | ||
| .toContain('__prefresh_utils__'); | ||
| done(); | ||
| }); |
There was a problem hiding this comment.
The done callback and its invocation have been removed from the it block. This indicates a shift from callback-based asynchronous testing to promise-based or async/await testing, which is common with modern test runners like rstest. Ensure that all asynchronous tests are correctly migrated to the new pattern to prevent flaky tests or unexpected behavior.
@rspack/test-toolsto1.7.2await NEXT_HMRinstead ofNEXTcallbackbefore:
after:
