test: migrate to vitest
#274
Conversation
|
WalkthroughThis pull request migrates the project’s testing framework from Jest to Vitest. The CI workflow now builds the project before running tests, and ESLint configurations are updated with the Vitest plugin and test file settings. Jest configuration files and related dependencies have been removed from package.json while new Vitest entries have been added. Numerous test files and fixtures have been updated to replace Jest-specific APIs with Vitest alternatives, including timeout settings, alias changes, and enhanced type annotations. A new Vitest configuration and snapshot serializer have also been introduced. Changes
Sequence Diagram(s)sequenceDiagram
participant CI as CI Runner
participant Build as Build Script
participant Test as Vitest Runner
CI->>Build: Execute build command (yarn run-s build)
Build->>Test: Trigger test process (yarn run-s test)
Test-->>CI: Return test results
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
src/rules/no-unused-modules.tsOops! Something went wrong! :( ESLint: 9.23.0 Error [ERR_MODULE_NOT_FOUND]: Cannot find module '/lib/index.js' imported from /eslint.config.js eslint.config.jsOops! Something went wrong! :( ESLint: 9.23.0 Error [ERR_MODULE_NOT_FOUND]: Cannot find module '/lib/index.js' imported from /eslint.config.js test/cli.spec.tsOops! Something went wrong! :( ESLint: 9.23.0 Error [ERR_MODULE_NOT_FOUND]: Cannot find module '/lib/index.js' imported from /eslint.config.js
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. |
There was a problem hiding this comment.
Pull Request Overview
This PR migrates the test suite from Jest to Vitest. Key changes include:
- Replacing Jest APIs (e.g., jest.fn, jest.setTimeout, jest.unstable_mockModule) with their Vitest equivalents (e.g., vi.fn, vi.setConfig, vi.doMock, vi.resetModules).
- Updating file imports and test file configurations to include the new srcDir where needed.
- Removing Jest configuration files and updating ESLint settings to recognize Vitest.
Reviewed Changes
Copilot reviewed 23 out of 27 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| test/utils/export-map.spec.ts | Updated test logic to use Vitest APIs and adjusted file paths using srcDir. |
| test/rules/no-unused-modules.spec.ts | Replaced Jest usage with Vitest and updated test naming for new file behavior. |
| test/rules/no-extraneous-dependencies.spec.ts | Updated dependency imports and error messages from Jest to Vitest. |
| test/node-resolver.spec.ts | Updated module resolution tests to reflect the migration to Vitest. |
| test/jest.serializer.cjs | Removed unused Jest snapshot serializer. |
| test/fixtures/webpack.config.js | Changed alias from Jest to Vitest for dependency testing. |
| test/fixtures/foo-bar-resolver-v1.js | Added JSDoc comments for clarity. |
| test/fixtures/deep/cache-1.js, cache-1-src.js, cache-1-lib.js | Adjusted cache files to support new srcDir usages. |
| test/cli.spec.ts | Added a Vitest-specific ESLint-disable comment where needed. |
| jest.config.ts | Removed Jest configuration in favor of Vitest. |
| eslint.config.js | Enhanced configuration to include Vitest plugin settings for test files. |
Files not reviewed (4)
- package.json: Language not supported
- patches/@swc-node+jest+1.8.13.patch: Language not supported
- test/snapshots/fixtures.spec.ts.snap: Language not supported
- test/snapshots/node-resolver.spec.ts.snap: Language not supported
Comments suppressed due to low confidence (3)
eslint.config.js:158
- Ensure that 'vitest.config.ts' exists and is correctly configured, or update the ignore patterns if the Vitest configuration is handled differently.
'vitest.config.ts',
jest.config.ts:1
- Review references to 'jest.config.ts' in CI/CD pipelines or scripts to ensure that its removal does not disrupt any workflows.
Entire file removed
test/rules/no-unused-modules.spec.ts:763
- [nitpick] Consider using more descriptive test names instead of incremental identifiers (e.g., 'new file 0, 1, ...') to clearly indicate the differences between test cases.
describe('test behavior for new file 0', () => {
commit: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
test/fixtures/foo-bar-resolver-v1.js (1)
3-8: Good addition of JSDoc comments for better type documentation.Adding JSDoc type annotations improves code readability and enhances IDE type support. This is a positive change that aligns with the project's migration to a more type-aware environment.
However, the
@returnstag doesn't specify the return type. Consider adding the return type information for completeness:/** * @param {string} modulePath * @param {string} sourceFile * @param {unknown} config - * @returns + * @returns {string|undefined} */test/rules/no-extraneous-dependencies.spec.ts (1)
1-589:❓ Verification inconclusive
Consider adding a global timeout configuration for test suite.
The previous test configuration might have included a
jest.setTimeout(10e3)call that needs to be replaced with its Vitest equivalent. This ensures tests have sufficient time to complete.+ import { vi } from 'vitest'; + + // Set a longer timeout for all tests in this file + vi.setConfig({ testTimeout: 10000 }); + import fs from 'node:fs' import { createRequire } from 'node:module'
🏁 Script executed:
#!/bin/bash # Check if there was a Jest timeout configuration in the previous version git show HEAD~1:test/rules/no-extraneous-dependencies.spec.ts | grep -n "setTimeout"Length of output: 84
Global Vitest Timeout Configuration Required
It seems that the previous commit did not include any timeout configuration (i.e. no
jest.setTimeout(10e3)call was found). To ensure that tests have sufficient time to complete under Vitest, please add a global timeout configuration at the top of the file. For example:+ import { vi } from 'vitest'; + + // Set a longer timeout for all tests in this file + vi.setConfig({ testTimeout: 10000 }); + import fs from 'node:fs' import { createRequire } from 'node:module'Please verify manually that this change aligns with your intended test configuration setups.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
test/__snapshots__/fixtures.spec.ts.snapis excluded by!**/*.snaptest/__snapshots__/node-resolver.spec.ts.snapis excluded by!**/*.snapyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (23)
.github/workflows/ci.yml(1 hunks)eslint.config.js(3 hunks)jest.config.ts(0 hunks)package.json(3 hunks)patches/@swc-node+jest+1.8.13.patch(0 hunks)src/rules/no-unused-modules.ts(2 hunks)test/cli.spec.ts(1 hunks)test/fixtures/deep/cache-1-lib.js(1 hunks)test/fixtures/deep/cache-1-src.js(1 hunks)test/fixtures/deep/cache-1.js(0 hunks)test/fixtures/foo-bar-resolver-v1.js(1 hunks)test/fixtures/webpack.config.js(1 hunks)test/global.d.ts(1 hunks)test/jest.serializer.cjs(0 hunks)test/node-resolver.spec.ts(1 hunks)test/rules/no-extraneous-dependencies.spec.ts(3 hunks)test/rules/no-unused-modules.spec.ts(8 hunks)test/utils/export-map.spec.ts(8 hunks)test/utils/hash.spec.ts(1 hunks)test/utils/parse.spec.ts(11 hunks)test/utils/resolve.spec.ts(4 hunks)test/vitest.serializer.ts(1 hunks)vitest.config.ts(1 hunks)
💤 Files with no reviewable changes (4)
- test/jest.serializer.cjs
- test/fixtures/deep/cache-1.js
- jest.config.ts
- patches/@swc-node+jest+1.8.13.patch
🧰 Additional context used
🧬 Code Definitions (1)
test/utils/resolve.spec.ts (1)
test/utils.ts (1)
srcDir(236-236)
🪛 GitHub Actions: CI
test/utils/parse.spec.ts
[error] 4-4: RollupError: Parse failure: Expected ',', got ':'
🔇 Additional comments (41)
test/global.d.ts (1)
1-1: LGTM: Correctly added Vitest types referenceThis change properly sets up TypeScript to recognize Vitest's global types, which is essential for the testing framework migration.
eslint.config.js (3)
12-12: LGTM: Properly imported Vitest ESLint pluginThe import follows the same pattern as other plugin imports and correctly brings in the Vitest linting rules.
112-115: LGTM: Added appropriate ESLint configuration for Vitest testsThe configuration correctly targets test files and extends the recommended Vitest ruleset, which will enforce Vitest best practices in test files.
158-158: LGTM: Updated file pattern for Vitest configCorrectly updated the file pattern to reference the new Vitest configuration file instead of the previous Jest configuration.
src/rules/no-unused-modules.ts (2)
10-10: Minor style improvement in comment formattingRemoved unnecessary space before comma in the ESLint directive comment, improving style consistency.
25-25: Minor style improvement in comment formattingSimilarly removed unnecessary space before comma in this ESLint directive comment for consistent style.
package.json (2)
64-64: LGTM: Updated test script for VitestThe test script has been properly updated to use Vitest instead of Jest. The command includes appropriate flags to suppress unnecessary warnings.
137-137:❓ Verification inconclusive
LGTM: Added required Vitest dependencies
Both Vitest and its ESLint plugin have been added as dev dependencies with recent versions.
Given that the PR mentions a blocker related to Vitest, let's verify if there are any known issues with the specific versions:
Also applies to: 156-156
🏁 Script executed:
#!/bin/bash # Check for any relevant issues with the Vitest versions used # Get the specific issue mentioned in the PR echo "Checking the specific Vitest issue mentioned in the PR:" curl -s "https://api.github.com/repos/vitest-dev/vitest/issues/3987" | jq -r '.title, .state, .html_url' # Check for any open issues related to the version 3.0.9 echo -e "\nChecking for issues related to Vitest 3.0.9:" curl -s "https://api.github.com/search/issues?q=repo:vitest-dev/vitest+is:issue+is:open+3.0.9+in:title,body" | jq -r '.items[] | "\(.title) - \(.html_url)"'Length of output: 1422
Vitest Dependencies Verified – Proceed with Caution
The added Vitest and ESLint plugin dependencies in package.json (lines 137 and 156) are correctly using recent versions. Our verification confirms that the specific issue reported in the PR (fails to run/parse
.ctssource files, issue #3987) remains open. Additionally, several open issues affecting Vitest 3.0.9 were found (e.g., errors withserver.deps.inline,vi.mockbehavior, and others).
- The dependency updates are correct and complete.
- Known Issues:
While these issues are upstream and do not block this PR, please continue to monitor their potential impact on our test suite and adjust if necessary.
vitest.config.ts (3)
1-52: Well-structured Vitest configurationThe configuration is well-organized with a reusable function for workspace setup and proper coverage configuration. The alias mappings are comprehensive and will ensure correct module resolution during testing for both compiled and source code.
6-41: Good abstraction withgetTestWorkspaceCreating a function to handle both compiled and source test configurations reduces duplication and makes the configuration more maintainable. The function is well-typed and properly handles the differences between compiled and source code testing environments.
43-52: Comprehensive test configurationThe test configuration properly sets up Istanbul coverage reporting with appropriate reporters and includes both source and compiled workspaces. The setupFiles points to the serializer which is also part of this PR.
test/fixtures/deep/cache-1-src.js (1)
1-2: Simple and clear module re-exportThis fixture correctly imports and re-exports the module for testing purposes.
test/fixtures/deep/cache-1-lib.js (1)
1-2: Simple and clear module re-exportThis fixture correctly imports and re-exports the module for testing compiled code.
test/vitest.serializer.ts (1)
1-3: Snapshot serializer configurationThe serializer is properly set up using the Vitest-compatible
expectglobal. This is the correct approach for migrating from Jest to Vitest as it maintains compatibility with existing snapshot tests.test/utils/hash.spec.ts (1)
1-1: Appropriate rule disabling for Vitest migrationThe ESLint rule
vitest/expect-expectis correctly disabled for this file since it uses a customexpectHashfunction that internally usesexpectstatements, which may not be directly recognized by the linter.test/fixtures/webpack.config.js (1)
6-6: Correctly updated alias for testing frameworkThe alias has been properly updated from Jest to Vitest as part of the migration, maintaining the same functionality for no-extraneous-dependencies tests.
.github/workflows/ci.yml (1)
55-55: Build command properly updated for Vitest workflowThe CI workflow has been correctly updated to run
buildbeforetestinstead oftest-compiledbeforetest. This change aligns with the Vitest migration and ensures that compiled artifacts are available before running tests.test/node-resolver.spec.ts (1)
50-50:❓ Verification inconclusive
Test case properly updated for Vitest
The test case has been correctly updated to check for resolution of
vitestinstead ofjest, consistent with the migration to Vitest.Ensure that snapshots have been updated elsewhere in the PR, as changing this expectation from 'jest' to 'vitest' will affect the snapshot test results.
🏁 Script executed:
#!/bin/bash # Check if snapshot files have been updated in this PR git diff --name-only HEAD~1 | grep -E '\.snap$'Length of output: 49
Test case updated for Vitest but snapshot updates need verification
- The test now correctly checks for the resolution of
vitestinstead ofjest, which aligns with the migration.- However, the snapshot diff command (
git diff --name-only HEAD~1 | grep -E '\.snap$') did not return any results—this raises uncertainty about whether the snapshot files were updated accordingly.Please verify manually that the necessary snapshot updates have been made in this PR to ensure consistency with the test change.
test/cli.spec.ts (3)
8-8: LGTM! Improved formatting for ESLint directive.The change removes unnecessary spaces before the comment dash for better consistency.
13-13: LGTM! Improved formatting for ESLint directive.The change removes unnecessary spaces before the comment dash for better consistency.
20-20: LGTM! Appropriate Vitest ESLint directive added.Adding the
vitest/expect-expectrule suppression is appropriate for this test since it's validating that no error is thrown rather than explicitly asserting on a result.test/utils/parse.spec.ts (5)
25-27: LGTM! Updated resolver paths with.cjsextension.The paths now explicitly include the CommonJS file extension, which is more accurate and helps with module resolution.
52-52: LGTM! Updated mock functions from Jest to Vitest.These changes correctly replace Jest's mocking functions with their Vitest equivalents:
jest.fn()→vi.fn()- Type annotations for mock functions are properly maintained
This is a key part of the migration to Vitest.
Also applies to: 94-96, 98-98, 147-147, 291-291, 313-313, 319-319, 349-349, 369-369
84-84: LGTM! Updated module reset function from Jest to Vitest.The module reset function has been updated from Jest to Vitest:
jest.resetModules()→vi.resetModules()This ensures test isolation by clearing the module cache between tests.
Also applies to: 139-140
231-231: LGTM! Updated test description to match correct functionality.The test description has been updated to reference
parseForESLintinstead ofparse, which more accurately reflects the function being tested.
4-4:❓ Verification inconclusive
Fix RollupError in TypeScript import.
There's a pipeline failure indicating a parse error on this line.
This is likely related to the TypeScript type import syntax. Since this file isn't shown in the diff, please check if there's a syntax error in an import statement, possibly related to how types are imported.
🏁 Script executed:
RollupError: Parse failure: Expected ',', got ':'Length of output: 150
🏁 Script executed:
#!/bin/bash # Check first few lines of the file to look for import syntax errors head -n 10 test/utils/parse.spec.tsLength of output: 310
Action: Review Rollup Configuration for Type-Only Imports
The file
test/utils/parse.spec.tsshows a valid TypeScript type import:import type { TSESLint } from '@typescript-eslint/utils'However, the Rollup error
Parse failure: Expected ',', got ':'indicates that Rollup is misinterpreting the
import typesyntax. This is typically because the bundler isn’t configured to handle TypeScript’s type-only imports.Recommendations:
Verify Rollup Plugins:
Ensure that your Rollup configuration includes an appropriate TypeScript plugin (e.g., @rollup/plugin-typescript or a compatible alternative) that properly handles type-only imports.Exclude Test Files:
If these tests aren’t meant to be bundled, consider updating the Rollup config to exclude files undertest/(or similar patterns) from the build process.Double Check Build Pipeline:
Confirm that the error arises during bundling and not due to any local syntax issue. The file itself appears syntactically correct, so the issue likely lies with how Rollup is processing it.Please review your Rollup build configuration to ensure it properly handles TypeScript syntax or excludes test files as necessary.
🧰 Tools
🪛 GitHub Actions: CI
[error] 4-4: RollupError: Parse failure: Expected ',', got ':'
test/rules/no-extraneous-dependencies.spec.ts (2)
96-96: LGTM! Updated import statements from Jest to Vitest.Import statements in test cases have been correctly updated to use
vitestinstead ofjest, ensuring the tests validate the correct dependency configuration.Also applies to: 101-101, 106-106, 312-312, 318-318, 324-324, 330-330, 418-418
315-315: LGTM! Updated error messages to reference Vitest.The expected error messages have been updated to check for
vitestinstead ofjestin dependency validation failures, maintaining test accuracy.Also applies to: 321-321, 327-327, 333-333, 422-422
test/utils/resolve.spec.ts (3)
9-9: Import updated to include srcDirThe import statement has been updated to include
srcDir, which is now used to dynamically construct file paths for tests.
493-493: File paths now use dynamic srcDir variableThis change makes the test more flexible by dynamically constructing file paths based on the source directory, which allows tests to work correctly whether running against compiled code ('lib') or source code ('src').
534-534: Jest timeout replaced with Vitest configurationThe timeout configuration has been updated from Jest to Vitest as part of the migration between testing frameworks. This pattern is consistent with Vitest's API for configuring test timeouts.
Also applies to: 547-547
test/utils/export-map.spec.ts (6)
6-6: Import updated to include srcDirThe import statement now includes
srcDir, which is used for dynamic file path construction in the tests.
145-152: Simplified file modification test with synchronous approachThe test for checking cached copies after modification has been changed from using a callback pattern to a synchronous approach with
fs.utimesSync. This simplifies the control flow while maintaining the same test functionality.
308-309: Dynamic file path construction with srcDirFile paths are now constructed dynamically using the
srcDirvariable, which makes the tests adaptable to different environments (compiled or source code).Also applies to: 318-319
329-329: Consistent synchronous file operationsFile operations have been updated to use synchronous versions when appropriate, maintaining a consistent approach throughout the test file.
Also applies to: 342-342
400-400: Jest timeout replaced with Vitest configurationThe timeout configuration has been updated to use Vitest's API instead of Jest's.
402-407: Mocking framework updated from Jest to VitestAll mocking functionality has been migrated from Jest to Vitest, including function mocks, module mocks, module resets, and spying.
Also applies to: 444-444, 489-489
test/rules/no-unused-modules.spec.ts (4)
7-8: Updated import comment for clarityThe eslint-disable comment has been updated for the eslint8UnsupportedApi import to clarify the reason for the rule exclusion.
285-285: Jest timeout replaced with Vitest configurationThe timeout configuration has been updated from Jest to Vitest, maintaining consistent timeout settings during the framework migration.
685-690: Simplified commented-out test codeThe commented-out test code has been simplified while still preserving the original intention of the commented section.
763-763: Improved describe block structureDescribe blocks have been renamed and restructured to improve clarity and avoid potential conflicts in the test suite organization.
Also applies to: 879-879, 921-921, 952-952, 1010-1010
|
IMHO, let's keep jest for now? Let's revisit vitest later after we replace cts with mts as much as possible. |
Sure! |
|
There is still a blocker I don't know how to resolve:
vitest-dev/vitest#3987 (comment)
Summary by CodeRabbit
Chores
Tests
Refactor