test: add unit tests for GeneralCompliantComponent #2018
Conversation
Summary by CodeRabbit
WalkthroughAdds a new Jest/React Testing Library unit test suite for GeneralCompliantComponent covering rendering, conditional styling, tooltip/title handling, accessibility, SVG structure, custom icon prop, and an empty title edge case. Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Assessment against linked issues
Suggested reviewers
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: .coderabbit.yaml 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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). (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. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, 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: 4
🧹 Nitpick comments (7)
frontend/package.json (1)
73-73: Avoid the extra dependency by re-exported utilitiesYou don't need a direct dependency on @testing-library/dom if you import screen from @testing-library/react. It re-exports DOM utilities. Simplify the tree and avoid version skew by removing this dep once imports are updated in the test.
- "@testing-library/dom": "^10.4.1",frontend/__tests__/unit/components/GeneralCompliantComponent.test.tsx (6)
9-13: Tighten test prop types; avoid anyPrefer the concrete icon type to catch mismatches at compile time.
+import type { IconDefinition } from '@fortawesome/fontawesome-svg-core'; @@ type GeneralCompliantComponentProps = { compliant: boolean; - icon: any; + icon: IconDefinition; title: string; };
39-42: Minor duplication with the first render testThis largely overlaps “renders successfully with minimal required props.” Consider merging or making the assertion more specific (e.g., verify structure/order around icon + title).
44-48: Tooltip assertion is brittle without interactionAsserting raw text risks false positives if the title is also visible inline. Prefer an accessible query (e.g., role="tooltip") and simulate hover if the tooltip is conditional.
Example approach:
- If using a real tooltip: use user-event to hover the trigger, then assert by role.
- If using title/aria-label: assert via getByTitle or toHaveAttribute.
-import { faCertificate } from '@fortawesome/free-solid-svg-icons'; +import { faCertificate } from '@fortawesome/free-solid-svg-icons'; +import userEvent from '@testing-library/user-event'; @@ - it('renders tooltip with the title', () => { - const { getByText } = render(<GeneralCompliantComponent {...baseProps} title="Tooltip Title" />); - // Tooltip content is rendered, but may require hover simulation in real DOM - expect(getByText('Tooltip Title')).toBeInTheDocument(); - }); + it('shows tooltip content on hover', async () => { + render(<GeneralCompliantComponent {...baseProps} title="Tooltip Title" />); + const trigger = screen.getByText('Tooltip Title'); // Or a more specific trigger + await userEvent.hover(trigger); + expect(screen.getByRole('tooltip')).toHaveTextContent('Tooltip Title'); + });If the component does not render a role="tooltip", adapt to its actual a11y contract (e.g., getByTitle).
50-53: Empty title test is too weakAsserting that container exists doesn't validate behavior. Check for expected fallback/absence instead (e.g., no visible title, aria-label fallback, or no crash with console error suppression if applicable).
64-68: Custom icon test should verify the override, not just presence of any SVGUse a different icon and assert by a stable attribute (FontAwesome sets data-icon). This ensures the passed icon is actually used.
-import { faCertificate } from '@fortawesome/free-solid-svg-icons'; +import { faCertificate, faXmark } from '@fortawesome/free-solid-svg-icons'; @@ - it('renders with custom icon', () => { - const customIcon = faCertificate; // Replace with another icon if needed - const { container } = render(<GeneralCompliantComponent {...baseProps} icon={customIcon} />); - expect(container.querySelector('svg')).toBeInTheDocument(); - }); + it('renders with custom icon', () => { + const customIcon = faXmark; + const { container } = render(<GeneralCompliantComponent {...baseProps} icon={customIcon} />); + expect(container.querySelector('svg[data-icon="xmark"]')).toBeInTheDocument(); + });If not using FontAwesome’s IconDefinition, assert with whatever stable selector the component renders.
4-4: Optional: move jest-dom setup to a global test setupImporting jest-dom per test file is repetitive. Consider moving to a setup file (e.g., setupTests.ts) via Jest’s setupFilesAfterEnv.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/__tests__/unit/components/GeneralCompliantComponent.test.tsx(1 hunks)frontend/package.json(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 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#1714
File: frontend/src/components/ProjectTypeDashboardCard.tsx:8-12
Timestamp: 2025-07-08T17:07:50.988Z
Learning: In the OWASP/Nest project, union types for component props are not necessary when they would require creating separate type definitions. The project prefers inline prop type definitions even for props with specific string values, maintaining consistency with the single-use component prop pattern.
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: 2025-07-12T17:36:57.255Z
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/GeneralCompliantComponent.test.tsx
frontend/__tests__/unit/components/GeneralCompliantComponent.test.tsx
Outdated
Show resolved
Hide resolved
frontend/__tests__/unit/components/GeneralCompliantComponent.test.tsx
Outdated
Show resolved
Hide resolved
frontend/__tests__/unit/components/GeneralCompliantComponent.test.tsx
Outdated
Show resolved
Hide resolved
frontend/__tests__/unit/components/GeneralCompliantComponent.test.tsx
Outdated
Show resolved
Hide resolved
|
Hi @arkid15r, I’ve finished the changes for this PR and all available checks have passed. Please review when you have time. |
|
@addresskrish Do you plan to address requested changes? |
|
Hi @kasya , yes, I'm working on the requested changes and will update the PR soon. |
| // Add more tests as needed for event handling, state, etc. | ||
| }); | ||
|
|
||
| // ...existing code... |
There was a problem hiding this comment.
@addresskrish We totally support using AI for tests generation, but at least check what it generates and clean the code up before committing changes.
Also the tests fail right now in many cases. Please run make test-frontend-unit locally and address all issues
| render(<GeneralCompliantComponent {...baseProps} />); | ||
| const iconElement = screen.getByText(baseProps.title); // Asserts the title text is visible | ||
| expect(iconElement).toBeInTheDocument(); | ||
| // Or, if the icon has a specific role, you can check for that |
There was a problem hiding this comment.
This also should be removed
frontend/package.json
Outdated
There was a problem hiding this comment.
Revert this too - we don't need this new package
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (10)
frontend/__tests__/unit/components/GeneralCompliantComponent.test.tsx (10)
8-15: Tooltip mock is fine; guard non-string content and slightly widen the typeIf the component ever passes a non-string (ReactNode) tooltip content, setting it directly as a title attribute would be unsafe. A tiny guard avoids brittle failures later.
jest.mock('@heroui/tooltip', () => ({ - Tooltip: ({ children, content }: { children: React.ReactNode; content: string }) => ( - <div data-testid="tooltip" title={content}> + Tooltip: ({ children, content }: { children: React.ReactNode; content?: React.ReactNode }) => ( + <div data-testid="tooltip" title={typeof content === 'string' ? content : undefined}> {children} </div> ), }))
19-23: Avoid duplicating the component’s prop type in testsDerive the props type directly from the component to prevent drift if props change. This also makes the import of IconProp unnecessary.
-type GeneralCompliantComponentProps = { - compliant: boolean - icon: IconProp - title: string -} +type GeneralCompliantComponentProps = React.ComponentProps<typeof GeneralCompliantComponent>Additionally, drop the now-unused IconProp import (outside this range):
- import { IconProp } from '@fortawesome/fontawesome-svg-core'
32-35: Strengthen the “renders successfully” assertionAsserting container presence is very weak and will always pass after render. Prefer asserting something the component actually renders.
- const { container } = render(<GeneralCompliantComponent {...baseProps} />) - expect(container).toBeInTheDocument() + render(<GeneralCompliantComponent {...baseProps} />) + // At least one icon should be in the tree + expect(screen.getAllByRole('img', { hidden: true }).length).toBeGreaterThanOrEqual(1)
37-42: Make the compliant=true color assertion robust (don’t assume the first SVG is the colored one)If the component renders multiple SVGs, the class might not be on the first one. Search among all SVGs and also assert the opposing color is absent.
- const { container } = render(<GeneralCompliantComponent {...baseProps} compliant={true} />) - const svg = container.querySelector('svg') - expect(svg).toBeInTheDocument() - expect(svg).toHaveClass('text-green-400/80') + const { container } = render(<GeneralCompliantComponent {...baseProps} compliant />) + const svgs = Array.from(container.querySelectorAll('svg')) + const green = svgs.find((el) => el.classList.contains('text-green-400/80')) + expect(green).toBeTruthy() + expect(svgs.some((el) => el.classList.contains('text-red-400/80'))).toBe(false)
44-49: Mirror the robustness for compliant=falseSame reasoning as the green case; avoid assuming a specific SVG index.
- const { container } = render(<GeneralCompliantComponent {...baseProps} compliant={false} />) - const svg = container.querySelector('svg') - expect(svg).toBeInTheDocument() - expect(svg).toHaveClass('text-red-400/80') + const { container } = render(<GeneralCompliantComponent {...baseProps} compliant={false} />) + const svgs = Array.from(container.querySelectorAll('svg')) + const red = svgs.find((el) => el.classList.contains('text-red-400/80')) + expect(red).toBeTruthy() + expect(svgs.some((el) => el.classList.contains('text-green-400/80'))).toBe(false)
51-55: Brittle: asserting exactly two SVGs couples tests to implementation detailsFontAwesome internals or the component’s structure may change. Prefer asserting there’s at least one SVG and that the intended icon is used (via data-icon), which is both stable and meaningful.
- const { container } = render(<GeneralCompliantComponent {...baseProps} />) - const svgs = container.querySelectorAll('svg') - expect(svgs).toHaveLength(2) + const { container } = render(<GeneralCompliantComponent {...baseProps} />) + const svgs = container.querySelectorAll('svg') + expect(svgs.length).toBeGreaterThanOrEqual(1) + // FontAwesome sets data-icon="<icon-name>" + expect(Array.from(svgs).some((el) => el.getAttribute('data-icon') === 'certificate')).toBe(true)
57-62: Assert the tooltip actually wraps the iconSince the mock renders a wrapper div with data-testid="tooltip", also verify the icon is inside it.
render(<GeneralCompliantComponent {...baseProps} title="Tooltip Title" />) const tooltip = screen.getByTestId('tooltip') expect(tooltip).toBeInTheDocument() expect(tooltip).toHaveAttribute('title', 'Tooltip Title') + expect(tooltip.querySelector('svg')).toBeInTheDocument()If you prefer RTL queries, you can use within (requires importing
withinfrom RTL):- import { render, screen } from '@testing-library/react' + import { render, screen, within } from '@testing-library/react'expect(within(tooltip).getByRole('img', { hidden: true })).toBeInTheDocument()
69-75: Rename and use a11y-first queries for decorative iconsThese look decorative (aria-hidden="true"), so they are not “accessible”. Use getAllByRole with {hidden: true} and assert aria-hidden.
- it('has accessible SVG icons', () => { - const { container } = render(<GeneralCompliantComponent {...baseProps} />) - const svgs = container.querySelectorAll('svg[role="img"]') - expect(svgs).toHaveLength(2) - expect(svgs[0]).toHaveAttribute('aria-hidden', 'true') - expect(svgs[1]).toHaveAttribute('aria-hidden', 'true') - }) + it('renders decorative SVG icons (aria-hidden)', () => { + render(<GeneralCompliantComponent {...baseProps} />) + const svgs = screen.getAllByRole('img', { hidden: true }) + expect(svgs.length).toBeGreaterThanOrEqual(1) + expect(svgs.every((el) => el.getAttribute('aria-hidden') === 'true')).toBe(true) + })
77-81: Actually verify that a custom icon is usedUsing the same icon as baseProps doesn’t prove the prop is honored. Use a different icon and assert the underlying SVG data attribute changes accordingly.
- const customIcon = faCertificate + const customIcon = faCheck const { container } = render(<GeneralCompliantComponent {...baseProps} icon={customIcon} />) - expect(container.querySelector('svg')).toBeInTheDocument() + const svg = container.querySelector('svg') + expect(svg).toBeInTheDocument() + expect(svg).toHaveAttribute('data-icon', 'check')Add the import (outside this range):
- import { faCertificate } from '@fortawesome/free-solid-svg-icons' + import { faCertificate, faCheck } from '@fortawesome/free-solid-svg-icons'
37-49: Optional: parametrize the color test to reduce duplicationYou can make the compliant color tests more compact and symmetric.
it.each` compliant | presentClass | absentClass ${true} | ${'text-green-400/80'} | ${'text-red-400/80'} ${false} | ${'text-red-400/80'} | ${'text-green-400/80'} `('applies correct color when compliant=$compliant', ({ compliant, presentClass, absentClass }) => { const { container } = render(<GeneralCompliantComponent {...baseProps} compliant={compliant} />) const svgs = Array.from(container.querySelectorAll('svg')) expect(svgs.find((el) => el.classList.contains(presentClass))).toBeTruthy() expect(svgs.some((el) => el.classList.contains(absentClass))).toBe(false) })
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
cspell/custom-dict.txt(1 hunks)frontend/__tests__/unit/components/GeneralCompliantComponent.test.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- cspell/custom-dict.txt
| it('handles edge case: empty title', () => { | ||
| const { container } = render(<GeneralCompliantComponent {...baseProps} title="" />) | ||
| expect(container).toBeInTheDocument() | ||
| }) |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Edge case: empty title should define behavior (tooltip rendered or not?)
Currently this test only checks that render doesn’t crash. Consider asserting the intended behavior: either no tooltip wrapper is rendered for empty title, or it is rendered with an empty title. Pick one based on the component’s spec and enforce it.
- const { container } = render(<GeneralCompliantComponent {...baseProps} title="" />)
- expect(container).toBeInTheDocument()
+ render(<GeneralCompliantComponent {...baseProps} title="" />)
+ // If empty title means "no tooltip":
+ expect(screen.queryByTestId('tooltip')).not.toBeInTheDocument()
+ // Otherwise (if it should be rendered), assert the wrapper exists and has an empty title:
+ // const tooltip = screen.getByTestId('tooltip')
+ // expect(tooltip).toHaveAttribute('title', '')To confirm the intended behavior in code, you can run:
🏁 Script executed:
#!/bin/bash
# Inspect how Tooltip is used in the component to determine if it's conditional on title.
fd --hidden --strip-cwd-prefix --full-path 'GeneralCompliantComponent' -t f \
| xargs -I {} rg -n -C3 -e 'Tooltip' -e 'title' {}Length of output: 2228
Enforce tooltip wrapper for empty title
Update the test at frontend/__tests__/unit/components/GeneralCompliantComponent.test.tsx to assert that the tooltip wrapper is rendered with an empty title attribute when title="":
it('handles edge case: empty title', () => {
- const { container } = render(<GeneralCompliantComponent {...baseProps} title="" />)
- expect(container).toBeInTheDocument()
+ render(<GeneralCompliantComponent {...baseProps} title="" />)
+ const tooltip = screen.getByTestId('tooltip')
+ expect(tooltip).toBeInTheDocument()
+ expect(tooltip).toHaveAttribute('title', '')
})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it('handles edge case: empty title', () => { | |
| const { container } = render(<GeneralCompliantComponent {...baseProps} title="" />) | |
| expect(container).toBeInTheDocument() | |
| }) | |
| it('handles edge case: empty title', () => { | |
| render(<GeneralCompliantComponent {...baseProps} title="" />) | |
| const tooltip = screen.getByTestId('tooltip') | |
| expect(tooltip).toBeInTheDocument() | |
| expect(tooltip).toHaveAttribute('title', '') | |
| }) |
🤖 Prompt for AI Agents
In frontend/__tests__/unit/components/GeneralCompliantComponent.test.tsx around
lines 64 to 67, the test only checks that the component renders when title=""
but does not assert the tooltip wrapper is present or that it carries an empty
title attribute; update the test to query the tooltip wrapper element (the
component's tooltip wrapper element or the element that receives the title
attribute) and add an assertion that its title attribute strictly equals an
empty string (title === "") to enforce the tooltip wrapper is rendered with an
empty title when title="".
* test: add unit tests for GeneralCompliantComponent * fix(OWASP#1835): Refactor tests based on reviewer feedback * Fix make check and revert unnecessary changes * Fix issues --------- Co-authored-by: Kate Golovanova <[email protected]>
Proposed change
Resolves #1835
Added comprehensive unit tests for
GeneralCompliantComponentto improve coverage and ensure consistent behavior across different props and edge cases.Changes include:
compliant=trueandcompliant=falseChecklist
make check-testlocally; all checks and tests passed.