Refactor: test from jest to vitest #2688
Conversation
WalkthroughThe pull request focuses on migrating the test suite for the Changes
Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
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: 2
🔭 Outside diff range comments (1)
src/screens/UserPortal/Volunteer/Groups/Groups.test.tsx (1)
Line range hint
229-239: Enhance error handling test coverageThe error handling test could be more comprehensive by testing different error scenarios.
it('Error while fetching groups data', async () => { renderGroups(link2); await waitFor(() => { expect(screen.getByTestId('errorMsg')).toBeInTheDocument(); + // Verify error message content + expect(screen.getByTestId('errorMsg')).toHaveTextContent(/error/i); + // Verify UI state during error + expect(screen.queryByTestId('groupName')).not.toBeInTheDocument(); }); + + // Verify user can retry + const retryButton = screen.getByRole('button', { name: /retry/i }); + expect(retryButton).toBeInTheDocument(); + await userEvent.click(retryButton); });
🧹 Nitpick comments (4)
src/screens/UserPortal/Volunteer/Groups/Groups.test.tsx (4)
Line range hint
1-1: Rename file extension to align with PR objectivesAccording to the PR objectives, the file should be renamed from
.test.tsxto.spec.tsxto align with Vitest conventions.
Line range hint
42-49: Consider adding parameter validation to debounceWaitThe function is well-implemented, but consider adding parameter validation for better type safety and error handling.
-const debounceWait = async (ms = 300): Promise<void> => { +const debounceWait = async (ms: number = 300): Promise<void> => { + if (ms < 0) { + throw new Error('Delay must be a non-negative number'); + } await act(() => { return new Promise((resolve) => { setTimeout(resolve, ms); }); }); };
Line range hint
51-77: Consider enhancing renderGroups with error boundaryThe utility function is well-structured, but consider adding an error boundary to catch and handle rendering errors gracefully during testing.
+class TestErrorBoundary extends React.Component { + componentDidCatch(error: Error) { + console.error('Rendering error in test:', error); + } + render() { + return this.props.children; + } +} const renderGroups = (link: ApolloLink): RenderResult => { return render( + <TestErrorBoundary> <MockedProvider addTypename={false} link={link}> {/* ... existing JSX ... */} </MockedProvider> + </TestErrorBoundary> ); };
82-89: Enhance type safety in router mockingConsider adding type safety to the router mock implementation.
- vi.mock('react-router-dom', async () => { + vi.mock('react-router-dom', async (): Promise<typeof import('react-router-dom')> => { const actual = await vi.importActual('react-router-dom'); return { ...actual, - useParams: () => ({ orgId: 'orgId' }), + useParams: (): { orgId: string } => ({ orgId: 'orgId' }), }; });
|
We have a policy of unassigning contributors who close PRs without getting validation from our reviewer team. This is because:
Please be considerate of our volunteers' limited time and our desire to improve our code base. This policy is stated as a pinned post in all our Talawa repositories. Our YouTube videos explain why this practice is not acceptable to our Community. In most cases you don’t have to close the PR to trigger the GitHub workflow to run again. Making a new commit and pushing it to your GitHub account will normally be sufficient. Unfortunately, if this continues we will have to close the offending PR and unassign you from the issue. |
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
src/screens/UserPortal/Volunteer/Groups/Groups.spec.tsx (1)
Migration to Vitest is incomplete - Jest configuration still present
The file under review has been properly migrated to use Vitest (using
vifrom 'vitest' for mocking), but there are conflicting test configurations in the project:
Both Jest and Vitest configurations are present:
vitest.config.tsexists with proper Vitest setupjest.config.jsis still present- Package.json has both Jest and Vitest test scripts
The project is in a transitional state with:
- Vitest scripts:
test:vitest,test:vitest:watch,test:vitest:coverage- Jest script:
teststill using Jest configurationRecommendation:
- Remove Jest configuration and dependencies
- Update main
testscript to use Vitest- Remove custom Jest test environment at
scripts/custom-test-env.js🔗 Analysis chain
Line range hint
1-266: Verify complete migration from Jest to VitestLet's ensure all Jest-specific patterns have been properly migrated to Vitest.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for remaining Jest-specific patterns that should be migrated to Vitest # Check for Jest-specific imports and patterns echo "Checking for Jest-specific patterns..." rg -i "jest\." --type=typescript --type=tsx # Check for common Jest globals that should be replaced with Vitest equivalents echo "Checking for Jest globals..." rg -i "describe|it|test|expect|mock|spy|jest" --type=typescript --type=tsx # Check for Jest configuration files that should be migrated echo "Checking for Jest config files..." fd "jest.config" -t fLength of output: 460
Script:
#!/bin/bash # Let's try a different approach without file type filters since they're not recognized # First, check for Jest-specific patterns echo "Checking for Jest-specific patterns..." rg -i "jest\." # Check for Jest configuration imports or requires echo "Checking for Jest configuration imports..." rg "require.*jest|import.*jest" # Check content of jest.config.js echo "Checking jest.config.js content..." cat jest.config.js # Look for testing-library/jest-dom imports echo "Checking for @testing-library/jest-dom imports..." rg "@testing-library/jest-dom"Length of output: 81108
Script:
#!/bin/bash # Let's check the specific file under review and its test configuration echo "Checking the specific file under review..." cat src/screens/UserPortal/Volunteer/Groups/Groups.spec.tsx # Check for vitest configuration echo "Checking for vitest configuration..." cat vitest.config.ts 2>/dev/null || cat vitest.config.js 2>/dev/null || echo "No vitest config found" # Check package.json for test-related dependencies echo "Checking package.json for test dependencies..." cat package.json | grep -A 5 -B 5 '"test":'Length of output: 10617
🧹 Nitpick comments (4)
src/screens/UserPortal/Volunteer/Groups/Groups.spec.tsx (4)
18-19: Consider cleaning up test-related importsWhile the Vitest-specific imports have been added correctly, consider reviewing if all other test-related imports are still necessary after the migration. For example, some Jest-specific imports might be redundant.
Line range hint
36-48: Consider moving debounceWait to shared test utilitiesThe
debounceWaitutility function is well-implemented and documented, but since it's a generic testing helper, consider moving it to a shared test utilities file for reuse across other test suites.
82-97: Consider adding afterEach cleanupWhile the test setup is good, consider adding an
afterEachblock to reset any side effects that might occur during individual tests (e.g., resetting localStorage, clearing mocked function calls). This helps ensure test isolation.Example:
afterEach(() => { vi.resetAllMocks(); localStorage.clear(); });
Line range hint
100-266: Consider adding more edge cases and error scenariosThe test suite has good coverage of happy paths, but consider adding tests for:
- Network timeout scenarios
- Invalid search inputs
- Boundary conditions for sorting (e.g., equal member counts)
- Modal interaction edge cases (e.g., rapid open/close)
- Keyboard navigation accessibility
Example test case:
it('should handle network timeout gracefully', async () => { const timeoutLink = new StaticMockLink([ { ...MOCKS[0], delay: 5000 // Simulate timeout } ]); renderGroups(timeoutLink); await waitFor(() => { expect(screen.getByTestId('errorMsg')).toHaveTextContent(/timeout/i); }); });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/screens/UserPortal/Volunteer/Groups/Groups.spec.tsx(11 hunks)
🔇 Additional comments (1)
src/screens/UserPortal/Volunteer/Groups/Groups.spec.tsx (1)
Line range hint 51-76: LGTM! Well-structured test setup utility
The renderGroups function is well-implemented with:
- Comprehensive provider wrapping
- Clear type definitions
- Good JSDoc documentation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop-postgres #2688 +/- ##
=====================================================
+ Coverage 68.61% 87.59% +18.97%
=====================================================
Files 296 313 +17
Lines 7358 8204 +846
Branches 1606 1848 +242
=====================================================
+ Hits 5049 7186 +2137
+ Misses 2068 824 -1244
+ Partials 241 194 -47 ☔ View full report in Codecov by Sentry. |
|
Please fix the failing tests. Our test code coverage system will fail if any of these conditions occur:
Here are some steps to take:
We do this to ensure the overall reliability of the code base is maintained. This is a prerequisite for getting your PR approved. Assigned reviewers regularly review the PR queue and tend to focus on PRs that are passing. Additional notes:
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/screens/UserPortal/Volunteer/Groups/Groups.spec.tsx (4)
6-12: Consider consolidating test utility importsThe testing utilities are imported individually, which could be simplified. Also, since Vitest includes Jest DOM matchers by default, the separate import of '@testing-library/jest-dom' might be unnecessary.
Consider this optimization:
-import { - cleanup, - fireEvent, - render, - screen, - waitFor, -} from '@testing-library/react'; +import * as rtl from '@testing-library/react'; import { vi } from 'vitest'; -import '@testing-library/jest-dom';Also applies to: 24-25
Line range hint
57-83: Consider enhancing renderGroups utilityWhile the implementation is solid, consider these improvements:
- Add error boundary wrapper for better error handling in tests
- Consider making the initial route path configurable
Here's a suggested enhancement:
-const renderGroups = (link: ApolloLink): RenderResult => { +interface RenderGroupsOptions { + link: ApolloLink; + initialPath?: string; +} + +const renderGroups = ({ + link, + initialPath = '/user/volunteer/orgId' +}: RenderGroupsOptions): RenderResult => { return render( <MockedProvider addTypename={false} link={link}> - <MemoryRouter initialEntries={['/user/volunteer/orgId']}> + <MemoryRouter initialEntries={[initialPath]}> <Provider store={store}> <LocalizationProvider dateAdapter={AdapterDayjs}> <I18nextProvider i18n={i18n}> - <Routes> - <Route path="/user/volunteer/:orgId" element={<Groups />} /> - <Route - path="/" - element={<div data-testid="paramsError"></div>} - /> - </Routes> + <ErrorBoundary fallback={<div>Something went wrong</div>}> + <Routes> + <Route path="/user/volunteer/:orgId" element={<Groups />} /> + <Route + path="/" + element={<div data-testid="paramsError"></div>} + /> + </Routes> + </ErrorBoundary> </I18nextProvider> </LocalizationProvider> </Provider> </MemoryRouter> </MockedProvider> ); };
88-104: Enhance test setup robustnessWhile the setup is good, consider these improvements:
- Add type safety to the mocked useParams
- Consider restoring all mocks after all tests
Here's a suggested enhancement:
beforeAll(() => { vi.mock('react-router-dom', async () => { const actual = await vi.importActual('react-router-dom'); return { ...actual, - useParams: () => ({ orgId: 'orgId' }), + useParams: vi.fn().mockReturnValue({ orgId: 'orgId' } as { orgId: string }), }; }); }); +afterAll(() => { + vi.restoreAllMocks(); +}); afterEach(() => { vi.resetAllMocks(); cleanup(); });
Line range hint
136-276: Enhance test assertions and error handlingThe test cases are comprehensive, but consider these improvements:
- Add more specific assertions for error cases
- Add timeout handling for async operations
- Add negative test cases for search functionality
Here's an example enhancement for the error test case:
it('Error while fetching groups data', async () => { renderGroups(link2); - await waitFor(() => { - expect(screen.getByTestId('errorMsg')).toBeInTheDocument(); - }); + await waitFor( + () => { + const errorElement = screen.getByTestId('errorMsg'); + expect(errorElement).toBeInTheDocument(); + expect(errorElement).toHaveTextContent(/error fetching groups/i); + }, + { timeout: 2000 } + ); }); +it('Search by Group with no results', async () => { + renderGroups(link1); + const searchInput = await screen.findByTestId('searchBy'); + + userEvent.type(searchInput, 'NonexistentGroup'); + await debounceWait(); + + expect(screen.getByText(t.noVolunteerGroups)).toBeInTheDocument(); +});
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/screens/UserPortal/Volunteer/Groups/Groups.spec.tsx(12 hunks)
🔇 Additional comments (1)
src/screens/UserPortal/Volunteer/Groups/Groups.spec.tsx (1)
Line range hint 42-56: Well-implemented debounce utility!
The debounceWait utility is well-documented and correctly implements async behavior with act() wrapper, which is crucial for handling state updates in tests.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/screens/UserPortal/Volunteer/Groups/Groups.spec.tsx (3)
Line range hint
41-82: LGTM: Well-structured utility functions with comprehensive setupThe utility functions are well-documented and implement best practices. The
renderGroupsfunction provides a clean wrapper for consistent component rendering across tests.Consider adding error handling to
debounceWaitfor negative timeout values:const debounceWait = async (ms = 300): Promise<void> => { + if (ms < 0) { + throw new Error('Timeout value must be non-negative'); + } await act(() => { return new Promise((resolve) => { setTimeout(resolve, ms); }); }); };
101-103: Consider adding cleanup for localStorage in afterEachWhile the current cleanup is good, consider also clearing localStorage to prevent potential test interference.
afterEach(() => { vi.resetAllMocks(); cleanup(); // from @testing-library/react + localStorage.clear(); // Clean up localStorage after each test });
Line range hint
106-276: LGTM: Comprehensive test coverage with clear organizationThe test suite demonstrates:
- Clear test case organization with descriptive JSDoc comments
- Proper async/await usage with waitFor
- Good coverage of different scenarios (happy path, error cases, empty states)
- Consistent use of test utilities
Consider adding test cases for:
- Search with special characters
- Edge cases for group names (empty, very long)
- Concurrent sorting and searching operations
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/screens/UserPortal/Volunteer/Groups/Groups.spec.tsx(12 hunks)
🔇 Additional comments (2)
src/screens/UserPortal/Volunteer/Groups/Groups.spec.tsx (2)
Line range hint 1-24: LGTM: Clean imports aligned with Vitest
The imports are properly organized and include all necessary testing utilities while correctly transitioning to Vitest.
87-94: LGTM: Clean Vitest mocking implementation
The mocking setup correctly uses Vitest's vi.mock with async imports, maintaining the actual router functionality while mocking only the necessary parts.
|
Minor PR |
palisadoes
merged commit c217676
into
PalisadoesFoundation:develop-postgres
…ation/talawa-admin into refactor/test-jest-to-vitest
…yanshuNegi/talawa-admin into refactor/test-jest-to-vitest
Refactored src/screens/UserPortal/Volunteer/Groups.test.tsx to src/screens/UserPortal/Volunteer/Groups.spec.tsx
Issue: #2584 : #2584
Changes Implemented:
- Renamed the test file:
- Ran all tests successfully under the Vitest environment.
Refactor:
src/screens/UserPortal/Volunteer/Groupsfrom Jest to Vitest #2584Other information
I have read the previous refactor PR and tried to keep things as uniform as possible.
Please suggest any other changes if required.
Summary by CodeRabbit