Improve frontend test coverage above 80% and add missing test files#3864
Improve frontend test coverage above 80% and add missing test files#3864kasya merged 9 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:
WalkthroughAdds and expands many frontend unit tests across components, pages, and utils; introduces extensive mocks and interaction assertions (keyboard, hover, pagination, form flows). One small production change: RecentPullRequests onClick now uses Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (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.
20 issues found across 50 files
Confidence score: 4/5
- Most issues are test assertion problems (e.g., always-true checks or unconfigured mocks) in
frontend/__tests__/unit/components/ProgramForm.test.tsxand related files, which reduce test reliability but don’t indicate runtime regressions. - The highest-severity items are incorrect or ineffective assertions in unit tests (severity 5–6/10), suggesting some tests may pass without actually validating behavior.
- This looks safe to merge with minimal risk since the issues are confined to test quality rather than production code.
- Pay close attention to
frontend/__tests__/unit/components/ProgramForm.test.tsx,frontend/__tests__/unit/components/DashboardWrapper.test.tsx,frontend/__tests__/unit/pages/ProjectsHealthDashboardMetrics.test.tsx,frontend/__tests__/unit/components/SingleModuleCard.test.tsx,frontend/__tests__/unit/pages/CreateModule.test.tsx,frontend/__tests__/unit/components/ModuleForm.test.tsx- improve assertions to ensure tests verify intended behavior.
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/utils/formValidationUtils.test.ts">
<violation number="1" location="frontend/__tests__/unit/utils/formValidationUtils.test.ts:20">
P3: Redundant test case: this test is functionally identical to line 13-15 ('returns error message when value is empty string'). Both test `validateRequired('', ...)` with an empty string. The description 'null-like empty' is misleading since no null value is being tested. Consider removing this duplicate test or actually testing null handling if that's the intent.</violation>
</file>
<file name="frontend/__tests__/unit/components/SingleModuleCard.test.tsx">
<violation number="1" location="frontend/__tests__/unit/components/SingleModuleCard.test.tsx:445">
P2: These tests have misleading names - they claim to verify EntityActions rendering behavior for admin/non-admin users, but the assertions only check that 'Test Module' text is present, which doesn't verify EntityActions at all. Consider either mocking EntityActions and asserting it's rendered/not rendered, or renaming the tests to accurately describe what they verify (e.g., 'renders without error when user is admin').</violation>
</file>
<file name="frontend/__tests__/unit/pages/CreateModule.test.tsx">
<violation number="1" location="frontend/__tests__/unit/pages/CreateModule.test.tsx:231">
P2: Test does not verify what its name claims. The test 'renders form with min and max dates when program has start and end dates' only checks that 'Create New Module' text exists, but doesn't actually verify that date inputs have the correct min/max constraints from the program dates. Consider adding assertions that verify the date input elements have the expected `min` and `max` attributes, or rename the test to accurately reflect what it tests.</violation>
</file>
<file name="frontend/__tests__/unit/components/DashboardWrapper.test.tsx">
<violation number="1" location="frontend/__tests__/unit/components/DashboardWrapper.test.tsx:138">
P2: The mock for `useDjangoSession` is configured AFTER the initial render, causing the first render to use an unconfigured mock (undefined). Move the mock setup before the `render()` call to ensure consistent test behavior.</violation>
</file>
<file name="frontend/__tests__/unit/components/CalendarButton.test.tsx">
<violation number="1" location="frontend/__tests__/unit/components/CalendarButton.test.tsx:343">
P2: This mouse-leave test doesn't verify the icon reverted; it only checks the button still exists. Assert that the icon markup returns to the initial state after mouseLeave.</violation>
<violation number="2" location="frontend/__tests__/unit/components/CalendarButton.test.tsx:343">
P2: This hover test never asserts the icon change; it only re-checks that the button exists, so the test will pass even if the hover state is broken. Add an assertion that the rendered icon changes after hover.</violation>
</file>
<file name="frontend/__tests__/unit/components/ProgramCard.test.tsx">
<violation number="1" location="frontend/__tests__/unit/components/ProgramCard.test.tsx:435">
P2: This test doesn’t assert that `stopPropagation` was called, so it doesn’t validate the behavior it claims to cover. Add an expectation on the mock event to ensure the handler is exercised.</violation>
</file>
<file name="frontend/__tests__/unit/components/ModuleForm.test.tsx">
<violation number="1" location="frontend/__tests__/unit/components/ModuleForm.test.tsx:421">
P2: Test 'handles single key selection' has no assertions. It performs actions (triggering search and clicking select) but never verifies the expected outcome. Tests without assertions will always pass.</violation>
<violation number="2" location="frontend/__tests__/unit/components/ModuleForm.test.tsx:477">
P2: Test 'sets all fields as touched on submit' has no assertion after form submission. The test description promises to verify fields are touched, but no actual assertion validates this behavior. Consider adding assertions to verify the touched state is set on form fields.</violation>
<violation number="3" location="frontend/__tests__/unit/components/ModuleForm.test.tsx:617">
P2: Test 'handles selection via Set' lacks meaningful assertions. The test performs actions but ends with a comment acknowledging the test doesn't properly verify behavior. Tests without assertions will always pass and provide no value.</violation>
<violation number="4" location="frontend/__tests__/unit/components/ModuleForm.test.tsx:630">
P2: Test 'handles "all" key selection' has a trivial assertion that only verifies the button still exists in the DOM. This doesn't test the actual handling of 'all' selection. Consider asserting the expected state after selection.</violation>
<violation number="5" location="frontend/__tests__/unit/components/ModuleForm.test.tsx:796">
P2: Test 'filters out currently selected project from suggestions' doesn't verify filtering behavior. It only asserts `mockQuery` was called but never checks that project-1 was actually filtered from results. Consider adding an assertion for the filtering logic.</violation>
</file>
<file name="frontend/__tests__/unit/pages/MyMentorship.test.tsx">
<violation number="1" location="frontend/__tests__/unit/pages/MyMentorship.test.tsx:264">
P2: Reset globalThis.scrollTo after this test (or in afterEach) to avoid leaking the mock into other tests and causing cross-test interference.</violation>
</file>
<file name="frontend/__tests__/unit/pages/ProjectsHealthDashboardMetrics.test.tsx">
<violation number="1" location="frontend/__tests__/unit/pages/ProjectsHealthDashboardMetrics.test.tsx:208">
P2: This test's assertion doesn't verify the filter was applied - the button was already in the document before clicking. Consider asserting that the filter action actually occurred (e.g., verify URL params changed, query variables updated, or UI state reflects the active filter).
This same issue affects multiple tests in this file: all health filters, level filters, sort options, and reset buttons.</violation>
</file>
<file name="frontend/__tests__/unit/components/ProgramForm.test.tsx">
<violation number="1" location="frontend/__tests__/unit/components/ProgramForm.test.tsx:188">
P2: This assertion always passes since `array.length` is always >= 0. The test named 'enables submit button when not loading' doesn't actually verify that the button is enabled. Consider asserting that the button exists and is not disabled.</violation>
<violation number="2" location="frontend/__tests__/unit/components/ProgramForm.test.tsx:188">
P2: This assertion always passes since `array.length` is always >= 0. The test named 'disables submit button when loading' doesn't actually verify that the button is disabled. Consider asserting that the button exists and has a `disabled` attribute.</violation>
<violation number="3" location="frontend/__tests__/unit/components/ProgramForm.test.tsx:241">
P2: This assertion always passes since `array.length` is always >= 0. The test named 'calls setFormData when mentees limit changes' doesn't actually verify the stated behavior. Consider finding the mentees limit input and testing the onChange handler.</violation>
<violation number="4" location="frontend/__tests__/unit/components/ProgramForm.test.tsx:820">
P2: Incorrect assertion: `queryByText` returns `null` (not `undefined`) when an element is not found, so `.not.toBeUndefined()` will always pass. Use `.toBeInTheDocument()` instead.</violation>
<violation number="5" location="frontend/__tests__/unit/components/ProgramForm.test.tsx:837">
P2: Incorrect assertion: `queryByText` returns `null` (not `undefined`) when an element is not found, so `.not.toBeUndefined()` will always pass. Use `.toBeInTheDocument()` instead.</violation>
</file>
<file name="frontend/__tests__/unit/components/ModuleCard.test.tsx">
<violation number="1" location="frontend/__tests__/unit/components/ModuleCard.test.tsx:729">
P2: Test has uncertain expected behavior and uses a loose assertion. The comment 'depending on implementation' indicates this test doesn't verify a specific expected outcome. Tests should document and assert known expected behavior - either determine what `getSimpleDuration` should return for negative durations and assert that specific value, or handle this edge case explicitly in the implementation.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
frontend/__tests__/unit/components/CalendarButton.test.tsx (1)
170-174:⚠️ Potential issue | 🟡 MinorThe "fallback" test doesn't actually test the fallback path.
The component uses
event.title || 'event'as the fallback for empty/missing titles. This test passestitle: 'Untitled'which is a truthy string, so it exercises the normal path, not the fallback. To test the actual fallback, passtitle: ''ortitle: undefinedand assert the aria-label is'Add event to Calendar'.Proposed fix
it('uses fallback for events without explicit title', () => { - render(<CalendarButton event={{ ...mockEvent, title: 'Untitled' }} />) + render(<CalendarButton event={{ ...mockEvent, title: '' }} />) const button = screen.getByRole('button') - expect(button).toHaveAttribute('aria-label', 'Add Untitled to Calendar') + expect(button).toHaveAttribute('aria-label', 'Add event to Calendar') })frontend/src/components/RecentPullRequests.tsx (1)
35-45:⚠️ Potential issue | 🟠 MajorExtend guard to also check
organizationNameto prevent malformed navigation URLs.Line 35 guards only
item?.repositoryName, but line 43 interpolates bothitem.organizationNameanditem.repositoryName. Since both fields are optional (?:in the PullRequest type), iforganizationNameis missing or falsy, the router will push to/organizations/null/repositories/....Proposed fix
- {item?.repositoryName && ( + {item?.organizationName && item?.repositoryName && (frontend/__tests__/unit/pages/Header.test.tsx (1)
484-499:⚠️ Potential issue | 🟡 MinorNo-op test: resize event never reaches the component handler.
window.addEventListeneris replaced withjest.fn()inbeforeEach(line 189), soglobalThis.dispatchEvent(new Event('resize'))at line 495 never triggers the component's registered resize callback. The only assertion isexpect(true).toBe(true), making this test vacuous — it will always pass regardless of component behavior.The dedicated resize tests in the "Resize Handler" describe block (lines 741–824) already cover this behavior properly by extracting and invoking the handler directly. Consider removing this test to avoid a false sense of coverage.
🤖 Fix all issues with AI agents
In `@frontend/__tests__/unit/components/CalendarButton.test.tsx`:
- Around line 335-363: The two hover tests in CalendarButton.test.tsx don't
assert the icon change; update each test (the 'toggles icon on hover - shows
FaCalendarPlus when hovering' and 'reverts to FaCalendar icon when mouse
leaves') to capture the button's rendered SVG before and after the mouse events
and assert they differ (or match expected values). Specifically, within the
tests that render <CalendarButton event={mockEvent} /> find the button
(screen.getByRole('button')), locate the contained <svg> (e.g.,
button.querySelector('svg')) and store its innerHTML or the SVG <path> 'd'
attribute, then fireEvent.mouseEnter(mouseLeave) and await waitFor to assert
that the post-event svg.innerHTML (or path 'd') is different for the hover test
(and equals the original for the mouse leave test); if needed, add a data-testid
on the SVG in the CalendarButton component to make selection reliable. Ensure
the assertions check the actual SVG content rather than only
toBeInTheDocument().
In `@frontend/__tests__/unit/components/DashboardWrapper.test.tsx`:
- Around line 138-175: The test currently calls render() before configuring the
mockUseDjangoSession return value, causing order-dependent behavior; fix by
setting mockUseDjangoSession.mockReturnValue(...) with the initial syncing state
before the first render of <DashboardWrapper> (or alternatively add
jest.resetAllMocks() in the test suite beforeEach), then proceed to rerender and
update the mock for the "after sync completes" state — target the
mockUseDjangoSession mock and the DashboardWrapper render/rerender calls to
reorder the setup so the initial render uses the intended mocked return value.
In `@frontend/__tests__/unit/components/ProgramCard.test.tsx`:
- Around line 422-442: The test currently doesn't verify stopPropagation because
passing a plain object to fireEvent.mouseDown won't replace
Event.prototype.stopPropagation and no assertion checks it; update the test
"stops propagation on mousedown for actions wrapper" to create a real MouseEvent
(e.g. new MouseEvent('mousedown', { bubbles: true })) or spy on
Event.prototype.stopPropagation, dispatch the event against the actionsWrapper
(using actionsWrapper!.dispatchEvent(event) or fireEvent(actionsWrapper!,
event)), assert that the spy was called (expect(spy).toHaveBeenCalled()), and
restore the spy after the test; reference the actionsWrapper variable and
ProgramCard test block so the change is applied in that spec.
In `@frontend/__tests__/unit/components/SingleModuleCard.test.tsx`:
- Around line 444-496: The tests under the "Admin Access and EntityActions"
describe block are only asserting the module title and never verify
EntityActions; mock the EntityActions component (similar to ShowMoreButton) to
return a predictable element (e.g., a span with text "MockedEntityActions") and
then update the three tests that call render(<SingleModuleCard ... />) to assert
presence (expect(...toBeInTheDocument())) of that mocked marker when accessLevel
is "admin" and session user is an admin, and assert absence
(expect(queryByText(...)).toBeNull() or .not.toBeInTheDocument()) when the user
is not admin or accessLevel is not "admin"; keep using mockUseSession and the
existing test names to locate where to change mocks and assertions.
In `@frontend/__tests__/unit/components/SkeletonBase.test.tsx`:
- Around line 144-152: Update the assertion in the SkeletonBase unit test so it
verifies all responsive grid classes output by the userCardRender used for both
users and organizations; in the test for SkeletonBase (component SkeletonBase,
test file SkeletonBase.test.tsx) replace the current
expect(gridContainer).toHaveClass(...) call to include the six classes 'grid',
'grid-cols-1', 'gap-6', 'sm:grid-cols-2', 'lg:grid-cols-3', and 'xl:grid-cols-4'
so the test accurately reflects the component's actual output.
In `@frontend/__tests__/unit/pages/About.test.tsx`:
- Around line 610-627: The mock response merges ...mockAboutData which yields {
project, topContributors, users } but the component under test expects
leader1/leader2/leader3 instead of users; update the three tests that build the
mocked data (the one setting project.recentMilestones and the two sibling tests)
to construct the shape explicitly: keep project with recentMilestones as you
have, preserve topContributors, and add leader1, leader2, leader3 properties
(copied from the appropriate entries in mockAboutData or created from
mockAboutData.topContributors) while removing the unexpected users field so the
mocked data matches the component's expected GraphQL response shape.
In `@frontend/__tests__/unit/pages/ProjectsHealthDashboardMetrics.test.tsx`:
- Around line 213-265: The tests currently assert only that the clicked button
is in the document which is tautological; update each health-filter test (the
ones that render <MetricsPage /> and use getByRole + fireEvent) to assert an
observable side-effect instead of the button presence. For example, after
clicking the "Healthy"/"Need Attention"/"Unhealthy" button assert the mocked
useQuery was invoked with the expected variables (e.g.
expect(useQuery).toHaveBeenLastCalledWith(expect.objectContaining({ variables:
expect.objectContaining({ level: 'healthy' }) }))), or assert
router.replace/push was called with the expected query, or assert the rendered
metric cards change to the expected filtered content from mockHealthMetricsData;
replace the tautological waitFor(expect(button).toBeInTheDocument()) with one of
these meaningful assertions referencing useQuery, router.replace/router.push, or
the metric card elements.
🟡 Minor comments (21)
frontend/__tests__/unit/pages/Contribute.test.tsx-224-226 (1)
224-226:⚠️ Potential issue | 🟡 MinorWrap post-close assertion in
waitForto avoid flakiness.Closing the modal triggers a state update; the "Read More" button may not reappear synchronously. Every other assertion in this file that follows a user interaction is wrapped in
waitFor— this one should be consistent.Proposed fix
- // After closing, the Read More button should be visible again - expect(screen.getByText('Read More')).toBeInTheDocument() + // After closing, the Read More button should be visible again + await waitFor(() => { + expect(screen.getByText('Read More')).toBeInTheDocument() + })frontend/__tests__/unit/pages/MyMentorship.test.tsx-280-310 (1)
280-310:⚠️ Potential issue | 🟡 MinorSearch callback test has no meaningful assertion.
screen.getByTestId('search-input')already throws if the element is missing, so theexpect(searchInput).toBeInTheDocument()on line 309 is redundant and tests nothing about the search behavior. The test fires a change event but never verifies any side effect (e.g., thatuseQuerywas re-invoked with updated variables, or that the rendered output changed).Either add a meaningful assertion — such as verifying that the query was called with the search term — or remove this test to avoid false confidence in coverage. Based on learnings, page-level tests should focus on submission handling and navigation logic, not just element presence.
frontend/__tests__/unit/pages/EditModule.test.tsx-257-263 (1)
257-263:⚠️ Potential issue | 🟡 MinorTest doesn't actually verify graceful error handling.
The test name says "handles form submission error gracefully," but the only assertion after clicking Save is that the mutation was called. There's no verification that:
- An error toast is shown, or
- The user is not redirected (i.e.,
mockPushwas not called), or- Any error UI is rendered.
Without these, the test only confirms the mutation fires — not that the error path is handled. Consider adding at least:
await waitFor(() => { expect(mockUpdateModule).toHaveBeenCalled() }) + + // Verify no navigation occurred on error + expect(mockPush).not.toHaveBeenCalled()frontend/__tests__/unit/components/FontLoaderWrapper.test.tsx-138-160 (1)
138-160:⚠️ Potential issue | 🟡 MinorBiome
noThenPropertylint violation — restructure the mock to use a realPromise.Line 147 assigns
thenas a plain object property, which Biome rightfully flags because any object with athenmethod is treated as a thenable by the JS runtime, leading to subtle issues. Since you're mockingdocument.fonts.ready, you can use an actual resolved promise and spy on itstheninstead:Proposed fix
it('calls document.fonts.ready.then() on mount', () => { - const thenSpy = jest.fn((callback: () => void) => { - callback() - return Promise.resolve() - }) - - Object.defineProperty(document, 'fonts', { - value: { - ready: { - then: thenSpy, - }, - }, - configurable: true, - }) + const fontsReadyPromise = Promise.resolve() + const thenSpy = jest.spyOn(fontsReadyPromise, 'then') + + Object.defineProperty(document, 'fonts', { + value: { + ready: fontsReadyPromise, + }, + configurable: true, + }) render( <FontLoaderWrapper> <div>Content</div> </FontLoaderWrapper> ) expect(thenSpy).toHaveBeenCalled() })frontend/__tests__/unit/components/SortBy.test.tsx-131-139 (1)
131-139:⚠️ Potential issue | 🟡 MinorMake
sortOptionsoptional in the component's prop types or remove thesortOptions={undefined}test case.The
SortByPropstype definessortOptionsas a required property ({ key: string; label: string }[]), not optional. The test passessortOptions={undefined}, which would be a TypeScript error in strict mode. While the component defensively handles undefined (line 14:if (!sortOptions || sortOptions.length === 0) return null), either the type definition should be updated tosortOptions?: { key: string; label: string }[]to match the implementation's expectations, or the test should be removed since the prop is required by contract.frontend/__tests__/unit/utils/formValidationUtils.test.ts-319-321 (1)
319-321:⚠️ Potential issue | 🟡 MinorIncorrect inline comment and inconsistent assertion matcher.
The comment states
(undefined && true) = false, but in JavaScriptundefined && trueevaluates toundefined, making the full expressionfalse || undefined=undefined(falsy, but notfalse).toBeFalsy()passes here but masks the actual value, and every othershouldValidateassertion in this suite uses the strictertoBe(false).Consider using
toBe(undefined)(to match the real runtime value) or better yet, check whether the production code should be normalized to always return a boolean — e.g. withBoolean(...)or!!— so allshouldValidatevalues are trulyboolean.Proposed fix
- // endedAt: (touched.endedAt ?? false) || (touched.startedAt && !!formData.endedAt) - // = (undefined ?? false) || (undefined && true) = false || false = false - expect(rules[3].shouldValidate).toBeFalsy() + // endedAt: (touched.endedAt ?? false) || (touched.startedAt && !!formData.endedAt) + // = (undefined ?? false) || (undefined && !!formData.endedAt) = false || undefined = undefined + expect(rules[3].shouldValidate).toBeFalsy()frontend/__tests__/unit/components/ContributorAvatar.test.tsx-326-341 (1)
326-341:⚠️ Potential issue | 🟡 MinorTest name contradicts its assertion.
The test is named "renders avatar without &s=60 suffix when treated as non-Algolia" but line 340 asserts the src includes
&s=60. The comments on lines 327-329 even acknowledge that the false branch can't actually be reached this way. This is misleading — a future reader will assume this test covers the non-Algolia path when it doesn't.Either rename the test to reflect what it actually verifies, or restructure to genuinely exercise the false branch (e.g., by mocking the internal
isAlgoliaContributorcheck).frontend/__tests__/unit/global-error.test.tsx-173-182 (1)
173-182:⚠️ Potential issue | 🟡 MinorWrap
Error.captureStackTracerestoration intry/finallyto prevent global state leakage.If the
AppErrorconstructor or any assertion throws,Error.captureStackTraceis never restored, which can break subsequent tests across the entire suite.Proposed fix
test('should handle when Error.captureStackTrace is not available', () => { const originalCaptureStackTrace = Error.captureStackTrace delete (Error as unknown as { captureStackTrace?: unknown }).captureStackTrace - const err = new AppError(500, 'Test') - expect(err.name).toBe('AppError') - expect(err.statusCode).toBe(500) - ;(Error as unknown as { captureStackTrace?: unknown }).captureStackTrace = - originalCaptureStackTrace + try { + const err = new AppError(500, 'Test') + expect(err.name).toBe('AppError') + expect(err.statusCode).toBe(500) + } finally { + ;(Error as unknown as { captureStackTrace?: unknown }).captureStackTrace = + originalCaptureStackTrace + } })frontend/__tests__/unit/components/Leaders.test.tsx-58-72 (1)
58-72:⚠️ Potential issue | 🟡 MinorThe field names in the test data are correct; however, use
member: nullinstead ofmember: undefined.The
userWithoutMemberobject correctly usesmemberNameanddescriptionas expected by the Leaders component. However, theLeadertype definition specifiesmember: { ... } | null, so the test should setmember: nullrather thanmember: undefinedto match the type contract. While the test will function correctly at runtime due to optional chaining and truthiness checks, the type is technically incorrect.frontend/__tests__/unit/components/MentorshipPullRequest.test.tsx-128-132 (1)
128-132:⚠️ Potential issue | 🟡 MinorLocale-dependent date assertion may cause flaky tests.
toLocaleDateString()without an explicit locale produces different output depending on the environment (e.g.,1/15/2024in en-US vs15/01/2024in en-GB). This can cause CI failures if the runner locale differs from the developer's machine. Pass an explicit locale to match the component's behavior.Suggested fix
test('renders date in correct format', () => { render(<MentorshipPullRequest pr={mockPullRequestOpen} />) - const dateStr = new Date('2024-01-15').toLocaleDateString() + const dateStr = new Date('2024-01-15').toLocaleDateString('en-US') expect(screen.getByText(new RegExp(dateStr))).toBeInTheDocument() })Use the same locale the component uses for formatting, or match against a locale-independent pattern.
frontend/__tests__/unit/components/MentorshipPullRequest.test.tsx-134-141 (1)
134-141:⚠️ Potential issue | 🟡 MinorFix
toHaveStyleassertions to use object format instead of camelCase strings.The jest-dom
toHaveStyleAPI expects CSS property names in kebab-case when using string format (e.g.,'background-color:#238636'), or as camelCase properties in an object (e.g.,{ backgroundColor: '#238636' }). Lines 137, 146, and 152 use camelCase in string format, which is incorrect.Suggested fixes
- expect(badge).toHaveStyle('backgroundColor: `#238636`') + expect(badge).toHaveStyle({ backgroundColor: '#238636' })- expect(badge).toHaveStyle('backgroundColor: `#8657E5`') + expect(badge).toHaveStyle({ backgroundColor: '#8657E5' })- expect(badge).toHaveStyle('backgroundColor: `#DA3633`') + expect(badge).toHaveStyle({ backgroundColor: '#DA3633' })frontend/__tests__/unit/components/ModuleCard.test.tsx-726-730 (1)
726-730:⚠️ Potential issue | 🟡 MinorRegex doesn't properly validate negative duration output.
Math.ceil(-60/7)yields-8, sogetSimpleDurationreturns"-8 weeks". The regex/\d+ weeks?|0 weeks/will still pass becausetoMatchfinds"8 weeks"as a substring within"-8 weeks", masking the negative sign. If the intent is to document the actual negative-duration behavior, assert the exact expected output (e.g.,"-8 weeks"or"0 weeks") so the test meaningfully captures the edge case.frontend/__tests__/unit/components/TruncatedText.test.tsx-166-171 (1)
166-171:⚠️ Potential issue | 🟡 MinorUse JSX children syntax instead of the
childrenprop.Biome flags
children={null}as an anti-pattern. Use the canonical JSX form instead.Proposed fix
- render(<TruncatedText text={undefined} children={null} />) + render(<TruncatedText text={undefined}>{null}</TruncatedText>)frontend/__tests__/unit/components/ModuleForm.test.tsx-773-798 (1)
773-798:⚠️ Potential issue | 🟡 MinorMissing assertion — test comment says "should not include project-1" but never verifies it.
The test triggers a search with
value: 'project-1'to exercise the filtering logic, but the final comment at Line 797 is just a description with no correspondingexpect(). Add an assertion (e.g., check that the rendered autocomplete items don't includeproject-1).frontend/__tests__/unit/components/CardDetailsPage.test.tsx-2220-2290 (1)
2220-2290:⚠️ Potential issue | 🟡 MinorEntityActions tests don't verify what they claim.
Both "renders EntityActions when user is admin" (Line 2221) and "does not render EntityActions when user is not admin" (Line 2258) only assert
screen.getByText(/Test Project/), which is the page title and is always present. Neither test checks for the actual presence or absence of theentity-actionstest ID.Use
screen.getByTestId('entity-actions')/screen.queryByTestId('entity-actions')to verify the component is or isn't rendered, respectively.frontend/__tests__/unit/components/ProgramForm.test.tsx-806-838 (1)
806-838:⚠️ Potential issue | 🟡 MinorAssertions are trivially true —
queryByTextreturnsnull, notundefined.
screen.queryByText('Start Date')returnsnull(notundefined) when not found, soexpect(startDateLabel).not.toBeUndefined()always passes regardless. Use.toBeInTheDocument()or.not.toBeNull()to actually verify the element exists. The same applies to the end date test.Proposed fix
- expect(startDateLabel).not.toBeUndefined() + expect(startDateLabel).toBeInTheDocument()frontend/__tests__/unit/components/CardDetailsPage.test.tsx-2369-2429 (1)
2369-2429:⚠️ Potential issue | 🟡 MinorSame issue: program EntityActions tests only assert page title, not EntityActions presence.
All three tests (Lines 2369, 2397, 2414) for program-level EntityActions rendering only check
screen.getByText('Test Project'). They should verifyentity-actionstest ID presence/absence to match their stated intent.frontend/__tests__/unit/components/ProgramForm.test.tsx-175-205 (1)
175-205:⚠️ Potential issue | 🟡 MinorTests don't verify what their names claim.
Both "disables submit button when loading" (Line 175) and "enables submit button when not loading" (Line 191) only assert
buttons.length >= 0, which is trivially always true. They should actually check thedisabledattribute on the submit button to match their names.Example fix for "disables submit button when loading"
const buttons = screen.getAllByRole('button') - // Component should render without errors - expect(buttons.length).toBeGreaterThanOrEqual(0) + const submitButton = buttons.find((btn) => btn.textContent?.includes('Saving...')) + expect(submitButton).toBeDisabled()frontend/__tests__/unit/pages/ModuleIssueDetailsPage.test.tsx-421-434 (1)
421-434:⚠️ Potential issue | 🟡 MinorTest intent is unclear — full data is provided despite empty
issueId.The test name says "Loading issue… title when issueId is not available", but
mockUseQuerystill returns a fully populatedmockIssueData. In production, an emptyissueIdwould likely result in no data or a different query response. This makes the test assert on a state that probably can't occur in practice (full data + empty issueId simultaneously).Consider either returning
loading: true/data: undefinedto match a realistic loading scenario, or clarifying the comment to explain that the component checksissueIdindependently of the query result.frontend/__tests__/unit/components/ChapterMapWrapper.test.tsx-265-290 (1)
265-290:⚠️ Potential issue | 🟡 MinorTest doesn't verify
_distanceremoval — only checks array length.The test is named "correctly maps sorted data removing _distance property" but the mock
ChapterMaponly rendersgeoLocData.length. The assertion at line 288 (toHaveTextContent('2')) passes whether or not_distanceis stripped. To actually verify the mapping, the mock would need to render or expose the properties of each chapter item (e.g., serialize and check that_distanceis absent).frontend/__tests__/unit/pages/Chapters.test.tsx-149-163 (1)
149-163:⚠️ Potential issue | 🟡 MinorTest doesn't actually verify the behavior it claims to test.
The test says "chapters data should be used for the map instead of geoLocData" when a search query is present (line 93 in
page.tsxdoes have this conditional:geoLocData={searchQuery ? chapters : geoLocData}), but the only assertion—screen.getByText('Chapter 1')—is identical to what the existing'renders chapter data correctly'test already verifies without any search query. Since both data sources are mocked to return the samemockChapterData.chapters, this assertion will pass regardless of which branch of the ternary is executed.To meaningfully verify this behavior, assert something that differs between the two code paths—e.g., that
ChapterMapWrapperreceiveschaptersas itsgeoLocDataprop whensearchQueryis set, or that the map renders different content based on the data source.
🧹 Nitpick comments (46)
frontend/__tests__/unit/components/UserMenu.test.tsx (1)
777-812: Describe block covers only the leader path; the OWASP Staff link is untested.The block is named "Project Leader and OWASP Staff links" but contains no test for the "Project Health Dashboard" link rendered when
isOwaspStaffistrue. Since the PR objective is to hit 80% coverage, this likely leaves the staff-link branch uncovered.Consider adding a parallel test:
🧪 Suggested additional test
+ it('closes dropdown when Project Health Dashboard link is clicked', async () => { + const staffSession: ExtendedSession = { + user: { + name: 'Staff User', + email: 'staff@example.com', + image: 'https://example.com/avatar.jpg', + isLeader: false, + isOwaspStaff: true, + }, + expires: '2024-12-31', + } + + mockUseSession.mockReturnValue({ + session: staffSession, + isSyncing: false, + status: 'authenticated', + }) + + render(<UserMenu isGitHubAuthEnabled={true} />) + + const avatarButton = screen.getByRole('button') + fireEvent.click(avatarButton) + + await waitFor(() => { + expect(screen.getByText('Project Health Dashboard')).toBeInTheDocument() + }) + + const dashboardLink = screen.getByText('Project Health Dashboard') + fireEvent.click(dashboardLink) + + await waitFor(() => { + expect(avatarButton).toHaveAttribute('aria-expanded', 'false') + }) + })frontend/__tests__/unit/components/MetricsPDFButton.test.tsx (1)
48-59: Test 3 is duplicative of Test 2.This test ("passes correct path and fileName props") does the exact same thing as the previous test ("calls fetchMetricsPDF when icon is clicked") — clicks the icon and asserts
fetchMetricsPDFwas called with the provided props. Only the prop values differ. Consider either removing this test or merging it into atest.eachif you want to verify multiple prop combinations.♻️ Consolidate into a parameterized test
- it('calls fetchMetricsPDF when icon is clicked', async () => { - mockFetchMetricsPDF.mockResolvedValueOnce(undefined) - - render(<MetricsPDFButton path="/api/metrics" fileName="metrics.pdf" />) - - const icon = screen.getByTestId('tooltip').querySelector('svg') - expect(icon).toBeInTheDocument() - - fireEvent.click(icon!) - - await waitFor(() => { - expect(mockFetchMetricsPDF).toHaveBeenCalledWith('/api/metrics', 'metrics.pdf') - }) - }) - - it('passes correct path and fileName props', async () => { - mockFetchMetricsPDF.mockResolvedValueOnce(undefined) - - render(<MetricsPDFButton path="/custom/path" fileName="custom-file.pdf" />) - - const icon = screen.getByTestId('tooltip').querySelector('svg') - fireEvent.click(icon!) - - await waitFor(() => { - expect(mockFetchMetricsPDF).toHaveBeenCalledWith('/custom/path', 'custom-file.pdf') - }) - }) + it.each([ + { path: '/api/metrics', fileName: 'metrics.pdf' }, + { path: '/custom/path', fileName: 'custom-file.pdf' }, + ])('calls fetchMetricsPDF with path=$path and fileName=$fileName on click', async ({ path, fileName }) => { + mockFetchMetricsPDF.mockResolvedValueOnce(undefined) + + render(<MetricsPDFButton path={path} fileName={fileName} />) + + const icon = screen.getByTestId('tooltip').querySelector('svg') + expect(icon).toBeInTheDocument() + + fireEvent.click(icon!) + + await waitFor(() => { + expect(mockFetchMetricsPDF).toHaveBeenCalledWith(path, fileName) + }) + })frontend/__tests__/unit/pages/Organization.test.tsx (1)
87-137: Mock data usesnullwhere theOrganizationtype expectsundefined.The
Organizationtype infrontend/src/types/organization.tsdefinescompany,locationas optional (string | undefined), but the mock data at lines 94, 97, 99, 110, 113, and 115 usesnull. This works at runtime because the component likely uses falsy checks, but it bypasses TypeScript's type safety in tests. Consider usingundefinedinstead ofnullto keep mocks aligned with the actual type, or omit those keys entirely.Suggested diff
{ objectID: 'org-no-optional', avatarUrl: 'https://avatars.githubusercontent.com/u/999999?v=4', collaboratorsCount: 10, - company: null, createdAt: 1596744799, description: 'Organization without optional fields', - email: null, followersCount: 100, - location: null, login: 'no-optional-org', name: 'No Optional Fields Org', publicRepositoriesCount: 50, updatedAt: 1727390473, url: 'https://github.com/no-optional-org', },frontend/__tests__/unit/pages/UserDetails.test.tsx (1)
895-917: Consider consolidating the two "name is null" tests.Lines 895–917 and 942–963 both set up the same mock (user with
name: null) but assert different things (login link presence vs. alt text). Merging them into a single test would reduce duplication of the mock setup without sacrificing clarity.♻️ Suggested consolidation
- test('renders with login as title when name is null', async () => { - const dataWithNullName = { - ...mockUserDetailsData, - user: { - ...mockUserDetailsData.user, - name: null, - }, - } - - ;(useQuery as unknown as jest.Mock).mockReturnValue({ - data: dataWithNullName, - loading: false, - error: null, - }) - - render(<UserDetailsPage />) - - await waitFor(() => { - // The page title should use login when name is null - const loginLink = screen.getByRole('link', { name: '@testuser' }) - expect(loginLink).toBeInTheDocument() - }) - }) - - test('renders placeholder avatar when avatarUrl is null', async () => { ... - }) - - test('renders with login as alt text when name is null', async () => { - const dataWithNullName = { - ...mockUserDetailsData, - user: { - ...mockUserDetailsData.user, - name: null, - }, - } - - ;(useQuery as unknown as jest.Mock).mockReturnValue({ - data: dataWithNullName, - loading: false, - error: null, - }) - - render(<UserDetailsPage />) - - await waitFor(() => { - const avatar = screen.getByAltText('testuser') - expect(avatar).toBeInTheDocument() - }) - }) + test('uses login as title and alt text when name is null', async () => { + const dataWithNullName = { + ...mockUserDetailsData, + user: { + ...mockUserDetailsData.user, + name: null, + }, + } + + ;(useQuery as unknown as jest.Mock).mockReturnValue({ + data: dataWithNullName, + loading: false, + error: null, + }) + + render(<UserDetailsPage />) + + await waitFor(() => { + const loginLink = screen.getByRole('link', { name: '@testuser' }) + expect(loginLink).toBeInTheDocument() + const avatar = screen.getByAltText('testuser') + expect(avatar).toBeInTheDocument() + }) + })Also applies to: 942-963
frontend/__tests__/unit/components/ToggleableList.test.tsx (1)
247-275: Consider adding a Space-key test for the disabled state.You test both click and Enter on a disabled button, but Space navigation (tested in the enabled case at line 219) has no corresponding disabled counterpart. For symmetry and completeness:
Suggested addition
+ it('does not navigate when pressing Space key on disabled item button', () => { + render( + <ToggleableList + entityKey="test" + items={['React', 'Next.js', 'FastAPI']} + label="Tags" + limit={2} + isDisabled={true} + /> + ) + const button = screen.getByText('React') + fireEvent.keyDown(button, { key: ' ' }) + expect(mockPush).not.toHaveBeenCalled() + })frontend/__tests__/unit/components/StructuredDataScript.test.tsx (1)
109-128: XSS test doesn't verify the sanitized output.The test asserts that
DOMPurify.sanitizewas called and that the input contains"John", but it never checks that the rendered script tag'sinnerHTMLis free of the<script>alert("xss")</script>payload. This means the test would pass even if the component ignored the sanitizer's return value.Proposed fix: assert on rendered output
render(<StructuredDataScript data={maliciousData} />) expect(mockDOMPurify).toHaveBeenCalled() const callArg = mockDOMPurify.mock.calls[0][0] expect(callArg).toContain('John') + + const { container } = render(<StructuredDataScript data={maliciousData} />) + const script = container.querySelector('script') + expect(script?.innerHTML).not.toContain('<script>') + expect(script?.innerHTML).toContain('John')Or more concisely, capture the container from the existing
rendercall:- render(<StructuredDataScript data={maliciousData} />) + const { container } = render(<StructuredDataScript data={maliciousData} />) expect(mockDOMPurify).toHaveBeenCalled() const callArg = mockDOMPurify.mock.calls[0][0] expect(callArg).toContain('John') + + const script = container.querySelector('script') + expect(script?.innerHTML).not.toContain('<script>')frontend/__tests__/unit/components/ContributionHeatmap.test.tsx (3)
196-227: Weak assertions — these tests verify rendering but not the fallback behavior they claim to test.The invalid-date tests (lines 196–227) only assert
toBeInTheDocument(), which passes even if the component silently breaks its date logic. To actually validate fallback behavior, assert ondata-series-length(like the missing-date tests above do) or check that the rendered data range matches the expected default 1-year range.
229-250: Near-duplicate tests for the same swapped-date scenario.Both tests verify
startDate > endDatehandling. The second test (line 241) is strictly weaker — it uses different dates but only assertstoBeInTheDocument(). Consider consolidating into a single test or, if keeping both, strengthen the second to assert the swap actually occurred (e.g., verify the resulting series data reflects the corrected range).
7-53: Mock exercisestooltip.customas a render side-effect purely for coverage — consider testing tooltip output explicitly.The mock invokes
tooltip.custom()during render (lines 22–34) but discards the return value. This achieves line coverage of the tooltip formatter but won't catch regressions in its output (e.g., wrong date format, wrong unit label). Consider capturing the return value and asserting on it in the tooltip-focused tests (lines 328–372), or at minimum storing it in a variable the tests can inspect.frontend/__tests__/unit/pages/MyMentorship.test.tsx (1)
263-264:globalThis.scrollTois overridden but never restored after the test.This leaks the mock to subsequent tests. Use
jest.spyOnor restore the original in anafterEachblock.Suggested fix using spyOn
- const scrollToMock = jest.fn() - globalThis.scrollTo = scrollToMock + const scrollToMock = jest.fn() + jest.spyOn(globalThis, 'scrollTo').mockImplementation(scrollToMock)This will be automatically restored by
jest.clearAllMocks()in the existingbeforeEach, or you can addjest.restoreAllMocks()if needed.frontend/__tests__/unit/pages/ProjectHealthDashboardMetricsDetails.test.tsx (1)
132-156: Test data matches the default mock — consider noting this for clarity.The overrides on lines 139-140 (
isFundingRequirementsCompliant: true,isLeaderRequirementsCompliant: false) are identical to the values inmockProjectsDashboardMetricsDetailsData. The test still works and the compliance-text assertions add coverage, but the explicit "override" is a no-op. This is fine functionally — just noting it for clarity so future readers don't assume a different scenario is being set up.frontend/__tests__/unit/pages/About.test.tsx (2)
606-697: Tests don't actually verify the progress-state rendering differences.Each test name implies it validates a distinct visual state ("Not Started", "In Progress", "Completed"), but all three only assert that the milestone title text appears in the document. There is no assertion on the progress bar value, status label/icon, or any CSS class that distinguishes the three states. As written, these tests would still pass even if the component ignored the
progressfield entirely.Consider asserting on the actual progress indicator (e.g., a status icon, a progress bar width/value, or a status label rendered by the component) to make the tests meaningful for their stated purpose. Alternatively, consolidate into a single parameterized test using
test.eachand add progress-specific assertions.
556-582: Duplicate of the "handles null project" test at lines 460–488.This test (
renders ErrorDisplay when project is null) asserts the same behavior and uses the same mock setup ashandles null project in data response gracefully(lines 460–488). One of them can be removed.frontend/__tests__/unit/pages/EditModule.test.tsx (4)
192-193: Misleading comment — this test is about a query error, not access denial.The comment "When denied but formData is null" doesn't match the test scenario, which mocks a GraphQL query error. Consider updating it to reflect the actual intent, e.g., "When query errors out, data is null so the component shows the spinner."
214-215: Same misleading comment as above — this is about an unauthenticated user, not denial.Update the comment to reflect that the unauthenticated session prevents form data from being prepared, hence the spinner.
305-308: Redundant assertion — both lines verify the same thing.Line 305 already waits for and asserts the "Minimal Module" value is present. Line 307 re-checks the same field via
getByLabelText. Consider dropping one or replacing Line 307 with an assertion on a different optional field (e.g., verifying description is empty, or domains field has a fallback value) to get more coverage value from this test.
176-194: Consider verifying that an error toast or redirect is (or isn't) triggered in these error/unauthenticated states.Both tests only assert the loading spinner is shown. For stronger coverage, you could additionally verify that
addToastwas or wasn't called, and that no navigation occurred, to confirm the component doesn't have unintended side effects in these states.Also applies to: 196-216
frontend/__tests__/unit/components/FontLoaderWrapper.test.tsx (1)
92-92: Nit: misleading test description."updates children when data prop changes" —
FontLoaderWrapperhas nodataprop; this test rerenders with differentchildren. Consider renaming to something like"updates rendered output when children change".frontend/__tests__/unit/components/SingleModuleCard.test.tsx (1)
396-411: Assertion could be more precise.Since
mentorsis set to[], only one show-more button (for mentees) should be present. UsingtoHaveLength(1)instead oftoBeGreaterThan(0)would make the test more specific and catch unexpected extra buttons.const showMoreButtons = screen.getAllByTestId('show-more-button') - expect(showMoreButtons.length).toBeGreaterThan(0) + expect(showMoreButtons).toHaveLength(1)frontend/__tests__/unit/utils/formValidationUtils.test.ts (2)
20-22: Redundant test — duplicates the empty-string case on Line 13.This test is described as "null-like empty" but passes an empty string
''exactly like the first test; onlyfieldNamediffers ('Name'vs'Field'). It doesn't exercise any new branch. Consider either removing it or replacing it with a genuinely distinct input (e.g., testing with a value coerced through the!valuecheck such asnullcast viaas unknown as string, if that's a realistic call-site scenario).
148-165: Consider extracting a shareddefaultFormDatafixture to reduce duplication.The same
formDataobject is redeclared in nearly every test. A singleconst defaultFormData = { ... }at thedescribescope (overridden per-test with spread where needed) would cut the boilerplate significantly.Also applies to: 186-203, 205-222, 224-241, 243-260, 262-279, 281-298, 300-314, 324-342, 344-362, 364-382, 384-402, 404-424
frontend/__tests__/unit/components/ContributorAvatar.test.tsx (3)
246-261: Casting away a required field doesn't reliably test the intended branch.Omitting
avatarUrlviaas unknown as Contributormeans the component receivesundefinedfor a required prop. This doesn't necessarily exercise theisAlgoliaContributor === falsebranch in a controlled way — it depends on how the component internally derives that flag. The test only asserts the element exists, without verifying any meaningful behavioral difference for the "non-Algolia" path.If the goal is to cover the false branch of
isAlgoliaContributor, consider mocking or spying on the logic that determines this flag, so the test is deterministic and documents the expected behavior difference.
263-279: Test name is misleading — duplicates an existing test.This test is named "shows repository info in tooltip when projectName is present" but the assertion on line 278 confirms that
projectNameis not shown. The comment on line 277 explains this, but the test name suggests the opposite.Additionally, this is functionally identical to the existing test on lines 210-217 ("does not include project name in tooltip"). Consider removing this duplicate or renaming it to avoid confusion.
244-359: Several tests in this block overlap with existing tests above.The new
describe('branch coverage for isAlgoliaContributor')block was intended to improve branch coverage, but multiple tests here duplicate scenarios already covered:
- Lines 263-279 ≈ Lines 210-217 (projectName not shown in tooltip)
- Lines 281-298 ≈ Lines 133-149 (no contributions shows only name)
Focus on tests that exercise genuinely uncovered branches rather than re-asserting existing behavior with slightly different fixture data.
frontend/__tests__/unit/pages/ProjectsHealthDashboardMetrics.test.tsx (2)
267-337: Extract repeated mock setup into a helper to reduce duplication.Every filter, sort, and reset test repeats the identical 7-line
mockFetchMore+useQuerysetup. A small helper would eliminate ~150 lines of boilerplate.♻️ Suggested helper
+ const renderWithFetchMore = () => { + const mockFetchMore = jest.fn() + ;(useQuery as unknown as jest.Mock).mockReturnValue({ + data: mockHealthMetricsData, + loading: false, + error: null, + fetchMore: mockFetchMore, + }) + render(<MetricsPage />) + return { mockFetchMore } + }Then each test becomes:
test('applies level filter - incubator', async () => { - const mockFetchMore = jest.fn() - ;(useQuery as unknown as jest.Mock).mockReturnValue({ - data: mockHealthMetricsData, - loading: false, - error: null, - fetchMore: mockFetchMore, - }) - render(<MetricsPage />) + renderWithFetchMore() const incubatorButton = screen.getByRole('button', { name: 'Incubator' }) fireEvent.click(incubatorButton) ...
580-665: URL-param initialization tests only check that the page renders — they don't verify the params had any effect.These tests set
order,health, orlevelURL params, then assert the heading exists. The heading renders regardless of params, so these tests would pass even if the URL-param parsing code were deleted. Consider asserting that the query variables or initial component state reflect the URL params.frontend/__tests__/unit/global-error.test.tsx (1)
19-36: Module-levelshouldThrowErrorflag lacks centralized reset.The mutable flag is toggled manually inside individual tests (set before render, reset at end). If an assertion throws before the reset line, the flag leaks into subsequent tests. Consider adding an
afterEachin theErrorWrapperdescribe block (or at the top level) to always reset it:Suggested improvement
describe('ErrorWrapper component', () => { + afterEach(() => { + shouldThrowError = false + }) + test('renders children without crashing', () => { - shouldThrowError = false render(frontend/__tests__/unit/pages/EditProgram.test.tsx (3)
43-51: Nit:clearAllMocksafter mock setup is confusing ordering.
jest.clearAllMocks()on line 50 only clears call/instance tracking (notmockReturnValueimplementations), so this works correctly. However, placing it after the mock setup is counterintuitive. Conventionally, clearing comes first so readers don't have to reason about whatclearAllMocksresets vs. doesn't.Suggested reorder
beforeEach(() => { + jest.clearAllMocks() ;(useRouter as jest.Mock).mockReturnValue({ push: mockPush, replace: mockReplace }) ;(useParams as jest.Mock).mockReturnValue({ programKey: 'program_1' }) ;(useApolloClient as jest.Mock).mockReturnValue({ query: mockQuery, }) ;(useMutation as unknown as jest.Mock).mockReturnValue([mockUpdateProgram, { loading: false }]) - jest.clearAllMocks() })
78-80: RedundantwaitForwrappingfindByText.
screen.findByTextalready polls/waits internally (it returns a promise that resolves when the element appears). Wrapping it inwaitForis unnecessary and adds noise. This pattern is repeated across multiple tests in this file.For example, this:
await waitFor(async () => { expect(await screen.findByText('Access Denied')).toBeInTheDocument() })can be simplified to:
expect(await screen.findByText('Access Denied')).toBeInTheDocument()Also applies to: 127-129, 144-146, 324-326, 346-348
276-298: GraphQL error test doesn't assert on the error itself.This test sets
error: new Error('GraphQL error')but only asserts the form renders. It doesn't verify any error-handling behavior (e.g., error message display, toast). If the component is expected to handle or surface this error, consider asserting that. If the component intentionally ignores query errors when data is still available, a brief comment explaining the intent would help.frontend/__tests__/unit/pages/CreateModule.test.tsx (1)
120-127: High timeouts (10s waitFor, 15s test) indicate potential test fragility.These timeouts are significantly above typical values and suggest the interaction chain (type → query → select → submit → navigate) may be slow or flaky in CI. This is acceptable as a pragmatic choice for a complex integration-style test, but consider whether the autocomplete/project-search interaction (lines 88-116) could be simplified by mocking at a higher level to reduce latency.
frontend/__tests__/unit/components/MentorshipPullRequest.test.tsx (2)
107-113: Weak assertion for avatar placeholder behavior.
expect(images.length).toBeLessThan(2)is overly permissive — it passes even if 0 images render (including when the component renders nothing at all). Consider directly asserting that the placeholder element exists, or at minimum assertimages.lengthequals the exact expected count.Suggested improvement
test('renders placeholder when author has no avatar URL', () => { const { container } = render(<MentorshipPullRequest pr={mockPullRequestNoAuthor} />) - // When no avatar URL, a div placeholder should be rendered instead of Image - const images = container.querySelectorAll('img') - // Only the TruncatedText link should have an image, not the avatar - expect(images.length).toBeLessThan(2) + // When no avatar URL, a placeholder div is rendered instead of an img element + const placeholder = container.querySelector('div.rounded-full') + expect(placeholder).toBeInTheDocument() })Adjust the selector to match whatever placeholder element the component actually renders.
155-160: Overlaps with the link test on lines 120–126.This test checks
target="_blank"andrel="noopener noreferrer"on the first link, which is already covered by the earlier test. Consider either removing this test or differentiating it (e.g., verifying that all links in the component have safe attributes, not just the first one).frontend/__tests__/unit/components/forms/shared/FormContainer.test.tsx (1)
112-124: Test name is misleading — FormContainer doesn't prevent default submission.The component simply passes
onSubmitthrough to the<form>element without callingpreventDefault()itself. This test only proves that the mock handler you wrote callspreventDefault(), not that the component does. Consider renaming this test (e.g.,"calls onSubmit handler on form submit") or removing it since Lines 31–37 already cover that scenario.frontend/__tests__/unit/components/forms/shared/FormButtons.test.tsx (1)
121-129: Duplicate test — already covered at Lines 110–119.The test at Line 121 (
does not call router.back when onCancel is provided) is fully redundant with the test at Line 110 which already assertsexpect(mockBack).not.toHaveBeenCalled()on Line 118. Consider removing this duplicate.frontend/__tests__/unit/components/TruncatedText.test.tsx (1)
203-216: No-op assertionexpect(true).toBe(true)— consider using a more meaningful assertion.While this is a known workaround for the
jest/expect-expectESLint rule, the test's intent (no error thrown after unmount + resize) would be better expressed withexpect(() => window.dispatchEvent(new Event('resize'))).not.toThrow()insideact, which is both self-documenting and satisfies the linter. Based on learnings,expect(true).toBe(true)no-op assertions are intentionally added as workarounds, but a more descriptive alternative exists here.frontend/__tests__/unit/components/ModuleForm.test.tsx (1)
589-658: Several selection-handling tests lack meaningful assertions after the action under test.
- Line 616–618: Comment acknowledges the assertion gap ("items array might not have project-1") but no follow-up.
- Line 628–631: Only asserts the button still exists after "all" selection — doesn't verify
onProjectChangewas (or wasn't) called.- Line 654–658: No assertion at all after clicking "Select Single Key."
These tests exercise code paths but don't verify observable outcomes, reducing their value as regression guards. Consider asserting on
mockOnProjectChangecalls or input value changes.frontend/__tests__/unit/pages/Header.test.tsx (5)
502-518: Outside-click test doesn't assert the actual behavior it claims to test.This test is titled "handles outside click correctly" but only asserts that
addEventListenerwas called with'click'— it never verifies that the menu actually closes after the outside click ondocument.body. The dedicated "Outside Click Handler" tests (lines 827–979) already do this properly.Consider either adding a menu-state assertion or removing this test in favor of the more thorough suite below.
741-784: Interaction betweenbeforeEachmock and per-test spy is fragile.
beforeEach(line 189) assignswindow.addEventListener = jest.fn(), then this test spies on it again withjest.spyOn(globalThis, 'addEventListener'). The spy wraps the already-mockedjest.fn(), so it records calls but the underlying handler is never truly registered on the window. The test works because it extracts and invokes the handler manually, but the layered mocking is confusing and easy to break.A cleaner approach: remove the blanket
window.addEventListener = jest.fn()frombeforeEachand usejest.spyOnconsistently across all tests that need to inspect event registration. Tests that only need to assert registration can use the spy'smock.calls.
828-868: Potential stale-handler issue:mockImplementationprevents real registration.The custom
mockImplementationat line 834 pushes click handlers into a local array but never calls the realaddEventListener, so the component's handler is never truly attached towindow. This is fine because the test invokes the handler directly (line 861), but it means the test is coupled to the internal registration order. If the component ever re-orders itsuseEffecthooks or batches handler registrations differently, grabbingclickHandlers[clickHandlers.length - 1]may silently pick the wrong handler.This isn't a blocking issue — just flagging it as a maintenance risk.
1080-1095: Fragile mobile-link identification via CSS class name.return link.className.includes('transition')Identifying the mobile menu's "About" link by checking for a
transitionCSS class is brittle — any Tailwind/styling refactor could break this selector without any functional change to the component. Consider querying within the mobile menu container (e.g.,findMobileMenu()?.querySelectorAll('a')) or using a more semantic selector to isolate the mobile instance.
610-614: "Handles missing headerLinks gracefully" doesn't actually test missing data.The mock in
jest.mock('utils/constants', ...)always providesheaderLinks, so this test is identical to the "handles undefined pathname gracefully" test above it — both just confirm the component renders without throwing under normal conditions. To truly test missingheaderLinks, the mock would need to be overridden to return an empty or undefined array for this specific test.frontend/__tests__/unit/components/DashboardWrapper.test.tsx (1)
36-38: Consider usingresetAllMocksinstead ofclearAllMocks.
jest.clearAllMocks()only clears call history (mock.calls, etc.) but preservesmockReturnValueandmockImplementationfrom prior tests. This creates implicit coupling between test cases (as seen in the "handles status transitions" test). Usingjest.resetAllMocks()would additionally clear return values and implementations, ensuring each test starts with a clean slate.Proposed fix
beforeEach(() => { - jest.clearAllMocks() + jest.resetAllMocks() + // Re-apply the notFound mock since resetAllMocks clears implementations + ;(notFound as jest.Mock).mockImplementation(() => { + throw new Error('notFound') + }) })frontend/__tests__/unit/pages/ModuleIssueDetailsPage.test.tsx (1)
502-528: Brittle DOM queries using CSS class selectors.Both placeholder avatar tests use
document.querySelector('[aria-hidden="true"].rounded-full.bg-gray-400')which couples the test to Tailwind class names. If the styling changes (e.g.,bg-gray-400→bg-gray-300), these tests break without any functional regression.Prefer adding a
data-testid="placeholder-avatar"to the component, or at minimum query byroleor other semantic attributes.Also applies to: 546-570
frontend/__tests__/unit/components/ChapterMapWrapper.test.tsx (2)
27-57:mockOnShareLocationis captured but never used in assertions.
mockOnShareLocation(line 27) has its implementation set toprops.onShareLocationeach time the mock renders (line 38), but no test ever calls or asserts onmockOnShareLocation. The share-location button'sonClickdirectly invokesprops.onShareLocation(line 50), so the real handler is exercised withoutmockOnShareLocation. This variable is dead code and can be removed to reduce confusion.♻️ Suggested cleanup
-const mockOnShareLocation = jest.fn() jest.mock('components/ChapterMap', () => { return function MockChapterMap(props: { geoLocData: Chapter[] showLocal: boolean style: React.CSSProperties userLocation?: { latitude: number; longitude: number } | null onShareLocation?: () => void }): JSX.Element { - // Capture the onShareLocation prop for testing - if (props.onShareLocation) { - mockOnShareLocation.mockImplementation(props.onShareLocation) - } return ( <div data-testid="chapter-map">
243-263: Redundantconsole.errorspy — already mocked inbeforeEach.Line 87 already spies on
console.errorwith a no-op implementation for every test. The additionaljest.spyOn(console, 'error')at line 246 creates a second spy wrapping the first, which works but is unnecessary. You can simply referenceconsole.errordirectly in the assertion.♻️ Suggested simplification
it('logs error when geolocation throws an error', async () => { const mockError = new Error('Geolocation permission denied') ;(geolocationUtils.getUserLocationFromBrowser as jest.Mock).mockRejectedValue(mockError) - const consoleSpy = jest.spyOn(console, 'error').mockImplementation(() => {}) const ChapterMapWrapper = await getChapterMapWrapper() const { getByTestId, queryByTestId } = render( <ChapterMapWrapper {...defaultProps} showLocationSharing={true} /> ) await waitFor(() => { expect(getByTestId('share-location-btn')).toBeInTheDocument() }) fireEvent.click(getByTestId('share-location-btn')) await waitFor(() => { - expect(consoleSpy).toHaveBeenCalledWith('Error detecting location:', mockError) + expect(console.error).toHaveBeenCalledWith('Error detecting location:', mockError) expect(queryByTestId('user-location')).not.toBeInTheDocument() }) })
There was a problem hiding this comment.
Actionable comments posted: 5
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/pages/ProjectsHealthDashboardMetrics.test.tsx (1)
70-72:⚠️ Potential issue | 🟡 Minor
clearAllMocksdoesn't resetmockReturnValue— tests that overrideuseSearchParamswill leak.
jest.clearAllMocks()only clears call history; it does not restore the implementation set bymockReturnValue. The URL-param tests (lines 588–689) overrideuseSearchParamsviamockReturnValue, and that override persists into subsequent tests. Today this works by accident because those tests happen to be last, but reordering or adding tests will cause surprising failures.Switch to
restoreAllMocksor explicitly resetuseSearchParamsinafterEach:Option A — restore in afterEach
afterEach(() => { - jest.clearAllMocks() + jest.restoreAllMocks() })Note:
restoreAllMocksrequires mocks created withjest.spyOn. Since these arejest.fn()insidejest.mockfactories, the safer alternative is:afterEach(() => { jest.clearAllMocks() + const { useSearchParams } = jest.requireMock('next/navigation') + ;(useSearchParams as jest.Mock).mockImplementation(() => new URLSearchParams()) })
🤖 Fix all issues with AI agents
In `@frontend/__tests__/unit/components/CardDetailsPage.test.tsx`:
- Around line 2325-2337: The test "does not render mentees section when no
mentees are provided" uses screen.getAllByTestId('contributors-list') which
throws when no matches exist; change that call to
screen.queryAllByTestId('contributors-list') in the CardDetailsPage.test.tsx
test so the negative assertion can safely check for absence (keep the rest of
the test logic that finds a list containing 'Mentees' and asserts it is
undefined or not present).
- Around line 2224-2259: The test titled "renders EntityActions for module type
when user is an admin" is asserting the page title instead of verifying
EntityActions; update the assertions in CardDetailsPage.test.tsx to explicitly
check the mocked EntityActions render by using
screen.getByTestId('entity-actions') for positive cases and
screen.queryByTestId('entity-actions') for negative cases (the "does not render"
tests), ensuring you target the CardDetailsPage render and the mocked
EntityActions component instead of relying on other text like "Test Project".
In `@frontend/__tests__/unit/components/ProgramForm.test.tsx`:
- Around line 293-301: The test currently uses buttons.find(...) to get
submitButton and guards with if (submitButton) which can silently skip the click
and let the test pass vacuously; replace that pattern by asserting the button
exists and then performing the click — e.g., use a direct query
(screen.getByRole or screen.getByText) to retrieve the submit button or add an
explicit expect(submitButton).toBeInTheDocument() before calling await
user.click(submitButton), updating every occurrence (the submitButton lookup and
click logic in these tests) so mockOnSubmit is actually exercised.
- Around line 226-240: The test named "calls setFormData when mentees limit
changes" is vacuous; update it to actually find the mentees limit input in the
ProgramForm (e.g., locate by label text/placeholder or role used by the
component), simulate changing its value (use fireEvent.change or userEvent.type
on that input), and assert that the mockSetFormData prop was called with the
expected updated formData (or at least called once with a payload containing the
new mentees limit). If the component updates asynchronously, wrap the assertion
in waitFor; reference ProgramForm, setFormData, and mockSetFormData to locate
the relevant props and behavior.
- Around line 804-836: The two tests named "handles start date changes and
triggers touched state" and "handles end date changes" in ProgramForm.test.tsx
only assert label presence; either rename them to reflect that they only check
rendering or update them to perform the interactions: render <ProgramForm ...>,
locate the actual date inputs via screen.getByLabelText('Start Date') /
screen.getByLabelText('End Date'), use fireEvent.change (or userEvent) to change
the value, and assert mockSetFormData (or the touched state handler) is called
with the expected updated formData; keep references to ProgramForm,
mockSetFormData, and the test titles when making the change.
🧹 Nitpick comments (14)
frontend/__tests__/unit/components/DashboardWrapper.test.tsx (1)
146-156: Redundant rerender before any state change.The
rerenderat lines 152–156 uses the same mock value as the initialrenderat line 146 — no state has changed, so it exercises nothing new. You can remove it without losing coverage.Proposed fix
const { rerender } = render( <DashboardWrapper> <div>Dashboard Content</div> </DashboardWrapper> ) - rerender( - <DashboardWrapper> - <div>Dashboard Content</div> - </DashboardWrapper> - ) - expect(screen.getByTestId('loading-spinner')).toBeInTheDocument()frontend/__tests__/unit/pages/About.test.tsx (2)
606-706: Consider usingtest.eachto reduce duplication across the three milestone tests.All three tests are identical except for
progressandtitle. A parameterized test would cut ~70 lines down to ~30 and make it trivial to add more boundary values later.♻️ Suggested refactor
- test('renders milestone with progress 0 - Not Started', async () => { - ;(useQuery as unknown as jest.Mock).mockImplementation((query) => { - if (query === GetAboutPageDataDocument) { - return { - loading: false, - data: { - project: { - ...mockAboutData.project, - recentMilestones: [ - { - ...mockAboutData.project.recentMilestones[0], - progress: 0, - title: 'Not Started', - }, - ], - }, - topContributors: mockAboutData.topContributors, - leader1: mockAboutData.users['arkid15r'], - leader2: mockAboutData.users['kasya'], - leader3: mockAboutData.users['mamicidal'], - }, - error: null, - } - } - return { loading: true } - }) - await act(async () => { - render(<About />) - }) - await waitFor(() => { - expect(screen.getByText('Not Started')).toBeInTheDocument() - }) - }) - - test('renders milestone with progress 50 - In Progress', async () => { - // ... ~30 nearly identical lines ... - }) - - test('renders milestone with progress 100 - Completed', async () => { - // ... ~30 nearly identical lines ... - }) + test.each([ + { progress: 0, title: 'Not Started' }, + { progress: 50, title: 'In Progress Milestone' }, + { progress: 100, title: 'Completed' }, + ])('renders milestone with progress $progress - $title', async ({ progress, title }) => { + ;(useQuery as unknown as jest.Mock).mockImplementation((query) => { + if (query === GetAboutPageDataDocument) { + return { + loading: false, + data: { + project: { + ...mockAboutData.project, + recentMilestones: [ + { + ...mockAboutData.project.recentMilestones[0], + progress, + title, + }, + ], + }, + topContributors: mockAboutData.topContributors, + leader1: mockAboutData.users['arkid15r'], + leader2: mockAboutData.users['kasya'], + leader3: mockAboutData.users['mamicidal'], + }, + error: null, + } + } + return { loading: true } + }) + await act(async () => { + render(<About />) + }) + await waitFor(() => { + expect(screen.getByText(title)).toBeInTheDocument() + }) + })
635-637: Tests only assert title text, not progress-specific rendering.Each test verifies the milestone title appears but doesn't check anything that varies with
progress(e.g., progress bar width, status icon, CSS class). As-is, these tests would still pass if the component ignored theprogressvalue entirely. Consider adding at least one assertion per test that validates the visual or structural difference driven byprogressto make the coverage more meaningful.Also applies to: 669-671, 703-705
frontend/__tests__/unit/components/ModuleCard.test.tsx (3)
284-310:preventDefault: jest.fn()infireEvent.keyDownhas no effect.
fireEvent.keyDowncreates a realKeyboardEvent; properties likepreventDefaultin the event-init object are ignored (they aren't part ofKeyboardEventInit). If the component callse.preventDefault()on Enter/Space, these tests don't actually verify that. The toggle itself still fires becausekeyDownis dispatched, so the test passes — but thepreventDefaultassertion is absent.If verifying
preventDefaultis a goal, usejest.spyOnon the event or check withuserEvent. Otherwise, simply drop thepreventDefaultfield to avoid implying it's tested.Proposed cleanup
- fireEvent.keyDown(showMoreButton, { key: 'Enter', preventDefault: jest.fn() }) + fireEvent.keyDown(showMoreButton, { key: 'Enter' })- fireEvent.keyDown(showMoreButton, { key: ' ', preventDefault: jest.fn() }) + fireEvent.keyDown(showMoreButton, { key: ' ' })
327-337: Test title overpromises — React keys aren't observable from the DOM.The test name says it verifies
keyvsidpriority, but it only asserts both modules render. React keys are internal and not reflected in the DOM. This is effectively a smoke test for the fallback code path, which is fine, but the title is misleading.Consider renaming to something like
"renders modules regardless of whether key or id is used"to match what's actually asserted.
726-730: Negative-duration test documents surprising behavior — consider flagging upstream.
getSimpleDuration('2024-03-01', '2024-01-01')returning'-8 weeks'is technically "working as coded," but it's almost certainly not a meaningful value for users. If the component is expected to guard against end < start, this test should assert a fallback (e.g.,'N/A'or'Invalid duration'). If this is intentional, a brief comment explaining why would help future readers.Not blocking, but worth raising with the component owner.
frontend/__tests__/unit/pages/ProjectsHealthDashboardMetrics.test.tsx (4)
215-267: Extract repeated mock setup into a helper to reduce ~100 lines of duplication.Every filter and sort test (lines 215–568) repeats the same
mockFetchMore+useQuerymock block verbatim. Additionally,mockFetchMoreis never exercised in these tests — it's dead code here.Suggested helper
+ const renderWithDefaults = () => { + ;(useQuery as unknown as jest.Mock).mockReturnValue({ + data: mockHealthMetricsData, + loading: false, + error: null, + fetchMore: jest.fn(), + }) + render(<MetricsPage />) + }Then each filter/sort test becomes:
test('applies health filter - healthy', async () => { - const mockFetchMore = jest.fn() - ;(useQuery as unknown as jest.Mock).mockReturnValue({ - data: mockHealthMetricsData, - loading: false, - error: null, - fetchMore: mockFetchMore, - }) - render(<MetricsPage />) + renderWithDefaults() const healthyButton = screen.getByRole('button', { name: 'Healthy' }) fireEvent.click(healthyButton)
587-601: URL-param initialization tests only verify the page doesn't crash — not that params are actually consumed.These tests (lines 587–689) set URL params like
order=scoreAscorhealth=healthybut only assert the heading renders. They won't catch a regression where the component silently ignores URL params.Consider asserting that
useQuerywas called with variables reflecting the URL params, or thatmockReplacewas called to round-trip the params:render(<MetricsPage />) - await expectHeaderVisible() - // Verify component rendered without errors - expect(screen.getByRole('heading', { name: 'Project Health Metrics' })).toBeInTheDocument() + await waitFor(() => { + expect(useQuery).toHaveBeenCalledWith( + expect.anything(), + expect.objectContaining({ + variables: expect.objectContaining({ orderBy: 'score_asc' }), + }) + ) + })(Adjust variable names to match the actual GraphQL query shape.)
192-213: Pagination test could assert the page/offset variable passed tofetchMore.The test confirms
fetchMorewas called and exercisesupdateQuery, but doesn't verify that the correct page or offset was requested. Consider:await waitFor(() => { - expect(mockFetchMore).toHaveBeenCalled() + expect(mockFetchMore).toHaveBeenCalledWith( + expect.objectContaining({ + variables: expect.objectContaining({ page: 2 }), + }) + ) })(Adjust the variable name to match your GraphQL query.)
122-161: Theexpect(true).toBe(true)lines are recognized as ESLint workarounds — no action needed.These no-ops exist because
jest/expect-expectdoesn't detect assertions inside helper functions. Consider configuringassertFunctionNamesin your ESLint config to recognize helpers likeexpectLoadingSpinnerExistsandexpectHeaderVisibleso these placeholders can be removed. Based on learnings, this pattern can be resolved by configuring the ESLint rule'sassertFunctionNamesoption.frontend/__tests__/unit/pages/CreateModule.test.tsx (2)
120-127: Consider reducing the excessive timeouts.A
waitFortimeout of 10 seconds and a Jest timeout of 15 seconds are unusually high for a unit test with fully mocked dependencies. If the mocks resolve synchronously (as they should), these should complete in well under 1 second. Excessive timeouts mask flaky tests and slow down CI feedback when a test does fail (it hangs for up to 15 s before reporting).If the project autocomplete involves a debounce (likely from the
user.type(projectInput, 'Aw')on line 92), consider usingjest.useFakeTimers()to advance past it deterministically, then reduce these timeouts to ~3–5 s as a safety margin.
199-229: Mixingjest.useFakeTimers()withwaitForis fragile.
waitForuses real timers internally for its retry/polling loop. When fake timers are active, those internal timers are frozen, which can causewaitForto hang or timeout if the assertion doesn't pass on the very first check. This is a well-known pitfall in Testing Library.Two suggestions:
- Use
advanceTimersoption: Passjest.useFakeTimers({ advanceTimers: true })(Jest ≥ 27) so that Testing Library's internal timers still advance automatically.- Move
jest.useRealTimers()intoafterEach: If the test fails before line 228, fake timers leak into subsequent tests.♻️ Suggested improvement
it('shows access denied and redirects when user is not an admin', async () => { - jest.useFakeTimers() + jest.useFakeTimers({ advanceTimers: true }) ;(useSession as jest.Mock).mockReturnValue({And for cleanup robustness, consider restoring timers in
afterEach:afterEach(() => { jest.clearAllMocks() + jest.useRealTimers() })Then remove the inline
jest.useRealTimers()call at line 228.frontend/__tests__/unit/components/ProgramForm.test.tsx (2)
940-960: Duplicate of the "marks fields as touched when submitting" test (lines 326–347).This test has the exact same setup (render with
defaultFormData), action (click Save), and assertion (mockOnSubmitnot called) as the earlier test. Neither test actually verifies the touched state — both only verify that submission is blocked. Remove this duplicate or make it meaningfully different (e.g., assert that validation error messages appear after the click).
875-910: Async uniqueness check asserted synchronously — inconsistent with similar tests.The name uniqueness check goes through an async Apollo query. Here the assertion at line 909 is synchronous, while the equivalent tests at lines 456–461 and 1074–1079 use
waitForwith a timeout. With immediately-resolving mocks this likely works, but for consistency and resilience against timing changes, prefer wrapping inwaitFor:- // Should prevent submission because 'Test Program' === 'test program' (case-insensitive) - expect(mockOnSubmit).not.toHaveBeenCalled() + await waitFor(() => { + expect(mockOnSubmit).not.toHaveBeenCalled() + })
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
frontend/__tests__/unit/components/ToggleableList.test.tsx (2)
205-317:⚠️ Potential issue | 🔴 CriticalCritical: File has multiple syntax errors from overlapping test blocks that will fail to parse.
Biome confirms a parse error at Line 281, but the issue is systemic. New test blocks are opened but never closed before existing test blocks begin, creating illegally nested
it()calls in at least four places:
New (unclosed) block opens at Existing it()illegally nested atLine 205 Line 214 Line 249 Line 258 Line 281 Line 291 Line 298 Line 312 In each case the new
it(...)render block is missing its closing})and assertions before the nextit(starts. This looks like a bad merge or copy-paste where new tests were inserted without removing or properly integrating with the existing overlapping tests.You need to either:
- Remove the duplicates — keep one version of each test (the new or the existing), or
- Close each new block before the next
it()and ensure both versions test meaningfully different things.For example, at lines 205–219 you have two tests for "Enter key navigation" — one with
limit={2}and three items, one with three different items and no limit. Decide which to keep (or keep both with proper closure):Example fix for lines 205–219 (apply similar pattern to the other three locations)
it('navigates when Enter key is pressed on item button', () => { render( <ToggleableList entityKey="test" items={['React', 'Next.js', 'FastAPI']} label="Tags" limit={2} /> ) + const button = screen.getByText('React') + fireEvent.keyDown(button, { key: 'Enter' }) + expect(mockPush).toHaveBeenCalledWith('/projects?q=React') + }) + it('handles Enter key press on item button', () => { render(<ToggleableList entityKey="test" items={['React', 'Vue', 'Angular']} label="Tags" />) const button = screen.getByText('React') fireEvent.keyDown(button, { key: 'Enter' }) expect(mockPush).toHaveBeenCalledWith('/projects?q=React') })
258-270:⚠️ Potential issue | 🟡 MinorTest at Line 265 (
'prevents default behavior on keyboard event') doesn't actually verifypreventDefault.Even after fixing the syntax errors above, this test only asserts
mockPushwas called — it doesn't verify thatpreventDefault()was invoked. Either rename it to reflect what it actually tests, or add a properpreventDefaultassertion:it('prevents default behavior on keyboard event', () => { render(<ToggleableList entityKey="test" items={['React']} label="Tags" />) const button = screen.getByText('React') - fireEvent.keyDown(button, { key: 'Enter' }) - expect(mockPush).toHaveBeenCalled() + const event = new KeyboardEvent('keydown', { key: 'Enter', bubbles: true }) + const preventSpy = jest.spyOn(event, 'preventDefault') + button.dispatchEvent(event) + expect(preventSpy).toHaveBeenCalled() })
🤖 Fix all issues with AI agents
In `@frontend/__tests__/unit/pages/ModuleIssueDetailsPage.test.tsx`:
- Around line 406-419: The test 'renders "(today)" for deadline that is exactly
today' is timezone-flaky because todayDeadline is built at UTC midnight; update
the test to produce a deadline at local midnight or else mock the current time:
change the construction of todayDeadline in the test (the variable todayDeadline
used in the dataWithTodayDeadline passed to mockUseQuery and rendered by
ModuleIssueDetailsPage) to use local-date midnight (e.g., using new Date(year,
month, date).toISOString()) or stub Date.now()/Date globally for the test so
ModuleIssueDetailsPage sees the intended "today" regardless of tester timezone.
🧹 Nitpick comments (3)
frontend/__tests__/unit/components/ToggleableList.test.tsx (1)
312-341: Redundant tests for disabled state — consider consolidating.After fixing the syntax errors, you'll have many near-duplicate disabled-state tests: Lines 272–279, 291–296, 312–317, 319–323, 325–329, 331–335, 337–341 all test small variations of
isDisabled. Consider consolidating into fewer, focused tests (e.g., one test for disabled interaction blocking across click/Enter/Space, one for CSS classes and thedisabledattribute). This reduces maintenance burden without sacrificing coverage.frontend/__tests__/unit/pages/ModuleIssueDetailsPage.test.tsx (2)
502-528: Querying by Tailwind class names is brittle.
document.querySelector('[aria-hidden="true"].rounded-full.bg-gray-400')couples the test to specific Tailwind utility classes. Any styling refactor (e.g., changing the placeholder color) will break these tests without any behavioral change. Prefer adding adata-testid(e.g.,data-testid="placeholder-avatar") to the placeholder element in the component and querying by that instead.Also applies to: 546-570
421-434: Test name is slightly misleading.The test is named
'shows "Loading issue…" title when issueId is not available'but it doesn't check for a page-level "Loading issue…" heading — it checks a button'stitleattribute. Consider renaming to something like'assign button shows "Loading issue…" tooltip when issueId is empty'to make the intent clearer to future readers.
anurag2787
force-pushed
the
fix/frontend-test
branch
from
f69fb5b to
f34e2a0
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
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/pages/Header.test.tsx (1)
188-193:⚠️ Potential issue | 🟠 Major
addEventListener/removeEventListenerreplaced by assignment —restoreAllMockswon't restore them.Direct assignment (
window.addEventListener = jest.fn()) bypasses Jest's spy tracking, sojest.restoreAllMocks()inafterEachnever restores the originals. This leaks into subsequent test files. Usejest.spyOninstead so the originals are properly restored.🐛 Proposed fix
- // Mock window methods - window.addEventListener = jest.fn() - window.removeEventListener = jest.fn() + jest.spyOn(window, 'addEventListener') + jest.spyOn(window, 'removeEventListener')
🤖 Fix all issues with AI agents
In `@frontend/__tests__/unit/components/CardDetailsPage.test.tsx`:
- Around line 2368-2428: The tests for program EntityActions in
CardDetailsPage.test.tsx currently only assert the page title and never check
whether EntityActions is present; update the three tests ("renders program
EntityActions when type is program with appropriate access", "does not render
program EntityActions when canUpdateStatus is false", "does not render program
EntityActions when accessLevel is not admin") to assert on the EntityActions
element instead of just the title: when EntityActions should be present use
screen.getByTestId('entity-actions') or screen.getByText for a known action to
assert presence, and when it should not be present use
expect(screen.queryByTestId('entity-actions')).toBeNull() or
expect(...).not.toBeInTheDocument(); locate assertions in the tests that call
render(<CardDetailsPage {...programProps} />) and replace the existing
expect(screen.getByText('Test Project')) checks with the appropriate
presence/absence assertions for EntityActions.
In `@frontend/__tests__/unit/components/ModuleForm.test.tsx`:
- Around line 602-635: The test's comment is misleading: handleSelectionChange
in ModuleForm.tsx supports Set<React.Key> and triggers onProjectChange for the
selected project; update the test that currently only asserts
mockOnProjectChange was called with (null, 'Test') to also assert that
mockOnProjectChange was called with the selected project's id and name (e.g.,
'project-1', 'Test Project 1') after the Set selection via the select button,
and remove or revise the comment that claims the Set is ignored; reference the
test helpers renderProjectSelector, the mock mockOnProjectChange, and the
component method handleSelectionChange when locating where to add the additional
assertion.
- Around line 440-491: Update the two weak tests: in the "prevents default form
submission" test (using renderModuleForm and mockOnSubmit) actually fire a
submit event with a jest.fn() preventDefault handler (e.g.,
fireEvent.submit(form, { preventDefault })) and assert that preventDefault was
called and mockOnSubmit was not called; in the "sets all fields as touched on
submit" test, after rendering via renderModuleForm and submitting the form,
assert that each form field (name, description, startedAt, endedAt, projectId,
experienceLevel) is marked as touched by checking for validation feedback (e.g.,
presence of validation error messages) or field state (aria-invalid or a touched
CSS class) so the test verifies the touched behavior rather than only submitting
the form.
In `@frontend/__tests__/unit/components/TruncatedText.test.tsx`:
- Around line 203-216: The test in TruncatedText.test.tsx falsely asserts the
null-branch of checkTruncation because unmount() removes the resize listener
before the dispatched event; either remove the test or change it to actually
invoke the stored resize handler after unmount: spy on window.addEventListener
in the test to capture the handler passed for 'resize', call unmount(), then
call that captured handler (so checkTruncation runs with no element) and assert
no throw; reference the checkTruncation behavior and the resize event handler
captured via window.addEventListener in your change.
🧹 Nitpick comments (7)
frontend/__tests__/unit/components/ChapterMapWrapper.test.tsx (4)
7-26: Mock fornext/dynamicworks but the unused-param workaround is noisy.The
if (options) { /* intentionally unused */ }block (lines 13-15) exists only to silence a lint warning. A cleaner approach is to prefix the parameter with an underscore.✏️ Suggested simplification
return function mockDynamic( importFn: () => Promise<{ default: React.ComponentType<unknown> }>, - options?: { ssr?: boolean } + _options?: { ssr?: boolean } ) { - // Ignore options for SSR: false — reference it without using `void` - if (options) { - /* intentionally unused */ - } - // Return a component that resolves the import synchronously for testing const Component = React.lazy(importFn)
245-265: Redundantconsole.errorspy —beforeEachalready installs one.Line 89 already spies on
console.errorwith a no-op implementation. The spy at line 248 creates a second layer on top. It works, but the localconsoleSpyvariable is unnecessary — you can assert directly on the spy installed inbeforeEachby capturing it at a higher scope, or simply referenceconsole.errorsince it's already mocked.✏️ Suggested change
it('logs error when geolocation throws an error', async () => { const mockError = new Error('Geolocation permission denied') ;(geolocationUtils.getUserLocationFromBrowser as jest.Mock).mockRejectedValue(mockError) - const consoleSpy = jest.spyOn(console, 'error').mockImplementation(() => {}) const ChapterMapWrapper = await getChapterMapWrapper() ... await waitFor(() => { - expect(consoleSpy).toHaveBeenCalledWith('Error detecting location:', mockError) + expect(console.error).toHaveBeenCalledWith('Error detecting location:', mockError) expect(queryByTestId('user-location')).not.toBeInTheDocument() }) })
267-292: Test name claims to verify_distanceremoval, but the assertion only checks array length.Asserting
geo-loc-data-lengthequals'2'doesn't prove_distancewas stripped. The mockChapterMapreceivesgeoLocDataas a prop — you could inspect the actual prop values to confirm_distanceis absent.✏️ Strengthen the assertion
One approach: have the mock
ChapterMaprender the stringifiedgeoLocData, then parse and check for_distance:// In MockChapterMap, add: + <span data-testid="geo-loc-data">{JSON.stringify(props.geoLocData)}</span> // In the test: await waitFor(() => { - expect(getByTestId('geo-loc-data-length')).toHaveTextContent('2') + const data = JSON.parse(getByTestId('geo-loc-data').textContent!) + expect(data).toHaveLength(2) + for (const chapter of data) { + expect(chapter).not.toHaveProperty('_distance') + } })
307-317: Style forwarding test doesn't actually verify the style prop value.The test renders with
customStylebut only asserts the element is in the document. It should check that the renderedchapter-mapelement has the expected inline styles.✏️ Suggested fix
await waitFor(() => { - expect(getByTestId('chapter-map')).toBeInTheDocument() + const map = getByTestId('chapter-map') + expect(map).toHaveStyle({ width: '500px', height: '300px' }) })frontend/__tests__/unit/pages/Header.test.tsx (1)
484-499: Resize test asserts nothing meaningful.The
globalThis.dispatchEvent(new Event('resize'))on line 495 doesn't reach the component's handler becauseaddEventListeneris mocked (no-op). The test ends withexpect(true).toBe(true), confirming nothing about resize behavior. This gives false confidence. Consider either removing this test (the "Resize Handler" describe block already covers this properly) or restructuring it to extract and invoke the handler like the later tests do.frontend/__tests__/unit/components/TruncatedText.test.tsx (1)
180-191: Misleading test name — doesn't verify null-ref behavior.The test is titled "handles textRef becoming null during lifecycle" but only does a normal rerender followed by unmount. React keeps the ref populated during rerender, so this never actually tests the null path. The assertions only verify that the updated text and title render correctly, which is already covered by the "handles multiple rapid text changes" test.
Consider renaming to something like "updates correctly on rerender and unmount" to match what it actually verifies.
frontend/__tests__/unit/components/CardDetailsPage.test.tsx (1)
449-473: Avoidanycasts in mock — consider a minimal type.The Leaders mock uses
as any[]and(user: any, ...)with eslint-disable comments. A simple inline type would remove the need for these suppressions and make the mock more self-documenting:Suggested refactor
- default: ({ users, ...props }: { users: unknown[]; [key: string]: unknown }) => { - // eslint-disable-next-line `@typescript-eslint/no-explicit-any` - const usersList = users as any[] + default: ({ users, ...props }: { users: Array<{ login?: string; member?: { name?: string }; memberName?: string; description?: string }>; [key: string]: unknown }) => { + const usersList = users return ( <div data-testid="leaders" {...props}> <h3>Leaders</h3> {Array.isArray(usersList) && - // eslint-disable-next-line `@typescript-eslint/no-explicit-any` - usersList.map((user: any, index: number) => { + usersList.map((user, index) => { const uniqueKey = `leader-${index}-${user.login || 'unknown'}`
There was a problem hiding this comment.
Actionable comments posted: 1
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/SortBy.test.tsx (1)
98-122:⚠️ Potential issue | 🔴 Critical
const orderButtonis redeclared in the same block scope — this is a SyntaxError.Lines 103 & 105 (and similarly 116 & 118) both declare
const orderButtoninside the sameasyncarrow. This will crash at parse time, preventing these two tests from running. It looks like the newgetByLabelTextlines were added but the oldbuttons[1]lines were not removed.Proposed fix — remove the stale declarations
await act(async () => { - const orderButton = screen.getByLabelText(/Sort in ascending order/i) - const buttons = screen.getAllByRole('button') - const orderButton = buttons[1] + const orderButton = screen.getByLabelText(/Sort in ascending order/i) fireEvent.keyDown(orderButton, { key: 'Enter' }) }) expect(defaultProps.onOrderChange).toHaveBeenCalledWith('desc') @@ -115,8 +113,6 @@ await act(async () => { - const orderButton = screen.getByLabelText(/Sort in descending order/i) - const buttons = screen.getAllByRole('button') - const orderButton = buttons[1] + const orderButton = screen.getByLabelText(/Sort in descending order/i) fireEvent.keyDown(orderButton, { key: ' ' }) })
🤖 Fix all issues with AI agents
In `@frontend/__tests__/unit/pages/CreateModule.test.tsx`:
- Around line 199-229: The test "shows access denied and redirects when user is
not an admin" currently calls jest.useFakeTimers() and then jest.useRealTimers()
at the end, which leaks fake timers if an assertion fails; fix by making timer
cleanup exception-safe: either move the test body into a try/finally and call
jest.useRealTimers() in the finally block (ensuring jest.useRealTimers() always
runs even if waitFor or expect throws), or better, remove the inline calls and
enable/restore timers via beforeEach/afterEach for this test scope; keep
references to the same mocks (useSession, useQuery, useMutation), and ensure
calls like jest.advanceTimersByTime(2000) and assertions against mockReplace
remain unchanged.
🧹 Nitpick comments (4)
frontend/__tests__/unit/pages/IssuesPage.test.tsx (1)
294-319: Consider combining the empty and nullavailableLabelstests withtest.each.This test is nearly identical to the one at lines 265–292, differing only in the
availableLabelsvalue (nullvs[]). Atest.eachordescribe.eachcould reduce duplication, similar to the pattern already used for issue states at lines 194–215. This is a minor nit — perfectly fine to leave as-is.frontend/__tests__/unit/components/SortBy.test.tsx (1)
145-159: Duplicate test — the new test (lines 145-151) asserts the same behavior as the existing test (lines 153-159).Both tests render with
selectedSortOption="default"and verify the sort-order button is absent viaqueryByLabelText. Remove one of them to avoid redundancy.Proposed fix — drop the duplicate
- it('hides sort order button when selectedSortOption is "default"', async () => { - await act(async () => { - render(<SortBy {...defaultProps} selectedSortOption="default" />) - }) - const orderButtons = screen.queryByLabelText(/Sort in ascending order/i) - expect(orderButtons).not.toBeInTheDocument() - }) - it('does not render order button when selectedSortOption is default', async () => {frontend/__tests__/unit/pages/ModuleIssueDetailsPage.test.tsx (1)
502-528: Prefer Testing Library queries overdocument.querySelectorwith CSS classes.Querying by CSS class names (
rounded-full.bg-gray-400) couples these tests to styling implementation details. If classes change (e.g., Tailwind refactor), these tests break silently without any actual behavioral regression. Consider adding adata-testid="placeholder-avatar"to the placeholder element in the component, then usingscreen.getByTestId/screen.getAllByTestIdhere.Also applies to: 546-570
frontend/__tests__/unit/pages/CreateModule.test.tsx (1)
120-127: Excessive timeouts suggest flaky test setup.A 10 s
waitFortimeout and a 15 s Jest timeout are red flags — they usually mean the mocked mutation or navigation isn't resolving synchronously as expected. Because all Apollo hooks are manually mocked,mockCreateModuleshould resolve in the same microtask queue, making a sub-second timeout sufficient. If these high values are needed to make the test pass, the root cause is likely that the component or mock wiring isn't firing the mutation as expected, and the test is just "timing out gracefully" rather than catching a real failure.Consider reducing both to something like
{ timeout: 3000 }/5000and investigating why the mock doesn't resolve quickly.
There was a problem hiding this comment.
1 issue found across 7 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/components/FontLoaderWrapper.test.tsx">
<violation number="1" location="frontend/__tests__/unit/components/FontLoaderWrapper.test.tsx:138">
P3: This test title claims to verify `document.fonts.ready.then()` is called, but the new implementation only checks that content renders after resolving the promise. Either reintroduce an explicit `.then()` spy or rename the test to match what it actually asserts to avoid misleading coverage.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
arkid15r
changed the title
kasya
left a comment
kasya
left a comment
There was a problem hiding this comment.
@anurag2787 Awesome work 👏🏼
Thank you!
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
frontend/__tests__/unit/components/StructuredDataScript.test.tsx
Dismissed
Show dismissed
Hide dismissed
frontend/__tests__/unit/components/StructuredDataScript.test.tsx
Dismissed
Show dismissed
Hide dismissed
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3864 +/- ##
==========================================
+ Coverage 93.13% 95.37% +2.23%
==========================================
Files 463 463
Lines 14531 14541 +10
Branches 2061 2016 -45
==========================================
+ Hits 13533 13868 +335
+ Misses 538 329 -209
+ Partials 460 344 -116
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 50 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
* Run make update * Clean up snapshot generated videos * Update backend/data/nest.dump * feat(ui): revamp corporate supporters carousel (Infinite Marquee + Dark Mode fix) (#3837) * feat(ui): revamp corporate supporters carousel (Infinite Marquee + Dark Mode fix) * fix: resolve failing test case * fix: add fallback text for unnamed sponsors * docs: add docstrings to satisfy coverage requirements * Run make check and fix tests. --------- Co-authored-by: Kate <kate@kgthreads.com> * Fix/redundant typescript assertion (#3834) * Fix Sonar S4325 by narrowing session user fields instead of casting * Fix unused ExtendedSession in mentorship page * fix: redundant-typescript-assertion * Fix stale latest date displayed in Project Health Dashboard metrics (#3842) * Fixed latest date in proejct health dashboard * updated order * Update code * Update code --------- Co-authored-by: Arkadii Yakovets <arkadii.yakovets@owasp.org> * feat: improve backend test coverage to 96% (#3840) * feat: improve backend test coverage to 96% * fix comments * fix issues * fix issue * fix cubic-dev-ai comments * Update code * Fix tests --------- Co-authored-by: Arkadii Yakovets <arkadii.yakovets@owasp.org> Co-authored-by: Arkadii Yakovets <2201626+arkid15r@users.noreply.github.com> * Fix: merge consecutive RUN instructions in frontend Dockerfile (#3644) * Fix: merge consecutive RUN instructions in frontend Dockerfile * fix: comment Dockerfile note to prevent syntax error * Update code * Update code --------- Co-authored-by: Arkadii Yakovets <arkadii.yakovets@owasp.org> Co-authored-by: Arkadii Yakovets <2201626+arkid15r@users.noreply.github.com> * Fix 'is_merged' not being available on the Issue (#3843) * Fix 'is_merged' not being available on the Issue * Update code --------- Co-authored-by: Arkadii Yakovets <arkadii.yakovets@owasp.org> * CI: Add ansible-lint workflow for Ansible playbooks (#3796) * ci: add ansible-lint workflow Signed-off-by: Muhammad Hassaan Saleem <iamhassaans@gmail.com> * Update .github/workflows/lint-ansible.yaml Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * ci: add ansible-lint make target and workflow Signed-off-by: Muhammad Hassaan Saleem <iamhassaans@gmail.com> * ci: add ansible-lint pre-commit hook Signed-off-by: Muhammad Hassaan Saleem <iamhassaans@gmail.com> * fix: whitespace & version Signed-off-by: Muhammad Hassaan Saleem <iamhassaans@gmail.com> * Update Makefile Co-authored-by: cubic-dev-ai[bot] <191113872+cubic-dev-ai[bot]@users.noreply.github.com> * ci: enable ansible-lint scanning and add requirements.yml Signed-off-by: Muhammad Hassaan Saleem <iamhassaans@gmail.com> * chore(ansible):align linting and module usage Signed-off-by: Muhammad Hassaan Saleem <iamhassaans@gmail.com> * ci(ansible): install collections before deploy playbooks Signed-off-by: Muhammad Hassaan Saleem <iamhassaans@gmail.com> * Update code * Update code * Update .github/workflows/run-ci-cd.yaml --------- Signed-off-by: Muhammad Hassaan Saleem <iamhassaans@gmail.com> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: cubic-dev-ai[bot] <191113872+cubic-dev-ai[bot]@users.noreply.github.com> Co-authored-by: Arkadii Yakovets <arkadii.yakovets@owasp.org> * Fix ElevenLabs API error (#3861) * use default liam voice * bump speed by 0.10 --------- Co-authored-by: Arkadii Yakovets <2201626+arkid15r@users.noreply.github.com> * Add Ime Iyonsi to MENTORS.md (#3866) * Add mentor profile for Ime Iyonsi Added Ime Iyonsi's mentor profile. * Fix GitHub link for Ime Iyonsi Corrected GitHub link for Ime Iyonsi. * Update code --------- Co-authored-by: Arkadii Yakovets <arkadii.yakovets@owasp.org> * Update MENTORS.md * Enabled Strict Mode (#3776) * Enabled Strict Mode * fixed ai review * fix * fixed review * fix * update test * Update code --------- Co-authored-by: Arkadii Yakovets <arkadii.yakovets@owasp.org> Co-authored-by: Arkadii Yakovets <2201626+arkid15r@users.noreply.github.com> * Resolve case-sensitivity in QueryParser to support Chapters/Members search (#3844) * resolve query parser blocker * use case_sensitive flag in QueryParser * feat: add case_sensitive option to QueryParser and update tests * Update code --------- Co-authored-by: Arkadii Yakovets <arkadii.yakovets@owasp.org> Co-authored-by: Arkadii Yakovets <2201626+arkid15r@users.noreply.github.com> * Update dependencies (#3874) * Update dependencies * Bump django-ninja version * fix(proxy): pin nginx and certbot images (#3848) * fix(proxy): pin nginx and certbot images Signed-off-by: Muhammad Hassaan Saleem <iamhassaans@gmail.com> * fix stable verssions Signed-off-by: Muhammad Hassaan Saleem <iamhassaans@gmail.com> --------- Signed-off-by: Muhammad Hassaan Saleem <iamhassaans@gmail.com> Co-authored-by: Arkadii Yakovets <2201626+arkid15r@users.noreply.github.com> * Update docker-compose/proxy/compose.yaml * Update backend/pyproject.toml * Update ansible lint configuration (#3880) * Update .github/ansible/.ansible-lint.yaml * Improve frontend test coverage above 80% and add missing test files (#3864) * Imrove test coverage to 80% and added test * Fixed coderabbit review * update code * fixed coderabbit ai * fixed soanrqube warning * fixed review * update * fixed aloglia cache_key (#3825) * fixed aloglia cache_key * change separator val to be semicolon (;) * Update code * add tests + use json filters * add trailing newline * Update code --------- Co-authored-by: Arkadii Yakovets <arkadii.yakovets@owasp.org> Co-authored-by: Arkadii Yakovets <2201626+arkid15r@users.noreply.github.com> * fix: remove unused className prop from AnchorTitle component (#3822) * fix: remove unused className prop from AnchorTitle component Fixes #3805 The className prop was defined in AnchorTitleProps but never used in the component implementation. Removing it resolves Sonar rule typescript:S6767 and improves code maintainability. * fix: use className prop instead of removing it - Added className back to AnchorTitleProps interface - Accept className parameter in component - Apply className to root div element - Resolves reviewer feedback on PR #3822 * Update code --------- Co-authored-by: Arkadii Yakovets <arkadii.yakovets@owasp.org> Co-authored-by: Arkadii Yakovets <2201626+arkid15r@users.noreply.github.com> --------- Signed-off-by: Muhammad Hassaan Saleem <iamhassaans@gmail.com> Co-authored-by: Arkadii Yakovets <arkadii.yakovets@owasp.org> Co-authored-by: Yashraj Pahuja <yashrajpahuja9999@gmail.com> Co-authored-by: Kate <kate@kgthreads.com> Co-authored-by: CodeAritraDhank <aritradhank21@gmail.com> Co-authored-by: Anurag Yadav <143180737+anurag2787@users.noreply.github.com> Co-authored-by: Harshit Verma <harshit1092004@gmail.com> Co-authored-by: Arkadii Yakovets <2201626+arkid15r@users.noreply.github.com> Co-authored-by: Shuban Mutagi <shubanmutagi55@gmail.com> Co-authored-by: Muhammad Hassaan Saleem <iamhassaans@gmail.com> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: cubic-dev-ai[bot] <191113872+cubic-dev-ai[bot]@users.noreply.github.com> Co-authored-by: emaybu <152900874+emaybu@users.noreply.github.com> Co-authored-by: sai chethana <saichethanavesireddy@gmail.com> Co-authored-by: Rahul Paul <179798584+Mr-Rahul-Paul@users.noreply.github.com> Co-authored-by: Lavanya <lavanyayadawad30@gmail.com>
2 participants
Proposed change
This PR improves frontend unit test coverage for files that were previously below the 80% line coverage threshold. It also adds new test files for components that previously had no test coverage.
Resolves #3810
Checklist
make check-testlocally: all warnings addressed, tests passed