Feature/test sortby component 1915 #1950
Conversation
Summary by CodeRabbit
WalkthroughA new unit test suite for the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~7 minutes Possibly related issues
Suggested reviewers
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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
|
![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 (3)
frontend/__tests__/unit/components/SortBy.test.tsx (3)
30-40: Reasonable approach for testing component library elements.The test correctly handles the limitation of testing dropdown options in component libraries. The use of
data-slot="value"selector suggests this is a NextUI Select component, and the approach is appropriate for such libraries.Consider verifying that all sort options are available by checking if they can be found when the dropdown is opened, if the component library supports it:
+ // Optionally test that options are available when dropdown opens + const select = screen.getByLabelText('Sort By :') + fireEvent.click(select) + expect(screen.getByText('Date')).toBeInTheDocument()
70-80: Consider more robust button selection.The test correctly verifies the order toggle functionality, but relying on button index (
buttons[1]) could be brittle if the component structure changes.Consider using a more specific selector for the sort order button:
- // Get the second button (the sort order button) - const buttons = screen.getAllByRole('button') - fireEvent.click(buttons[1]) // The sort order button is the second button + // Get the sort order button by its icon or aria-label + const sortOrderButton = screen.getByRole('button', { name: /sort order/i }) + // or find by the icon class if no aria-label exists + fireEvent.click(sortOrderButton)
82-94: Enhance accessibility testing specificity.The test covers basic accessibility verification but could be more comprehensive in testing specific accessibility attributes.
Consider testing additional accessibility attributes:
it('uses proper accessibility attributes', async () => { await act(async () => { render(<SortBy {...defaultProps} />) }) const select = screen.getByLabelText('Sort By :') expect(select.tagName).toBe('SELECT') + expect(select).toHaveAttribute('aria-label', 'Sort By :') - // Use getAllByText to handle multiple elements with same text - const containers = screen.getAllByText('Sort By :') - const container = containers[0].closest('div') - expect(container).toBeInTheDocument() + // Test for proper labeling relationship + const label = screen.getByText('Sort By :') + expect(label.tagName).toBe('LABEL') })
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/__tests__/unit/components/SortBy.test.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: Rajgupta36
PR: OWASP/Nest#1717
File: frontend/__tests__/unit/pages/createProgram.test.tsx:70-86
Timestamp: 2025-07-12T17:36:57.255Z
Learning: When testing React page components that use mocked form components, validation logic should be tested at the form component level, not the page level. Page-level tests should focus on authentication, role checking, submission handling, and navigation logic.
Learnt from: ahmedxgouda
PR: OWASP/Nest#1718
File: frontend/src/app/projects/dashboard/metrics/page.tsx:96-96
Timestamp: 2025-07-09T08:37:10.241Z
Learning: In the OWASP Nest project's MetricsPage component (frontend/src/app/projects/dashboard/metrics/page.tsx), the sorting dropdown intentionally uses selectionMode="multiple" to allow users to select multiple sorting criteria simultaneously. This enables complex sorting scenarios where users can sort by multiple fields in sequence.
Learnt from: codic-yeeshu
PR: OWASP/Nest#1444
File: frontend/src/components/NavDropDown.tsx:0-0
Timestamp: 2025-04-30T13:41:20.846Z
Learning: Use React's useId() hook rather than manually generating random strings when creating accessibility identifiers for UI components. This creates stable, unique IDs without causing hydration mismatches.
📚 Learning: when testing react page components that use mocked form components, validation logic should be teste...
Learnt from: Rajgupta36
PR: OWASP/Nest#1717
File: frontend/__tests__/unit/pages/createProgram.test.tsx:70-86
Timestamp: 2025-07-12T17:36:57.255Z
Learning: When testing React page components that use mocked form components, validation logic should be tested at the form component level, not the page level. Page-level tests should focus on authentication, role checking, submission handling, and navigation logic.
Applied to files:
frontend/__tests__/unit/components/SortBy.test.tsx
📚 Learning: in the owasp nest project's metricspage component (frontend/src/app/projects/dashboard/metrics/page....
Learnt from: ahmedxgouda
PR: OWASP/Nest#1718
File: frontend/src/app/projects/dashboard/metrics/page.tsx:96-96
Timestamp: 2025-07-09T08:37:10.241Z
Learning: In the OWASP Nest project's MetricsPage component (frontend/src/app/projects/dashboard/metrics/page.tsx), the sorting dropdown intentionally uses selectionMode="multiple" to allow users to select multiple sorting criteria simultaneously. This enables complex sorting scenarios where users can sort by multiple fields in sequence.
Applied to files:
frontend/__tests__/unit/components/SortBy.test.tsx
🔇 Additional comments (5)
frontend/__tests__/unit/components/SortBy.test.tsx (5)
1-3: LGTM!The imports are appropriate for React component testing with the correct utilities from React Testing Library and the
acthelper from React.
5-19: LGTM!The test setup is well-structured with comprehensive default props and proper mock cleanup. The use of
jest.clearAllMocks()inafterEachensures test isolation.
21-28: LGTM!The test properly verifies basic rendering and uses accessibility-friendly queries with
getByLabelText. The async/await pattern withact()is correctly implemented.
42-50: LGTM!The test properly simulates user interaction and verifies the callback behavior. The use of
act()around both rendering and the event ensures proper React state handling.
52-68: LGTM!Both icon tests properly verify conditional rendering based on the
selectedOrderprop. The use ofgetByRole('img', { hidden: true })is correct for Font Awesome icons, and testing the CSS classes is an appropriate approach for verifying icon states.
|
kasya
left a comment
kasya
left a comment
There was a problem hiding this comment.
These are good! Thank you @Dhirajsharma2060!
* test: add unit tests for SortBy component * test: update SortBy component tests for prop naming and accessibility * test: import React in SortBy component tests for consistency * made some chnages * test: update SortBy component tests for improved accessibility and clarity * test: ensure order toggle button functionality in SortBy component tests --------- Co-authored-by: Kate Golovanova <[email protected]>
3 participants
Created tests for task issue #1915
This PR introduces a complete unit test suite for the
<SortBy />component to ensure it behaves correctly under various scenarios. The tests focus on rendering validation, prop-driven behavior, user interactions, accessibility, and UI logic.Summary of Changes
selectedOrderselectedSortOptiononSortChange)onOrderChange)label,select,img) are correctly appliedafterEachwithjest.clearAllMocksChecklist
renders successfully with minimal required propsrenders ascending/descending icon based on selectedOrderrenders all options and selects the correct one, other prop-driven testscalls onSortChange,toggles order when the button is clickeduses proper accessibility attributesfa-arrow-up-wide-short) testedDeveloper Notes
make check-testlocally; all checks and tests passed.AutoScrollToTop.test.tsxfor consistency@testing-library/reactandact()for rendering and async interactionscomponents/__tests__folder