Refactor: src/screens/OrganizationVenues from Jest to Vitest #2665
Conversation
WalkthroughThis pull request focuses on migrating test files from Jest to Vitest across multiple components and screens. The changes primarily involve replacing Jest-specific mocking functions with Vitest equivalents, such as changing Changes
Assessment against linked issues
Possibly related issues
Possibly related PRs
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}
|
@im-vedant Please fix the failed tests. |
|
@palisadoes These tests are failing because of some issue while parsing css. Many people are facing the same issue. It would be better if you create separate issue to fix this issue. I would love to work on this issue. |
Please fix the issue in this PR. |
|
Okay. I have started working on this issue. |
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (5)
src/setupTests.ts (2)
Line range hint
1-8: Migration required: Replace Jest-specific imports and configurationsThis file still contains Jest-specific code that needs to be migrated to Vitest:
- Replace
@testing-library/jest-domwith@testing-library/jest-dom/vitest- Replace
jest.fn()withvi.fn()- Update
jest-previewconfiguration for Vitest- import '@testing-library/jest-dom'; + import '@testing-library/jest-dom/vitest' - global.fetch = jest.fn(); + global.fetch = vi.fn();
Line range hint
34-43: Update test preview configuration for VitestThe jest-preview configuration needs to be updated for Vitest compatibility.
- import { jestPreviewConfigure } from 'jest-preview'; + import { vitestPreviewConfigure } from 'vitest-preview'; - jestPreviewConfigure({ + vitestPreviewConfigure({ autoPreview: true, }); - jest.setTimeout(15000); + vi.setConfig({ testTimeout: 15000 });src/components/OrganizationScreen/OrganizationScreen.test.tsx (1)
Line range hint
16-20: Migration required: Update Jest mocks to Vitest syntaxThe module mocking needs to be updated to use Vitest:
- jest.mock('react-router-dom', () => ({ - ...jest.requireActual('react-router-dom'), + vi.mock('react-router-dom', () => ({ + ...vi.importActual('react-router-dom'), useParams: () => ({ orgId: mockID }), useMatch: () => ({ params: { eventId: 'event123', orgId: '123' } }), }));src/components/MemberDetail/customTableCell.test.tsx (2)
Line range hint
8-13: Migration required: Update mock implementation to VitestUpdate the react-toastify mock to use Vitest:
- jest.mock('react-toastify', () => ({ + vi.mock('react-toastify', () => ({ toast: { - success: jest.fn(), - error: jest.fn(), + success: vi.fn(), + error: vi.fn(), }, }));
Line range hint
77-143: Uncomment and fix error handling testsThe commented-out tests for error handling and "no event found" scenarios should be uncommented and updated to work with Vitest. These tests are important for ensuring robust error handling.
Would you like me to help update these tests to work with Vitest? I can provide the corrected implementation that includes:
- Proper error handling assertions
- Updated toast mock verifications
- Improved timeout handling for Vitest
🧹 Nitpick comments (1)
src/setupTests.ts (1)
11-21: Consider a more robust solution for CSS parsing errorsThe current approach of suppressing CSS parsing errors is a workaround that might hide potential issues. Consider:
- Using a proper CSS loader configuration in Vitest
- Mocking CSS imports directly
Example Vitest configuration for handling CSS:
// vitest.config.ts export default defineConfig({ test: { environment: 'jsdom', globals: true, setupFiles: ['./src/setupTests.ts'], css: { modules: { classNameStrategy: 'non-scoped' } } } });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/components/MemberDetail/customTableCell.test.tsx(1 hunks)src/components/OrganizationScreen/OrganizationScreen.test.tsx(1 hunks)src/setupTests.ts(1 hunks)
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop-postgres #2665 +/- ##
====================================================
+ Coverage 79.58% 86.39% +6.80%
====================================================
Files 295 312 +17
Lines 7274 8134 +860
Branches 1592 1841 +249
====================================================
+ Hits 5789 7027 +1238
+ Misses 1222 929 -293
+ Partials 263 178 -85 ☔ View full report in Codecov by Sentry. |
|
@palisadoes I have fixed those failing test. |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
|
Please fix the code so that the tests are not commented out. CodeRabbiti.ai mentioned this as part of its analysis. We need the quality of the code base to improve. Thanks. You can find it here: |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/components/MemberDetail/customTableCell.spec.tsx (3)
15-49: Consider improving mock data maintainabilityWhile the mock data is well-structured, consider these improvements:
- Extract test IDs as constants for reusability
- Use Date objects for date fields instead of strings
+const TEST_IDS = { + EVENT: 'event123', + RECURRING_EVENT: 'recurringEvent123', + ORGANIZATION: 'org456', +}; + +const TEST_DATES = { + START: new Date('2023-05-01'), + END: new Date('2023-05-02'), +}; + const mocks = [ { request: { query: EVENT_DETAILS, - variables: { id: 'event123' }, + variables: { id: TEST_IDS.EVENT }, }, result: { data: { event: { - _id: 'event123', + _id: TEST_IDS.EVENT, title: 'Test Event', description: 'This is a test event description', - startDate: '2023-05-01', - endDate: '2023-05-02', + startDate: TEST_DATES.START.toISOString().split('T')[0], + endDate: TEST_DATES.END.toISOString().split('T')[0],
99-127: Consider enhancing error state verificationThe error test could be more robust by:
- Verifying the exact error message matches the component
- Testing different types of GraphQL errors
it('displays error state', async () => { + const errorMessage = 'Network error occurred'; const errorMock = [ { request: { query: EVENT_DETAILS, variables: { id: 'event123' }, }, - error: new Error('An error occurred'), + error: new Error(errorMessage), }, ]; render( <MockedProvider mocks={errorMock} addTypename={false}> <table> <tbody> <CustomTableCell eventId="event123" /> </tbody> </table> </MockedProvider>, ); await waitFor(() => { expect( screen.getByText( 'Unable to load event details. Please try again later.', ), ).toBeInTheDocument(); + // Verify toast error was called with the correct message + expect(toast.error).toHaveBeenCalledWith( + expect.stringContaining(errorMessage) + ); }); });
51-160: Consider adding test cases for edge scenariosThe test suite would benefit from additional test cases:
- Date formatting for different timezones
- Very long event titles
- Missing optional fields
Example test case for date formatting:
it('handles different timezones correctly', async () => { const datesMock = { ...mocks[0], result: { data: { event: { ...mocks[0].result.data.event, startDate: '2023-12-31', endDate: '2024-01-01', } } } }; render( <MockedProvider mocks={[datesMock]} addTypename={false}> <BrowserRouter> <table> <tbody> <CustomTableCell eventId="event123" /> </tbody> </table> </BrowserRouter> </MockedProvider>, ); await waitFor(() => screen.getByTestId('custom-row')); // Verify date appears correctly for different locales/timezones expect(screen.getByText(/December 31, 2023/)).toBeInTheDocument(); });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/MemberDetail/customTableCell.spec.tsx(1 hunks)
🔇 Additional comments (3)
src/components/MemberDetail/customTableCell.spec.tsx (3)
1-14: LGTM! Clean setup of imports and mocks
The imports are well-organized and the toast mock is properly implemented using Vitest's mocking functionality.
51-83: LGTM! Well-structured main test case
The main test case thoroughly verifies the component's rendering and data display.
85-98: LGTM! Loading state test
Good coverage of the loading state scenario.
|
@coderabbit.ai full review |
|
@im-vedant: Understood. I will perform a full review. ✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/screens/OrganizationVenues/OrganizationVenues.spec.tsx (1)
285-289: Consider moving the router mock outside beforeAllWhile the migration to Vitest is correct, consider moving the react-router-dom mock outside the test blocks for better maintainability. This would also prevent potential issues with mock timing.
- beforeAll(() => { - vi.doMock('react-router-dom', async () => ({ - ...(await vi.importActual('react-router-dom')), - useParams: () => ({ orgId: 'orgId' }), - })); - }); - - afterAll(() => { - vi.clearAllMocks(); - }); +vi.mock('react-router-dom', async () => ({ + ...(await vi.importActual('react-router-dom')), + useParams: () => ({ orgId: 'orgId' }), +}));Also applies to: 293-293, 324-326, 331-331
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/components/MemberDetail/customTableCell.spec.tsx(1 hunks)src/components/MemberDetail/customTableCell.test.tsx(0 hunks)src/components/OrganizationScreen/OrganizationScreen.spec.tsx(1 hunks)src/screens/OrganizationVenues/OrganizationVenues.spec.tsx(5 hunks)
💤 Files with no reviewable changes (1)
- src/components/MemberDetail/customTableCell.test.tsx
🧰 Additional context used
📓 Learnings (2)
src/screens/OrganizationVenues/OrganizationVenues.spec.tsx (1)
Learnt from: bitbard3
PR: PalisadoesFoundation/talawa-admin#2588
File: src/components/ChangeLanguageDropdown/ChangeLanguageDropdown.spec.tsx:145-155
Timestamp: 2024-12-02T04:20:11.745Z
Learning: In PRs focused solely on refactoring test cases from Jest to Vitest, avoid suggesting optimizations or changes outside the migration scope.
src/components/OrganizationScreen/OrganizationScreen.spec.tsx (1)
Learnt from: bitbard3
PR: PalisadoesFoundation/talawa-admin#2588
File: src/components/ChangeLanguageDropdown/ChangeLanguageDropdown.spec.tsx:145-155
Timestamp: 2024-12-02T04:20:11.745Z
Learning: In PRs focused solely on refactoring test cases from Jest to Vitest, avoid suggesting optimizations or changes outside the migration scope.
🔇 Additional comments (3)
src/components/OrganizationScreen/OrganizationScreen.spec.tsx (1)
13-13: LGTM! Correct migration to Vitest
The migration from Jest to Vitest is implemented correctly, using the recommended async/await pattern for mocking modules.
Also applies to: 15-19
src/components/MemberDetail/customTableCell.spec.tsx (1)
7-13: LGTM! Proper Vitest mock implementation
The migration to Vitest is implemented correctly, with proper mocking of the react-toastify module using vi.fn().
src/screens/OrganizationVenues/OrganizationVenues.spec.tsx (1)
253-259: LGTM! Correct toast mock implementation
The migration of react-toastify mock to Vitest is implemented correctly.
|
@palisadoes I have removed the commented code, but still CodeRabbit.ai is not approving my changes. |
palisadoes
merged commit d5a790d
into
PalisadoesFoundation:develop-postgres
|
I merged as an exception |
What kind of change does this PR introduce?
This PR migrates the test cases in src/screens/OrganizationVenues/OrganizationVenues.test.tsx from Jest to Vitest, ensuring compatibility with Vitest .
✅ Replace Jest-specific functions and mocks with Vitest equivalents
✅ Ensure all tests in src/screens/OrganizationVenues/OrganizationVenues.test.tsx pass after migration using npm run test:vitest
✅ Maintain the test coverage for the file as 100% after migration
✅ Upload a video or photo for this specific file coverage is 100% in the PR description
Issue Number:
Fixes #2563
Did you add tests for your changes?
No
ScreenShot
Summary by CodeRabbit
react-toastifyandreact-router-domto align with Vitest.CustomTableCellcomponent, covering rendering, loading, error, and no event found scenarios.OrganizationScreentests to check fordata-expandedattribute instead of CSS classes.customTableCell.test.tsxfile.