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 and testing functions with their Vitest equivalents. Key modifications include updating test files for Changes
Assessment against linked issues
Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (2)
🧰 Additional context used📓 Learnings (1)src/components/OrganizationScreen/OrganizationScreen.spec.tsx (1)🔇 Additional comments (4)src/components/OrganizationScreen/OrganizationScreen.spec.tsx (2)
The migration changes follow the correct pattern:
Also applies to: 15-18
The test assertions have been properly updated to check the Also applies to: 92-94 src/components/MemberDetail/customTableCell.spec.tsx (2)
The migration changes follow the correct pattern:
The test assertion has been improved to use 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 92.23% 83.92% -8.32%
====================================================
Files 295 312 +17
Lines 7036 8129 +1093
Branches 1516 1836 +320
====================================================
+ Hits 6490 6822 +332
- Misses 342 1158 +816
+ Partials 204 149 -55 ☔ View full report in Codecov by Sentry. |
|
@palisadoes I have fixed those failing test. |
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (2)
src/components/OrganizationScreen/OrganizationScreen.test.tsx (2)
Line range hint
1-19: Update Jest-specific imports and mocks for Vitest compatibilityThe test file still contains Jest-specific code that needs to be migrated to Vitest:
Apply these changes for Vitest compatibility:
- import 'jest-location-mock'; + import 'vitest-location-mock'; - jest.mock('react-router-dom', () => ({ - ...jest.requireActual('react-router-dom'), + vi.mock('react-router-dom', () => ({ + ...vi.importActual('react-router-dom'),
Line range hint
98-106: Maintain consistency in test assertionsWhile the drawer toggle tests have been updated to use data attributes, the window resize test still relies on class-based assertions. This inconsistency could contribute to the CSS parsing issues mentioned in the PR comments.
Consider updating the window resize test to match the new pattern:
test('handles window resize', () => { renderComponent(); window.innerWidth = 800; fireEvent(window, new Event('resize')); - expect(screen.getByTestId('mainpageright')).toHaveClass(styles.expand); + expect(screen.getByTestId('mainpageright')).toHaveAttribute('data-expanded', 'true'); });This change would:
- Maintain consistency with other tests
- Reduce dependency on CSS modules
- Help mitigate the CSS parsing issues
🧹 Nitpick comments (1)
src/components/OrganizationScreen/OrganizationScreen.test.tsx (1)
84-87: Update test comments to match new assertion styleThe migration to data-attribute assertions is a good improvement, matching our previous recommendations. However, the comments still reference the old class-based checks.
Update the comments to reflect the new assertion style:
- // Check for contract class after closing + // Check for expanded state after closing expect(screen.getByTestId('mainpageright')).toHaveAttribute( 'data-expanded', 'true', ); - // Check for expand class after opening + // Check for expanded state after opening expect(screen.getByTestId('mainpageright')).toHaveAttribute( 'data-expanded', 'false', );Also applies to: 93-95
📜 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/components/OrganizationScreen/OrganizationScreen.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/components/OrganizationScreen/OrganizationScreen.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/MemberDetail/customTableCell.test.tsx
palisadoes
changed the title
|
@palisadoes Some jest test were failing because jsdom css parser was not able to parse the stylesheet. Only solution to fix this issue is to ignore that error. That's why I have to edit the setupTest.ts file. You can refer to this jsdom/jsdom#2177 issue for more details. |
|
The solution is to fix the parsing of the CSS file, not to ignore errors. Please fix it. |
…bj-proxy to mock Css modules
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
jest.config.js (1)
38-38: Fix formatting: Remove leading whitespaceRemove the extra space at the beginning of line 38 to maintain consistent formatting.
- '\\.(css|less)$': 'identity-obj-proxy', + '\\.(css|less)$': 'identity-obj-proxy',🧰 Tools
🪛 eslint
[error] 38-38: Delete
·(prettier/prettier)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (1)
jest.config.js(1 hunks)
🧰 Additional context used
🪛 eslint
jest.config.js
[error] 38-38: Delete ·
(prettier/prettier)
|
@palisadoes I fixed the issue by installing identity-obj-proxy, as suggested in the Jest documentation on mocking CSS modules. |
…rc\components\OrganizationScreen\OrganizationScreen.test.tsx from jest to vitest
| @@ -35,6 +36,7 @@ export default { | |||
| '<rootDir>/src', | |||
| ], | |||
| moduleNameMapper: { | |||
| '\\.(css|less)$': 'identity-obj-proxy', | |||
There was a problem hiding this comment.
Some jest test were failing because jsdom css parser was not able to parse the stylesheet. I have fixed this issue by installing this package. These line are added to the Jest configuration file to ensure that our Jest test environment can properly handle imported CSS/LESS modules during testing. It is recommended in Jest documentation. https://jestjs.io/docs/webpack#mocking-css-modules
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
New Features
data-expandedattribute to theOrganizationScreencomponent to indicate the state of the drawer.Bug Fixes
CustomTableCellcomponent to dynamically generate event dates for improved flexibility.OrganizationScreento verify drawer state using the newdata-expandedattribute.Tests
react-toastifyin multiple test files to use Vitest.Chores