Improve frontend test coverage above 90% and add missing test files#3923
Improve frontend test coverage above 90% and add missing test files#3923kasya merged 7 commits intoOWASP:mainfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
Summary by CodeRabbit
WalkthroughAdds extensive unit tests covering null/undefined/fallback cases, keyboard interactions, navigation and accessibility; introduces small source edits to Pagination, Milestones, SecondaryCard, and proxy exports; and updates footer tests to exercise empty-href fallback and a mutable env mock. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts (beta)
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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
10 issues found across 33 files
Confidence score: 4/5
- Most issues are test-quality gaps (misleading assertions or missing coverage) in
frontend/__tests__/unit/pages/ProgramDetailsMentorship.test.tsx, which raises mild uncertainty but doesn’t indicate runtime regressions. - Several tests claim to cover error or fallback behavior without asserting it (e.g.,
ProgramDetailsMentorship.test.tsx,ChapterDetails.test.tsx,Organization.test.tsx,MenteeProfilePage.test.tsx), so the safety risk is low and localized to test fidelity. - This PR looks safe to merge, but tightening the assertions would better align tests with intended behavior and reduce false confidence.
- Pay close attention to
frontend/__tests__/unit/pages/ProgramDetailsMentorship.test.tsx,frontend/__tests__/unit/pages/ChapterDetails.test.tsx,frontend/__tests__/unit/pages/Organization.test.tsx,frontend/__tests__/unit/pages/MenteeProfilePage.test.tsx,frontend/__tests__/unit/pages/SnapshotDetails.test.tsx,frontend/__tests__/unit/components/SecondaryCard.test.tsx,frontend/__tests__/unit/components/MultiSearch.test.tsx- misleading or weak test assertions.
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="frontend/__tests__/unit/pages/ChapterDetails.test.tsx">
<violation number="1" location="frontend/__tests__/unit/pages/ChapterDetails.test.tsx:169">
P2: Weak assertion: this test only verifies the component doesn't crash, but doesn't assert that the null `suggestedLocation` is actually handled (i.e., that `'Test City, Test Country'` is absent from the DOM). Add a negative assertion to verify the expected behavior, e.g.:
```ts
expect(screen.queryByText('Test City, Test Country')).not.toBeInTheDocument()
The same issue applies to the region: null and combined tests below — they should similarly assert that 'Test Region' (and/or 'Test City, Test Country') is not rendered.
If the goal is to cover the = {} default parameter in the component signature, consider removing that default from the component instead (React always passes an object for props via JSX), which would eliminate the untested branch without needing this workaround.
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
frontend/__tests__/unit/pages/ProgramDetailsMentorship.test.tsx
Outdated
Show resolved
Hide resolved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/__tests__/unit/components/Footer.test.tsx (1)
369-377:⚠️ Potential issue | 🟡 MinorTest name doesn't match what's being asserted.
The test is named "applies correct section content classes for collapsed/expanded states" but only verifies
aria-expanded='true'after a click — it doesn't assert any CSS classes for the collapsed or expanded content. Either rename the test to reflect what it actually checks, or add assertions for the section content's class changes.
🤖 Fix all issues with AI agents
In `@frontend/__tests__/unit/components/Footer.test.tsx`:
- Around line 150-156: The loop over regularLinks embeds the fallback logic in
the assertion (expect(...).toHaveAttribute('href', link.href || '/')), which
reduces clarity; instead compute an explicit expectedHref before asserting so
the test checks a concrete value. Update the test loop that references
regularLinks and link.href to set a local expectedHref (e.g., const expectedHref
= link.href === '' ? '/' : link.href) then assert
expect(linkElement).toHaveAttribute('href', expectedHref); keep the existing
target assertion and use the same screen.getByRole('link', { name: link.text })
lookup.
In `@frontend/__tests__/unit/components/MultiSearch.test.tsx`:
- Around line 588-605: The test title says "Enter/Space" but only triggers
Enter; either add a Space key assertion or rename the test to mention only
Enter. To add Space: after focusing suggestionBtn (from MultiSearchBar test),
send a second keyboard event via user.keyboard('{Space}') (or in a separate it
block) and assert mockPush was called with '/chapters/test-chapter' (resetting
or checking call counts as needed). If Space isn't supported, simply rename the
test string for the it(...) that references suggestionBtn and
user.keyboard('{Enter}') to remove "Space". Use the existing identifiers
suggestionBtn, user.keyboard, mockPush, MultiSearchBar and defaultProps to
locate and modify the test.
In `@frontend/__tests__/unit/pages/MenteeProfilePage.test.tsx`:
- Around line 527-545: The test never triggers the onSelectionChange
empty-selection branch; update the test's Select mock to expose a way to invoke
onSelectionChange (e.g., render a hidden/test-only button or element inside the
mock that calls props.onSelectionChange(new Set()) or new Set(['']) when
clicked), then in the test render MenteeProfilePage, locate and click that
trigger to simulate an empty selection, and assert that the displayed issues
remain unchanged; target the mocked Select component and the onSelectionChange
handler in the MenteeProfilePage test so the empty Set branch is exercised.
In `@frontend/__tests__/unit/pages/ModuleDetails.test.tsx`:
- Around line 121-167: Update the three test case names in
ModuleDetails.test.tsx so they accurately reflect that the mocked fields are set
to null (not undefined): change "renders module without admins (uses undefined
fallback)" to "renders module without admins (uses null fallback)", change
"renders module without domains (uses undefined fallback)" to "renders module
without domains (uses null fallback)", and change "renders module without tags
(uses undefined fallback)" to "renders module without tags (uses null
fallback)"; these are the it(...) blocks that set admins: null, domains: null,
and tags: null via mockUseQuery using mockModuleData.
In `@frontend/__tests__/unit/pages/ProgramDetailsMentorship.test.tsx`:
- Around line 427-453: The test claims to assert that handleAppError is called
when the mutation fails but never mocks or spies on handleAppError; either mock
handleAppError from the app/global-error module and assert it was called with
the rejection (e.g., jest.mock('app/global-error', () => ({ handleAppError:
jest.fn() })) and expect(handleAppError).toHaveBeenCalledWith(mockError)), or
change the test name/expectations to only assert the observable behavior (that
mockUpdateProgram was called and failed). Locate the failing flow around
capturedSetStatus, mockUpdateProgram and the test block in
ProgramDetailsMentorship.test.tsx to apply the mock or rename the test
accordingly.
In `@frontend/__tests__/unit/pages/ProjectHealthDashboardMetricsDetails.test.tsx`:
- Around line 158-182: The mock used in the test for ProjectHealthMetricsDetails
omits the createdAt field and the assertion doesn't verify null-handling; update
the test to include a createdAt value on
mockProjectsDashboardMetricsDetailsData.project.healthMetricsList[0] (matching
the GraphQL schema) or explicitly set it to null only if you intend to test the
fallback, then assert the component's fallback rendering (for example check for
the fallback text like "N/A" or the empty label used by
MetricsCard/HealthMetrics) instead of just expecting 'Stars'; if this
null-createdAt scenario is unrealistic for production, consider removing the
defensive test and focusing on more realistic edge cases.
🧹 Nitpick comments (26)
frontend/__tests__/unit/components/HealthMetrics.test.tsx (1)
122-128: Empty array test may not reflect real usage.Based on learnings, the parent
DetailsCardcomponent guards rendering ofHealthMetricswith ahealthMetricsData.length > 0check, sodata={[]}shouldn't occur in practice. This test isn't harmful, but be aware it's testing a path that's unreachable in the actual app. Based on learnings from PR#1633: the DetailsCard component ensures HealthMetrics is only rendered when data has at least one element.frontend/__tests__/unit/components/SecondaryCard.test.tsx (1)
98-102: Avoid calling React components directly as functions in tests.
SecondaryCard()bypasses React's reconciler (hooks, context, error boundaries won't work correctly). The default parameter= {}only triggers when the argument is literallyundefined, which never happens through normal JSX usage —<SecondaryCard />always passes{}. You're covering a code path that's unreachable in production.If the sole goal is to hit the
= {}default for coverage metrics, consider adding a/* istanbul ignore next */or equivalent hint on the default parameter instead. Alternatively, if you still want a test, prefer JSX:Suggested change
- it('renders correctly when called directly as a function (default props)', () => { - // This covers the default parameter `= {}` in the component signature - const { container } = render(SecondaryCard()) - expect(container.querySelector('.mb-8')).toBeInTheDocument() - })The existing test on line 24–27 (
renders successfully with no props at all) already validates this behavior via JSX.frontend/__tests__/unit/components/SingleModuleCard.test.tsx (1)
326-367: New edge-case tests look good overall.The three added tests appropriately cover contributor name fallback, missing pathname segments, and empty enum values. A few minor observations:
Line 353-357: The "handles pathname without program key gracefully" test only asserts the component doesn't crash. Consider also verifying the generated module link
hrefto confirm the path construction degrades as expected (e.g., doesn't produce a malformed URL like//modules/test-module). This would make the test more meaningful since the PR objective includes path construction changes.Line 366:
toBeGreaterThan(0)is a loose assertion. If the component renders multiple "Unknown" texts unexpectedly, this wouldn't catch it. A precise count (toHaveLength(1)) would be more robust, assuming onlyexperienceLevelwas emptied.Neither of these is blocking — the tests are functional and achieve coverage goals.
frontend/__tests__/unit/components/IssuesTable.test.tsx (1)
138-140: ClearmockPushinbeforeEachto prevent cross-test leakage.Only
LabelListis cleared here, butmockPushis module-scoped and only manually cleared in one test (line 414). If any future or reordered test triggersrouter.push, assertions onmockPushin the "Click Handlers" block could produce false positives or negatives.Proposed fix
beforeEach(() => { + mockPush.mockClear() jest.mocked(LabelList).mockClear() })frontend/__tests__/unit/pages/RepositoryDetails.test.tsx (1)
262-323: Consider strengthening assertions for null-data tests.The three tests for null
issues,recentMilestones, andreleasesonly assert that'Test Repo'renders, which effectively just verifies the component doesn't crash. They don't confirm that the corresponding UI sections are correctly hidden or empty. For example, you could additionally assert that milestone titles or issue titles from the default mock data are not present:await waitFor(() => { expect(screen.getByText('Test Repo')).toBeInTheDocument() }) + // Verify the section that depends on null data is absent + expect(screen.queryByText('Recent Milestones')).not.toBeInTheDocument()This would make the tests more meaningful by verifying graceful degradation rather than just no-crash behavior.
frontend/__tests__/unit/components/ScrollToTop.test.tsx (1)
91-119:setTimeoutspy counts may be fragile due to React internals also callingsetTimeoutunder fake timers.When
jest.useFakeTimers()is active, React's scheduler can fall back tosetTimeout, which would inflate your spy's call count and cause flaky failures. Instead of asserting exacttoHaveBeenCalledTimes, consider filtering by the throttle delay argument:♻️ Suggested improvement
- expect(setTimeoutSpy).toHaveBeenCalledTimes(1) + const throttleCalls = () => + setTimeoutSpy.mock.calls.filter(([, delay]) => delay === 100).length + + expect(throttleCalls()).toBe(1) // Immediate second scroll event - should NOT set a new timeout due to throttling globalThis.dispatchEvent(new Event('scroll')) - expect(setTimeoutSpy).toHaveBeenCalledTimes(1) // Still 1 + expect(throttleCalls()).toBe(1) // Still 1 // Fast forward time to clear the timeout act(() => { jest.advanceTimersByTime(100) }) // Third scroll event - should set a new timeout now globalThis.dispatchEvent(new Event('scroll')) - expect(setTimeoutSpy).toHaveBeenCalledTimes(2) + expect(throttleCalls()).toBe(2)frontend/__tests__/unit/pages/Home.test.tsx (2)
343-380: Test name promises icon verification but only asserts text presence.The test is named "renders project icons correctly for different types" but only checks that each project's name text appears. It never asserts which icon (
FaCode,FaBook,FaTag,FaFileCode) is rendered for each type. This likely still achieves the coverage goal (exercising allgetProjectIconbranches), but the test name overpromises.Consider either:
- Renaming to something like
"renders projects for all type variants", or- Adding assertions on the rendered icon elements (e.g., via
data-testidor SVGrole/title attributes) to truly verify icon correctness.
382-413: Null-stats assertions are weak — consider verifying the rendered stat values.Line 410–411 only checks that the
"Active Projects"header text exists, which would pass even without the null-stat mock data (the header renders regardless). This doesn't confirm the component handlesnullstats gracefully vs. crashing or showing wrong values.Consider asserting what the component actually renders for null stats (e.g.,
"0+","0", or absence of stat values) to make the test meaningful beyond just "it didn't throw."frontend/__tests__/unit/pages/SnapshotDetails.test.tsx (3)
218-264: Tests exercise fallback paths but don't assert fallback behavior.The "without level" and "without summary" tests only verify the component doesn't crash (by checking the project name renders). They don't assert the effect of the missing field — e.g., that no level badge is displayed or that no summary text appears. This means a regression where
undefinedrenders as literal text"undefined"would go undetected.Consider adding negative assertions:
await waitFor(() => { expect(screen.getByText('OWASP Nest')).toBeInTheDocument() }) + expect(screen.queryByText('INCUBATOR')).not.toBeInTheDocument()Similarly for the summary test, assert that the summary text from the mock data is absent.
266-312: Same observation: chapter fallback tests lack negative assertions.The "without relatedUrls" and "without summary" chapter tests only confirm the chapter name renders. Adding assertions that verify the absent data isn't displayed (e.g.,
queryByTextfor the original summary text) would strengthen these tests.
192-402: Consider a helper to reduce repetitive mock setup.All seven new tests repeat the same boilerplate: spread
mockSnapshotDetailsData.snapshot, override one entity array, and zero out the other two. A small helper would reduce duplication and make tests easier to maintain:Example helper
function mockSnapshotWith(overrides: Partial<typeof mockSnapshotDetailsData.snapshot>) { ;(useQuery as unknown as jest.Mock).mockReturnValue({ data: { snapshot: { ...mockSnapshotDetailsData.snapshot, newChapters: [], newProjects: [], newReleases: [], ...overrides, }, }, error: null, loading: false, }) }Then each test becomes:
mockSnapshotWith({ newProjects: [projectWithNullKey] })frontend/__tests__/unit/components/skeletons/sharedSkeletons.test.tsx (2)
142-153: Remove stale reasoning comments and the redundant{ exact: false }option.The block of comments (lines 145–152) reads like working notes rather than test documentation. Also,
{ exact: false }is the default behavior oftoHaveClass, so it's redundant.✏️ Suggested cleanup
it('renders without rounded class when false', () => { render(<SectionHeader titleSkeletonWidth="w-20" rounded={false} />) const skeletons = screen.getAllByTestId('skeleton') - // The first skeleton has 'w-5' in base class, check it doesn't have 'rounded' explicitly added by the prop logic - // Actually code is: `h-5 w-5 ${rounded ? 'rounded' : ''}`. - // If rounded is false, it's empty string. - // But note that skeletons might have rounded-lg or similar from base styles if Skeleton component had them, - // but here we are mocking Skeleton. - // We check for exact class match or presence. - expect(skeletons[0]).not.toHaveClass('rounded', { exact: false }) - // Wait, 'rounded' might be a partial match if we aren't careful, but here expecting class list to NOT contain the specific class 'rounded'. + expect(skeletons[0]).not.toHaveClass('rounded') })
28-39: Hardcoded skeleton counts are brittle — consider a comment or constant to ease future maintenance.Tests like
expect(skeletons).toHaveLength(17)(line 57) andtoHaveLength(54)(line 130) embed the formula2 + N * 5and2 * (2 + 5 * 5)respectively. The inline comments explaining the math are helpful, but if the underlying component's structure changes (e.g., an extra skeleton is added toItemCardSkeleton), multiple tests will fail with opaque count mismatches.This is a known trade-off with structural skeleton tests and fine to keep as-is for now, but worth noting.
Also applies to: 42-58, 117-131
frontend/__tests__/unit/components/LogoCarousel.test.tsx (4)
446-494: Coverage edge-case tests look reasonable overall.The tests exercise the key-generation fallback chain (
id→url→name→'sponsor') and thename || 'Sponsor'text fallback path, which is appropriate for branch coverage. Since React keys aren't observable in the DOM, the count-based assertions are the practical approach here.One small observation: the first three tests (lines 447–481) are nearly identical in their assertions — they all just verify that 4
sponsor-linkelements are rendered. The real value they add is triggering different branches in thekeyBaseternary, but a reader can't tell from the assertions alone whether the correct branch was hit. Consider adding a brief inline comment in each test (e.g.,// Exercises: sponsor.url || …) to make it immediately clear why each variant exists, since the assertions themselves don't distinguish the cases.
349-356: Duplicate test: "renders image alt text correctly" is identical to "renders sponsor names correctly in alt attributes" (lines 341–347).Both tests render the same sponsors and assert the same
altattributes on the same elements. One of these can be removed without losing any coverage.
233-275: Test description "duplicates innerHTML on mount" is misleading.The component doesn't use
innerHTMLor ref-based DOM cloning — it renders the sponsor list twice via JSX (keySuffix 'a'and'b'). The test name and thedescribeblock title ("State Changes and Internal Logic") suggest implementation details that don't match the actual component. Consider renaming to something like"renders each sponsor twice and updates on rerender".
438-438: Assertion expects 202 links for 100 sponsors — verify this is intentional.100 sponsors × 2 (duplicate lists) = 200 sponsor links + 2 footer links = 202. The math checks out, but this relies on the footer links also matching
data-testid="sponsor-link"due to thenext/linkmock. This coupling is fragile — if the footer links ever change or the mock is refined, this test breaks for the wrong reason.frontend/__tests__/unit/components/forms/shared/FormDateInput.test.tsx (2)
4-10: No-op mock — remove or justify.This mock re-exports the actual module unchanged, so it has no effect. It appears to be leftover boilerplate. Removing it reduces noise.
Suggested fix
-// Mock styles from imported component -jest.mock('components/forms/shared/FormDateInput', () => { - const originalModule = jest.requireActual('components/forms/shared/FormDateInput') - return { - ...originalModule, - } -}) - // Mock `@heroui/react` Input component
144-159: ExportCOMMON_INPUT_CLASS_NAMESand use it in the test to avoid brittleness.The hardcoded class-name values duplicate the
COMMON_INPUT_CLASS_NAMESconstant. If those values are updated, this test fails even though the component still works correctly.First, export the constant from FormDateInput.tsx, then import and use it in the test:
Suggested fix
In
frontend/src/components/forms/shared/FormDateInput.tsx:-const COMMON_INPUT_CLASS_NAMES = { +export const COMMON_INPUT_CLASS_NAMES = {In
frontend/__tests__/unit/components/forms/shared/FormDateInput.test.tsx:import { render, screen, fireEvent } from '@testing-library/react' -import { FormDateInput } from 'components/forms/shared/FormDateInput' +import { FormDateInput, COMMON_INPUT_CLASS_NAMES } from 'components/forms/shared/FormDateInput' // Mock styles from imported component jest.mock('components/forms/shared/FormDateInput', () => { @@ -143,14 +143,7 @@ describe('FormDateInput Component', () => { it('passes common class names to Input', () => { render(<FormDateInput {...defaultProps} />) const input = screen.getByLabelText('Test Date') const classNames = JSON.parse(input.getAttribute('data-class-names') || '{}') - expect(classNames).toEqual( - expect.objectContaining({ - base: 'w-full min-w-0', - label: 'text-sm font-semibold text-gray-600 dark:text-gray-300', - input: 'text-gray-800 dark:text-gray-200', - inputWrapper: 'bg-gray-50 dark:bg-gray-800', - helperWrapper: 'min-w-0 max-w-full w-full', - errorMessage: 'break-words whitespace-normal max-w-full w-full', - }) - ) + expect(classNames).toEqual(COMMON_INPUT_CLASS_NAMES)frontend/__tests__/unit/pages/Contribute.test.tsx (1)
247-315: Consider consolidatingundefinedandnullvariant tests.The four new tests follow a near-identical pattern, differing only in whether
objectID/summaryisundefinedvsnull. While both paths are worth exercising, you could usetest.eachto reduce duplication.Not a blocker — fine to keep as-is if you prefer explicit test names.
♻️ Example using test.each
- test('renders issue card without objectID (uses url fallback)', async () => { - ... - }) - - test('renders issue card with null objectID (uses url fallback)', async () => { - ... - }) + test.each([undefined, null])('renders issue card when objectID is %s (uses url fallback)', async (objectID) => { + const issue = { + ...mockContributeData.issues[0], + objectID, + url: 'https://github.com/test/issue/1', + } + ;(fetchAlgoliaData as jest.Mock).mockResolvedValue({ + hits: [issue], + totalPages: 1, + }) + + render(<ContributePage />) + + await waitFor(() => { + expect(screen.getByText('Contribution 1')).toBeInTheDocument() + }) + })frontend/__tests__/unit/pages/ProgramDetailsMentorship.test.tsx (4)
188-203: Misleading test name — this doesn't test invalid status or error toast.This test clicks the "Publish" button (which sends a valid
'PUBLISHED'status via the mock) and only asserts the mutation was called. It doesn't pass an invalid status or verify any error toast. It's effectively a duplicate of the test at Line 165 ("successfully updates status from Draft to Published").The actual invalid-status test is at Line 328 using
capturedSetStatus('INVALID_STATUS'). Consider removing this test to avoid confusion.
281-302: Test is a no-op — the comments themselves acknowledge it can't test what it claims.Lines 294–301 explicitly state the limitation. The test ends up only asserting the publish button exists — it doesn't exercise invalid status or toast logic. The actual meaningful test for this scenario lives at Line 328. This test should be removed.
205-219: Duplicate of "renders program details correctly for a non-admin" (Line 123).Both tests set a non-admin user and assert the actions button is absent. The test name claims "shows permission denied toast" but no toast assertion is made. The real permission-denied-toast test is at Line 358 using
capturedSetStatus. Consider removing this to reduce noise.
11-11: Type annotation doesn't reflect async nature of the captured callback.
capturedSetStatusis typed as(status: string) => void, but Line 404 awaits it, implying the real callback is async. Consider typing it as(status: string) => void | Promise<void>for accuracy.Proposed fix
-let capturedSetStatus: ((status: string) => void) | null = null +let capturedSetStatus: ((status: string) => void | Promise<void>) | null = nullfrontend/__tests__/unit/pages/ProjectHealthDashboardMetricsDetails.test.tsx (1)
211-258: Assertions only verify section headers, not how null values are rendered.This test is useful as a smoke test (component doesn't crash with all-null counts), but the assertions at Lines 255-256 only check for static section headers that would appear regardless of the data. Consider adding at least one assertion on the rendered fallback for a null metric (e.g., checking that
0,N/A, or an empty cell appears where a count would normally be) to confirm the null-handling logic is actually correct.frontend/__tests__/unit/components/Footer.test.tsx (1)
105-116: RedundantclearAllMocksin bothbeforeEachandafterEach.
clearAllMocks()inbeforeEachalready guarantees a clean state before each test. The duplicate call inafterEach(line 114) is unnecessary. Consider keeping only thebeforeEachcleanup (withrestoreAllMocksremaining inafterEachif needed).♻️ Suggested simplification
beforeEach(() => { jest.clearAllMocks() mockEnv = { ENVIRONMENT: 'production', RELEASE_VERSION: '1.2.3', } }) afterEach(() => { - jest.clearAllMocks() jest.restoreAllMocks() })
frontend/__tests__/unit/pages/ProjectHealthDashboardMetricsDetails.test.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
3 issues found across 15 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="frontend/__tests__/unit/pages/ModuleDetails.test.tsx">
<violation number="1" location="frontend/__tests__/unit/pages/ModuleDetails.test.tsx:121">
P3: The renamed test description is less accurate than the original. The component applies `admins ?? undefined`, so the fallback value is `undefined`, not `null`. The test provides `null` as the *input*, but the *fallback* used is `undefined`. Consider reverting to the original description or using something like `'renders module without admins (input is null, falls back to undefined)'`.
(Based on your team's feedback about maintaining consistent and accurate naming.) [FEEDBACK_USED]</violation>
<violation number="2" location="frontend/__tests__/unit/pages/ModuleDetails.test.tsx:137">
P3: Same inaccuracy: the component uses `domains ?? undefined`, so the fallback is `undefined`. Consider reverting to the original description.
(Based on your team's feedback about maintaining consistent and accurate naming.) [FEEDBACK_USED]</violation>
<violation number="3" location="frontend/__tests__/unit/pages/ModuleDetails.test.tsx:153">
P3: Same inaccuracy: the component uses `tags ?? undefined`, so the fallback is `undefined`. Consider reverting to the original description.
(Based on your team's feedback about maintaining consistent and accurate naming.) [FEEDBACK_USED]</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@frontend/__tests__/unit/pages/ProgramDetailsMentorship.test.tsx`:
- Around line 195-210: The test "shows error toast when trying to update with
invalid status" is misleading and duplicates the valid-status test; update it to
exercise the invalid-status path: render ProgramDetailsPage, open the actions
menu, and instead of clicking the 'Publish' menuitem, trigger the invalid status
flow by invoking capturedSetStatus('INVALID_STATUS') (or simulate the UI action
that leads to capturedSetStatus with 'INVALID_STATUS'); then assert that
mockUpdateProgram is either not called and that addToast was invoked with an
error message (or called with the correct error toast arguments). Use the
existing identifiers mockUpdateProgram, addToast, capturedSetStatus and
ProgramDetailsPage to locate and modify the test.
- Around line 212-226: The test titled "shows permission denied toast when
non-admin tries to update status" is a duplicate that only checks the absence of
the actions button (same as the non-admin render test) and never asserts on
addToast; remove this test block (the test(...) that mocks useSession with
non-admin and renders ProgramDetailsPage) or replace it by implementing the
proper permission-denied flow: mock addToast, simulate the status update
attempt, and assert addToast was called with the permission-denied message
(refer to useSession, render, ProgramDetailsPage, and addToast for locating and
fixing the test).
🧹 Nitpick comments (7)
frontend/__tests__/unit/pages/MenteeProfilePage.test.tsx (1)
453-497: Consider consolidating the null and undefined labels tests withit.each.The two tests (null labels at Line 453, undefined labels at Line 474) are structurally identical — only the
labelsvalue and the title string differ. A parameterized test would reduce duplication.♻️ Optional consolidation
- it('handles issues with null labels using fallback', () => { - const dataWithNullLabels = { - ...mockMenteeData, - getMenteeModuleIssues: [ - { - id: 'issue1', - number: 101, - title: 'Issue with null labels', - state: 'open', - url: 'http://example.com/issue1', - labels: null, - }, - ], - } - mockUseQuery.mockReturnValue({ data: dataWithNullLabels, loading: false, error: undefined }) - render(<MenteeProfilePage />) - - // Should render without crashing - expect(screen.getAllByText('Issue with null labels').length).toBeGreaterThan(0) - }) - - it('handles issues with undefined labels using fallback', () => { - const dataWithUndefinedLabels = { - ...mockMenteeData, - getMenteeModuleIssues: [ - { - id: 'issue1', - number: 101, - title: 'Issue with undefined labels', - state: 'open', - url: 'http://example.com/issue1', - labels: undefined, - }, - ], - } - mockUseQuery.mockReturnValue({ - data: dataWithUndefinedLabels, - loading: false, - error: undefined, - }) - render(<MenteeProfilePage />) - - // Should render without crashing - expect(screen.getAllByText('Issue with undefined labels').length).toBeGreaterThan(0) - }) + it.each([ + ['null', null], + ['undefined', undefined], + ])('handles issues with %s labels using fallback', (_desc, labels) => { + const data = { + ...mockMenteeData, + getMenteeModuleIssues: [ + { + id: 'issue1', + number: 101, + title: `Issue with ${_desc} labels`, + state: 'open', + url: 'http://example.com/issue1', + labels, + }, + ], + } + mockUseQuery.mockReturnValue({ data, loading: false, error: undefined }) + render(<MenteeProfilePage />) + expect(screen.getAllByText(`Issue with ${_desc} labels`).length).toBeGreaterThan(0) + })frontend/__tests__/unit/components/LogoCarousel.test.tsx (2)
446-493: Test assertions don't verify the stated behavior.All three key-generation tests (lines 447–481) share the same assertion (
toHaveLength(4)) regardless of which fallback path is taken. They confirm the component renders without crashing, which is sufficient for branch-coverage metrics, but the test names ("uses url for key generation…", "uses name for key generation…") promise more than the assertions deliver.If the goal is purely coverage, this is fine. If you also want to guard against regressions in key logic, consider asserting on the actual
keyprop (e.g., via adata-keyattribute or snapshot) so the tests fail when fallback order changes.
448-454: Repeatedundefined as unknown as stringcasts — extract a helper.The triple type-cast pattern to create a sponsor with a missing
idis duplicated three times. A small factory function would reduce noise and make the intent clearer.♻️ Suggested helper
+const makeSponsor = (overrides: Partial<Sponsor> = {}): Sponsor => ({ + id: undefined as unknown as string, + name: '', + imageUrl: '', + url: '', + sponsorType: 'Gold', + ...overrides, +}) + describe('Coverage Edge Cases', () => { it('uses url for key generation when id is missing', () => { - const sponsor: Sponsor = { - id: undefined as unknown as string, - name: 'Name', - imageUrl: 'https://img.com', - url: 'https://url.com', - sponsorType: 'Gold', - } + const sponsor = makeSponsor({ name: 'Name', imageUrl: 'https://img.com', url: 'https://url.com' }) render(<MovingLogos sponsors={[sponsor]} />) expect(screen.getAllByTestId('sponsor-link')).toHaveLength(4) })Also applies to: 460-466, 472-478
frontend/__tests__/unit/pages/Organization.test.tsx (2)
58-61: Redundant mock setup — already done inbeforeEach.Lines 34–37 in
beforeEachset the exact samefetchAlgoliaDatamock (samehits, sametotalPages: 2). This duplicate can be removed without affecting the test.Suggested diff
test('handles page change correctly', async () => { window.scrollTo = jest.fn() - ; (fetchAlgoliaData as jest.Mock).mockResolvedValue({ - hits: mockOrganizationData.hits, - totalPages: 2, - }) render(<Organization />)
34-39: Nit: inconsistent indentation on semicolon-prefixed lines.Lines 34 and 39 (and similarly 125, 161) use
; (…)for ASI protection but with varying indentation — some are flush, others are indented an extra level. Consider aligning them consistently for readability.frontend/__tests__/unit/pages/ProgramDetailsMentorship.test.tsx (2)
327-330: Silent skip ifcapturedSetStatusis null — tests will falsely pass.The
if (capturedSetStatus)guard (used here and at Lines 358, 388, 428) silently skips the core action when the mock fails to capture the callback. If this ever becomesnulldue to a refactor, the tests pass without exercising any meaningful logic.Replace with an explicit assertion or throw:
Proposed fix (apply at all four call sites)
- if (capturedSetStatus) { - capturedSetStatus('INVALID_STATUS') - } + expect(capturedSetStatus).not.toBeNull() + capturedSetStatus!('INVALID_STATUS')
460-473: Most "default value" checks are only comments, not assertions.Lines 467–472 describe expected defaults for
key,name,description,status,startedAt, andendedAtbut never assert them. The test only verifies the component doesn't crash and checks one value (Mentees Limit0). Either add assertions for the commented expectations or remove the misleading comments.
frontend/__tests__/unit/pages/ProgramDetailsMentorship.test.tsx
Outdated
Show resolved
Hide resolved
frontend/__tests__/unit/pages/ProgramDetailsMentorship.test.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
2 issues found across 8 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="frontend/src/components/UserCard.tsx">
<violation number="1">
P1: Accessibility regression: removing `role="img"` and `aria-label="Default profile picture"` makes the fallback avatar invisible to screen readers. These are not test-only attributes — they provide essential accessibility semantics. The PR goal is to improve test coverage, not modify production accessibility behavior.
(Based on your team's feedback about not mixing unrelated changes into a PR.) [FEEDBACK_USED]</violation>
</file>
<file name="frontend/__tests__/unit/pages/Organization.test.tsx">
<violation number="1" location="frontend/__tests__/unit/pages/Organization.test.tsx:167">
P2: Removing the `getByRole('img', { name: 'Default profile picture' })` assertion weakens this test. The test is named "renders organization card with null avatarUrl **using fallback**" but no longer verifies the fallback is actually rendered. Confirming the avatar image is NOT shown doesn't prove a fallback IS shown — both could be absent (a bug). Consider restoring the positive assertion or replacing it with an equivalent check for the fallback element.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/__tests__/unit/components/Footer.test.tsx (1)
295-344:⚠️ Potential issue | 🟠 MajorTwo conflicting environment-mock strategies — one of them may silently not work.
The file uses two different techniques to control
ENVIRONMENT/RELEASE_VERSION:
- Mutable
mockEnv+ getters (lines 76-88, used at line 457): mutatesmockEnv.ENVIRONMENTand relies on the getter to pick up the change.- Direct assignment to
envModule(lines 309-310, 315-316, 329-330): writesenvModule.ENVIRONMENT = 'staging'on the object returned byjest.requireMock.If Jest preserves the accessor descriptors from the mock factory, the direct assignments in approach 2 are silently ignored in non-strict mode (no setter exists), meaning these tests pass only because
mockEnvstill holds thebeforeEachdefault. Conversely, if Jest flattens them to data properties, the getter-based approach in the "Edge Cases" block won't work.Recommend unifying on a single strategy — the
mockEnvmutation approach is simpler and self-consistent:Suggested refactor — use mockEnv everywhere
describe('Version Link Behavior', () => { - let originalEnvironment: string - let originalReleaseVersion: string - let envModule: typeof import('utils/env.client') - - beforeEach(() => { - jest.clearAllMocks() - envModule = jest.requireMock<typeof import('utils/env.client')>('utils/env.client') - originalEnvironment = envModule.ENVIRONMENT - originalReleaseVersion = envModule.RELEASE_VERSION - }) - - afterEach(() => { - if (envModule) { - envModule.ENVIRONMENT = originalEnvironment - envModule.RELEASE_VERSION = originalReleaseVersion - } - }) - test('renders version as commit link in staging environment', () => { - envModule.ENVIRONMENT = 'staging' - envModule.RELEASE_VERSION = '24.2.10-12c25c5' + mockEnv.ENVIRONMENT = 'staging' + mockEnv.RELEASE_VERSION = '24.2.10-12c25c5' const { container } = render(<Footer />) ... }) test('renders version as release tag link in production environment', () => { - envModule.ENVIRONMENT = 'production' - envModule.RELEASE_VERSION = '1.2.3' + mockEnv.ENVIRONMENT = 'production' + mockEnv.RELEASE_VERSION = '1.2.3' const { container } = render(<Footer />) ... }) })The outer
beforeEach(lines 105-111) already resetsmockEnvto production defaults, so the dedicated setup/teardown in this block becomes unnecessary.
🤖 Fix all issues with AI agents
In `@frontend/__tests__/unit/pages/ProgramDetailsMentorship.test.tsx`:
- Around line 305-333: The test "calls addToast with error when setStatus is
called with invalid status" is a duplicate of the earlier test "shows error
toast when trying to update with invalid status" (both call
capturedSetStatus('INVALID_STATUS') and assert the same addToast payload);
remove one of these duplicate tests (either delete the test whose name starts
with "calls addToast with error when setStatus is called with invalid status" or
the earlier one) so only a single test asserting that
capturedSetStatus('INVALID_STATUS') triggers addToast with the expected payload
remains, keeping references to capturedSetStatus, addToast, and
ProgramDetailsPage intact.
- Around line 203-205: The tests currently skip body when the mock callback
variable capturedSetStatus is null due to the `if (capturedSetStatus)` guard,
which can hide failures; replace each guard in ProgramDetailsMentorship.test.tsx
(all occurrences referencing capturedSetStatus) with an explicit assertion that
the callback was captured (e.g., assert or expect that capturedSetStatus is
defined/not null) and then invoke capturedSetStatus('INVALID_STATUS'); this
ensures a missing callback fails the test instead of silently passing.
- Around line 281-303: The test ProgramDetailsMentorship.test.tsx at the
described diff is misnamed and duplicates the 404 case: update or remove the
test so it matches its intent — either delete the entire test block if
redundant, or repurpose it to assert permission-denied behavior by making
getProgram return a valid program (instead of null) and keeping useSession
mocked with a non-admin user; then assert the permission-denied toast or UI
(rather than "Program Not Found"). Locate the test function (the one starting
with test('shows permission denied when non-admin program is null' ...) and
modify the useQuery mock (getProgram/getProgramModules), useSession mock, and
expectations against ProgramDetailsPage accordingly, or remove the test if
redundant with the existing 404 test.
🧹 Nitpick comments (2)
frontend/__tests__/unit/pages/ProjectHealthDashboardMetricsDetails.test.tsx (2)
191-216: Missing assertion for nullscorerendering.The mock sets
score: nullbut there's no assertion verifying how the component renders a null score (e.g., does it show "0", "N/A", or nothing?). The compliance flag assertions are good — they confirm null is treated as not-compliant. Add a similar assertion for the score fallback to make this test meaningful for all three null fields it introduces.
218-265: Test sets ~20 fields to null but only asserts two section headers exist.This test exercises null paths for coverage but doesn't verify the component's actual behavior with null values — it only confirms the page doesn't crash. Consider asserting on at least a few representative fallback outputs (e.g., chart series containing
0instead of actual counts, or specific fallback text) to give the test real regression-catching value.
frontend/__tests__/unit/pages/ProgramDetailsMentorship.test.tsx
Outdated
Show resolved
Hide resolved
frontend/__tests__/unit/pages/ProgramDetailsMentorship.test.tsx
Outdated
Show resolved
Hide resolved
frontend/__tests__/unit/pages/ProgramDetailsMentorship.test.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@frontend/__tests__/unit/components/Footer.test.tsx`:
- Around line 75-88: Add a test that exercises the non-production branch by
mutating the existing mockEnv before rendering Footer: set mockEnv.ENVIRONMENT
to a non-'production' value and set mockEnv.RELEASE_VERSION to a commit-hash,
render the Footer component, and assert that the output contains the commit-hash
link (the same behavior previously covered by the "Version Link Behavior"
tests). Use the current getter-based jest.mock for utils/env.client (mockEnv,
ENVIRONMENT, RELEASE_VERSION) so the mock picks up the mutated values, and
ensure you reset mockEnv back to defaults after the test to avoid leaking state.
🧹 Nitpick comments (3)
frontend/__tests__/unit/pages/ProgramDetailsMentorship.test.tsx (3)
310-347: Tests at Lines 310 and 349 largely duplicate those at Lines 172 and 219.
- Lines 172 vs 310: Both render as admin and trigger publish. Line 310 additionally asserts the success toast, which is valuable — but the mutation assertion is identical. Consider merging the
addToastassertion into the test at Line 172 instead of having a separate test.- Lines 219 vs 349: Both test mutation failure →
handleAppError. The only difference is invocation method (UI click vscapturedSetStatus). Unless you specifically intend to test both paths, one can be removed.Also applies to: 349-373
375-411: Weak assertions — null-field fallback behavior is mostly untested.Lines 401–409 comment on expected fallback values for
key,name,description,status,startedAt, andendedAt, but onlymenteesLimitis actually asserted. The test proves it doesn't crash, but doesn't verify that fallback values render correctly. Consider adding assertions for at least the commented expectations (e.g., checking for empty strings or specific fallback text in the rendered output).
238-278: Null-field fallback tests only assert the title renders — they don't verify the fallback itself.All three tests (null admins, null domains, null tags) only check that
'Test Program'is still rendered. They confirm the component doesn't crash, but they don't assert that theadmins/domains/tagssection is absent or shows a fallback value. Strengthening the assertions would make these tests more meaningful.
|
Hi @kasya the pr is ready for review |
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@frontend/__tests__/unit/pages/ChapterDetails.test.tsx`:
- Around line 175-192: The test "handles missing region gracefully" sets
chapter.region to null but never asserts the region text is absent; update that
test in ChapterDetails.test.tsx (the test block named 'handles missing region
gracefully') to also assert that 'Test Region' (the region string from
mockChapterDetailsData) is not rendered—use screen.queryByText('Test Region')
and expect it toBeNull() or expect(...).not.toBeInTheDocument() after rendering
ChapterDetailsPage with the useQuery mock.
🧹 Nitpick comments (9)
frontend/__tests__/unit/pages/Committees.test.tsx (1)
147-163: Consider asserting the actual fallback behavior, not just non-crash.The test name claims it "uses empty string fallback," but it only verifies that the committee name renders. It doesn't confirm the summary is actually absent or empty. Adding an assertion like
expect(screen.queryByText('This is a summary of Committee 1.')).not.toBeInTheDocument()would make the test actually validate the fallback path rather than just proving the page doesn't crash.Suggested assertion addition
await waitFor(() => { expect(screen.getByText('Committee 1')).toBeInTheDocument() }) + expect(screen.queryByText('This is a summary of Committee 1.')).not.toBeInTheDocument() })frontend/__tests__/unit/components/Footer.test.tsx (1)
285-300: Redundant manual reset ofmockEnvat end of test.The
beforeEachat lines 106–109 already resetsmockEnvto production defaults before every test. The manual reset at lines 298–299 is unnecessary and adds maintenance burden.Suggested cleanup
expect(versionLink).toHaveAttribute('rel', 'noopener noreferrer') - - mockEnv.ENVIRONMENT = 'production' - mockEnv.RELEASE_VERSION = '1.2.3' })frontend/__tests__/unit/components/ScrollToTop.test.tsx (1)
91-115: Well-structured throttle test — minor fragility note onsetTimeoutspy counting.The test logic correctly validates the throttle mechanism: first scroll schedules a timeout, immediate second scroll is suppressed, and after advancing timers past the 100ms window a third scroll schedules a new timeout. The
act()wrapping aroundjest.advanceTimersByTimeis correctly placed where the state update (setIsVisible) fires.One minor note:
jest.spyOn(globalThis, 'setTimeout')captures allsetTimeoutcalls, including any from React internals or the fake timer infrastructure. ThetoHaveBeenCalledTimesassertions could become fragile across React version upgrades. A more resilient approach would be to track the spy's call count at a baseline before each scroll dispatch:♻️ Optional: make assertions resilient to unrelated setTimeout calls
render(<ScrollToTop />) + const baselineCalls = setTimeoutSpy.mock.calls.length + Object.defineProperty(globalThis, 'scrollY', { value: 400, writable: true }) globalThis.dispatchEvent(new Event('scroll')) - expect(setTimeoutSpy).toHaveBeenCalledTimes(1) + expect(setTimeoutSpy.mock.calls.length).toBe(baselineCalls + 1) globalThis.dispatchEvent(new Event('scroll')) - expect(setTimeoutSpy).toHaveBeenCalledTimes(1) // Still 1 + expect(setTimeoutSpy.mock.calls.length).toBe(baselineCalls + 1) // Still same act(() => { jest.advanceTimersByTime(100) }) globalThis.dispatchEvent(new Event('scroll')) - expect(setTimeoutSpy).toHaveBeenCalledTimes(2) + expect(setTimeoutSpy.mock.calls.length).toBe(baselineCalls + 2)frontend/__tests__/unit/components/skeletons/sharedSkeletons.test.tsx (3)
34-44: Magic skeleton counts are fragile.The assertions on exact skeleton counts (17, 54) are tightly coupled to the internal DOM structure of
SectionSkeletonandTwoColumnSection. Any future tweak to skeleton markup (e.g., adding or removing a decorative skeleton) will silently break these tests even though the component is still correct. Consider asserting a minimum count or a structural property (e.g., number ofItemCardSkeletonwrappers) instead.
124-128:{ exact: false }is the default fortoHaveClass— the option is unnecessary here.
toHaveClass('rounded')already performs a subset check by default. The explicit{ exact: false }adds no value and may confuse future readers into thinking the default isexact: true.Suggested simplification
- expect(skeletons[0]).not.toHaveClass('rounded', { exact: false }) + expect(skeletons[0]).not.toHaveClass('rounded')
137-157: Consider adding a test for default/empty optional props onCardSection.Currently only the "all-options-provided" path is tested. A test rendering
<CardSection><div /></CardSection>with noclassName,minHeight, orcolSpanwould cover the|| ''fallback branches in the source and help reach the 90 % branch-coverage target the PR is aiming for.Example additional test
it('renders with default props (no className, minHeight, or colSpan)', () => { const { container } = render( <CardSection> <div>Default</div> </CardSection> ) // Base classes are always present expect(container.firstChild).toHaveClass('mb-8') // Optional classes are absent expect(container.firstChild).not.toHaveClass('min-h-10') expect(container.firstChild).not.toHaveClass('col-span-2') })frontend/__tests__/unit/pages/ProjectHealthDashboardMetricsDetails.test.tsx (3)
158-186: Assertions don't verify the null-createdAt fallback behavior.The test name says "handles null createdAt gracefully" but the assertions only confirm general UI elements (Stars, Forks, Issues, charts) render — the same elements that render in the happy-path test. There's no assertion on the actual fallback output (e.g., empty chart labels or "N/A" text) that would prove the null-createdAt path is exercised correctly. Consider asserting on the specific fallback value produced when
createdAtis null.
188-213: Consider asserting the fallback forscore: null.The test sets
score: nullbut never asserts what the UI renders for a null score. The compliance flag assertions are good (verifying "Not Compliant" when null), but adding an assertion for the score fallback (e.g., checking that no score number appears, or that a default/fallback is shown) would make the test name accurate for all three null fields.
215-262: Extensive null setup with minimal assertions — this is effectively a smoke test.The mock carefully nullifies ~20 fields across
healthMetricsListandhealthMetricsLatest, but the assertions only check that "Days Metrics" and "Stars" headings render. This confirms the component doesn't crash, but doesn't verify any fallback/default values (e.g., that charts receive0for null counts, or that bar chart days/requirements default to0). If the goal is branch coverage, the assertions should target the specific fallback branches being exercised.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3923 +/- ##
==========================================
- Coverage 95.39% 94.00% -1.40%
==========================================
Files 463 512 +49
Lines 14540 15814 +1274
Branches 2061 2131 +70
==========================================
+ Hits 13871 14866 +995
- Misses 328 676 +348
+ Partials 341 272 -69
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 77 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
kasya
left a comment
kasya
left a comment
There was a problem hiding this comment.
@anurag2787 these are awesome! Thanks!
I cleaned up comments a little bit but the rest looks great👍🏼
2 participants
Proposed change
his PR improves frontend unit test coverage for files that were previously below the 90% line coverage threshold.
Resolves #3881
Checklist
make check-testlocally: all warnings addressed, tests passed