Add Pagination Unit Tests & Accessibility Tweaks (Closes #1864)#1879
Add Pagination Unit Tests & Accessibility Tweaks (Closes #1864)#1879suryansh-it wants to merge 3 commits intoOWASP:mainfrom
Conversation
Summary by CodeRabbit
WalkthroughA new unit test suite for the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Assessment against linked 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 ignored due to path filters (139)
📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
✨ 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 (1)
frontend/__tests__/unit/components/Pagination.test.tsx (1)
88-89: Simplify ellipsis detection using data-testid!Since the component now includes
data-testid="ellipsis"on ellipsis spans, you can simplify the detection:- const ellipses = screen.getAllByText((_, el) => el?.tagName === 'SPAN' && !el.textContent?.trim()) - expect(ellipses.length).toBeGreaterThanOrEqual(2); + const ellipses = screen.getAllByTestId('ellipsis') + expect(ellipses).toHaveLength(2);This approach is more reliable and takes advantage of the testability improvements made to the component.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/__tests__/unit/components/Pagination.test.tsx(1 hunks)frontend/src/components/Pagination.tsx(3 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: Rajgupta36
PR: OWASP/Nest#1717
File: frontend/__tests__/unit/pages/ProgramDetails.test.tsx:35-0
Timestamp: 2025-07-12T17:12:25.807Z
Learning: In React applications, button elements should always have an explicit type attribute (type="button", type="submit", or type="reset") to prevent unintended form submission behavior, even when the button appears to be outside of a form context. The default type is "submit" which can cause unexpected behavior.
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.
frontend/src/components/Pagination.tsx (1)
Learnt from: Rajgupta36
PR: #1717
File: frontend/tests/unit/pages/ProgramDetails.test.tsx:35-0
Timestamp: 2025-07-12T17:12:25.807Z
Learning: In React applications, button elements should always have an explicit type attribute (type="button", type="submit", or type="reset") to prevent unintended form submission behavior, even when the button appears to be outside of a form context. The default type is "submit" which can cause unexpected behavior.
frontend/__tests__/unit/components/Pagination.test.tsx (1)
Learnt from: Rajgupta36
PR: #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.
🔇 Additional comments (8)
frontend/src/components/Pagination.tsx (3)
64-64: Excellent addition of explicit button types!Adding
type="button"to all Button components prevents unintended form submission behavior and aligns with React Testing Library'sgetByRole('button')queries. This follows established best practices for button elements.Also applies to: 82-82, 97-97
83-83: Great accessibility enhancement for current page indication!The conditional
aria-current="page"attribute properly indicates the active page to screen readers, improving accessibility. Setting it toundefinedwhen not current correctly removes the attribute.
74-77: Proper semantic markup for ellipsis elements!Adding
role="presentation"correctly marks the ellipsis spans as decorative elements, whiledata-testid="ellipsis"provides stable test hooks. This enhances both accessibility and testability.frontend/__tests__/unit/components/Pagination.test.tsx (5)
1-26: Well-structured test setup with proper cleanup!The test setup follows best practices with comprehensive imports, proper DOM cleanup using
afterEach(cleanup), and a flexiblerenderComponenthelper function. The mock clearing inbeforeEachensures test isolation.
28-36: Comprehensive coverage of conditional rendering!These tests properly verify the component's early return conditions when
isLoadedis false ortotalPagesis 1 or less. Usingcontainer.firstChildto check for null rendering is the correct approach.
38-77: Excellent coverage of core pagination functionality!These tests thoroughly verify button rendering, proper enabling/disabling of navigation buttons at boundaries, and correct callback invocation with expected page numbers. The use of
within(container)for scoped queries is a good practice.
100-108: Excellent accessibility testing!This test properly validates both the visual styling (
bg-[#83a6cc]class) and accessibility (aria-current="page"attribute) of the active page button, ensuring the component enhancements work as expected.
110-133: Comprehensive edge case coverage!These tests effectively cover important edge cases including default prop values, current page positioning in large sets, and minimal page counts. The regex pattern for exact button matching is well-implemented.
kasya
left a comment
kasya
left a comment
There was a problem hiding this comment.
Thanks for working on this.
Added a comment in the code. Also, please run make check locally and push updated files - this is required before opening any PRs in OWASP Nest.
There was a problem hiding this comment.
Actual components should not have a data-testid set on the elements. Please check other test files to see how to address this.
|
@kasya Thanks for the feedback and Guidance! I’ll remove the "data-testid" from the component, add an appropriate "aria-label" to the ellipsis "span", update the test to use visible text for ellipses, and run "make check" locally before pushing the changes. |
|
|
✅ Changes made as requested:
Let me know if any further updates are needed! |
arkid15r
left a comment
arkid15r
left a comment
There was a problem hiding this comment.
Please remove the unrelated files from the PR
@arkid15r thanks for catching that! I’ll prune the PR to only include the Pagination component and its tests, run make check, and push an updated commit soon. |
suryansh-it
changed the base branch from
main
to
dependabot/npm_and_yarn/frontend/main/heroui/react-2.8.2
suryansh-it
changed the base branch from
dependabot/npm_and_yarn/frontend/main/heroui/react-2.8.2
to
main
3 participants
Proposed change
Resolves #1864
This PR adds comprehensive unit tests for the
<Pagination>component and applies three minimal, non‑functional tweaks to improve accessibility and test reliability:type="button"on all<Button>elementsgetByRole('button')queriesaria-current="page"on the active page buttondata-testid="ellipsis"androle="presentation"on each ellipsis<span>getAllByTestId('ellipsis')Additionally, this PR introduces
__tests__/unit/components/Pagination.test.tsxwith 11 test cases covering:isLoaded=false,totalPages<=1)onPageChange)aria-current)totalPages)Tests use
afterEach(cleanup)andwithin(container)to scope queries per render.Checklist
make check-testlocally; all checks and tests passed.