Refactor: src/screens/SubTags from Jest to Vitest #2733
Conversation
WalkthroughThis pull request focuses on migrating the test suite for the Changes
Assessment against linked issues
Possibly related issues
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous 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
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Congratulations on making your first PR! 🎊 If you haven't already, check out our Contributing Guidelines and PR Reporting Guidelines to ensure that you are following our guidelines for contributing and creating PR.
![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
🧹 Nitpick comments (2)
src/screens/SubTags/SubTags.spec.tsx (2)
82-84: Flexible routing approach
Accepting an 'initialRoute' parameter is a good practice for dynamic test scenarios. This design is commendable for future test expansions.
121-121: Consider removing or keeping vi.clearAllMocks()
Though clearing mocks in afterEach is typical, confirm whether you need additional cleanup steps or if some mocks should persist.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/screens/SubTags/SubTags.spec.tsx(17 hunks)src/screens/SubTags/SubTags.tsx(2 hunks)
🧰 Additional context used
📓 Learnings (1)
src/screens/SubTags/SubTags.tsx (1)
Learnt from: meetulr
PR: PalisadoesFoundation/talawa-admin#2362
File: src/components/TagActions/TagNode.tsx:79-109
Timestamp: 2024-11-12T10:41:04.351Z
Learning: In the `TagNode` component at `src/components/TagActions/TagNode.tsx`, the infinite scroll component efficiently manages the calls to `loadMoreSubTags`, preventing multiple simultaneous requests.
🔇 Additional comments (21)
src/screens/SubTags/SubTags.spec.tsx (19)
24-24: LGTM for Vitest migration
This import effectively transitions from Jest to Vitest, aligning with the new test framework.
48-48: Check for full coverage of toast functionalities
This mock is adequate for basic success/error coverage, but ensure any other toast methods (info, warning) are not needed.
125-125: Test block for component loading
This test thoroughly checks the initial load scenario. Code coverage looks solid.
135-135: Test block for error scenario
Successfully verifies error handling when the subtags query fails.
145-145: Test block for create tag modal flow
Proper test flow to open and close the modal. No issues found.
167-167: Test block for manage tag navigation
Ensures correct route transition. Implementation is correct.
182-182: Test block for sub tag screen navigation
Verifies accurate navigation upon clicking a tag. LGTM.
197-197: Test block for breadcrumbs navigation
Checks that clicking tag in breadcrumbs leads to the correct sub tags screen.
212-212: Breadcrumbs to org tags screen
This confirms the 'allTagsBtn' triggers correct routing.
227-227: Manage current tag
No issues with the logic that directs to manageTag.
242-242: Test block for search functionality
Properly tests searching by name. Good coverage.
263-263: Testing sort order
Successfully verifies switching between latest and oldest sorting.
320-320: Test block for infinite scroll
This effectively ensures loadMoreSubTags is triggered.
349-349: Adding a new sub tag
Verifies successful creation and toast notification. Well covered.
364-367: Ensuring addSubTagSubmitBtn presence
A reliable wait step ensures no race conditions with the UI.
377-401: Invalid OrgID scenario
Appropriately tests error notification on an invalid organization ID.
50-51: Double-check usage of toast mock calls in tests
The success and error mocks look good. Verify their invocation in newly added tests to ensure correctness.
✅ Verification successful
Let me search for the test file and check the toast mock usage.
Let me search for the actual test cases where these mocks might be used.
Toast mocks are properly used and verified in tests
The test file correctly verifies toast mock calls. Specifically:
toast.successis verified when adding a new sub tag successfully- The test "adds a new sub tag to the current tag" explicitly checks
toast.successcall with the correct message - The test "Adding a subtag with invalid OrgID" appears to test the error case
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Find and show the test file content
rg -A 5 -B 5 "success: vi.fn\(\)" "src/screens/SubTags/SubTags.spec.tsx"
Length of output: 214
Script:
#!/bin/bash
# Search for test cases that might use toast.success or toast.error
rg -A 10 -B 10 "it\(.*" "src/screens/SubTags/SubTags.spec.tsx"
Length of output: 7180
25-25: Confirm that matchers are properly installed
While this import from '@testing-library/jest-dom/vitest' is valid for Vitest environment, verify that all expected matchers (e.g., toBeInTheDocument) remain functional.
✅ Verification successful
Let me verify the functionality of Jest DOM matchers by checking the test files and their usage.
Let me try different commands to verify the Jest DOM setup and usage.
Based on the results, I can now generate the final response:
Jest DOM matchers are properly configured and functional
The codebase shows consistent usage of Jest DOM matchers across multiple test files, with proper configuration in both vitest.setup.ts and setupTests.ts. The import from '@testing-library/jest-dom/vitest' is valid and the extensive test suite demonstrates active usage of matchers like toBeInTheDocument(), toHaveClass(), toHaveAttribute(), etc.
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Search for usage of Jest DOM matchers in test files
rg "toBeInTheDocument|toHaveAttribute|toHaveClass|toHaveStyle|toBeVisible|toBeDisabled" -t tsx -t ts --context 2
Length of output: 142
Script:
#!/bin/bash
# Check test setup files for Jest DOM configuration
fd "setup" -e ts -e js -e tsx
Length of output: 133
Script:
#!/bin/bash
# Check package.json for testing dependencies
cat package.json | jq '.dependencies + .devDependencies'
Length of output: 3659
Script:
#!/bin/bash
# Look for other test files using the same import
rg "@testing-library/jest-dom/vitest" -t tsx -t ts
Length of output: 78
Script:
#!/bin/bash
# Search for test files and their content without specifying file extensions
rg "toBeInTheDocument|toHaveAttribute|toHaveClass|toHaveStyle|toBeVisible|toBeDisabled"
Length of output: 65952
Script:
#!/bin/bash
# Check content of test setup files
cat vitest.setup.ts setup.ts src/setupTests.ts
Length of output: 7058
Script:
#!/bin/bash
# Look for jest-dom imports without specifying file extensions
rg "@testing-library/jest-dom"
Length of output: 946
114-115: Ensure partial mocking doesn’t break navigation
Mocking 'react-router-dom' in this manner might introduce unintended side effects if key exports are overridden. Confirm everything still navigates as expected.
✅ Verification successful
Let me analyze the codebase to verify if the partial mocking of 'react-router-dom' could potentially cause issues.
This will help us understand:
- How widespread is the mocking of react-router-dom
- What specific implementations are used for mocking
- Which router features are actually being used in the codebase
Let me modify the search to handle TypeScript files correctly.
Let me try without specifying file types, as ripgrep should handle TypeScript files by default.
Based on the analysis of the codebase, I can now provide a final response regarding the partial mocking of 'react-router-dom' in SubTags.spec.tsx.
Partial mocking implementation is safe and follows best practices
The current implementation using vi.mock('react-router-dom', async () => ({...(await vi.importActual('react-router-dom')),})) is a safe approach as it preserves all original router functionality while allowing specific overrides. The test file doesn't override any navigation-related functions, making it impossible to break navigation behavior.
Reasoning:
- The mock implementation properly imports the actual module first using
vi.importActual - No navigation-related functions (useNavigate, Link, etc.) are being overridden in the mock
- The pattern is consistently used across many test files in the codebase
- The file under test (SubTags.tsx) uses standard router hooks (useNavigate, useParams, Link) which remain intact with this mocking approach
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# First, let's find all test files that mock react-router-dom
rg -l "mock.*react-router-dom" --type ts --type tsx
# Then, let's see the specific mock implementations
rg "mock.*react-router-dom" -B2 -A5 --type ts --type tsx
# Let's also check for router-related imports and usage
rg "from 'react-router-dom'" -B1 -A1 --type ts --type tsx
Length of output: 262
Script:
#!/bin/bash
# Find all test files that mock react-router-dom
rg -l "mock.*react-router-dom" -t typescript
# See the specific mock implementations
rg "mock.*react-router-dom" -B2 -A5 -t typescript
# Check for router-related imports and usage
rg "from 'react-router-dom'" -B1 -A1 -t typescript
# Also check for any navigation-related functions or hooks usage
rg "useNavigate|useLocation|useParams|Link|NavLink|Navigate" -t typescript
Length of output: 374
Script:
#!/bin/bash
# Find all test files that mock react-router-dom
rg -l "mock.*react-router-dom"
# See the specific mock implementations with context
rg "mock.*react-router-dom" -B2 -A5
# Check for router-related imports and usage
rg "from 'react-router-dom'"
# Check for any navigation-related functions or hooks usage
rg "useNavigate|useLocation|useParams|Link|NavLink|Navigate"
Length of output: 65804
src/screens/SubTags/SubTags.tsx (2)
96-97: Clearer coverage approach
Removing the "istanbul ignore" comment enhances transparency in test coverage. The logical check for fetchMoreResult remains intact.
157-157: No changes to functionality
This line simply references subTagsList creation. No concerns or performance bottlenecks identified.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #2733 +/- ##
===========================================
- Coverage 90.52% 85.16% -5.36%
===========================================
Files 295 312 +17
Lines 7249 8125 +876
Branches 1591 1783 +192
===========================================
+ Hits 6562 6920 +358
- Misses 455 1049 +594
+ Partials 232 156 -76 ☔ View full report in Codecov by Sentry. |
|
@PratapRathi Please fix the first comment so the issue is automatically closed when the PR is merged. Please fix the failed test. |
|
@Cioppolo14 I updated the comment so the Issue will close after merge, and Revert some "istanbul ignore next" comments which I was removed that's why test coverage decreased, now test coverage is almost 95% which is 5% higher than initial jest test, but codecov report is not updated yet. Current code coverage Old code coverage in Jest |
|
Please submit your PRs against our Also if applicable, make sure your branch name is different from Closing. |
PratapRathi
deleted the
Migrate-src/screens/SubTags-from-Jest-to-Vitest
branch
What kind of change does this PR introduce?
This PR migrates the test cases in src/screens/SubTags from Jest to Vitest, ensuring compatibility with Vitest and maintaining 100% test coverage.
Added few more test case and some 'istanbul ignore next' comments
Issue Number:
Fixes #2570
Did you add tests for your changes?
Yes
Snapshots/Videos:
If relevant, did you update the documentation?
N/A
Summary
Does this PR introduce a breaking change?
No
Other information
N/A
Have you read the contributing guide?
Yes
Summary by CodeRabbit
Summary by CodeRabbit
Tests
SubTagscomponent.initialRouteparameter.Bug Fixes
SubTagscomponent, clarifying code comments without changing functionality.