test: add comprehensive unit tests for SkeletonBase component #2080
Conversation
- Cover all conditional rendering logic for different indexName values - Test prop-based behavior and component configurations - Add edge case handling for null/undefined inputs - Verify DOM structure, accessibility, and component integration - Achieve 100% test coverage with 35+ test cases
Summary by CodeRabbit
WalkthroughAdds a new unit test file for the SkeletonBase React component, covering rendering across indexName variants, conditional rendering, prop-driven behavior, edge cases, accessibility/DOM checks, and interactions with mocked child components. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10–15 minutes Assessment against linked issues
Suggested labels
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. ✨ 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: 0
🧹 Nitpick comments (8)
frontend/__tests__/unit/components/SkeletonBase.test.tsx (8)
13-17: Loosen mock prop typing to match tests passing null/undefined.Tests pass empty string and null for loadingImageUrl. Make the mock accept an optional prop to avoid casting with
as neverand better match runtime behavior.-jest.mock('components/LoadingSpinner', () => { - return function MockLoadingSpinner({ imageUrl }: { imageUrl: string }) { +jest.mock('components/LoadingSpinner', () => { + return function MockLoadingSpinner({ imageUrl }: { imageUrl?: string }) { return <div data-testid="loading-spinner" data-image-url={imageUrl || ''} /> } })
62-66: Be explicit: assert the known count instead of indexing into the array.Indexing
[0]is slightly fragile and less expressive. Sinceprojectsrenders 4 skeletons, assert that directly.- render(<SkeletonBase {...defaultProps} />) - expect(screen.getAllByTestId('card-skeleton')[0]).toBeInTheDocument() + render(<SkeletonBase {...defaultProps} />) + expect(screen.getAllByTestId('card-skeleton')).toHaveLength(4)
241-248: Use RTL’s unmount instead ofcontainer.remove()to avoid leaks.Calling
container.remove()doesn’t invoke React cleanup and can leave event listeners hanging. Prefer theunmount()returned byrender.- unhandledValues.forEach((value) => { - const { container } = render( - <SkeletonBase indexName={value} loadingImageUrl="fallback.jpg" /> - ) - - expect(screen.getByTestId('loading-spinner')).toBeInTheDocument() - container.remove() - }) + unhandledValues.forEach((value) => { + const { unmount } = render( + <SkeletonBase indexName={value} loadingImageUrl="fallback.jpg" /> + ) + expect(screen.getByTestId('loading-spinner')).toBeInTheDocument() + unmount() + })
326-331: Same here: replacecontainer.remove()withunmount().- specialNames.forEach((name) => { - const { container } = render(<SkeletonBase indexName={name} loadingImageUrl="test.jpg" />) - - expect(screen.getByTestId('loading-spinner')).toBeInTheDocument() - container.remove() - }) + specialNames.forEach((name) => { + const { unmount } = render(<SkeletonBase indexName={name} loadingImageUrl="test.jpg" />) + expect(screen.getByTestId('loading-spinner')).toBeInTheDocument() + unmount() + })
354-361: Select the rendered root deterministically.
querySelector('div')can match nested elements if structure changes.container.firstChildmore reliably points to the rendered root.- const mainContainer = container.querySelector('div') + const mainContainer = container.firstChild as HTMLElement
370-372: Remove redundant assertion.
toHaveClass('grid')already implies the element has a class attribute; the next check is redundant.- expect(gridContainer).toHaveClass('grid') - expect(gridContainer).toHaveAttribute('class') + expect(gridContainer).toHaveClass('grid')
408-422: Avoid duplicated coverage of user grid classes.These grid-class assertions are already covered earlier (Lines 96–111). Consider removing this duplicate to keep the suite lean.
- it('applies correct grid classes for users', () => { - render(<SkeletonBase indexName="users" loadingImageUrl="test.jpg" />) - - const userCards = screen.getAllByTestId('user-card-skeleton') - const gridContainer = userCards[0].parentElement - - expect(gridContainer).toHaveClass( - 'grid', - 'grid-cols-1', - 'gap-6', - 'sm:grid-cols-2', - 'lg:grid-cols-3', - 'xl:grid-cols-4' - ) - })
424-435: Preferunmount()andfirstChildin this looped structure test.
- Use
unmount()overcontainer.remove()for correct React cleanup.- Use
firstChildto target the root container deterministically.- skeletonTypes.forEach((type) => { - const { container } = render(<SkeletonBase indexName={type} loadingImageUrl="test.jpg" />) - - const mainContainer = container.querySelector('div') - expect(mainContainer).toHaveClass('flex', 'w-full', 'flex-col') - - container.remove() - }) + skeletonTypes.forEach((type) => { + const { container, unmount } = render( + <SkeletonBase indexName={type} loadingImageUrl="test.jpg" /> + ) + const mainContainer = container.firstChild as HTMLElement + expect(mainContainer).toHaveClass('flex', 'w-full', 'flex-col') + unmount() + })
📜 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 (1)
frontend/__tests__/unit/components/SkeletonBase.test.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 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.
🔇 Additional comments (2)
frontend/__tests__/unit/components/SkeletonBase.test.tsx (2)
55-473: Comprehensive, well-structured test suite — aligns with PR objectives.Broad coverage across switch logic, prop-driven configurations, edge cases, DOM/class assertions, and integration with mocked children. No conditional expects; follows repository testing patterns. Nice work.
3-3: Update import path to match the actual component file nameIt looks like no
SkeletonBaseorSkeletonsBasecomponent file was found in the repo, so the import in your test will almost certainly be broken.Please verify the real filename and its export, then update the import in
frontend/__tests__/unit/components/SkeletonBase.test.tsx:• Location: line 3
- import SkeletonBase from 'components/SkeletonsBase' + import SkeletonBase from 'components/SkeletonBase'If the component is actually named or located differently, adjust the path accordingly and confirm all relevant imports/tests are updated.
|
2 participants
Proposed change
Resolves #1913
Added comprehensive unit tests for the
SkeletonBasecomponent to ensure robust behavior and maintain code quality standards.📋 What Changed
SkeletonBase.test.tsxwith 35+ comprehensive test cases🧪 Test Coverage
Core Functionality
indexNamevaluesEdge Cases & Validation
Quality Assurance
🛠️ Technical Details
Checklist
make check-testlocally; all checks and tests passed.