Added unit tests for components#581
Open
Abhishekmaurya12367 wants to merge 5 commits intoAOSSIE-Org:mainfrom
Open
Added unit tests for components#581Abhishekmaurya12367 wants to merge 5 commits intoAOSSIE-Org:mainfrom
Abhishekmaurya12367 wants to merge 5 commits intoAOSSIE-Org:mainfrom
Conversation
📝 WalkthroughWalkthroughIntroduces comprehensive Jest testing infrastructure for React components with configuration files, DOM mocking utilities, and nine test suites covering Card, Container, Banner, SectionHeading, CardHome, XIcon, Prose, Header, and Footer components. Also adds a new XIcon component and replaces Twitter icon references with it. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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: 4
🧹 Nitpick comments (7)
src/components/__tests__/Banner.test.jsx (1)
42-56: Simplify the apply link assertion.The
closest('[href]')fallback on Line 54 could match an unrelated ancestor<a>element and produce a false positive. Since the Next.jsLinkmock renders a standard<a>, you can assert directly on the anchor:♻️ Suggested simplification
it('renders apply link with correct href', () => { - const { container } = render(<Banner />) - const linkText = screen.getByText('Apply to GSoC with AOSSIE') - // The Banner uses legacyBehavior, so Link returns Fragment and anchor is in children - // Find the anchor tag that contains the text - const links = container.querySelectorAll('a') - const applyLink = Array.from(links).find(link => - link.textContent.includes('Apply to GSoC with AOSSIE') - ) - expect(applyLink).toBeInTheDocument() - // With legacyBehavior, the href might be on a parent or the anchor itself - // Check if href exists on the link or its parent - const href = applyLink.getAttribute('href') || applyLink.closest('[href]')?.getAttribute('href') - expect(href).toBe('/apply') + render(<Banner />) + const applyLink = screen.getByRole('link', { name: /Apply to GSoC with AOSSIE/i }) + expect(applyLink).toHaveAttribute('href', '/apply') })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/__tests__/Banner.test.jsx` around lines 42 - 56, The test's fallback using applyLink.closest('[href]') can match an unrelated ancestor and yield false positives; update the assertion to verify the href directly on the anchor element found (the applyLink variable) and remove the closest('[href]') fallback. Locate the anchor by its text (or use screen.getByRole('link', { name: /Apply to GSoC with AOSSIE/ })) to ensure you have the actual <a> element rendered by the mocked Next.js Link, then assert applyLink.getAttribute('href') === '/apply'.src/pages/index.jsx (1)
7-7: Twitter → X rebranding looks good; consider updating the URL too.The icon and aria-label are updated to reflect the "X" branding, but the
hrefon Line 93 still points tohttps://twitter.com/aossie_org. Whiletwitter.comredirects tox.com, you may want to update it tohttps://x.com/aossie_orgfor consistency. The same applies insrc/components/Footer.jsx(Line 49).Also applies to: 91-95
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/index.jsx` at line 7, Update the external profile links that still point to twitter.com to use the new X domain: replace instances of "https://twitter.com/aossie_org" with "https://x.com/aossie_org" (e.g., the anchor using the XIcon and aria-label "X" in the main page component and the anchor in Footer.jsx); ensure the hrefs in the link elements that render the XIcon and the Footer anchor are changed so both components point to the updated URL.src/components/__tests__/Header.test.jsx (1)
6-12: Redundantnext/imagemock — already defined globally injest.setup.js.This local mock duplicates the global one in
jest.setup.js(lines 28-34). Unless you need Header-specific mock behavior, remove this to reduce noise.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/__tests__/Header.test.jsx` around lines 6 - 12, Remove the redundant local mock of next/image in Header.test.jsx: delete the jest.mock('next/image', ...) block since the same mock is already provided globally in jest.setup.js; only reintroduce a local mock here if Header.test.jsx requires behavior different from the global mock.jest.setup.js (2)
5-25:popis not a Next.js router method.Line 13 mocks
pop: jest.fn(), butuseRouterin Next.js Pages Router doesn't expose apopmethod. This won't cause failures but is misleading. Consider removing it or replacing withreplace: jest.fn()which is an actual router method that's missing from this mock.Fix router mock
push: jest.fn(), - pop: jest.fn(), + replace: jest.fn(), reload: jest.fn(),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@jest.setup.js` around lines 5 - 25, The router mock in the jest.mock call incorrectly includes a non-existent Next.js method `pop`; update the mocked return object from the `useRouter` function to remove `pop` and add the real router method `replace: jest.fn()` instead (or simply remove `pop` if you prefer no replacement). Locate the jest.mock('next/router', ...) block and modify the object returned by `useRouter()`—specifically replace the `pop: jest.fn()` entry with `replace: jest.fn()` to align the mock with Next.js's actual router API.
37-45:legacyBehaviorbranch is identical to the default branch.Both arms of the conditional produce the exact same element, making the
legacyBehaviorcheck a no-op. If you don't need to differentiate behavior in tests, simplify by removing the branch. If you do, the legacy mock should passhrefthrough the child<a>clone rather than wrapping it.Simplified mock
jest.mock('next/link', () => { const React = require('react') - return ({ children, href, className, legacyBehavior, ...props }) => { - if (legacyBehavior) { - // For legacyBehavior, wrap children in an anchor with href - return React.createElement('a', { href, className, ...props }, children) - } + return ({ children, href, className, legacyBehavior, ...props }) => { return React.createElement('a', { href, className, ...props }, children) } })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@jest.setup.js` around lines 37 - 45, The mock for next/link currently returns a function with a redundant legacyBehavior branch; either remove the conditional and always return a single anchor element, or implement the legacyBehavior behavior correctly: when legacyBehavior is true, clone/wrap the provided children (instead of creating a new anchor) and pass the href through to the child anchor. Update the mocked factory (the function returned in jest.mock('next/link')) to either drop the legacyBehavior check or to inject href into the child element when legacyBehavior is true so behavior differs from the default branch.TESTING_SUMMARY.md (1)
1-129: Consider moving this content into CONTRIBUTING.md or README.md instead of a standalone file.
TESTING_SUMMARY.mdreads more like a PR description than permanent documentation. The "Running Tests" section (lines 80-91) is valuable for contributors, but the point-in-time statistics (lines 93-97: "69 tests passing") and the file change log (lines 99-116) will become stale immediately. Consider:
- Moving the useful parts ("Running Tests", "Testing Infrastructure" overview) into an existing doc like README or CONTRIBUTING.
- Removing transient statistics and file lists that won't be maintained.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TESTING_SUMMARY.md` around lines 1 - 129, Move the permanent, useful docs (the "Running Tests" section and the "Testing Infrastructure" overview) into an existing contributor-facing doc (preferably CONTRIBUTING.md or README.md) by copying the headings "Running Tests" and "Testing Infrastructure" and their content; remove transient sections "Test Statistics" and "Files Modified/Created" from the repo docs (delete them from TESTING_SUMMARY.md) and either remove TESTING_SUMMARY.md or convert it into a temporary PR description/note; ensure references to scripts in package.json remain accurate and update CONTRIBUTING.md/README.md to include the test commands and a brief infra summary while omitting point-in-time counts like "69 tests passing."src/components/__tests__/Prose.test.jsx (1)
44-64: Tests are coupled to implementation-detail CSS class names.Asserting exact Tailwind utility classes like
prose-code:p-2andprose-headings:font-extraboldmakes these tests brittle — any styling tweak will break them without an actual behavior change. Consider testing only the semantically meaningful aspects (renders children, accepts customclassName) and dropping the granular class assertions, or at minimum consolidating them into a single test that checks the class string contains expected substrings.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/__tests__/Prose.test.jsx` around lines 44 - 64, The tests in Prose.test.jsx are tightly coupled to Tailwind utility class names (e.g., the assertions looking for 'prose-code:p-2', 'prose-code:bg-slate-300', 'prose-headings:font-extrabold', 'prose-headings:mt-0'); update the tests for the Prose component to avoid brittle, implementation-detail assertions by removing the specific utility-class expects and instead verify semantic behavior: ensure Prose renders its children correctly and accepts/merges a custom className prop (or, if you need to assert styling, replace multiple exact class checks with a single assertion that the container.className contains expected substrings like 'prose-code' and 'prose-headings'); update the test cases around render(<Prose>...</Prose>) and the variable prose = container.firstChild to reflect these new, less brittle assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/__tests__/CardHome.test.jsx`:
- Around line 24-48: The two tests ("has correct styling classes" and "has dark
mode classes") claim to verify CSS classes but only assert element presence;
update the tests in CardHome.test.jsx to either (A) assert the expected
className values on the elements rendered by <CardHome heading=... content=...
/> — e.g., check the Link's className and the h5/class ".block" contains the
expected class tokens (use toHaveClass or classList checks) — or (B) if class
names are unstable or not intended to be tested, remove these two tests to avoid
redundant existence assertions; locate the tests by their titles and queries for
'a', 'h5', and '.block' and replace the duplicate/empty expects with meaningful
class assertions or delete the test blocks.
In `@src/components/__tests__/Header.test.jsx`:
- Around line 217-222: The test "renders mobile navigation menu items" in
Header.test.jsx is a duplicate and only asserts the "Menu" button; either remove
this test or change it to actually open the Popover and assert the mobile nav
entries appear: render the Header component, trigger the Menu toggle (e.g.,
fireEvent.click or userEvent.click on the "Menu" button), then assert that the
expected mobile items like "About", "Projects", etc. are present in the
Popover.Panel using screen.getByText or await screen.findByText; update the test
name if you keep it to reflect that it checks the Popover panel contents.
- Around line 187-215: The "highlights active navigation item" test in
Header.test.jsx is duplicating href checks and not asserting the active styling;
update the test for the Header component (the "highlights active navigation
item" it block) to assert the active CSS class on the active NavItem instead of
only checking href: set mockRouter.pathname = '/projects', render <Header />,
locate the Projects link (as you already do) and add an expectation that the
link or its NavItem root element has the active class (e.g.,
toHaveClass('text-[`#00843D`]') or whatever className NavItem uses when isActive),
or if you prefer to keep test surface area minimal, remove this entire it block
to avoid duplicating the existing href test.
- Around line 120-154: The test for Header's dark mode toggle currently only
asserts the button exists; update the test in the 'dark mode toggle button calls
toggleMode when clicked' case so it verifies an actual side effect of clicking
toggleButton: either assert that document.documentElement.classList.toggle (or
contains 'dark' class) was called/changed, or assert that the mock
localStorage.setItem was called with the expected theme key/value after await
user.click(toggleButton). Locate the test referencing Header and toggleButton,
ensure matchMedia/localStorage mocks remain or are removed if unused, and
replace the final expect(toggleButton).toBeInTheDocument() with an assertion on
document.documentElement.classList or localStorage.setItem to validate the
toggle behavior.
---
Nitpick comments:
In `@jest.setup.js`:
- Around line 5-25: The router mock in the jest.mock call incorrectly includes a
non-existent Next.js method `pop`; update the mocked return object from the
`useRouter` function to remove `pop` and add the real router method `replace:
jest.fn()` instead (or simply remove `pop` if you prefer no replacement). Locate
the jest.mock('next/router', ...) block and modify the object returned by
`useRouter()`—specifically replace the `pop: jest.fn()` entry with `replace:
jest.fn()` to align the mock with Next.js's actual router API.
- Around line 37-45: The mock for next/link currently returns a function with a
redundant legacyBehavior branch; either remove the conditional and always return
a single anchor element, or implement the legacyBehavior behavior correctly:
when legacyBehavior is true, clone/wrap the provided children (instead of
creating a new anchor) and pass the href through to the child anchor. Update the
mocked factory (the function returned in jest.mock('next/link')) to either drop
the legacyBehavior check or to inject href into the child element when
legacyBehavior is true so behavior differs from the default branch.
In `@src/components/__tests__/Banner.test.jsx`:
- Around line 42-56: The test's fallback using applyLink.closest('[href]') can
match an unrelated ancestor and yield false positives; update the assertion to
verify the href directly on the anchor element found (the applyLink variable)
and remove the closest('[href]') fallback. Locate the anchor by its text (or use
screen.getByRole('link', { name: /Apply to GSoC with AOSSIE/ })) to ensure you
have the actual <a> element rendered by the mocked Next.js Link, then assert
applyLink.getAttribute('href') === '/apply'.
In `@src/components/__tests__/Header.test.jsx`:
- Around line 6-12: Remove the redundant local mock of next/image in
Header.test.jsx: delete the jest.mock('next/image', ...) block since the same
mock is already provided globally in jest.setup.js; only reintroduce a local
mock here if Header.test.jsx requires behavior different from the global mock.
In `@src/components/__tests__/Prose.test.jsx`:
- Around line 44-64: The tests in Prose.test.jsx are tightly coupled to Tailwind
utility class names (e.g., the assertions looking for 'prose-code:p-2',
'prose-code:bg-slate-300', 'prose-headings:font-extrabold',
'prose-headings:mt-0'); update the tests for the Prose component to avoid
brittle, implementation-detail assertions by removing the specific utility-class
expects and instead verify semantic behavior: ensure Prose renders its children
correctly and accepts/merges a custom className prop (or, if you need to assert
styling, replace multiple exact class checks with a single assertion that the
container.className contains expected substrings like 'prose-code' and
'prose-headings'); update the test cases around render(<Prose>...</Prose>) and
the variable prose = container.firstChild to reflect these new, less brittle
assertions.
In `@src/pages/index.jsx`:
- Line 7: Update the external profile links that still point to twitter.com to
use the new X domain: replace instances of "https://twitter.com/aossie_org" with
"https://x.com/aossie_org" (e.g., the anchor using the XIcon and aria-label "X"
in the main page component and the anchor in Footer.jsx); ensure the hrefs in
the link elements that render the XIcon and the Footer anchor are changed so
both components point to the updated URL.
In `@TESTING_SUMMARY.md`:
- Around line 1-129: Move the permanent, useful docs (the "Running Tests"
section and the "Testing Infrastructure" overview) into an existing
contributor-facing doc (preferably CONTRIBUTING.md or README.md) by copying the
headings "Running Tests" and "Testing Infrastructure" and their content; remove
transient sections "Test Statistics" and "Files Modified/Created" from the repo
docs (delete them from TESTING_SUMMARY.md) and either remove TESTING_SUMMARY.md
or convert it into a temporary PR description/note; ensure references to scripts
in package.json remain accurate and update CONTRIBUTING.md/README.md to include
the test commands and a brief infra summary while omitting point-in-time counts
like "69 tests passing."
Comment on lines
+24
to
+48
| it('has correct styling classes', () => { | ||
| const { container } = render( | ||
| <CardHome heading="Heading" content="Content" /> | ||
| ) | ||
| const link = container.querySelector('a') | ||
| expect(link).toBeInTheDocument() | ||
| // The Link component wraps the content, so classes are on the Link's className | ||
| // Since Next.js Link mock passes className through, we check the structure | ||
| expect(link).toBeInTheDocument() | ||
| // Verify the card structure exists | ||
| const h5 = container.querySelector('h5') | ||
| expect(h5).toBeInTheDocument() | ||
| }) | ||
|
|
||
| it('has dark mode classes', () => { | ||
| const { container } = render( | ||
| <CardHome heading="Heading" content="Content" /> | ||
| ) | ||
| const link = container.querySelector('a') | ||
| expect(link).toBeInTheDocument() | ||
| // Verify component structure - classes are applied in the component | ||
| const cardContent = container.querySelector('.block') | ||
| // The classes are in the component definition, so we verify structure instead | ||
| expect(link).toBeInTheDocument() | ||
| }) |
There was a problem hiding this comment.
Tests "has correct styling classes" and "has dark mode classes" don't assert what they claim.
Both tests have descriptive names implying they verify CSS classes, but neither actually asserts on any class names. They only re-check element existence (already covered by other tests). Line 32 is a duplicate of Line 29, and Lines 45-47 query .block without asserting on the result.
Either add meaningful class assertions or remove these tests to avoid giving false confidence in styling coverage.
♻️ Suggested fix — actually test the classes
it('has correct styling classes', () => {
const { container } = render(
<CardHome heading="Heading" content="Content" />
)
const link = container.querySelector('a')
expect(link).toBeInTheDocument()
- // The Link component wraps the content, so classes are on the Link's className
- // Since Next.js Link mock passes className through, we check the structure
- expect(link).toBeInTheDocument()
- // Verify the card structure exists
- const h5 = container.querySelector('h5')
- expect(h5).toBeInTheDocument()
+ expect(link).toHaveClass('block', 'rounded-lg', 'shadow')
})
it('has dark mode classes', () => {
const { container } = render(
<CardHome heading="Heading" content="Content" />
)
const link = container.querySelector('a')
expect(link).toBeInTheDocument()
- // Verify component structure - classes are applied in the component
- const cardContent = container.querySelector('.block')
- // The classes are in the component definition, so we verify structure instead
- expect(link).toBeInTheDocument()
+ expect(link).toHaveClass('dark:bg-gray-800', 'dark:border-gray-700')
})🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/__tests__/CardHome.test.jsx` around lines 24 - 48, The two
tests ("has correct styling classes" and "has dark mode classes") claim to
verify CSS classes but only assert element presence; update the tests in
CardHome.test.jsx to either (A) assert the expected className values on the
elements rendered by <CardHome heading=... content=... /> — e.g., check the
Link's className and the h5/class ".block" contains the expected class tokens
(use toHaveClass or classList checks) — or (B) if class names are unstable or
not intended to be tested, remove these two tests to avoid redundant existence
assertions; locate the tests by their titles and queries for 'a', 'h5', and
'.block' and replace the duplicate/empty expects with meaningful class
assertions or delete the test blocks.
Comment on lines
+120
to
+154
| it('dark mode toggle button calls toggleMode when clicked', async () => { | ||
| const user = userEvent.setup() | ||
|
|
||
| // Mock localStorage | ||
| const localStorageMock = { | ||
| getItem: jest.fn(), | ||
| setItem: jest.fn(), | ||
| removeItem: jest.fn(), | ||
| } | ||
| global.localStorage = localStorageMock | ||
|
|
||
| // Mock matchMedia | ||
| Object.defineProperty(window, 'matchMedia', { | ||
| writable: true, | ||
| value: jest.fn().mockImplementation((query) => ({ | ||
| matches: false, | ||
| media: query, | ||
| onchange: null, | ||
| addListener: jest.fn(), | ||
| removeListener: jest.fn(), | ||
| addEventListener: jest.fn(), | ||
| removeEventListener: jest.fn(), | ||
| dispatchEvent: jest.fn(), | ||
| })), | ||
| }) | ||
|
|
||
| render(<Header />) | ||
| const toggleButton = screen.getByLabelText('Toggle dark mode') | ||
|
|
||
| await user.click(toggleButton) | ||
|
|
||
| // Check if document.documentElement.classList.toggle was called | ||
| // This is tested indirectly through the button click | ||
| expect(toggleButton).toBeInTheDocument() | ||
| }) |
There was a problem hiding this comment.
Dark mode toggle test doesn't actually verify toggle behavior.
The test sets up localStorage and matchMedia mocks (lines 124-144) but the only assertion is expect(toggleButton).toBeInTheDocument() (line 153), which just checks the button exists — already covered by the test on line 98-102. The elaborate mock setup is wasted. Either assert on an observable side effect (e.g., classList change on document.documentElement, or localStorage.setItem being called), or remove this test to avoid false confidence.
Example: actually verify a side effect
await user.click(toggleButton)
- // Check if document.documentElement.classList.toggle was called
- // This is tested indirectly through the button click
- expect(toggleButton).toBeInTheDocument()
+ // Verify dark mode was toggled via observable side effect
+ expect(localStorageMock.setItem).toHaveBeenCalled()📝 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.
Suggested change
| it('dark mode toggle button calls toggleMode when clicked', async () => { | |
| const user = userEvent.setup() | |
| // Mock localStorage | |
| const localStorageMock = { | |
| getItem: jest.fn(), | |
| setItem: jest.fn(), | |
| removeItem: jest.fn(), | |
| } | |
| global.localStorage = localStorageMock | |
| // Mock matchMedia | |
| Object.defineProperty(window, 'matchMedia', { | |
| writable: true, | |
| value: jest.fn().mockImplementation((query) => ({ | |
| matches: false, | |
| media: query, | |
| onchange: null, | |
| addListener: jest.fn(), | |
| removeListener: jest.fn(), | |
| addEventListener: jest.fn(), | |
| removeEventListener: jest.fn(), | |
| dispatchEvent: jest.fn(), | |
| })), | |
| }) | |
| render(<Header />) | |
| const toggleButton = screen.getByLabelText('Toggle dark mode') | |
| await user.click(toggleButton) | |
| // Check if document.documentElement.classList.toggle was called | |
| // This is tested indirectly through the button click | |
| expect(toggleButton).toBeInTheDocument() | |
| }) | |
| it('dark mode toggle button calls toggleMode when clicked', async () => { | |
| const user = userEvent.setup() | |
| // Mock localStorage | |
| const localStorageMock = { | |
| getItem: jest.fn(), | |
| setItem: jest.fn(), | |
| removeItem: jest.fn(), | |
| } | |
| global.localStorage = localStorageMock | |
| // Mock matchMedia | |
| Object.defineProperty(window, 'matchMedia', { | |
| writable: true, | |
| value: jest.fn().mockImplementation((query) => ({ | |
| matches: false, | |
| media: query, | |
| onchange: null, | |
| addListener: jest.fn(), | |
| removeListener: jest.fn(), | |
| addEventListener: jest.fn(), | |
| removeEventListener: jest.fn(), | |
| dispatchEvent: jest.fn(), | |
| })), | |
| }) | |
| render(<Header />) | |
| const toggleButton = screen.getByLabelText('Toggle dark mode') | |
| await user.click(toggleButton) | |
| // Verify dark mode was toggled via observable side effect | |
| expect(localStorageMock.setItem).toHaveBeenCalled() | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/__tests__/Header.test.jsx` around lines 120 - 154, The test
for Header's dark mode toggle currently only asserts the button exists; update
the test in the 'dark mode toggle button calls toggleMode when clicked' case so
it verifies an actual side effect of clicking toggleButton: either assert that
document.documentElement.classList.toggle (or contains 'dark' class) was
called/changed, or assert that the mock localStorage.setItem was called with the
expected theme key/value after await user.click(toggleButton). Locate the test
referencing Header and toggleButton, ensure matchMedia/localStorage mocks remain
or are removed if unused, and replace the final
expect(toggleButton).toBeInTheDocument() with an assertion on
document.documentElement.classList or localStorage.setItem to validate the
toggle behavior.
Comment on lines
+187
to
+215
| it('highlights active navigation item', () => { | ||
| mockRouter.pathname = '/projects' | ||
| const { container } = render(<Header />) | ||
|
|
||
| // Verify that when pathname is /projects, the NavItem component receives isActive=true | ||
| // This is tested indirectly by checking that navigation renders correctly | ||
| // The actual styling is applied by clsx based on isActive prop | ||
|
|
||
| // Find Projects links in the header | ||
| const projectsLinks = screen.getAllByText('Projects') | ||
| expect(projectsLinks.length).toBeGreaterThan(0) | ||
|
|
||
| // Verify at least one Projects link exists in header | ||
| const headerProjectsLink = projectsLinks.find(link => | ||
| link.closest('header') !== null | ||
| ) | ||
| expect(headerProjectsLink).toBeInTheDocument() | ||
|
|
||
| // The active state is determined by useRouter().pathname === href | ||
| // Since we've set pathname to /projects, the link with href="/projects" should be active | ||
| // We verify the link exists and the router logic will handle the active state | ||
| const linkElement = headerProjectsLink.closest('a') | ||
| expect(linkElement).toBeInTheDocument() | ||
| expect(linkElement).toHaveAttribute('href', '/projects') | ||
|
|
||
| // Note: The actual className with active state is applied by clsx conditionally | ||
| // In a real scenario, this would have 'text-[#00843D]' when active | ||
| // For testing purposes, we verify the component structure and router integration | ||
| }) |
There was a problem hiding this comment.
"Highlights active navigation item" test doesn't verify active styling.
This test only asserts the link exists with the correct href — the same thing the test at lines 156-185 already verifies. The comments (lines 191-214) explain what it should test but acknowledge it doesn't. Either assert on the active CSS class (e.g., toHaveClass('text-[#00843D]')) or remove this test to avoid duplication and false coverage.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/__tests__/Header.test.jsx` around lines 187 - 215, The
"highlights active navigation item" test in Header.test.jsx is duplicating href
checks and not asserting the active styling; update the test for the Header
component (the "highlights active navigation item" it block) to assert the
active CSS class on the active NavItem instead of only checking href: set
mockRouter.pathname = '/projects', render <Header />, locate the Projects link
(as you already do) and add an expectation that the link or its NavItem root
element has the active class (e.g., toHaveClass('text-[`#00843D`]') or whatever
className NavItem uses when isActive), or if you prefer to keep test surface
area minimal, remove this entire it block to avoid duplicating the existing href
test.
Comment on lines
+217
to
+222
| it('renders mobile navigation menu items', () => { | ||
| render(<Header />) | ||
| // Mobile nav items should be present (they're in the Popover.Panel) | ||
| // We can check if the navigation structure exists | ||
| expect(screen.getByText('Menu')).toBeInTheDocument() | ||
| }) |
There was a problem hiding this comment.
"Renders mobile navigation menu items" test only checks "Menu" button exists.
This duplicates the test on line 92-96. It doesn't verify that mobile nav items (About, Projects, etc.) appear in the Popover panel. Consider asserting on the actual mobile nav content or removing this test.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/__tests__/Header.test.jsx` around lines 217 - 222, The test
"renders mobile navigation menu items" in Header.test.jsx is a duplicate and
only asserts the "Menu" button; either remove this test or change it to actually
open the Popover and assert the mobile nav entries appear: render the Header
component, trigger the Menu toggle (e.g., fireEvent.click or userEvent.click on
the "Menu" button), then assert that the expected mobile items like "About",
"Projects", etc. are present in the Popover.Panel using screen.getByText or
await screen.findByText; update the test name if you keep it to reflect that it
checks the Popover panel contents.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Unit test setup for this Next.js project using Jest + React Testing Library
jest.config.js, jest.setup.js
Test scripts in package.json
test, test:watch, test:coverage
Component unit tests in src/components/tests/
Tests for Header, Footer, Banner, Card, Container, SectionHeading, CardHome, Prose, XIcon
Why we added it
Catch UI regressions early (links, headings, toggle buttons, render logic breaking after changes).
Improve maintainability by documenting expected component behavior via tests.
Increase confidence in refactors and future PRs with automated checks.
Summary by CodeRabbit
New Features
Updates
Tests