-
-
Notifications
You must be signed in to change notification settings - Fork 811
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
convert from jest to vitest #2486 #2705
convert from jest to vitest #2486 #2705
Conversation
WalkthroughThis pull request focuses on refactoring the Changes
Possibly related issues
Possibly related PRs
Suggested labels
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
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/components/Advertisements/Advertisements.spec.tsx (1)
Line range hint
697-698
: Consider removing console.log statementsThese debug statements might have been left accidentally. While not critical, removing them would improve test readability.
- console.log('before scroll', moreiconbtn); fireEvent.scroll(window, { target: { scrollY: 500 } }); moreiconbtn = await screen.findAllByTestId('moreiconbtn'); - console.log('after scroll', moreiconbtn);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/Advertisements/Advertisements.spec.tsx
(4 hunks)
🧰 Additional context used
📓 Learnings (2)
📓 Common learnings
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/Advertisements/Advertisements.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 (2)
src/components/Advertisements/Advertisements.spec.tsx (2)
2-2
: LGTM: Successful migration to Vitest mocking
The migration from Jest to Vitest mocking has been implemented correctly. The vi
mock implementations follow Vitest's best practices.
Also applies to: 59-74
474-489
: LGTM: Date comparison logic is correct
The date comparison logic properly verifies that the advertisement's end date is in the future, which is the expected behavior for active campaigns.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop-postgres #2705 +/- ##
====================================================
+ Coverage 82.81% 87.30% +4.48%
====================================================
Files 295 313 +18
Lines 7274 8583 +1309
Branches 1592 1933 +341
====================================================
+ Hits 6024 7493 +1469
+ Misses 1012 896 -116
+ Partials 238 194 -44 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please see comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/components/Advertisements/Advertisements.spec.tsx (1)
487-487
: Remove debugging console.log statementThis appears to be debugging code that was accidentally left in during the migration process.
- console.log(dateObject);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/Advertisements/Advertisements.spec.tsx
(4 hunks)
🧰 Additional context used
📓 Learnings (2)
📓 Common learnings
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/Advertisements/Advertisements.spec.tsx (2)
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.
Learnt from: IITI-tushar
PR: PalisadoesFoundation/talawa-admin#2680
File: src/components/Advertisements/core/AdvertisementEntry/AdvertisementEntry.spec.tsx:528-712
Timestamp: 2024-12-22T07:43:26.168Z
Learning: You prefer to keep migrated tests even if they appear duplicated because they originated from the old AdvertisementEntry.test.tsx file.
🔇 Additional comments (3)
src/components/Advertisements/Advertisements.spec.tsx (3)
2-2
: LGTM: Correct Vitest imports
The migration from Jest to Vitest imports is implemented correctly.
19-24
: LGTM: Import paths updated correctly
The conversion from absolute to relative imports is implemented correctly.
Also applies to: 31-33
58-73
: LGTM: Mock implementations migrated correctly
The migration from Jest's mocking syntax to Vitest's vi
is implemented correctly, maintaining the same functionality.
cda2836
into
PalisadoesFoundation:develop-postgres
I had to revert this PR. You were not assigned the issue. There isn’t an issue assigned to you for this PR. Please follow the guidelines in our PR_GUIDELINES.md file. We have the procedures in place so that everyone has a fair chance of contributing. I will be closing this pull request. Please follow the procedures and resubmit when ready. |
Hi @khushipatil1523. You have mistakenly referenced #2484 where it should have been #2486. Can u please correct the PR with right details so that the issue #2486 can be closed? |
@bandhan-majumder This PR was reverted. |
Yes @palisadoes I noticed this later. It references to a different issue. Actually seeing the status of PR |
As the PR was reverted, the original files were not changed so the |
You made a typo in your PR template which made me think you had not been assigned the original issue. Please resubmit your PR. |
What kind of change does this PR introduce?
refactoring
Issue Number: #2484
Fixes ##2484
Did you add tests for your changes?
Vitest test passed 👍:
Summary by CodeRabbit
Bug Fixes
Tests