#1841 Add tests for <LoadingSpinner> component #1847
Conversation
Summary by CodeRabbit
WalkthroughA new unit test file for the Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~4 minutes Assessment against linked issues
Suggested reviewers
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial 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). (2)
✨ 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. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
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: 8
🧹 Nitpick comments (5)
frontend/package.json (1)
129-134: Complete the package metadata fields.Several metadata fields are empty, which reduces the usefulness of the package information. Consider providing appropriate values for description, keywords, and author.
- "description": "", + "description": "Frontend application for OWASP Nest security testing platform", - "keywords": [], + "keywords": ["owasp", "security", "testing", "vulnerability", "frontend", "nextjs"], - "author": "", + "author": "OWASP Nest Team",frontend/__tests__/unit/components/LoadingSpinner.test.tsx (4)
17-21: Improve type safety in the invalid input test.Using
as anybypasses TypeScript's type checking. Consider using a more type-safe approach or adding a comment explaining why this specific invalid input scenario is being tested.it('handles invalid imageUrl input gracefully', () => { - render(<LoadingSpinner imageUrl={123 as any} />) + render(<LoadingSpinner imageUrl={123 as unknown as string} />) const images = screen.getAllByAltText('Loading indicator') expect(images.length).toBe(2) })Or add a comment explaining the test case:
it('handles invalid imageUrl input gracefully', () => { + // Testing component resilience when receiving unexpected prop types at runtime render(<LoadingSpinner imageUrl={123 as any} />) const images = screen.getAllByAltText('Loading indicator') expect(images.length).toBe(2) })
22-26: Remove redundant accessibility test.This test duplicates the logic from the first test. The accessibility aspect is already covered by using
getAllByAltTextin other tests.- it('has appropriate alt text for accessibility', () => { - render(<LoadingSpinner />) - const images = screen.getAllByAltText('Loading indicator') - expect(images.length).toBe(2) - })The alt text verification is already covered in other tests, making this one redundant.
48-52: Improve spinner container test specificity.The test uses chained optional operators and assumes a specific DOM structure. Consider making the test more robust and specific about what it's testing.
it('renders spinner container with correct styles', () => { render(<LoadingSpinner />) - const spinnerContainer = screen.getByAltText('Loading indicator').closest('div') - expect(spinnerContainer?.parentElement?.className).toContain('animate-fade-in-out') + // Test the specific container that should have the animation class + const container = screen.getByAltText('Loading indicator').closest('[class*="animate-fade-in-out"]') + expect(container).toBeInTheDocument() + expect(container).toHaveClass('animate-fade-in-out') })
54-59: Simplify the accessibility test.The test checks both alt text and element type, but the element type check is redundant since
getByAltTextalready ensures it's an accessible element.it('has appropriate alt text and accessibility image role', () => { render(<LoadingSpinner />) const image = screen.getByAltText('Loading indicator') expect(image).toBeInTheDocument() - expect(image.tagName.toLowerCase()).toBe('img') })The
getByAltTextquery already ensures the element has proper accessibility attributes, making the tag name check unnecessary.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
frontend/package-lock.jsonis excluded by!**/package-lock.jsonpackage-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (3)
frontend/__tests__/unit/components/LoadingSpinner.test.tsx(1 hunks)frontend/package.json(3 hunks)package.json(1 hunks)
🔇 Additional comments (3)
frontend/package.json (2)
16-16: LGTM: Simplified test script addition.The new
"test": "jest"script provides a convenient way to run tests without the TypeScript compilation step, which is useful for quick test runs.
75-75: LGTM: Correct dependency categorization.Moving
@testing-library/user-eventtodevDependenciesis the correct approach since it's only used during testing and not in production code.frontend/__tests__/unit/components/LoadingSpinner.test.tsx (1)
4-8: LGTM: Proper Next.js Image component mocking.The mock implementation correctly replaces the Next.js Image component with a standard img element to avoid SSR-related issues during testing. This is a best practice for testing Next.js components.
package.json
Outdated
| "name": "nest-owasp", | ||
| "version": "1.0.0", | ||
| "description": "<div align=\"center\">", | ||
| "main": "index.js", |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify the main entry point exists.
The main field points to "index.js" but this file may not exist in the root directory. Consider updating this to the correct entry point or removing it if not applicable.
🏁 Script executed:
#!/bin/bash
# Check if index.js exists in the root directory
if [ -f "index.js" ]; then
echo "index.js exists"
else
echo "index.js does not exist - main field may be incorrect"
fiLength of output: 138
Fix missing main entry in package.json
We ran a check ([ -f index.js ]) and confirmed that index.js does not exist at the project root. This will break module resolution when users import your package.
Please update the main field in package.json to point to your actual entry file or add the missing index.js. For example:
- If your build outputs to
dist/index.js:
• Change"main": "index.js"to"main": "dist/index.js". - If your source entry lives under
src/:
• Either add a root-levelindex.jsthat re-exports fromsrc/
• Or adjust"main"to"src/index.js"(and ensure your publishing setup includes it).
🤖 Prompt for AI Agents
In package.json at line 5, the "main" field is set to "index.js" but this file
does not exist at the project root, causing module resolution failures. To fix
this, update the "main" field to point to the actual entry file path, such as
"dist/index.js" if your build outputs there, or "src/index.js" if your source
entry is under src/. Alternatively, add a root-level index.js that re-exports
from your source entry. Ensure the specified entry file exists and is included
in your package.
|
@ParthGupta1304 Could you resolve conflicts and run |
|
Sure Kasya, I will work on it |
|
I have resolved the conflicts and ran the make check locally |
kasya
left a comment
kasya
left a comment
There was a problem hiding this comment.
hey @ParthGupta1304 Left some questions! Also there are conflicting files in this PR.
frontend/package.json
Outdated
| "@next/third-parties": "^15.4.3", | ||
| "@sentry/nextjs": "^9.41.0", | ||
| "@testing-library/user-event": "^14.6.1", | ||
| "@testing-library/dom": "^10.4.0", |
There was a problem hiding this comment.
Why did you make these updates? 🤔
There was a problem hiding this comment.
Hi kasya, I made these changes as these dependencies were required to test the changes
package.json
Outdated
There was a problem hiding this comment.
Same question here. The task was to only create tests for the component 🤔
…while keeping testing dependencies
|
Hi code reviewers, I have tried my best to resolve the conflict, but I think this is the best I could do for now. I would really appreciate any suggestions and guidance since I am new to open source :). |
arkid15r
left a comment
arkid15r
left a comment
There was a problem hiding this comment.
Please revert all unrelated changes. Keep frontend/tests/unit/components/LoadingSpinner.test.tsx file only.
- Reverted frontend/package.json to main branch version - Reverted frontend/src/components/LoadingSpinner.tsx to main branch version - Removed all extra files (index.js, root package.json, package-lock files) - Kept comprehensive LoadingSpinner.test.tsx with 8 test cases - Project now matches main branch with only test file addition
|
Hi code managers, I have reverted all the changes except the test file, and I can see no conflicts in the PR. |
|
3 participants
Proposed change
Added a test file for the Loading Spinner React component.
Resolves #1841
🧠 Overview
This PR introduces a complete set of unit tests for the LoadingSpinner component to improve the project's test coverage and reliability. The tests were written using Jest and React Testing Library in accordance with the Essential Test Coverage Checklist provided in the issue.
🔍 Test Scenarios Covered
Renders without crashing with minimal props
Handles invalid imageUrl input gracefully
Renders default fallback images for light/dark themes
Accepts and renders custom imageUrl when provided
Applies correct Tailwind CSS classes for theme modes
Ensures alt text for accessibility
Checks for expected DOM structure and animation class presence
🙏 Acknowledgements
Thanks to the OWASP/Nest team for the well-structured codebase and clear contribution guidelines. Happy to iterate if feedback is provided!
Checklist
make check-testlocally; all checks and tests passed.