Refactor: src/screens/OrganizationDashboard from Jest to Vitest #2678
Conversation
… the failing tests
WalkthroughThe pull request involves two test files: Changes
Assessment against linked issues
Possibly related issues
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
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: 0
🔭 Outside diff range comments (1)
src/components/OrganizationScreen/OrganizationScreen.test.tsx (1)
Line range hint
16-17: Migrate Jest mocks to VitestAs part of the Jest to Vitest migration, update the mocking syntax:
-jest.mock('react-router-dom', () => ({ +vi.mock('react-router-dom', () => ({ ...jest.requireActual('react-router-dom'), useParams: () => ({ orgId: mockID }), useMatch: () => ({ params: { eventId: 'event123', orgId: '123' } }), }));
🧹 Nitpick comments (3)
jest.config.js (1)
Line range hint
1-89: Consider removing jest.config.js if migrating to VitestSince the PR objective is to migrate tests from Jest to Vitest, maintaining both Jest and Vitest configurations might lead to confusion. Consider:
- Moving all tests to Vitest
- Removing jest.config.js
- Ensuring all configurations are in vitest.config.ts
src/components/OrganizationScreen/OrganizationScreen.test.tsx (1)
79-85: Remove debug console.log statementsDebug console.log statements should be removed before committing:
- console.log('before close', screen.getByTestId('mainpageright').className); - console.log('after close', screen.getByTestId('mainpageright').className); - console.log('styles', styles.expand); - console.log('before 800', screen.getByTestId('mainpageright').className); - console.log('******', screen.getByTestId('mainpageright').className); - console.log('styles expanddd', styles.expand);Also applies to: 96-100
src/screens/OrganizationDashboard/OrganizationDashboard.spec.tsx (1)
Line range hint
1-311: Consider consistent test file extensionsThe project uses both
.test.tsxand.spec.tsxextensions for test files. Consider standardizing on one extension throughout the project for consistency:
OrganizationScreen.test.tsxOrganizationDashboard.spec.tsxChoose either
.test.tsxor.spec.tsxand apply it consistently.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
jest.config.js(2 hunks)src/components/OrganizationScreen/OrganizationScreen.test.tsx(1 hunks)src/screens/OrganizationDashboard/OrganizationDashboard.spec.tsx(13 hunks)
🧰 Additional context used
📓 Learnings (1)
jest.config.js (1)
Learnt from: MayankJha014
PR: PalisadoesFoundation/talawa-admin#2619
File: vitest.config.ts:0-0
Timestamp: 2024-12-09T15:54:04.872Z
Learning: The files `scripts/custom-test-env.js`, `src/utils/i18nForTest.ts`, and `vitest.setup.ts` are not test files and should not be included in the test pattern.
🔇 Additional comments (3)
jest.config.js (1)
39-39: LGTM: CSS module mapping is correctly configured
The addition of identity-obj-proxy for CSS/LESS files is the correct approach for handling CSS modules in tests.
src/screens/OrganizationDashboard/OrganizationDashboard.spec.tsx (2)
20-31: LGTM: Well-documented test suite
The comprehensive documentation clearly outlines the test coverage and scenarios. This is excellent practice for maintaining test suites.
33-38: LGTM: Proper Vitest migration
The migration from Jest to Vitest mocks is done correctly:
- Using
vi.mock()instead ofjest.mock() - Using
vi.fn()instead ofjest.fn() - Proper async module mocking
Also applies to: 106-112
|
@Cioppolo14 @varshith257 I have fixed all the failing tests . Could you please review it ? |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop-postgres #2678 +/- ##
====================================================
+ Coverage 83.81% 86.29% +2.47%
====================================================
Files 295 312 +17
Lines 7274 8134 +860
Branches 1592 1841 +249
====================================================
+ Hits 6097 7019 +922
- Misses 940 943 +3
+ Partials 237 172 -65 ☔ View full report in Codecov by Sentry. |
varshith257
dismissed stale reviews from coderabbitai[bot] and themself
via
5b77244
|
It seems the changes doesn't include into the coverage of vitest. Take a Lokok |
…esolve merge conflicts
…4code/talawa-admin into OrgDashboard-jest-to-vitest
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
palisadoes
merged commit 7bf7ab1
into
PalisadoesFoundation:develop-postgres
What kind of change does this PR introduce?
This PR migrates the OrganizationDashboard tests from Jest to Vitest and addresses all failing tests to ensure they pass successfully
Issue Number:
Fixes #2558
Did you add tests for your changes?
Yes
Snapshots/Videos:
Reason for Adding identity-obj-proxy for CSS Modules:
The purpose of adding the identity-obj-proxy mapping and the CSS transformation is to properly parse CSS Modules in Jest.However, CSS Modules by default may still get ignored by Jest's configuration. To ensure proper handling, we need to map CSS Modules to identity-obj-proxy, which allows Jest to mock and return class names as string values. This avoids errors in Jest tests and ensures that styles are correctly interpreted in the test environment.
Alternative Approach with jest-css-modules-transform
I also got a alternative approach in which i can replace the below configuration with jest-css-modules-transform, which specifically targets CSS Modules.
with
Replacing
jest-preview/transforms/csswithjest-css-modules-transform:The key difference with this approach is that
jest-css-modules-transformis specifically designed for transforming , parsing and mocking CSS Modules in a Jest environment. It automatically handles CSS Modules by transforming them into mockable objects, which eliminates the need for usingidentity-obj-proxy.Test Cases Results(Using 1st approach)
Summary by CodeRabbit
OrganizationScreencomponent with improved assertions using module-scoped styles.OrganizationDashboardcomponent tests to use Vitest for mocking, with detailed comments and structured test cases for various scenarios.