imporove test covarage to 100 percent for EntityActions.tsx #2898
Conversation
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughAdds a new comprehensive unit test suite for the EntityActions component that exercises navigation, dropdown open/close behavior, action visibility across entity kinds/statuses, status-change actions, click-outside behavior, event propagation, and edge cases. No production code changes. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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: 2
🧹 Nitpick comments (2)
frontend/__tests__/unit/components/EntityActions.test.tsx (2)
20-119: Navigation and “missingmoduleKey” coverage is solid; selectors may be slightly brittle.
UsinggetByText('Edit')/getByText('Add Module')can become ambiguous if those labels appear elsewhere in the rendered tree. Consider scoping to the opened menu (e.g.,within(menu).getByRole('menuitem', { name: 'Edit' })) if the component exposes roles/structure.
121-361: Status transition assertions look correct and cover visibility matrix well.
Minor maintainability note: these blocks repeat the same “open menu → click item → assert aria-expanded false” flow; a small helper would reduce repetition and make future label changes cheaper.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/__tests__/unit/components/EntityActions.test.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-07-12T17:36:57.255Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 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/EntityActions.test.tsx
📚 Learning: 2025-11-17T17:30:42.139Z
Learnt from: anurag2787
Repo: OWASP/Nest PR: 2671
File: frontend/__tests__/unit/components/MultiSearch.test.tsx:169-171
Timestamp: 2025-11-17T17:30:42.139Z
Learning: In the OWASP/Nest frontend tests (PR #2671 context), wrapper functions like `expectChaptersCountEqualsThree` that simply call another helper with a fixed parameter (e.g., `expectChaptersCountEquals(3)`) are intentionally used to avoid arrow function callbacks in `waitFor` calls. This pattern prevents adding nesting depth that would trigger rule typescript:S2004. Example: `await waitFor(expectChaptersCountEqualsThree)` avoids the extra nesting from `await waitFor(() => expectChaptersCountEquals(3))`.
Applied to files:
frontend/__tests__/unit/components/EntityActions.test.tsx
🧬 Code graph analysis (1)
frontend/__tests__/unit/components/EntityActions.test.tsx (1)
backend/apps/mentorship/api/internal/nodes/enum.py (1)
ProgramStatusEnum(22-27)
🔇 Additional comments (4)
frontend/__tests__/unit/components/EntityActions.test.tsx (4)
1-10: Mock placement and shape look fine; consider making the mock more robust to Jest/ESM behavior.
If this repo is running Jest in ESM mode,jest.mock()hoisting is not guaranteed; the safer pattern is to keep the mock at the very top (or use ESM mocking APIs). If you’ve confirmed this repo uses the hoisted CJS/Babel flow, you can ignore this.+// Mock next/navigation (keep mocks above component imports in ESM-sensitive setups) +jest.mock('next/navigation', () => ({ + useRouter: jest.fn(), +})) + import { render, screen, fireEvent, waitFor } from '@testing-library/react' import { useRouter } from 'next/navigation' import { ProgramStatusEnum } from 'types/__generated__/graphql' import EntityActions from 'components/EntityActions' -// Mock next/navigation const mockPush = jest.fn() -jest.mock('next/navigation', () => ({ - useRouter: jest.fn(), -}))
13-18: Good test isolation viajest.clearAllMocks()+ consistentuseRouterreturn.
364-375: Avoid arrow callback insidewaitForif this repo enforces typescript:S2004.
This repo has an established pattern to avoid extra nesting inwaitForcallbacks. (Based on learnings, PR #2671.)+ const expectDropdownClosed = (button: HTMLElement) => () => + expect(button).toHaveAttribute('aria-expanded', 'false') + it('closes dropdown when clicking outside', async () => { render(<EntityActions type="program" programKey="test-program" />) const button = screen.getByTestId('program-actions-button') fireEvent.click(button) expect(button).toHaveAttribute('aria-expanded', 'true') fireEvent.mouseDown(document.body) - await waitFor(() => { - expect(button).toHaveAttribute('aria-expanded', 'false') - }) + await waitFor(expectDropdownClosed(button)) })
399-482: Edge-case + propagation coverage is useful and scoped appropriately.
These tests align well with component-level responsibilities (state, navigation, and event behavior).
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/__tests__/unit/components/EntityActions.test.tsx (1)
13-18: Non-standard semicolon formatting in beforeEach block.The semicolon placement on line 15 with extra indentation and a space before the parenthesis is unconventional. Standard ASI protection places the semicolon at the start of the line without extra indentation.
beforeEach(() => { jest.clearAllMocks() - ; (useRouter as jest.Mock).mockReturnValue({ + ;(useRouter as jest.Mock).mockReturnValue({ push: mockPush, }) })
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/__tests__/unit/components/EntityActions.test.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-11-17T16:47:05.578Z
Learnt from: anurag2787
Repo: OWASP/Nest PR: 2671
File: frontend/__tests__/unit/components/MultiSearch.test.tsx:427-427
Timestamp: 2025-11-17T16:47:05.578Z
Learning: In the frontend test files for the OWASP/Nest repository, `expect(true).toBe(true)` no-op assertions may be intentionally added as workarounds when ESLint rule jest/expect-expect doesn't detect expectations inside helper functions or waitFor callbacks. These can be resolved by configuring the ESLint rule's assertFunctionNames option to recognize custom assertion helpers instead of flagging them as redundant.
Applied to files:
frontend/__tests__/unit/components/EntityActions.test.tsx
📚 Learning: 2025-11-17T17:30:42.139Z
Learnt from: anurag2787
Repo: OWASP/Nest PR: 2671
File: frontend/__tests__/unit/components/MultiSearch.test.tsx:169-171
Timestamp: 2025-11-17T17:30:42.139Z
Learning: In the OWASP/Nest frontend tests (PR #2671 context), wrapper functions like `expectChaptersCountEqualsThree` that simply call another helper with a fixed parameter (e.g., `expectChaptersCountEquals(3)`) are intentionally used to avoid arrow function callbacks in `waitFor` calls. This pattern prevents adding nesting depth that would trigger rule typescript:S2004. Example: `await waitFor(expectChaptersCountEqualsThree)` avoids the extra nesting from `await waitFor(() => expectChaptersCountEquals(3))`.
Applied to files:
frontend/__tests__/unit/components/EntityActions.test.tsx
📚 Learning: 2025-08-30T12:52:32.214Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2155
File: frontend/graphql-codegen.ts:52-52
Timestamp: 2025-08-30T12:52:32.214Z
Learning: When using GraphQL Codegen with async configuration that needs to fetch data (like CSRF tokens), export the async function directly instead of using top-level await: `export default async function()` rather than `export default await function()`.
Applied to files:
frontend/__tests__/unit/components/EntityActions.test.tsx
📚 Learning: 2025-07-12T17:36:57.255Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 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/EntityActions.test.tsx
🧬 Code graph analysis (1)
frontend/__tests__/unit/components/EntityActions.test.tsx (1)
backend/apps/mentorship/api/internal/nodes/enum.py (1)
ProgramStatusEnum(22-27)
🔇 Additional comments (9)
frontend/__tests__/unit/components/EntityActions.test.tsx (9)
20-43: LGTM!Well-structured tests covering navigation and dropdown closure for the "Add Module" action. Good use of
aria-expandedassertions to verify UI state.
45-81: LGTM!Good coverage of the Edit Module functionality including the important edge case where
moduleKeyis missing. The test at line 59-68 verifies the guard clause prevents navigation.
83-119: LGTM!Consistent test structure mirroring the Edit Module tests. Good coverage of View Issues navigation behavior.
121-184: LGTM!Thorough testing of the Publish status transition. Good coverage of visibility conditions and the
setStatuscallback invocation.
186-282: LGTM!Excellent coverage of the Unpublish action including transitions from both
PUBLISHEDandCOMPLETEDstates back toDRAFT. The visibility tests properly verify the action is hidden when already inDRAFTstatus.
284-361: LGTM!Complete coverage of the "Mark as Completed" action with correct visibility assertions for all three status states.
363-399: LGTM!Good coverage of click-outside behavior and event listener cleanup. The
removeEventListenerassertion correctly uses partial matching to handle potential third argument variations.
401-453: LGTM!Solid defensive testing for edge cases. The
expect(...).not.toThrow()assertions appropriately verify graceful handling when optional props are undefined.
455-485: LGTM!Event propagation tests correctly verify that clicks on the action button and menu items don't bubble up to parent handlers. The empty
onKeyDownhandlers satisfy accessibility requirements forrole="button"elements.
|
1 participant
Proposed change
Resolves #2850
Improved test covarage to 100 percent for EntityActions.tsx
Checklist
make check-testlocally and all tests passed