test: add comprehensive LogoCarousel unit tests #2275
Conversation
Add 33 unit tests covering rendering, props, events, accessibility, and edge cases. Handle component's scrolling duplication behavior and clean up code formatting.
Summary by CodeRabbit
WalkthroughAdds a new unit test suite for the LogoCarousel (MovingLogos) component, covering rendering, conditional logic, prop-driven behavior, interactions, state/internal logic, fallbacks, accessibility, DOM structure, and edge cases. Mocks Next/Image and Next/Link. No exported API changes. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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: 0
🧹 Nitpick comments (9)
frontend/__tests__/unit/components/LogoCarousel.test.tsx (9)
6-21: Harden Next.js mocks for ESM/CJS interop (avoid brittle default export resolution).Return an object with
__esModule: trueanddefaultto keep the mocks stable across Jest/Babel/ts-jest configurations.Apply this diff:
-jest.mock('next/image', () => { - return function MockImage({ - src, - alt, - style, - fill, - }: { - src: string - alt: string - style?: React.CSSProperties - fill?: boolean - }) { - // eslint-disable-next-line @next/next/no-img-element - return <img src={src} alt={alt} style={style} data-testid="sponsor-image" data-fill={fill} /> - } -}) +jest.mock('next/image', () => ({ + __esModule: true, + default: function MockImage({ + src, + alt, + style, + fill, + }: { + src: string + alt: string + style?: React.CSSProperties + fill?: boolean + }) { + // eslint-disable-next-line @next/next/no-img-element + return <img src={src} alt={alt} style={style} data-testid="sponsor-image" data-fill={fill} /> + }, +}))-jest.mock('next/link', () => { - return function MockLink({ - href, - children, - target, - rel, - className, - }: { - href: string - children: React.ReactNode - target?: string - rel?: string - className?: string - }) { - return ( - <a href={href} target={target} rel={rel} className={className} data-testid="sponsor-link"> - {children} - </a> - ) - } -}) +jest.mock('next/link', () => ({ + __esModule: true, + default: function MockLink({ + href, + children, + target, + rel, + className, + }: { + href: string + children: React.ReactNode + target?: string + rel?: string + className?: string + }) { + return ( + <a href={href} target={target} rel={rel} className={className} data-testid="sponsor-link"> + {children} + </a> + ) + }, +}))Also applies to: 23-43
81-88: Scope link/image queries to the scroller to avoid coupling to footer links.Counting all
sponsor-linkanchors conflates carousel items with footer links. Query within.animate-scrollfor carousel-specific assertions.Apply this diff:
-import { render, screen, fireEvent } from '@testing-library/react' +import { render, screen, fireEvent, within } from '@testing-library/react'- expect(screen.getAllByTestId('sponsor-link')).toHaveLength(8) - expect(screen.getAllByTestId('sponsor-image')).toHaveLength(4) + const scroller = document.querySelector('.animate-scroll') as HTMLElement + expect(within(scroller).getAllByTestId('sponsor-link')).toHaveLength(6) + expect(within(scroller).getAllByTestId('sponsor-image')).toHaveLength(4)
216-245: Make effect-driven assertions robust with waitFor (useEffect timing).Several tests rely on DOM duplication performed in
useEffect. Wrap these assertions inwaitForto eliminate timing flakiness.Apply this diff pattern (and replicate across similar spots):
-import { render, screen, fireEvent } from '@testing-library/react' +import { render, screen, fireEvent, waitFor } from '@testing-library/react'Example update:
- const scroller = document.querySelector('.animate-scroll') - expect(scroller).toHaveStyle('animation-duration: 6s') + await waitFor(() => { + const scroller = document.querySelector('.animate-scroll') + expect(scroller).toHaveStyle('animation-duration: 6s') + })And:
- expect(sponsorLinks).toHaveLength(6) + await waitFor(() => expect(sponsorLinks).toHaveLength(6))Also applies to: 149-168, 376-391, 395-403
453-461: Prefer toHaveStyle over exact style attribute string.Asserting the raw
styleattribute is brittle. Check the computed style instead.Apply this diff:
- images.forEach((image) => { - expect(image).toHaveAttribute('style', 'object-fit: contain;') - expect(image).toHaveAttribute('data-fill', 'true') - }) + images.forEach((image) => { + expect(image).toHaveStyle('object-fit: contain') + expect(image).toHaveAttribute('data-fill', 'true') + })
204-213: Use user-event for clicks (more realistic interactions).
@testing-library/user-eventbetter simulates user behavior vs.fireEvent.Apply this diff:
+import userEvent from '@testing-library/user-event' ... - expect(() => { - fireEvent.click(links[0]) - fireEvent.click(links[1]) - }).not.toThrow() + const user = userEvent.setup() + await user.click(links[0]) + await user.click(links[1])
376-391: Trim the “very large dataset” test to keep unit tests fast.100 items (200 DOM nodes post-duplication) can slow CI. 40–60 typically suffices to validate behavior.
Apply this diff:
- const manySponsors: Sponsor[] = Array.from({ length: 100 }, (_, i) => ({ + const manySponsors: Sponsor[] = Array.from({ length: 50 }, (_, i) => ({ ... - expect(screen.getAllByTestId('sponsor-link')).toHaveLength(202) - expect(screen.getAllByTestId('sponsor-image')).toHaveLength(200) + expect(screen.getAllByTestId('sponsor-link')).toHaveLength(102) + expect(screen.getAllByTestId('sponsor-image')).toHaveLength(100) ... - expect(scroller).toHaveStyle('animation-duration: 200s') + expect(scroller).toHaveStyle('animation-duration: 100s')
299-306: Consolidate duplicate alt-text tests.These two cases assert the same thing; merge to a single test to reduce noise.
Suggested approach: remove one block and extend the remaining test’s assertions as needed.
Also applies to: 307-314
101-108: Prefer accessible queries over test IDs where possible.Use roles/names for images to make tests reflect user-observable behavior.
Example:
- const images = screen.getAllByTestId('sponsor-image') + const images = screen.getAllByRole('img', { name: /logo$/ })Also applies to: 318-336, 348-353
77-79: Unnecessaryjest.clearAllMocks()(no spies to reset).No jest.fn() spies are created here. Safe to remove to shave a few ms per test, or keep if you expect to add spies later.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/__tests__/unit/components/LogoCarousel.test.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/__tests__/unit/components/LogoCarousel.test.tsx (1)
frontend/src/components/LogoCarousel.tsx (1)
MovingLogos(10-85)
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
frontend/__tests__/unit/components/LogoCarousel.test.tsx (1)
44-63: Use reserved/example domains in fixtures to avoid security tooling false positives.Static .com hostnames (sponsor1.com, new.com, etc.) have triggered “Incomplete URL substring sanitization” alerts in this repo before. Switch to RFC‑reserved domains (.example/.test/.invalid) to keep code scanning quiet and avoid implying trust in real hosts.
Example replacements:
- https://sponsor1.com → https://sponsor1.example
- https://new.com → https://new.example
- https://custom.com → https://custom.example
- https://incomplete.com → https://incomplete.example
- https://noimage.com → https://noimage.example
- https://example.com/logo.png is fine to keep.
I can push a patch touching all affected fixtures in this file if you want.
Also applies to: 65-72, 129-136, 230-233, 257-262, 321-329, 339-346, 356-363
🧹 Nitpick comments (7)
frontend/__tests__/unit/components/LogoCarousel.test.tsx (7)
85-87: Scope link/image counts to the scroller to avoid coupling to footer links.Counting all elements with data-testid also includes footer links. Query within the scroller to assert carousel items, then separately assert footer links if needed. This reduces brittleness if footer markup changes.
- expect(screen.getAllByTestId('sponsor-link')).toHaveLength(8) - expect(screen.getAllByTestId('sponsor-image')).toHaveLength(4) + const scroller = document.querySelector('.animate-scroll') as HTMLElement + expect(scroller).toBeInTheDocument() + expect(within(scroller).getAllByTestId('sponsor-link')).toHaveLength(6) + expect(within(scroller).getAllByTestId('sponsor-image')).toHaveLength(4) + // footer links (kept optional) + expect(screen.getAllByTestId('sponsor-link')).toHaveLength(8)
149-167: Avoid magic numbers in animation-duration assertions.Compute expected duration from input to keep tests resilient if defaults change.
- expect(scroller).toHaveStyle('animation-duration: 6s') + expect(scroller).toHaveStyle(`animation-duration: ${mockSponsors.length * 2}s`) ... - expect(scroller).toHaveStyle('animation-duration: 6s') + expect(scroller).toHaveStyle(`animation-duration: ${mockSponsors.length * 2}s`) ... - expect(scroller).toHaveStyle('animation-duration: 12s') + expect(scroller).toHaveStyle(`animation-duration: ${newSponsors.length * 2}s`)
217-244: This test asserts an implementation detail (innerHTML duplication). Consider asserting outcome, not mechanism.Mutation via innerHTML is brittle. Prefer verifying two full sequences of sponsors exist (order/length), not that duplication is achieved by concatenating innerHTML.
If you want, I can draft an alternative that:
- Queries the scroller’s children and verifies the second half equals the first by key order and content.
- Ensures re-render with the same sponsors reference doesn’t duplicate again.
299-314: Duplicate coverage for image alt text.“renders sponsor names correctly in alt attributes” and “renders image alt text correctly” assert the same thing. Drop one to cut noise and speed up the suite.
- it('renders image alt text correctly', () => { - render(<MovingLogos sponsors={mockSponsors} />) - - const images = screen.getAllByTestId('sponsor-image') - expect(images[0]).toHaveAttribute('alt', 'Test Sponsor 1 logo') - expect(images[1]).toHaveAttribute('alt', 'Test Sponsor 2 logo') - })
355-374: Add a negative-security case (javascript: URLs).Right now we only test “invalid” opaque strings. Add a case that ensures “javascript:” (and possibly “data:”) are rejected or rendered inert by the component (ideally sanitized upstream). If the component intentionally passes through, at least codify that decision.
I can add a test like:
- sponsors: [{ url: 'javascript:alert(1)' }]
- expectation: href is not “javascript:…”, or click is prevented, or link omitted.
376-391: Large dataset test is heavier than necessary.Rendering 100 items (200 after duplication) is fine locally but can slow CI. A smaller N (e.g., 30–50) still validates behavior and halves test time.
- const manySponsors: Sponsor[] = Array.from({ length: 100 }, (_, i) => ({ + const manySponsors: Sponsor[] = Array.from({ length: 50 }, (_, i) => ({ ... - expect(screen.getAllByTestId('sponsor-link')).toHaveLength(202) - expect(screen.getAllByTestId('sponsor-image')).toHaveLength(200) + expect(screen.getAllByTestId('sponsor-link')).toHaveLength(102) + expect(screen.getAllByTestId('sponsor-image')).toHaveLength(100) ... - expect(scroller).toHaveStyle('animation-duration: 200s') + expect(scroller).toHaveStyle('animation-duration: 100s')
439-451: Class-based DOM queries are fragile with utility CSS.Tailwind class names change easily. Prefer structural queries (roles, testids) narrowed to the scroller rather than brittle class chains or attribute contains selectors with arbitrary values.
Example:
- Use data-testid on the scroller and sponsor cards in the component for stable hooks, then query with within().
Also applies to: 511-520
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/__tests__/unit/components/LogoCarousel.test.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-06T12:57:58.021Z
Learnt from: rudransh-shrivastava
PR: OWASP/Nest#2223
File: backend/apps/owasp/models/common.py:0-0
Timestamp: 2025-09-06T12:57:58.021Z
Learning: When filtering URLs by domain in the OWASP project, use `urlparse(url).netloc == domain` instead of `domain in url` to avoid security vulnerabilities where malicious subdomains could bypass filtering (e.g., `testowasp.org` would match when filtering for `owasp.org`).
Applied to files:
frontend/__tests__/unit/components/LogoCarousel.test.tsx
🧬 Code graph analysis (1)
frontend/__tests__/unit/components/LogoCarousel.test.tsx (1)
frontend/src/components/LogoCarousel.tsx (1)
MovingLogos(10-85)
🔇 Additional comments (1)
frontend/__tests__/unit/components/LogoCarousel.test.tsx (1)
172-184: Good accessibility assertions on external links.Validating target and rel for all outbound links is solid and prevents reverse‑tabnabbing.
Optionally add a check that footer links have meaningful text for context (already present via “this page”/“click here”).
Also applies to: 408-413
kasya
left a comment
kasya
left a comment
There was a problem hiding this comment.
These looks good! Thank you 👍🏼
* test: add comprehensive LogoCarousel unit tests Add 33 unit tests covering rendering, props, events, accessibility, and edge cases. Handle component's scrolling duplication behavior and clean up code formatting. * Fixed Issues flagged by the bot
2 participants
Proposed change
Resolves #1843
This PR adds comprehensive unit tests for the LogoCarousel component to improve test coverage and ensure component reliability.
What was added:
Technical details:
ImageandLinkcomponents for testingtypes/homeTest coverage:
Checklist
make check-testlocally; all checks and tests passed.