Remove data-testid from production components#3341
Conversation
Summary by CodeRabbitRelease Notes
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughReplaced production Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (1)📚 Learning: 2025-06-21T12:21:32.372ZApplied to files:
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @frontend/src/components/ContributionStats.tsx:
- Line 15: The region landmark in the ContributionStats component (the <div
role="region">) lacks an accessible name; add either an aria-label (e.g.,
aria-label="Contribution statistics") or aria-labelledby that points to a nearby
heading ID to provide an accessible name for screen readers, and remove the
empty className="" attribute for cleanliness; ensure any referenced ID used by
aria-labelledby exists and is unique within the component (e.g., the heading
element inside ContributionStats).
🧹 Nitpick comments (6)
frontend/__tests__/unit/components/Search.test.tsx (1)
375-379: Good use of role-based query.Using
getByRole('textbox')aligns with Testing Library's recommended query priority and the PR's goal of eliminatingdata-testidselectors.Minor observations:
- This test appears to overlap with existing tests like "renders input with placeholder" (line 40) and "shows input when isLoaded is true" (line 64). Consider whether this test adds distinct value or if it was simply converted from a prior
data-testidassertion.- The
isLoadedshorthand is inconsistent with explicitisLoaded={true}used elsewhere in this file—either style works, but consistency aids readability.frontend/src/app/members/[memberKey]/page.tsx (1)
84-90: Good accessibility improvement usingrole="status".The
role="status"is a valid ARIA live region that allows assistive technologies to announce the loading state. This is a sound replacement for the test-onlydata-testidattribute.Consider adding an
aria-labelfor additional context:💡 Optional enhancement
- <div role="status"> + <div role="status" aria-label="Loading user details"> <MemberDetailsPageSkeleton /> </div>frontend/__tests__/unit/pages/OrganizationDetails.test.tsx (1)
56-59: LGTM! Test correctly migrated to semantic role query.The test now uses
getByRole('status')matching the production component'srole="status"attribute.Minor nit: The comment mentions "CSS selectors" but the previous approach used
data-testid. Consider updating the comment for accuracy:📝 Suggested comment update
- // Use semantic role query instead of CSS selectors for better stability + // Use semantic role query instead of data-testid for better accessibility testingfrontend/__tests__/unit/components/ToggleableList.test.tsx (1)
13-20: Consider using consistent selector strategy within the test file.The mock retains
data-testid="show-more-button"while some tests use role-based queries (getByRole) and others usequeryByTestId. For consistency, consider updating the negative assertions (lines 74, 122, 129, 137) to also use role-based queries:- expect(screen.queryByTestId('show-more-button')).not.toBeInTheDocument() + expect(screen.queryByRole('button', { name: /show more/i })).not.toBeInTheDocument()Since the mock is test-only code, keeping
data-testidthere is acceptable per PR objectives, but aligning all queries to role-based selectors improves test clarity.frontend/__tests__/unit/components/RepositoryCard.test.tsx (1)
22-25: Mock retains unuseddata-testidattribute.The mock
ShowMoreButtonstill hasdata-testid="show-more-button"but tests now use role-based selectors. While this is acceptable per the PR requirements (data-testid allowed in test mocks), you could remove it for consistency since it's no longer used.♻️ Optional cleanup
return ( - <button type="button" onClick={handleToggle} data-testid="show-more-button"> + <button type="button" onClick={handleToggle}> {isExpanded ? 'Show less' : 'Show more'} </button> )frontend/__tests__/unit/components/ContributionStats.test.tsx (1)
60-66: Minor redundancy in assertions.Line 65's assertion
expect(screen.getByRole('region')).toBeInTheDocument()is redundant sincegetByRoleon line 60 already throws if the element doesn't exist. The test would already fail if the region wasn't present.♻️ Optional cleanup
const container = screen.getByRole('region') const icons = container.querySelectorAll('svg') expect(icons).toHaveLength(5) // Title icon + 4 stat icons - // Verify specific icon data attributes - expect(screen.getByRole('region')).toBeInTheDocument() expect(screen.getByText('Test Contribution Activity')).toBeInTheDocument()
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
frontend/__tests__/a11y/components/EntityActions.a11y.test.tsxfrontend/__tests__/a11y/components/Footer.a11y.test.tsxfrontend/__tests__/a11y/components/RepositoryCard.a11y.test.tsxfrontend/__tests__/unit/components/ContributionStats.test.tsxfrontend/__tests__/unit/components/EntityActions.test.tsxfrontend/__tests__/unit/components/ProgramCard.test.tsxfrontend/__tests__/unit/components/RepositoryCard.test.tsxfrontend/__tests__/unit/components/Search.test.tsxfrontend/__tests__/unit/components/ToggleableList.test.tsxfrontend/__tests__/unit/pages/OrganizationDetails.test.tsxfrontend/__tests__/unit/pages/ProgramDetailsMentorship.test.tsxfrontend/__tests__/unit/pages/UserDetails.test.tsxfrontend/src/app/members/[memberKey]/page.tsxfrontend/src/app/organizations/[organizationKey]/page.tsxfrontend/src/components/Badges.tsxfrontend/src/components/ContributionStats.tsxfrontend/src/components/EntityActions.tsxfrontend/src/components/Footer.tsxfrontend/src/components/Search.tsxfrontend/src/components/ShowMoreButton.tsx
💤 Files with no reviewable changes (4)
- frontend/src/components/ShowMoreButton.tsx
- frontend/src/components/EntityActions.tsx
- frontend/src/components/Search.tsx
- frontend/src/components/Footer.tsx
🧰 Additional context used
🧠 Learnings (10)
📚 Learning: 2025-04-30T13:41:20.846Z
Learnt from: codic-yeeshu
Repo: OWASP/Nest PR: 1444
File: frontend/src/components/NavDropDown.tsx:0-0
Timestamp: 2025-04-30T13:41:20.846Z
Learning: When implementing dropdown menus or similar interactive components, always include proper accessibility features: ARIA attributes (aria-expanded, aria-haspopup, aria-controls), keyboard navigation support (Enter, Space, Escape keys), and mechanisms to close dropdowns when clicking outside.
Applied to files:
frontend/__tests__/unit/components/EntityActions.test.tsx
📚 Learning: 2025-08-31T13:47:15.861Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2155
File: frontend/src/server/queries/programsQueries.ts:81-81
Timestamp: 2025-08-31T13:47:15.861Z
Learning: In frontend/src/server/queries/programsQueries.ts, GET_PROGRAM_DETAILS is actively used in frontend/src/app/my/mentorship/programs/[programKey]/edit/page.tsx for program editing functionality and cannot be removed. It serves a different purpose than GET_PROGRAM_ADMIN_DETAILS, providing comprehensive program information needed for editing.
Applied to files:
frontend/__tests__/unit/components/EntityActions.test.tsxfrontend/__tests__/unit/components/ProgramCard.test.tsxfrontend/__tests__/unit/pages/ProgramDetailsMentorship.test.tsx
📚 Learning: 2025-11-17T16:47:05.578Z
Learnt from: anurag2787
Repo: OWASP/Nest PR: 2671
File: frontend/__tests__/unit/components/MultiSearch.test.tsx:427-427
Timestamp: 2025-11-17T16:47:05.578Z
Learning: In the frontend test files for the OWASP/Nest repository, `expect(true).toBe(true)` no-op assertions may be intentionally added as workarounds when ESLint rule jest/expect-expect doesn't detect expectations inside helper functions or waitFor callbacks. These can be resolved by configuring the ESLint rule's assertFunctionNames option to recognize custom assertion helpers instead of flagging them as redundant.
Applied to files:
frontend/__tests__/unit/components/RepositoryCard.test.tsxfrontend/__tests__/unit/pages/OrganizationDetails.test.tsxfrontend/__tests__/unit/components/ProgramCard.test.tsxfrontend/__tests__/unit/pages/ProgramDetailsMentorship.test.tsxfrontend/__tests__/unit/pages/UserDetails.test.tsx
📚 Learning: 2025-11-17T17:30:42.139Z
Learnt from: anurag2787
Repo: OWASP/Nest PR: 2671
File: frontend/__tests__/unit/components/MultiSearch.test.tsx:169-171
Timestamp: 2025-11-17T17:30:42.139Z
Learning: In the OWASP/Nest frontend tests (PR #2671 context), wrapper functions like `expectChaptersCountEqualsThree` that simply call another helper with a fixed parameter (e.g., `expectChaptersCountEquals(3)`) are intentionally used to avoid arrow function callbacks in `waitFor` calls. This pattern prevents adding nesting depth that would trigger rule typescript:S2004. Example: `await waitFor(expectChaptersCountEqualsThree)` avoids the extra nesting from `await waitFor(() => expectChaptersCountEquals(3))`.
Applied to files:
frontend/__tests__/unit/components/RepositoryCard.test.tsxfrontend/__tests__/unit/pages/OrganizationDetails.test.tsxfrontend/__tests__/unit/pages/ProgramDetailsMentorship.test.tsx
📚 Learning: 2025-07-12T17:36:57.255Z
Learnt from: Rajgupta36
Repo: OWASP/Nest 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.
Applied to files:
frontend/__tests__/unit/pages/OrganizationDetails.test.tsxfrontend/__tests__/unit/pages/ProgramDetailsMentorship.test.tsxfrontend/__tests__/unit/pages/UserDetails.test.tsx
📚 Learning: 2025-06-21T12:21:32.372Z
Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 1633
File: frontend/src/components/GradientRadialChart.tsx:67-116
Timestamp: 2025-06-21T12:21:32.372Z
Learning: The react-apexcharts Chart component does not support ARIA attributes like aria-label and role as direct props. To add accessibility attributes to ApexCharts in React, wrap the Chart component in a container div with the appropriate ARIA attributes.
Applied to files:
frontend/src/components/ContributionStats.tsx
📚 Learning: 2025-09-17T02:42:41.928Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 2288
File: frontend/src/components/ActionButton.tsx:0-0
Timestamp: 2025-09-17T02:42:41.928Z
Learning: In frontend/src/components/ActionButton.tsx, the user Rajgupta36 intentionally changed text-blue-600 to text-[#1D7BD7] to align the text color with the border color (#1D7BD7) for visual consistency, prioritizing design alignment over theme tokens.
Applied to files:
frontend/src/components/Badges.tsxfrontend/__tests__/unit/components/ProgramCard.test.tsxfrontend/__tests__/a11y/components/EntityActions.a11y.test.tsx
📚 Learning: 2025-08-31T13:47:15.861Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2155
File: frontend/src/server/queries/programsQueries.ts:81-81
Timestamp: 2025-08-31T13:47:15.861Z
Learning: In frontend/src/server/queries/programsQueries.ts, GET_PROGRAM_DETAILS and GET_PROGRAM_ADMIN_DETAILS are two separate queries serving different purposes: GET_PROGRAM_DETAILS fetches comprehensive program information while GET_PROGRAM_ADMIN_DETAILS fetches only admin-related details. These queries cannot be removed or merged as they serve different use cases in the application.
Applied to files:
frontend/__tests__/unit/components/ProgramCard.test.tsxfrontend/__tests__/unit/pages/ProgramDetailsMentorship.test.tsx
📚 Learning: 2025-07-12T17:14:28.536Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/src/app/mentorship/programs/[programKey]/edit/page.tsx:90-128
Timestamp: 2025-07-12T17:14:28.536Z
Learning: Both ProgramForm (programCard.tsx) and ModuleForm (mainmoduleCard.tsx) components already implement HTML validation using the `required` attribute on form fields. The browser's native validation prevents form submission and displays error messages when required fields are empty, eliminating the need for additional JavaScript validation before GraphQL mutations.
Applied to files:
frontend/__tests__/unit/components/ProgramCard.test.tsx
📚 Learning: 2025-08-31T13:47:15.861Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2155
File: frontend/src/server/queries/programsQueries.ts:81-81
Timestamp: 2025-08-31T13:47:15.861Z
Learning: In frontend/src/server/queries/programsQueries.ts, GET_PROGRAM_DETAILS and GET_PROGRAM_ADMIN_DETAILS are two separate queries serving different purposes: GET_PROGRAM_DETAILS fetches comprehensive program information while GET_PROGRAM_ADMIN_DETAILS fetches only admin-related details.
Applied to files:
frontend/__tests__/unit/components/ProgramCard.test.tsxfrontend/__tests__/unit/pages/ProgramDetailsMentorship.test.tsx
🧬 Code graph analysis (1)
frontend/src/components/Badges.tsx (1)
frontend/src/wrappers/IconWrapper.tsx (1)
IconWrapper(13-13)
🔇 Additional comments (21)
frontend/__tests__/unit/pages/UserDetails.test.tsx (2)
107-110: LGTM! Correctly migrated to semantic role-based query.The test now uses
getByRole('status')to match the production component'srole="status"attribute, aligning with accessibility-first testing practices.
17-40: Mock component appropriately retainsdata-testidfor test isolation.Per the PR objective,
data-testidattributes are allowed in test mocks. TheMockBadgescomponent correctly uses test IDs to verify badge rendering behavior without affecting production code.frontend/src/app/organizations/[organizationKey]/page.tsx (1)
30-36: LGTM! Consistent accessibility pattern with the members page.The
role="status"wrapper follows the same pattern established in the members page, maintaining consistency across the codebase. The corresponding test correctly queries by this role.frontend/src/components/Badges.tsx (1)
31-33: LGTM!data-testidcorrectly removed from production component.The
IconWrapperno longer receives a test-only attribute. Tests correctly use the mockedIconWrappercomponent which providesdata-testid="badge-icon"for test queries, ensuring production code remains clean while maintaining testability.frontend/__tests__/a11y/components/EntityActions.a11y.test.tsx (1)
37-38: LGTM!The migration from
getByTestIdtogetByRole('button', { name: /Program actions menu/ })aligns with accessibility-first testing practices and ensures the component has proper ARIA labeling for screen readers.frontend/__tests__/a11y/components/Footer.a11y.test.tsx (1)
19-20: LGTM!Role-based selector for the Resources button is appropriate and maintains the test's accessibility focus.
frontend/__tests__/unit/components/ToggleableList.test.tsx (3)
80-81: LGTM!Role-based query correctly locates the Show More button using accessible name matching.
91-109: LGTM!Expand/collapse tests properly use role-based selectors for button interactions.
143-143: LGTM!Role-based query for edge case with
limit={0}is correct.frontend/__tests__/unit/components/ProgramCard.test.tsx (3)
150-164: LGTM!The migration to role-based selector for the actions button maintains test intent while aligning with accessibility-first practices. The test flow (click actions → click Edit → verify navigation) remains clear.
419-420: LGTM!Role-based query correctly verifies the actions button renders for admin menu.
434-434: LGTM!Consistent use of role-based selector in edge case testing.
frontend/__tests__/unit/components/EntityActions.test.tsx (4)
23-24: LGTM!Role-based selectors properly distinguish between "Program actions menu" and "Module actions menu" buttons based on the
typeprop.
48-49: LGTM!Correct use of
/Module actions menu/pattern for module-type entity actions.
363-386: LGTM!Click outside behavior tests properly use role-based selectors while maintaining thorough coverage of dropdown close/open states via
aria-expandedassertions.
455-486: LGTM!Event propagation tests correctly verify that clicks on the actions button and menu items don't bubble to parent elements. The role-based selector migration preserves this important behavioral test coverage.
frontend/__tests__/unit/pages/ProgramDetailsMentorship.test.tsx (2)
96-96: LGTM!Correctly uses
queryByRolefor the negative assertion, which is the appropriate method when verifying an element is not present.
132-133: LGTM!The switch from
findByTestIdtofindByRolewith an accessible name aligns with the PR's accessibility-first testing approach. UsingfindByRoleis appropriate here since it's an async operation before interacting with the button.frontend/__tests__/unit/components/RepositoryCard.test.tsx (1)
78-78: LGTM!The role-based queries using
{ name: /Show/ }correctly match both "Show more" and "Show less" button states. The use ofqueryByRolefor negative assertions andgetByRolefor positive assertions is appropriate.Also applies to: 107-107, 115-115, 128-128, 135-135, 319-319, 323-323
frontend/__tests__/unit/components/ContributionStats.test.tsx (1)
232-233: LGTM!The consistent use of
getByRole('region')across accessibility and visual element tests aligns with the component's updated semantic structure.Also applies to: 296-297, 310-311
frontend/__tests__/a11y/components/RepositoryCard.a11y.test.tsx (1)
48-49: LGTM!The role-based query with case-insensitive matching
/show more/iis appropriate for a11y tests running against the real component. TheShowMoreButtoncomponent renders the text "Show more" in its initial state, which matches the selector pattern.
|
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/src/components/ContributionStats.tsx (1)
15-19: Missingidattribute referenced byaria-labelledby.The
aria-labelledby="contribution-stats-title"references an ID that doesn't exist. The<h2>element needsid="contribution-stats-title"for the association to work; otherwise, assistive technologies cannot announce the section's accessible name.🛠️ Proposed fix
<section aria-labelledby="contribution-stats-title"> - <h2 className="mb-4 flex items-center gap-2 text-lg font-semibold text-gray-800 sm:text-xl md:text-2xl dark:text-gray-200"> + <h2 id="contribution-stats-title" className="mb-4 flex items-center gap-2 text-lg font-semibold text-gray-800 sm:text-xl md:text-2xl dark:text-gray-200"> <FaChartLine className="h-5 w-5 text-gray-600 sm:h-6 sm:w-6 dark:text-gray-400" /> {title} </h2>If multiple
ContributionStatscomponents are rendered on the same page, the hardcodedidwill cause duplicate IDs in the DOM. Consider whether a unique ID (e.g., viauseId()) is needed:#!/bin/bash # Check how ContributionStats is used across the codebase to assess duplicate ID risk rg -n "ContributionStats" --type=tsx --type=ts -g '!*.test.*' -g '!*__tests__*'
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/src/components/ContributionStats.tsx
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-06-21T12:21:32.372Z
Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 1633
File: frontend/src/components/GradientRadialChart.tsx:67-116
Timestamp: 2025-06-21T12:21:32.372Z
Learning: The react-apexcharts Chart component does not support ARIA attributes like aria-label and role as direct props. To add accessibility attributes to ApexCharts in React, wrap the Chart component in a container div with the appropriate ARIA attributes.
Applied to files:
frontend/src/components/ContributionStats.tsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Run backend tests
- GitHub Check: Run frontend unit tests
- GitHub Check: Run frontend e2e tests
- GitHub Check: Run frontend accessibility tests
2 participants
Proposed change
This PR removes data-testid attributes from production components and pages to ensure test-only selectors are not shipped in the UI. Any affected tests were updated with no functional changes to the application.
Resolves #3325
Checklist
make check-testlocally: all warnings addressed, tests passed