Added tests for src/screens/Users/Users.tsx #2745
Conversation
WalkthroughThe pull request enhances the Changes
Assessment against linked issues
Possibly related issues
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.
- Please convert
src/screens/Users/Users.test.tsxto use vitest. we are migrating to that platform. - The file should be renamed to
src/screens/Users/Users.spec.tsx - Here is an example of a similar conversion
- Make sure all tests pass. The test code coverage levels for Users.tsx is too low
2. https://app.codecov.io/gh/PalisadoesFoundation/talawa-admin/pull/2745/blob/src/screens/Users/Users.tsx
- Ensure coderabbit.ai approves your changes.
![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/Users/Users.tsx (2)
126-126: Consider removing or converting console logs to a logging utility.Debug logs are useful during development but might clutter production logs.
267-272: Sorting logic for date fields.Sorting by date is handled cleanly using a comparator. For performance in very large datasets, consider server-side sorting or more efficient in-memory approaches.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/screens/Users/Users.test.tsx(6 hunks)src/screens/Users/Users.tsx(4 hunks)
🔇 Additional comments (12)
src/screens/Users/Users.tsx (7)
130-131: Verify pagination edge cases.
Using the length-based condition helps maintain correct pagination flow. However, ensure you handle edge cases where the response might return fewer items than expected.
216-216: Safe fallback for skip variable.
Providing a fallback of 0 is a good defensive measure. This ensures that null or undefined values won't break pagination logic.
233-239: De-duplicating fetched users.
Using a Map to remove duplicates by user ID is clean and efficient. Ensure user IDs are globally unique across all data sets.
245-247: Early return prevents redundant sorts.
This avoids unnecessary state changes if the same sorting option is reselected.
250-250: Reset displayed users appropriately before refetching.
Emptying displayed users before re-sorting helps ensure a clean state.
276-278: Prevent re-filtering if option is unchanged.
This is a solid check to avoid redundant computations.
281-281: Reset pagination after filtering.
Resetting 'hasMore' ensures proper pagination flow for newly filtered data.
src/screens/Users/Users.test.tsx (5)
33-33: Consistent ISO date format.
Switching to ISO 8601 format improves clarity and consistency for date comparisons in tests.
Also applies to: 109-109, 185-185
304-304: Clearing mocks after each test.
Using jest.clearAllMocks() ensures test isolation by preventing mock state from leaking.
705-705: Renamed test case for clarity.
Renaming to “Users should be sorted in newest order” aligns better with the underlying logic.
734-739: Verifying sorted outcome.
Confirms the test accurately checks if the “newest” user (Jane) is displayed first. Looks good.
742-764: Testing search trigger on Enter key.
The new test ensures proper event handling. Test coverage is expanded effectively.
|
@Aad1tya27 , are you still working on it? The test code coverage levels for Users.tsx are too low. Please add test cases to bring it to an acceptable range. |
Yeah I am working on it. I have shifted from jest to vitest. I am facing some errors related to variables not updating which leads to unexpected values. |
|
Actually the time consuming part is fixing the state management in |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (10)
src/screens/Users/Users.spec.tsx (7)
Line range hint
307-323: Add more assertions for rendered component
You test for the presence oftestcomp. Consider adding more checks (e.g., relevant text or subcomponents) to validate actual functionality.
Line range hint
344-362: Confirm localstorage-based flows
Ensure there's a clear assertion about the component’s behavior when userId does not exist. Adding an expectation to confirm the fallback would improve test clarity.
Line range hint
418-447: Handling empty data
Test correctly verifies "No User Found" availability. Consider also verifying if any UI elements (like a retry button) need to appear.
Line range hint
465-484: Warning alert test
Well done verifying the alert for the absence of organizations. Consider adding an assertion on how dismissals or subsequent calls are handled, if present.
Line range hint
579-608: Rerendering coverage
You confirm the component can remount or update with new data. Consider an additional assertion verifying the new data is actually displayed.
Line range hint
704-740: Sorting newest order
Good approach. You confirm the correct row ordering. For further robustness, consider verifying the entire user list order if more users exist.
741-766: Enter key triggers search
The test checks user input and pressing Enter. A quick check for updated state or displayed results would add coverage.src/screens/Users/Users.tsx (3)
121-125: Syncing local state with remote data
ThisuseEffectsetsusersDataupon new data. IfisLoadingchanges rapidly, consider a guard to avoid flicker.
Line range hint
226-264: Extending loadMoreUsers for pagination
Nice use of skip and limit. The fallback to aMapfor deduplication is helpful. Carefully validate that large additions won’t degrade performance.
292-297: Sorting in oldest order
Straightforward approach. Confirm you handle tie-breakers consistently if multiple users share the same date.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/screens/Users/Users.spec.tsx(22 hunks)src/screens/Users/Users.tsx(9 hunks)
🔇 Additional comments (28)
src/screens/Users/Users.spec.tsx (19)
15-15: Switching to Vitest is appropriate for new test framework requirements
Good job replacing Jest imports with Vitest. Ensure all test references (e.g., jest.mock) are also updated if used elsewhere.
32-32: Adopting the ISO 8601 date format
This standardized date format makes parsing and sorting easier. Verify that the component correctly handles this new format.
108-108: Revisiting consistent date format
Again, good move to ISO 8601. Ensure test data fully reflects real usage scenarios for robust coverage.
184-184: Ensuring all user data is uniformly formatted
Maintaining a consistent date format across all test users is helpful for predictable behavior in sorting and filtering.
303-303: Cleaning up mocks after each test
Using vi.restoreAllMocks() helps avoid cross-test contamination. This is a good practice.
Line range hint 324-343: Clarify user state verification
Tests handle scenarios when the user isn't superAdmin and has no userId. Confirm we see expected fallback UI or redirection, if any.
Line range hint 363-378: Validate superAdmin scenario
Looks good. You might want to verify that a superAdmin sees additional admin-specific elements, if applicable.
Line range hint 379-417: Search feature coverage
Your test steps are comprehensive, covering different typed inputs and clearing. Good job ensuring “not found” text is tested.
Line range hint 448-464: Same scenario, fewer users
By testing explicitly for an empty list, you confirm that the correct fallback text or actions are displayed. Looks good.
Line range hint 485-505: Negative check for empty organizations
Clear approach verifying the alert does not appear. This is a robust check.
Line range hint 506-578: Filter functionality coverage
Your approach systematically tests toggling each filter option. Good job ensuring transitions between filter states are tested.
Line range hint 609-630: Edge case: fewer users than perPageResult
This check ensures pagination logic updates hasMore appropriately. The test looks solid.
Line range hint 631-703: Filter by role
Good test verifying all roles: Admin, SuperAdmin, User, and Cancel. The multi-step approach ensures coverage of repeated toggles.
660-660: Multiple user validations
Your “expect to see user name” checks are correct. This confirms each user type is displayed post-filter.
Also applies to: 673-673, 685-685, 698-698
721-726: Clicking sort button
The step-by-step firing of events is correct. Test coverage ensures the sorting function is triggered.
Also applies to: 727-731, 733-739
767-802: Oldest sorting scenario
Similar to newest sorting checks. Tests appear thorough; reaffirm the entire user list order is correct.
804-843: Role filter idempotency
Ensures repeated clicks on the same role filter do not cause an unnecessary re-fetch. Good coverage.
844-897: Sort filter idempotency
Great job verifying subsequent clicks on the same sort option do not alter state or cause redundant calls.
898-919: Empty search refetch
Tests confirm a cleared search reverts to the full user list. This is a key scenario. Looks good.
src/screens/Users/Users.tsx (9)
80-80: Introducing loadUnqUsers
Tracking newly loaded unique users is a good approach. Confirm usage in loadMoreUsers logic is robust.
87-89: Refined usersData state
Storing fetched user data explicitly is clear. Just ensure references to data?.users are updated consistently.
92-92: GraphQL query updates
Ensure user query variables remain in sync with the new sorting approach. This looks correct so far.
135-141: Conditional logic for hasMore & displayedUsers
You correctly limit hasMore if fewer results are returned. Sorting and filtering the stored list is well-structured.
178-183: Loads more users upon recognizing unique user count
Ensure no infinite loop arises if displayedUsers updates loadUnqUsers repeatedly. The approach seems correct, but keep an eye on potential recursion.
271-273: Preventing redundant sort calls
Skipping a re-fetch if the sort option is already selected optimizes performance. Well done.
301-305: Filtering idempotency
Similar logic to sorting: short-circuiting repeated clicks. Good approach to reduce unnecessary re-renders.
389-391: Callbacks for sorting dropdown
Your onClick handlers for newest/oldest link nicely to the handleSorting. No issues here.
Also applies to: 397-399
485-487: InfiniteScroll’s next function
Passes correct skip/limit to load more. Validate that scrolling event is tested in e2e or integration tests.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
src/screens/Users/Users.tsx (3)
Line range hint
226-265: Simplify the skip calculationThe pagination and duplicate handling logic is good, but the skip calculation could be simplified.
Consider simplifying line 231:
- skip: skipValue - perPageResult >= 0 ? skipValue - perPageResult : 0, + skip: Math.max(0, skipValue - perPageResult),
271-273: Optimize the sorting implementationGood optimization to prevent unnecessary re-sorting, but the sorting implementation could be more concise.
Consider simplifying the sorting implementation:
-sortedUsers.sort( - (a, b) => - new Date(a.user.createdAt).getTime() - - new Date(b.user.createdAt).getTime(), -); +sortedUsers.sort((a, b) => + Date.parse(a.user.createdAt) - Date.parse(b.user.createdAt) +);Also applies to: 292-297
485-487: Consider memoizing the loadMoreUsers callbackThe infinite scroll implementation works correctly, but could benefit from performance optimization.
Consider memoizing the callback to prevent unnecessary re-renders:
+const handleLoadMore = useCallback(() => { + loadMoreUsers(displayedUsers.length, perPageResult); +}, [displayedUsers.length, perPageResult]); -next={() => { - loadMoreUsers(displayedUsers.length, perPageResult); -}} +next={handleLoadMore}src/screens/Users/Users.spec.tsx (3)
24-249: Reduce mock data duplicationThe mock data is comprehensive but contains repeated structures that could be extracted into helper functions.
Consider creating helper functions to generate common objects:
const createAddress = (overrides = {}) => ({ city: 'Kingston', countryCode: 'JM', dependentLocality: 'Sample Dependent Locality', line1: '123 Jamaica Street', line2: 'Apartment 456', postalCode: 'JM12345', sortingCode: 'ABC-123', state: 'Kingston Parish', ...overrides }); const createCreator = (overrides = {}) => ({ _id: '123', firstName: 'Jack', lastName: 'Smith', image: null, email: '[email protected]', createdAt: '19/06/2022', ...overrides });
1223-1871: Enhance test descriptions for clarityThe test coverage is comprehensive, but some test descriptions could be more descriptive.
Consider updating these test descriptions to be more specific:
-it('Component should be rendered properly', async () => { +it('should render the Users component with all expected elements', async () => { -it('Testing seach by name functionality', async () => { +it('should filter users correctly when searching by name', async () => { -it('check for rerendering', async () => { +it('should maintain state consistency when component rerenders', async () => {
1856-1856: Remove console.log statementsDebug console.log statements should be removed before merging.
-console.log(users, users.length); +// Remove debug statement -console.log(users2, users2.length); +// Remove debug statementAlso applies to: 1868-1868
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/screens/Users/Users.spec.tsx(1 hunks)src/screens/Users/Users.tsx(9 hunks)
🔇 Additional comments (2)
src/screens/Users/Users.tsx (2)
80-80: LGTM: State management improvements
The state management changes improve type safety and handle duplicate users effectively:
- Added
loadUnqUsersto track unique users count - Enhanced type definition for
usersData
Also applies to: 87-89
121-125: LGTM: Improved data synchronization
The useEffect hooks effectively manage data synchronization:
- Syncs local state with GraphQL query results
- Handles loading of additional users when duplicates are detected
Also applies to: 178-182
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
src/screens/Users/UsersMocks.ts (2)
506-583: Consider removing data duplication in repeated user subobjects.
It appears that many fields (city, countryCode, postalCode, etc.) are repeated across user objects. Although these mocks are solely for testing, consider representing repeated data in a more concise manner to avoid maintenance overhead.
734-1600: Check for large mock arrays' impact on performance.
MOCK_USERS2significantly extends the user list, which is helpful for scroll/load testing. However, if test performance degrades, consider dynamic generation or limiting the data volume.src/screens/Users/Users.tsx (2)
80-80: Use a more descriptive name for theloadUnqUsersstate.
While it’s clear enough in context, a name likenumAdditionalUniqueUsersmight better convey the meaning.
Line range hint
226-264: Handle potential concurrency edge cases inloadMoreUsers.
If multiple scroll or fetch events fire in quick succession, ensuresetIsLoadingMoreis properly managed and that repeated merges do not cause inconsistency.src/screens/Users/Users.spec.tsx (3)
30-58: Avoid duplicating large test data inMOCKS_NEW
MOCKS_NEWreferencesMOCK_USERS. If additional mocking is needed, consider reusing or augmenting existing mock objects to reduce data redundancy.
Line range hint
108-189: Ensure naming consistency of new mock blocks
MOCKS_NEW2,MOCKS_NEW3might benefit from more descriptive naming, e.g.MOCKS_FILTERED_USERS,MOCKS_PAGINATED_USERSto quickly convey the scenario each set tests.
868-896: Ensure unique user merges are tested for different data sets
This test validates no duplicates on scroll. Consider adding assert checks for the user IDs themselves to confirm no repeated_id.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/screens/Users/Users.spec.tsx(22 hunks)src/screens/Users/Users.tsx(9 hunks)src/screens/Users/UsersMocks.ts(1 hunks)
🔇 Additional comments (8)
src/screens/Users/UsersMocks.ts (2)
584-659: Validate test data consistency.
Jane Doe and John Doe share the same email address in their user objects ([email protected]). This may be intentional, but if not, it could cause confusion or collisions in tests.
660-731: Ensure date formatting is handled in tests.
User date fields, like createdAt, use different formats (sometimes an ISO string with time zone, sometimes a date string like 19/06/2022). If the test logic depends on date comparisons, ensure consistency to avoid unexpectedly failing tests.
src/screens/Users/Users.tsx (3)
178-183: Confirm loadMoreUsers is always triggered safely.
This effect calls loadMoreUsers whenever displayedUsers updates and loadUnqUsers > 0. Ensure no race conditions or infinite loops can occur if displayedUsers changes cause repeated increments.
268-270: Avoid re-fetching if the sort option is unchanged.
You already guard against changing to the same sortingOption in line 269, which is excellent. This helps prevent unnecessary network requests.
482-484: Confirm infinite scrolling logic handles partial loads
When fewer than perPageResult items are available, ensure the hasMore toggling remains accurate, preventing unnecessary or repeated requests.
src/screens/Users/Users.spec.tsx (3)
Line range hint 492-522: Validate the rerender approach
Re-rendering the component with the same link can help detect unintended state resets. Confirm that test coverage ensures no hidden state persists between renders.
Line range hint 544-611: Test coverage is thorough and well-structured
These filter tests effectively validate role-based user filtering. Good job including multiple roles and verifying results.
835-866: Infinite scroll test is clear and marks user additions
Your approach properly checks that the row count increased from 12 to 15. This is a solid test verifying scroll-based data loading.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
src/screens/Users/Users.spec.tsx (5)
1288-1294: Consider making the wait timeout configurableThe wait function has a hardcoded default timeout of 1000ms. Consider making this configurable at the test suite level to allow for different timing requirements in different environments.
const DEFAULT_TIMEOUT = 1000; async function wait(ms = DEFAULT_TIMEOUT): Promise<void> { await act(() => { return new Promise((resolve) => { setTimeout(resolve, ms); }); }); }
1295-1301: Consolidate localStorage setup into a helper functionThe beforeEach hook sets multiple localStorage items individually. Consider consolidating these into a helper function for better maintainability.
const setupLocalStorage = (overrides = {}) => { const defaults = { id: '123', SuperAdmin: true, FirstName: 'John', AdminFor: [{ name: 'adi', _id: '1234', image: '' }], LastName: 'Doe' }; const settings = { ...defaults, ...overrides }; Object.entries(settings).forEach(([key, value]) => { setItem(key, value); }); }; beforeEach(() => { setupLocalStorage(); });
1308-1324: Improve test description clarityThe test description "Component should be rendered properly" is too generic. Consider making it more specific about what "properly" means.
-it('Component should be rendered properly', async () => { +it('should render Users component with initial data when user is authenticated', async () => {
1662-1663: Remove commented out wait statementsThere are several commented out wait statements in the test file. Either remove them if they're unnecessary or uncomment them if they're needed for test stability.
- // await wait(); expect(screen.getByText('Jane Doe')).toBeInTheDocument(); - // await wait(); expect(screen.getByText('John Doe')).toBeInTheDocument(); - // await wait(); expect(screen.getByText('John Doe')).toBeInTheDocument();Also applies to: 1675-1676, 1700-1701
1924-1953: Add error handling for scroll event testsThe scroll event tests should include error cases and verify error handling behavior.
it('should handle errors when loading more users on scroll', async () => { // Add test implementation for error scenarios // For example: // - Network errors // - Invalid response data // - Rate limiting });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/screens/Users/Users.spec.tsx(1 hunks)src/screens/Users/UsersMocks.ts(0 hunks)
💤 Files with no reviewable changes (1)
- src/screens/Users/UsersMocks.ts
🔇 Additional comments (1)
src/screens/Users/Users.spec.tsx (1)
1955-1985: Verify cleanup after scroll tests
The scroll tests should verify that resources are properly cleaned up, including scroll event listeners.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/screens/Users/Users.spec.tsx (4)
856-862: Consider reducing the default wait timeoutThe wait function uses a default timeout of 1000ms, which might be longer than necessary for most tests. Consider reducing this to improve test execution time.
-async function wait(ms = 1000): Promise<void> { +async function wait(ms = 100): Promise<void> { await act(() => { return new Promise((resolve) => { setTimeout(resolve, ms); }); }); }
1230-1231: Remove commented out wait() callsThere are several commented out wait() calls in the test file. These should be removed as they add noise to the code.
Also applies to: 1243-1244, 1255-1256, 1268-1269
894-895: Improve test descriptions for claritySome test descriptions could be more descriptive. For example:
-it(`Component should be rendered properly when user is not superAdmin - and or userId does not exists in localstorage`, async () => { +it('should render correctly for non-superAdmin users without userId in localStorage', async () => { -it(`Component should be rendered properly when userId does not exists in localstorage`, async () => { +it('should render correctly when userId is missing from localStorage', async () => {Also applies to: 914-914
1467-1490: Consolidate assertions in search functionality testThe test "Reset and Refetch function should work when we search an empty string" has multiple separate expect statements that could be combined using
expect.toEqualfor better readability.-expect(screen.queryByText(/Jane Doe/i)).toBeInTheDocument(); -expect(screen.queryByText(/John Doe/i)).toBeInTheDocument(); -expect(screen.queryByText(/Jack Smith/i)).toBeInTheDocument(); +expect(screen.queryAllByRole('row').map(row => row.textContent)).toEqual( + expect.arrayContaining([ + expect.stringMatching(/Jane Doe/i), + expect.stringMatching(/John Doe/i), + expect.stringMatching(/Jack Smith/i) + ]) +);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/screens/Users/Users.spec.tsx(1 hunks)
🔇 Additional comments (3)
src/screens/Users/Users.spec.tsx (3)
24-33: Convert objects to factory functions for better maintainability
The createAddress and createCreator are defined as objects, which limits their reusability and makes it harder to create variations of the mock data.
Note: This is a duplicate of a previous review comment that was acknowledged by the author.
Also applies to: 35-42
863-875: LGTM! Well-structured test setup and teardown
The beforeEach and afterEach hooks properly manage test state by setting up localStorage and cleaning up after each test.
1492-1552: LGTM! Well-implemented infinite scroll tests
The tests for loading more users on scroll and handling duplicate users are well-structured and cover important edge cases.
|
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop-postgres #2745 +/- ##
=====================================================
+ Coverage 68.60% 87.76% +19.15%
=====================================================
Files 296 313 +17
Lines 7358 8222 +864
Branches 1606 1854 +248
=====================================================
+ Hits 5048 7216 +2168
+ Misses 2069 807 -1262
+ Partials 241 199 -42 ☔ View full report in Codecov by Sentry. |
palisadoes
merged commit 9227500
into
PalisadoesFoundation:develop-postgres
|
You made significant changes. I merged it and opened another issue to get the coverage to 100% Thanks |
What kind of change does this PR introduce?
Added tests for
src/screens/Users/Users.tsx, and fixed bugs in Users.tsx in sorting and filtering.Issue Number:
Fixes #2376
Did you add tests for your changes?
Yes
Summary
To add further tests for the above file.
Does this PR introduce a breaking change?
No
Have you read the contributing guide?
Yes
Summary by CodeRabbit
New Features
Bug Fixes
Tests