feat!: Migrate Testing Framework from Jest to Vitest #2457
Conversation
|
Note Reviews pausedUse the following commands to manage reviews:
WalkthroughThe pull request introduces several updates to the Changes
Possibly related issues
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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
Documentation and Community
|
Our Pull Request Approval ProcessThanks for contributing! Testing Your CodeRemember, your PRs won't be reviewed until these criteria are met:
Our policies make our code better. ReviewersDo not assign reviewers. Our Queue Monitors will review your PR and assign them.
Reviewing Your CodeYour reviewer(s) will have the following roles:
CONTRIBUTING.mdRead our CONTRIBUTING.md file. Most importantly:
Other
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
vitest.config.ts (1)
13-16: Consider additional test file patternsThe current patterns might miss some common test file locations. Consider adding:
src/**/*.{test,spec}.{cjs,mjs}for CommonJS/ESM teststests/**/*.{js,jsx,ts,tsx}for tests outside srcinclude: [ 'src/**/__tests__/**/*.{js,jsx,ts,tsx}', 'src/**/*.{spec,test}.{js,jsx,ts,tsx}', + 'src/**/*.{test,spec}.{cjs,mjs}', + 'tests/**/*.{js,jsx,ts,tsx}' ],package.json (3)
Line range hint
76-77: Test script configuration needs updateThe test scripts still reference Jest configurations while the PR aims to migrate to Vitest. This could cause confusion during the transition period.
Consider:
- Gradually migrating tests to Vitest
- Running both test suites during transition
- Creating a documentation note about the migration status
Also applies to: 83-84
Line range hint
164-167: Review version constraints in resolutionsThe resolutions section specifies older versions of React types (17.0.2) while the project uses React 18. This might cause type mismatches.
Update the resolutions to match the project's React version:
"resolutions": { "@apollo/client": "^3.4.0-beta.19", - "@types/react": "17.0.2", - "@types/react-dom": "17.0.2", + "@types/react": "^18.3.12", + "@types/react-dom": "^18.3.1", "graphql": "^16.5.0" },
Node.js version requirements are inconsistent across the project
The CI workflows are configured to use Node.js 22.x, while package.json specifies ">=20.x". This inconsistency could lead to confusion:
- CI workflows in
.github/workflows/enforce Node.js 22.xpackage.jsonallows Node.js versions >=20.x- This mismatch should be aligned to ensure consistent behavior across development and CI environments
🔗 Analysis chain
Line range hint
169-171: Verify Node.js version compatibilityThe Node.js version requirement (>=20.x) is quite high and might affect CI/CD environments or contributors with older Node versions.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if CI configs specify Node version rg -l "node-version|nodejs-version" .github/workflows/Length of output: 118
Script:
#!/bin/bash # Check Node.js version specifications in CI workflows rg "node-version" .github/workflows/Length of output: 429
Script:
#!/bin/bash # Check package.json for Node.js version requirements cat package.json | jq '.engines'Length of output: 55
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (3)
package.json(3 hunks)src/vitestconfig.test.ts(1 hunks)vitest.config.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/vitestconfig.test.ts
🔇 Additional comments (4)
vitest.config.ts (3)
19-23: 🛠️ Refactor suggestion
Add coverage thresholds
While coverage reporting is configured, setting minimum coverage thresholds helps maintain code quality standards during the migration from Jest.
coverage: {
enabled: true,
provider: 'istanbul',
reporter: ['text', 'html', 'text-summary'],
+ thresholds: {
+ statements: 80,
+ branches: 80,
+ functions: 80,
+ lines: 80
+ }
},Let's check the current coverage thresholds in Jest configuration to maintain consistency:
✅ Verification successful
Let me check the actual thresholds from the Jest configuration file.
Update coverage thresholds to match or improve upon Jest configuration
The Jest configuration has lower coverage thresholds (20% for lines and statements) compared to the suggested 80%. Since this is a migration from Jest to Vitest, we should at least maintain the existing thresholds and potentially improve them gradually.
coverage: {
enabled: true,
provider: 'istanbul',
reporter: ['text', 'html', 'text-summary'],
+ thresholds: {
+ statements: 20,
+ lines: 20
+ }
},Consider gradually increasing these thresholds as test coverage improves during the migration process.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for Jest coverage configuration
echo "Searching for existing Jest coverage thresholds..."
rg -t json -t js "coverageThreshold" .
Length of output: 189
Script:
#!/bin/bash
# Get the content of jest.config.js to see the coverage thresholds
cat jest.config.js
Length of output: 2174
1-11: Verify if additional polyfills are needed for Jest compatibility
The current configuration only includes the 'events' polyfill. During migration from Jest, additional Node.js built-ins might be needed (e.g., 'buffer', 'process', 'util') as Jest automatically provides these in its test environment.
Let's scan the codebase for potential Node.js built-in usage in tests:
✅ Verification successful
No additional Node.js polyfills needed beyond 'events'
The codebase scan shows that while there is significant usage of event-related functionality (EventEmitter patterns, emit/on), there are no direct imports or usage of other Node.js built-in modules like 'buffer', 'process', 'util', etc. in the test files. The only Node.js module imports found are 'fs' in setup files which are not part of the test suite execution. The current polyfill configuration including only 'events' is sufficient.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for common Node.js built-in modules in test files
echo "Searching for Node.js built-in usage in tests..."
rg -t ts -t tsx -t js -t jsx \
-e "require\(['\"](?:buffer|process|util|stream|path)" \
-e "from ['\"](?:buffer|process|util|stream|path)" \
'src/**/*.{test,spec}.*'
Length of output: 300
Script:
#!/bin/bash
# Let's try a different approach to search for Node.js built-in usage
# First, let's find all test files
echo "Finding test files..."
fd -e test.ts -e test.tsx -e test.js -e test.jsx -e spec.ts -e spec.tsx -e spec.js -e spec.jsx
echo -e "\nSearching for Node.js built-in usage in these files..."
# Use ripgrep without file type restrictions
rg "(?:require|from) ['\"](?:buffer|process|util|stream|path|crypto|fs|os)" \
-g '*.test.*' -g '*.spec.*'
echo -e "\nChecking for event emitter usage in tests..."
rg "(?:EventEmitter|emit\(|on\()" \
-g '*.test.*' -g '*.spec.*'
Length of output: 51120
5-25: Verify Jest compatibility settings
As this is a migration from Jest, ensure these critical compatibility aspects are addressed:
- Test environment globals (Jest auto-includes certain globals)
- Timer mocks behavior
- Snapshot testing compatibility
Let's scan for Jest-specific features that might need migration:
✅ Verification successful
Based on the search results, I can see extensive usage of Jest-specific features in the test files, including:
- Timer mocks (
jest.useFakeTimers(),jest.useRealTimers()) - Spy functionality (
jest.spyOn()) - Mock implementations (
jest.mock(),jest.fn())
The current Vitest configuration already includes:
- jsdom environment
- coverage reporting
- globals enabled
However, let's verify if Vitest has built-in compatibility with these Jest APIs:
Based on the findings, I can now provide a definitive response about the Vitest configuration and Jest compatibility:
Vitest configuration is compatible with Jest APIs
The current configuration already includes the essential compatibility settings:
globals: true- Enables Jest-compatible global APIsenvironment: 'jsdom'- Provides Jest-like DOM environment- Package.json shows both Vitest and Jest-compatible testing libraries are installed
No additional configuration is needed as:
- Vitest has built-in Jest API compatibility including timer mocks, spies, and mock implementations
- The test files can continue using Jest-style APIs like
jest.spyOn(),jest.mock(), etc. - Coverage reporting is properly configured with Istanbul provider
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for Jest-specific features that might need migration
echo "Checking Jest-specific feature usage..."
rg -t ts -t tsx -t js -t jsx \
-e "jest\.(useFakeTimers|spyOn|mock|unmock)" \
-e "beforeEach\(jest" \
-e "toMatchSnapshot" \
'src/**/*.{test,spec}.*'
Length of output: 258
Script:
#!/bin/bash
# Let's try a different approach to search for Jest-specific features
echo "Checking for Jest-specific features in test files..."
rg --type-add 'test:*.{test,spec}.{js,jsx,ts,tsx}' --type test \
-e "jest\." \
-e "toMatchSnapshot" \
-e "useFakeTimers" \
-e "spyOn" \
-A 2
# Also check for test files to understand the test patterns
echo -e "\nListing test files:"
fd -e test.ts -e test.tsx -e test.js -e test.jsx -e spec.ts -e spec.tsx -e spec.js -e spec.jsx
Length of output: 183081
Script:
#!/bin/bash
# Check if vitest package.json or docs are present to verify compatibility
fd -e json -e md "vitest|jest" . | xargs cat
Length of output: 46
Script:
#!/bin/bash
# Let's check package.json for vitest dependencies and configuration
cat package.json
# Also check if there are any vitest setup files
fd -e ts -e js "vitest.setup|vitest.config"
Length of output: 5770
Script:
#!/bin/bash
# Let's check the vitest config file to understand the setup
cat vitest.config.ts
# Also check if there are any test setup files that might handle Jest compatibility
fd -e ts -e js "setup" test
Length of output: 699
package.json (1)
69-69: Verify polyfill plugin compatibility with testing setup
The addition of vite-plugin-node-polyfills is good for handling Node.js built-ins, but ensure it's properly configured in your Vitest setup.
✅ Verification successful
vite-plugin-node-polyfills is properly configured and used
The plugin is correctly set up in vitest.config.ts with specific polyfill inclusion for the 'events' module, which is appropriate for the testing environment. The configuration aligns well with both Vitest and the project's testing needs.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if vite config exists and uses the polyfills plugin
rg -l "vite-plugin-node-polyfills" --type ts
Length of output: 61
Script:
#!/bin/bash
# Check how the polyfills plugin is configured in vitest.config.ts
cat vitest.config.ts
Length of output: 579
varshith257
force-pushed
the
migration_to_vitest
branch
from
cb477a5 to
17baee2
Compare
|
Try migrating these tests into vitest and rename these vitest files as |
|
In vitest config similar to jest patter match with *vitest.test.tsx |
varshith257
added
test
|
Add package.json and package.lock.json to ignore lint files so that workflow passes |
to do this I will need to extend Vitest's expect with the custom matchers from jest-dom (as it uses toBeInTheDocument) , I will need to install |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
vitest.config.ts (1)
7-13: Consider including additional Node.js polyfills.While 'events' is included, you might need other common Node.js built-ins that Jest automatically provides. Common modules include 'buffer', 'process', and 'util'.
nodePolyfills({ - include: ['events'], + include: ['events', 'buffer', 'process', 'util'], }),src/components/AddOn/AddOn.spec.tsx (3)
7-8: Consider organizing imports by category.Group related imports together for better maintainability:
- React and testing libraries
- Router and state management
- Component and utilities
import React from 'react'; import { render } from '@testing-library/react'; +import '@testing-library/jest-dom'; +import { describe, test, expect, vi } from 'vitest'; import { Provider } from 'react-redux'; import { BrowserRouter } from 'react-router-dom'; import { MockedProvider } from '@apollo/react-testing'; import { I18nextProvider } from 'react-i18next'; -import '@testing-library/jest-dom'; -import { describe, test, expect, vi } from 'vitest';
17-24: Enhance store mock configuration.The current store mock only provides basic function stubs. Consider adding:
- Initial state configuration
- Return values for
getState- Implementation for
dispatchto track dispatched actionsvi.mock('state/store', () => ({ store: { - // Mock store configuration if needed + // Provide initial state matching the actual store structure + getState: vi.fn(() => ({ + // Add your initial state here + // Example: user: { isLoggedIn: false } + })), subscribe: vi.fn(), - dispatch: vi.fn(), + dispatch: vi.fn((action) => { + // Track dispatched actions if needed for assertions + return action; + }), }, }));
Line range hint
33-65: Consider adding more comprehensive test coverage.While the current tests cover basic rendering, consider adding:
- Error handling scenarios
- User interaction tests (clicks, inputs)
- Loading states
- Edge cases with different prop combinations
Example additional test:
test('should handle user interactions', async () => { const { getByTestId } = render( <MockedProvider addTypename={false} link={link}> <BrowserRouter> <Provider store={store}> <I18nextProvider i18n={i18nForTest}> <AddOn /> </I18nextProvider> </Provider> </BrowserRouter> </MockedProvider> ); // Add interaction tests here // Example: await userEvent.click(getByTestId('some-button')); // Example: expect(someMockFunction).toHaveBeenCalled(); });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (5)
.eslintignore(1 hunks)package.json(4 hunks)src/components/AddOn/AddOn.spec.tsx(1 hunks)src/vitest.test.tsx(1 hunks)vitest.config.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- .eslintignore
🚧 Files skipped from review as they are similar to previous changes (1)
- package.json
🔇 Additional comments (6)
src/vitest.test.tsx (2)
1-1: LGTM! Correct imports for Vitest.
The import statement correctly includes the essential Vitest testing utilities.
1-8:
Missing tests mentioned in PR objectives.
The PR objectives mention tests for the askForCustomPort function that should demonstrate:
- Return of default port when no input is provided
- Return of user-provided port
- Mocking of the
inquirermodule
These tests are essential for validating the Vitest configuration with real use cases from your codebase.
Let's verify if these tests exist elsewhere:
Would you like me to help you implement these tests in this file or create a separate test file?
vitest.config.ts (3)
1-4: LGTM! All necessary imports are present.
The imports cover all essential packages needed for React testing with Vitest, including TypeScript path resolution and Node.js polyfills.
18-22: 🛠️ Refactor suggestion
Add coverage thresholds to maintain code quality.
Consider adding coverage thresholds to ensure maintaining the same code coverage standards that existed with Jest.
coverage: {
enabled: true,
provider: 'istanbul',
reporter: ['text', 'html', 'text-summary'],
+ lines: 80,
+ functions: 80,
+ branches: 80,
+ statements: 80,
},✅ Verification successful
Add coverage thresholds to match or exceed existing Jest thresholds
Based on the Jest configuration, the codebase previously had coverage thresholds of 20% for lines and statements. Consider adding equivalent or higher thresholds in the Vitest configuration:
coverage: {
enabled: true,
provider: 'istanbul',
reporter: ['text', 'html', 'text-summary'],
+ lines: 20,
+ statements: 20,
},🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if Jest configuration has coverage thresholds
echo "Checking Jest configuration for coverage thresholds:"
rg -A 5 "coverageThreshold|collectCoverage" package.json jest.config.js jest.config.ts
Length of output: 797
15-15: Consider standardizing test file patterns.
The current pattern includes both .spec and .test extensions. For consistency during the migration from Jest to Vitest, it's recommended to standardize on one pattern, preferably .spec for Vitest tests.
✅ Verification successful
Standardizing test file patterns is recommended but requires careful migration
The codebase currently has a mix of .test (151 files) and .spec (2 files) patterns, with .test being the predominant convention. Given this distribution:
- Most tests follow the
.testpattern, suggesting it's the established convention - Only 2 files use
.spec:constant.spec.tsandAddOn.spec.tsx
Recommend:
- Standardize on
.testpattern since it's already used by 98% of test files - Update the 2
.specfiles to maintain consistency - Update
vitest.config.tsto only include.testpattern to reflect the standardized convention
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check current test file patterns in the codebase
echo "Current test file patterns:"
fd -e test.tsx -e test.ts -e spec.tsx -e spec.ts . src/
Length of output: 11056
src/components/AddOn/AddOn.spec.tsx (1)
Line range hint 1-65: Successfully migrated to Vitest ✨
The test file has been successfully migrated from Jest to Vitest while:
- Maintaining existing test coverage
- Following the new
.spec.tsxnaming convention - Properly implementing Vitest mocking capabilities
|
Please make the changes so that coderabbit.ai approves the PR |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
src/vitest.test.tsx (3)
12-15: Remove unnecessary async keywordThe test doesn't contain any asynchronous operations, making the
asynckeyword redundant.- it('should support DOM testing', async () => { + it('should support DOM testing', () => { render(<div data-testid="test">Hello</div>); expect(screen.getByTestId('test')).toBeInTheDocument(); });
17-20: Enhance mocking test coverageThe test demonstrates basic mocking but could be expanded to cover more scenarios. Also, the
asynckeyword is unnecessary.- it('should support mocking', async () => { + it('should support mocking', () => { const mock = vi.fn().mockReturnValue('mocked'); expect(mock()).toBe('mocked'); + + // Test spy functionality + const spy = vi.spyOn(console, 'log'); + console.log('test'); + expect(spy).toHaveBeenCalledWith('test'); + + // Test mock implementation + const mockWithImpl = vi.fn().mockImplementation((x: number) => x * 2); + expect(mockWithImpl(2)).toBe(4); });
1-21: Plan for test file evolutionSince this is a temporary configuration validation file, consider:
- Adding a TODO comment indicating its temporary nature
- Creating a ticket to track its removal or evolution after the Jest to Vitest migration is complete
- Moving the configuration validation tests to a dedicated test utilities directory if they become permanent
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
src/vitest.test.tsx(1 hunks)
🔇 Additional comments (1)
src/vitest.test.tsx (1)
1-4: LGTM! All necessary testing utilities are properly imported.
The imports cover all required dependencies for React components testing, DOM assertions, and Vitest utilities.
|
named this commit suggestion #2 by mistake, sorry if it causes any confusion |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
Could you provide a video/image of latest coverage report on terminal of vitest for test has been migrated? |
vitest.mp4vitest.test.tsx file |
…ation#2457) * configuring vitest * migrating AddOn tests to vitest * removing unnecessary alias * eslint fix * suggestion PalisadoesFoundation#2 * workflow setup * fix: tsdoc workflow * reformat comment to tsdoc fmt * resolving jest, vitest workflow * lint error * add tsdoc comments * exit for workflow * format yml fix * fixing workflow * cleanup * ts error * Update .eslintignore * tsconfig edit * removing suggestions 2 * Update vitest.config.ts * fmt * config update * ignoring .spec files for cov * new suggestions * merge reports * Update vitest.config.ts * vitest run fix * report dir change * report dir change 2 * check workflow * workflow update * fixing merge report * suggestion reverted * merge cov reports * changing format of jest cov reports * Merge Coverage Reports Using lcov-result-merger * fixing merge report * final changes * final changes 2 --------- Co-authored-by: Vamshi Maskuri <[email protected]>
…ation#2457) * configuring vitest * migrating AddOn tests to vitest * removing unnecessary alias * eslint fix * suggestion PalisadoesFoundation#2 * workflow setup * fix: tsdoc workflow * reformat comment to tsdoc fmt * resolving jest, vitest workflow * lint error * add tsdoc comments * exit for workflow * format yml fix * fixing workflow * cleanup * ts error * Update .eslintignore * tsconfig edit * removing suggestions 2 * Update vitest.config.ts * fmt * config update * ignoring .spec files for cov * new suggestions * merge reports * Update vitest.config.ts * vitest run fix * report dir change * report dir change 2 * check workflow * workflow update * fixing merge report * suggestion reverted * merge cov reports * changing format of jest cov reports * Merge Coverage Reports Using lcov-result-merger * fixing merge report * final changes * final changes 2 --------- Co-authored-by: Vamshi Maskuri <[email protected]>
What kind of change does this PR introduce?
configuring vitest for testing, adds a test file to check if this configuration is working as expected
Issue Number:
#2456
Fixes #
Did you add tests for your changes?
No
Snapshots/Videos:
Screencast.from.2024-11-20.19-38-55.webm
This is test was using Jest, it was updated to use vitest to show vitest configuration is working as expected
Screencast.from.2024-11-20.19-42-04.webm
If relevant, did you update the documentation?
No
Summary
Does this PR introduce a breaking change?
Yes
Note
This PR introduces a breaking change, and as a result, the overall test coverage may temporarily fall below 95% during the migration process.
Action Items:
This will help maintain the integrity of our test suite while we transition.
Have you read the contributing guide?
Yes
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
Chores