Fix #1859: Added test to Modal Component#2016
Conversation
Summary by CodeRabbit
WalkthroughThe test suite for the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes found. Suggested reviewers
Note 🔌 MCP (Model Context Protocol) integration is now available in Early Access!Pro users can now connect to remote MCP servers under the Integrations page to get reviews and chat conversations that understand additional development context. ✨ 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 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: 2
🧹 Nitpick comments (3)
frontend/__tests__/unit/components/Modal.test.tsx (3)
3-4: Preferuser-eventoverfireEventfor realistic interaction simulation
@testing-library/user-eventprovides higher-level, more accurate user-interaction APIs. ReplacingfireEventwithuserEvent(e.g.user.click()) will better reflect real-world behaviour and remove the need for manual event propagation.
92-100: Redundant mock resets
jest.clearAllMocks()is called in bothbeforeEachandafterEach, doubling work with no benefit. Keeping it inafterEachis sufficient and avoids unnecessary noise.
192-197: Class-name queries couple tests to implementation detailsUsing
querySelector('.animate-scaleIn')& friends makes tests brittle against harmless style refactors. Prefer accessibility-oriented or semantic queries unless the class itself is the contract under test.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/__tests__/unit/components/Modal.test.tsx(3 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.
📚 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/Modal.test.tsx
🔇 Additional comments (2)
frontend/__tests__/unit/components/Modal.test.tsx (2)
78-90: Inconsistent camel-casing (onclick) may mask real API contractsEven if the production
DialogCompexpects a lowercaseonclick, the conventional React naming isonClick/onPress. Verify the source component and update for consistency to reduce cognitive load and typo risk.
150-154: Missing assertion that action button triggers callbackPresence is verified, but the click behaviour (critical for UX) isn’t. Add:
fireEvent.click(screen.getByTestId('action-button')) expect(mockOnClick).toHaveBeenCalledTimes(1)(After fixing the prop name to
onPress/onClick).
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
frontend/__tests__/unit/components/Modal.test.tsx (1)
188-189: Selector still tied to visible text “Close”A previous review flagged this fragility; the new accessibility test re-introduces it. Prefer
getByRole('button', { name: /close/i })orgetByLabelText('close-modal')to stay resilient if the button becomes icon-only.
🧹 Nitpick comments (3)
frontend/__tests__/unit/components/Modal.test.tsx (3)
23-33: Consider replacing the native<dialog>element in the mock
jsdomdoes not implement the HTML Dialog element API. While you only use it as a container, some future assertions (e.g.getByRole('dialog')) or code paths that expect the element’s methods (showModal,close) could break. A safer stub is a plain<div role="dialog">…</div>which keeps accessibility semantics without relying on unsupported DOM APIs.
92-99: Duplicate mock resets –clearAllMocksis called twice
beforeEachandafterEachboth invokejest.clearAllMocks(). The latter is redundant becausejest.restoreAllMocks()already resets spies and mocks. Removing the extra call avoids needless work and keeps the setup tidy.
150-154: Missing assertion that the action button fires its callbackThe test confirms that the action button renders but never verifies that
onPressis invoked when the user clicks it. Adding a simplefireEvent.click(screen.getByTestId('action-button'))followed byexpect(mockOnClick).toHaveBeenCalled()would cover the most important behavioural path.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/__tests__/unit/components/Modal.test.tsx(3 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 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.
📚 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/Modal.test.tsx
📚 Learning: 2025-07-12T17:12:25.807Z
Learnt from: Rajgupta36
PR: OWASP/Nest#1717
File: frontend/__tests__/unit/pages/ProgramDetails.test.tsx:35-0
Timestamp: 2025-07-12T17:12:25.807Z
Learning: In React applications, button elements should always have an explicit type attribute (type="button", type="submit", or type="reset") to prevent unintended form submission behavior, even when the button appears to be outside of a form context. The default type is "submit" which can cause unexpected behavior.
Applied to files:
frontend/__tests__/unit/components/Modal.test.tsx
|
* Added more test to Modol Component * fixed coderabbitai review --------- Co-authored-by: Kate Golovanova <kate@kgthreads.com>
2 participants
Resolves #1859
Description
Added Unit Tests for Modal Component
Added comprehensive test suite covering:
Checklist
make check-testlocally; all checks and tests passed.