Client table pages functionality tests #50
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThis pull request adds a new test case to the Changes
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
✨ Finishing Touches
🪧 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
CodeRabbit Configuration File (
|
![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 (1)
src/components/ClientTable/ClientTable.spec.tsx (1)
25-74: Comprehensive test for pagination functionalityThe new test case thoroughly validates both pagination behavior and item selection through a well-structured workflow. It provides good coverage of the component's interactive features.
Consider these minor improvements:
- Use
toBeCalledWith()instead oftoBeCalled()to verify that the click handler receives the correct data:- expect(handleClientTableItemClick).toBeCalled(); + expect(handleClientTableItemClick).toBeCalledWith({ + f1: 23, + f2: 24 + });
- Add tests for button disabled states when at pagination boundaries (first/last pages).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/components/ClientTable/ClientTable.spec.tsx(2 hunks)src/components/ClientTable/ClientTablePagination.tsx(1 hunks)
🔇 Additional comments (3)
src/components/ClientTable/ClientTablePagination.tsx (1)
26-29: Good addition of test identifierAdding the
data-testid="page-numbers"attribute to the pagination paragraph enables reliable element selection in the test suite, following the existing pattern in the component.src/components/ClientTable/ClientTable.spec.tsx (2)
2-3: Proper import of testing utilitiesThe added imports appropriately support the new test case -
userEventfor simulating user interactions andvifor mocking.
11-11: Consistent test ID constantThe new constant follows the existing naming pattern and provides a clear reference to the pagination element.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/components/ClientTable/ClientTable.spec.tsx (2)
25-74: Comprehensive test covering pagination and click handling.This test effectively verifies both the pagination functionality and item selection through user interactions. A few suggestions to enhance the test:
- Consider resetting the mock between assertions to verify exact call counts
- Verify the arguments passed to the click handler to ensure correct data is being passed
- Consider extracting the repeated pattern into a helper function to reduce repetition
Here's a refactored version with these improvements:
it('should function correctly', async () => { const handleClientTableItemClick = vi.fn(); render( <ClientTable columns={[ { field: 'f1', label: 'Field 1' }, { field: 'f2', label: 'Field 2' } ]} data={[ { f1: 1, f2: 2 }, { f1: 23, f2: 24 } ]} entriesPerPage={1} minRows={3} onEntryClick={handleClientTableItemClick} /> ); expect(screen.getByTestId(PAGE_NUMBER_TEST_ID)).toBeInTheDocument(); + // Helper function to test pagination and click behavior + const testPaginationAndClick = async (buttonId, expectedText, expectedPage, expectedItem) => { + await userEvent.click(screen.getByTestId(buttonId)); + const itemElement = await screen.findByText(expectedText); + await userEvent.click(itemElement); + expect(handleClientTableItemClick).toHaveBeenCalledWith(expectedItem); + expect(screen.getByTestId(PAGE_NUMBER_TEST_ID).textContent).toBe(expectedPage); + handleClientTableItemClick.mockClear(); + }; + + // Test next button navigation and item click + await testPaginationAndClick(NEXT_BUTTON_ID, '23.00', '2 - 2 / 2', {f1: 23, f2: 24}); + + // Test previous button navigation and item click + await testPaginationAndClick(PREVIOUS_BUTTON_ID, '1.00', '1 - 1 / 2', {f1: 1, f2: 2}); + + // Test last button navigation and item click + await testPaginationAndClick(LAST_BUTTON_ID, '23.00', '2 - 2 / 2', {f1: 23, f2: 24}); + + // Test first button navigation and item click + await testPaginationAndClick(FIRST_BUTTON_ID, '1.00', '1 - 1 / 2', {f1: 1, f2: 2}); - await userEvent.click(screen.getByTestId(NEXT_BUTTON_ID)); - await userEvent.click(await screen.findByText('23.00')); - expect(handleClientTableItemClick).toBeCalled(); - - expect(screen.getByTestId(PAGE_NUMBER_TEST_ID).textContent).toBe('2 - 2 / 2'); - await userEvent.click(screen.getByTestId(PREVIOUS_BUTTON_ID)); - await userEvent.click(await screen.findByText('1.00')); - expect(handleClientTableItemClick).toBeCalled(); - - expect(screen.getByTestId(PAGE_NUMBER_TEST_ID).textContent).toBe('1 - 1 / 2'); - await userEvent.click(screen.getByTestId(LAST_BUTTON_ID)); - await userEvent.click(await screen.findByText('23.00')); - expect(handleClientTableItemClick).toBeCalled(); - - expect(screen.getByTestId(PAGE_NUMBER_TEST_ID).textContent).toBe('2 - 2 / 2'); - await userEvent.click(screen.getByTestId(FIRST_BUTTON_ID)); - await userEvent.click(await screen.findByText('1.00')); - expect(handleClientTableItemClick).toBeCalled(); - expect(screen.getByTestId(PAGE_NUMBER_TEST_ID).textContent).toBe('1 - 1 / 2'); });
25-74: Consider adding tests for edge cases.While the current test covers the basic functionality well, consider adding tests for:
- Empty data array behavior
- Testing with multiple pages (more than 2)
- Behavior when
onEntryClickis not providedExample for testing with empty data:
it('should handle empty data correctly', () => { render( <ClientTable columns={[{ field: 'f1', label: 'Field 1' }]} data={[]} entriesPerPage={1} minRows={3} /> ); expect(screen.getByTestId(PAGE_NUMBER_TEST_ID).textContent).toBe('0 - 0 / 0'); expect(screen.getByTestId(NEXT_BUTTON_ID)).toBeDisabled(); expect(screen.getByTestId(LAST_BUTTON_ID)).toBeDisabled(); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/ClientTable/ClientTable.spec.tsx(2 hunks)
🔇 Additional comments (2)
src/components/ClientTable/ClientTable.spec.tsx (2)
2-3: Good imports for the test requirements!Adding
userEventfor simulating user interactions andvifor creating mock functions is appropriate for the test scenario. This follows modern React testing best practices.
11-11: Good addition of the page number test ID constant.Defining the
PAGE_NUMBER_TEST_IDas a constant maintains consistency with the other test IDs in the file and makes the tests more maintainable.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/components/ClientTable/ClientTable.spec.tsx (2)
26-82: Comprehensive pagination and item click test, but could be more specific in assertions.This test effectively verifies both pagination navigation and item click functionality, which directly addresses the PR objectives. However, the assertions for the click handler could be more specific.
Consider using more specific assertions to verify the click handler is called with the expected arguments and the correct number of times:
- expect(handleClientTableItemClick).toBeCalled(); + expect(handleClientTableItemClick).toHaveBeenCalledTimes(1); + expect(handleClientTableItemClick).toHaveBeenCalledWith(expect.objectContaining({ f1: 23, f2: 24 }));Also, although you've included
handleClientTableDropdownClickand dropdown options, the test doesn't verify this functionality. Consider either:
- Adding assertions for the dropdown functionality, or
- Removing the unused mock if not testing this feature in this test case
26-82: Split the test into smaller, focused test cases for better maintainability.The current test covers multiple functionalities (pagination navigation and item clicks) in a single test case. While comprehensive, this approach can make it harder to isolate failures when they occur.
Consider splitting this into separate test cases, each focusing on a specific aspect of functionality:
it('should navigate through pages correctly', async () => { // Test only pagination navigation and page number display }); it('should call onEntryClick with correct data when entries are clicked', async () => { // Test only the item click functionality });This approach makes tests more maintainable and failures more immediately identifiable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/ClientTable/ClientTable.spec.tsx(3 hunks)
🔇 Additional comments (2)
src/components/ClientTable/ClientTable.spec.tsx (2)
2-3: Appropriate import additions for testing interactive behavior.The added imports for
userEventandviare necessary for the new interactive test. Good practice to keep imports organized and only include what's needed.
12-12: Good addition of a test ID constant.Adding the
PAGE_NUMBER_TEST_IDconstant follows the existing pattern in the file and promotes consistent test ID references throughout the test suite.
|
🎉 This PR is included in version 3.9.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
2 participants
tests paging function as well as onEntryClick functions
works on issue #49
Summary by CodeRabbit