Add comprehensive unit tests for ContributionHeatmap #2751
Conversation
|
Warning Rate limit exceeded@Swyamk has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 16 minutes and 14 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughAdds a new comprehensive unit test file for the ContributionHeatmap component that mocks chart and theme dependencies and validates rendering, data-edge cases, date-range handling, theming, accessibility, chart configuration, tooltip behavior, lifecycle, and large-data scenarios. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
frontend/__tests__/unit/components/ContributionHeatmap.test.tsx (4)
33-35: Avoid using array index as React key in the mock (Sonar warning).Even though this is test‑only code, switching away from index keys removes the Sonar warning and better models real usage:
- {mockSeries.map((series, idx) => ( - <div key={idx} data-testid={`series-${series.name}`}>{series.name}: {series.data.length} data points</div> - ))} + {mockSeries.map(series => ( + <div key={series.name} data-testid={`series-${series.name}`}>{series.name}: {series.data.length} data points</div> + ))}This keeps behavior identical for the tests while satisfying the static analysis rule.
As per static analysis hints.
135-143: Make the “large datasets (365 days)” test deterministic (avoidMath.random).Using
Math.random()here doesn’t currently affect assertions, but it adds unnecessary nondeterminism to the test run. You can keep the same coverage with a simple deterministic pattern:- for (let i = 0; i < 365; i++) { - const date = new Date('2024-01-01') - date.setDate(date.getDate() + i) - largeData[date.toISOString().split('T')[0]] = Math.floor(Math.random() * 20) - } + for (let i = 0; i < 365; i++) { + const date = new Date('2024-01-01') + date.setDate(date.getDate() + i) + // Deterministic but varied values in [0, 19] + largeData[date.toISOString().split('T')[0]] = i % 20 + }Same shape, simpler debugging if a future assertion is added around values.
147-155: Improve dark‑mode styling assertion so it actually differentiates themes.In the light/dark theme test, both expectations currently check for the same class (
text-gray-700) regardless of theme:expect(screen.getByText('Light').parentElement).toHaveClass('text-gray-700') ... expect(screen.getByText('Dark').parentElement).toHaveClass('text-gray-700')If the component is supposed to style light and dark modes differently, this won’t catch regressions. Consider asserting a dark‑specific class (or some other dark‑mode indicator) for the second render, or checking that the theme‑dependent class actually changes between the two cases.
356-367: Consider tightening lifecycle test assertions beyond “chart exists”.The “maintains consistent rendering across multiple updates” test currently only asserts that the mock chart is present after several rerenders. This gives you crash/regression coverage, but not much signal on state consistency.
If you want slightly stronger guarantees with minimal extra work, you could also assert on something like
data-series-lengthor the presence/absence of expectedseries-*nodes after the final rerender, to ensure the component actually respects the updated data.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/__tests__/unit/components/ContributionHeatmap.test.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/__tests__/unit/pages/createProgram.test.tsx:70-86
Timestamp: 2025-07-12T17:36:57.255Z
Learning: When testing React page components that use mocked form components, validation logic should be tested at the form component level, not the page level. Page-level tests should focus on authentication, role checking, submission handling, and navigation logic.
📚 Learning: 2025-07-03T03:08:03.290Z
Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 1703
File: frontend/src/components/BarChart.tsx:33-46
Timestamp: 2025-07-03T03:08:03.290Z
Learning: In the OWASP Nest project's BarChart component (frontend/src/components/BarChart.tsx), the days and requirements arrays are guaranteed to always have the same length in their use cases, so input validation for array length matching is not needed.
Applied to files:
frontend/__tests__/unit/components/ContributionHeatmap.test.tsx
🪛 GitHub Check: SonarCloud Code Analysis
frontend/__tests__/unit/components/ContributionHeatmap.test.tsx
[failure] 23-23: Remove this use of the "void" operator.
[warning] 34-34: Do not use Array index in keys
frontend/__tests__/unit/components/ContributionHeatmap.test.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
frontend/__tests__/unit/components/ContributionHeatmap.test.tsx (1)
19-24: Remove thevoidoperator usage to fix SonarCloud failure.This issue was previously flagged. The
void (hasLightMode || hasDarkMode)is a no-op that Sonar rejects. Simply remove thevoidwrapper:if (typeof result === 'string') { - const hasLightMode = result.includes('#FFFFFF') || result.includes('#E5E7EB') - const hasDarkMode = result.includes('#1F2937') || result.includes('#374151') - // This ensures both light and dark mode branches are covered - void (hasLightMode || hasDarkMode) + // Force evaluation of both light and dark mode template branches + result.includes('#FFFFFF') || result.includes('#E5E7EB') + result.includes('#1F2937') || result.includes('#374151') }
🧹 Nitpick comments (3)
frontend/__tests__/unit/components/ContributionHeatmap.test.tsx (3)
33-35: Useseries.nameas key instead of array index.Since
series.nameis unique (Mon, Tue, etc.), prefer it over the array index to satisfy the React key best practice and Sonar warning.- {mockSeries.map((series, idx) => ( - <div key={idx} data-testid={`series-${series.name}`}>{series.name}: {series.data.length} data points</div> + {mockSeries.map((series) => ( + <div key={series.name} data-testid={`series-${series.name}`}>{series.name}: {series.data.length} data points</div> ))}
208-241: Tooltip tests lack meaningful assertions.These tests verify that the chart renders, but don't assert actual tooltip behavior. The mock invokes
tooltip.custom, but none of these tests verify the returned HTML content. Consider adding assertions on the tooltip output in the mock, or explicitly capturing and testing the tooltip string.For example, you could store the tooltip result and verify it contains expected date formats or unit labels:
// In mock or test const tooltipResult = tooltip.custom({ seriesIndex: 0, dataPointIndex: 0, w: { config: { series: mockSeries } } }) expect(tooltipResult).toContain('Jan 01, 2024') expect(tooltipResult).toContain('5 commits')
407-410: Test name doesn't match assertions.The test is named "disables toolbar and legend as expected" but only asserts that the chart renders. Either rename to match behavior, or enhance the mock to expose and verify these options.
- it('disables toolbar and legend as expected', () => { + it('renders chart with configured options', () => { renderWithTheme(<ContributionHeatmap {...defaultProps} />) expect(screen.getByTestId('mock-heatmap-chart')).toBeInTheDocument() })Alternatively, capture the
props.optionsin the mock and verifyoptions.chart.toolbar.show === falseandoptions.legend.show === false.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/__tests__/unit/components/ContributionHeatmap.test.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/__tests__/unit/pages/createProgram.test.tsx:70-86
Timestamp: 2025-07-12T17:36:57.255Z
Learning: When testing React page components that use mocked form components, validation logic should be tested at the form component level, not the page level. Page-level tests should focus on authentication, role checking, submission handling, and navigation logic.
📚 Learning: 2025-11-17T16:47:05.578Z
Learnt from: anurag2787
Repo: OWASP/Nest PR: 2671
File: frontend/__tests__/unit/components/MultiSearch.test.tsx:427-427
Timestamp: 2025-11-17T16:47:05.578Z
Learning: In the frontend test files for the OWASP/Nest repository, `expect(true).toBe(true)` no-op assertions may be intentionally added as workarounds when ESLint rule jest/expect-expect doesn't detect expectations inside helper functions or waitFor callbacks. These can be resolved by configuring the ESLint rule's assertFunctionNames option to recognize custom assertion helpers instead of flagging them as redundant.
Applied to files:
frontend/__tests__/unit/components/ContributionHeatmap.test.tsx
📚 Learning: 2025-11-17T17:30:42.139Z
Learnt from: anurag2787
Repo: OWASP/Nest PR: 2671
File: frontend/__tests__/unit/components/MultiSearch.test.tsx:169-171
Timestamp: 2025-11-17T17:30:42.139Z
Learning: In the OWASP/Nest frontend tests (PR #2671 context), wrapper functions like `expectChaptersCountEqualsThree` that simply call another helper with a fixed parameter (e.g., `expectChaptersCountEquals(3)`) are intentionally used to avoid arrow function callbacks in `waitFor` calls. This pattern prevents adding nesting depth that would trigger rule typescript:S2004. Example: `await waitFor(expectChaptersCountEqualsThree)` avoids the extra nesting from `await waitFor(() => expectChaptersCountEquals(3))`.
Applied to files:
frontend/__tests__/unit/components/ContributionHeatmap.test.tsx
📚 Learning: 2025-07-03T03:08:03.290Z
Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 1703
File: frontend/src/components/BarChart.tsx:33-46
Timestamp: 2025-07-03T03:08:03.290Z
Learning: In the OWASP Nest project's BarChart component (frontend/src/components/BarChart.tsx), the days and requirements arrays are guaranteed to always have the same length in their use cases, so input validation for array length matching is not needed.
Applied to files:
frontend/__tests__/unit/components/ContributionHeatmap.test.tsx
🪛 GitHub Check: SonarCloud Code Analysis
frontend/__tests__/unit/components/ContributionHeatmap.test.tsx
[failure] 23-23: Remove this use of the "void" operator.
[warning] 34-34: Do not use Array index in keys
🔇 Additional comments (4)
frontend/__tests__/unit/components/ContributionHeatmap.test.tsx (4)
41-50: LGTM!The mock setup for
next-themesand therenderWithThemehelper are well-structured. The helper properly configures the theme mock before rendering, enabling theme-aware tests.
52-95: LGTM!The test setup with
mockDataanddefaultPropsis clean. The "Rendering & Props" tests appropriately verify minimal props rendering, optional props handling, all 7-day series creation, and chart type configuration. The semicolon before the array on line 84 correctly prevents ASI issues.
378-385: Verify non-ISO date format handling.The test uses
'2024-1-2'(single-digit month/day) which is not ISO 8601 compliant ('2024-01-02'). This is valuable edge case testing, but verify the component actually handles this format, or clarify the test intent with a comment.it('formats date strings correctly in different formats', () => { const dateFormatData = { '2024-01-01': 5, - '2024-1-2': 10, // Single digit month/day + '2024-1-2': 10, // Non-ISO format - tests component resilience to malformed input }
428-487: LGTM!The integration and real-world scenario tests provide excellent coverage of realistic use cases including GitHub-like contribution patterns, empty data handling, and combined edge cases (leap year, year boundaries, zero activity). These tests effectively validate the component's behavior in production-like scenarios.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
frontend/__tests__/unit/components/ContributionHeatmap.test.tsx (1)
12-25: Unresolved: Replace no-op expression with explicit assertion.This workaround still requires an eslint-disable comment and executes a side-effect-free expression just for branch coverage. The past review correctly identified this pattern as a code smell.
Replace the no-op with an explicit assertion to document the intent:
- if (typeof result === 'string') { - const hasLightMode = result.includes('#FFFFFF') || result.includes('#E5E7EB') - const hasDarkMode = result.includes('#1F2937') || result.includes('#374151') - // This ensures both light and dark mode branches are covered - // eslint-disable-next-line @typescript-eslint/no-unused-expressions - hasLightMode || hasDarkMode - } + if (typeof result === 'string') { + // Verify tooltip contains theme-appropriate colors + const hasThemeColors = + result.includes('#FFFFFF') || result.includes('#E5E7EB') || + result.includes('#1F2937') || result.includes('#374151') + expect(hasThemeColors).toBe(true) + }This removes the eslint-disable, clarifies intent, and makes the check meaningful.
Based on learnings, if this assertion helper pattern doesn't satisfy jest/expect-expect, you can configure the ESLint rule's
assertFunctionNamesoption rather than disabling the unused-expressions rule.
🧹 Nitpick comments (2)
frontend/__tests__/unit/components/ContributionHeatmap.test.tsx (2)
7-40: Consider separating tooltip test logic from mock render.The mock currently executes
tooltip.customduring render (lines 12-30) to achieve branch coverage. While this works, it couples test execution to component instantiation and makes the mock harder to maintain.Consider extracting tooltip testing to dedicated test cases that directly invoke the formatter, or verify tooltip content through rendered output rather than inline execution.
That said, if this pattern successfully achieves the desired coverage for all template literal branches and aligns with the project's testing strategy, it's acceptable to keep as-is.
79-79: Optional: Consider helper for theme-switching rerenders.Several tests manually wrap components in
ThemeProviderduring rerenders (lines 79, 154, 199, 230, 364, 396). You could extract arerenderWithThemehelper to reduce repetition:const rerenderWithTheme = (rerender: ReturnType<typeof render>['rerender'], ui: React.ReactElement, theme: 'light' | 'dark' = 'light') => { const { useTheme } = require('next-themes') useTheme.mockReturnValue({ theme, setTheme: jest.fn() }) rerender(<ThemeProvider attribute="class">{ui}</ThemeProvider>) }Then use:
rerenderWithTheme(rerender, <ContributionHeatmap {...props} />, 'dark')The current explicit wrapping is clear and maintainable, so this is purely a DRY consideration.
Also applies to: 154-154, 199-199, 230-230, 364-364, 396-396
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/__tests__/unit/components/ContributionHeatmap.test.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/__tests__/unit/pages/createProgram.test.tsx:70-86
Timestamp: 2025-07-12T17:36:57.255Z
Learning: When testing React page components that use mocked form components, validation logic should be tested at the form component level, not the page level. Page-level tests should focus on authentication, role checking, submission handling, and navigation logic.
📚 Learning: 2025-11-17T16:47:05.578Z
Learnt from: anurag2787
Repo: OWASP/Nest PR: 2671
File: frontend/__tests__/unit/components/MultiSearch.test.tsx:427-427
Timestamp: 2025-11-17T16:47:05.578Z
Learning: In the frontend test files for the OWASP/Nest repository, `expect(true).toBe(true)` no-op assertions may be intentionally added as workarounds when ESLint rule jest/expect-expect doesn't detect expectations inside helper functions or waitFor callbacks. These can be resolved by configuring the ESLint rule's assertFunctionNames option to recognize custom assertion helpers instead of flagging them as redundant.
Applied to files:
frontend/__tests__/unit/components/ContributionHeatmap.test.tsx
📚 Learning: 2025-11-17T17:30:42.139Z
Learnt from: anurag2787
Repo: OWASP/Nest PR: 2671
File: frontend/__tests__/unit/components/MultiSearch.test.tsx:169-171
Timestamp: 2025-11-17T17:30:42.139Z
Learning: In the OWASP/Nest frontend tests (PR #2671 context), wrapper functions like `expectChaptersCountEqualsThree` that simply call another helper with a fixed parameter (e.g., `expectChaptersCountEquals(3)`) are intentionally used to avoid arrow function callbacks in `waitFor` calls. This pattern prevents adding nesting depth that would trigger rule typescript:S2004. Example: `await waitFor(expectChaptersCountEqualsThree)` avoids the extra nesting from `await waitFor(() => expectChaptersCountEquals(3))`.
Applied to files:
frontend/__tests__/unit/components/ContributionHeatmap.test.tsx
📚 Learning: 2025-07-03T03:08:03.290Z
Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 1703
File: frontend/src/components/BarChart.tsx:33-46
Timestamp: 2025-07-03T03:08:03.290Z
Learning: In the OWASP Nest project's BarChart component (frontend/src/components/BarChart.tsx), the days and requirements arrays are guaranteed to always have the same length in their use cases, so input validation for array length matching is not needed.
Applied to files:
frontend/__tests__/unit/components/ContributionHeatmap.test.tsx
🔇 Additional comments (2)
frontend/__tests__/unit/components/ContributionHeatmap.test.tsx (2)
47-51: LGTM: Clean theme testing helper.The
renderWithThemehelper provides a convenient way to test components under different theme contexts. The use ofrequire()to access the mockeduseThemeis appropriate for Jest mocks.
53-489: Excellent comprehensive test coverage.The test suite thoroughly addresses all objectives from issue #2630:
- ✅ Minimal and full props rendering
- ✅ Conditional rendering and prop-based behavior
- ✅ Theme support (light/dark modes)
- ✅ Edge cases (empty data, date boundaries, leap years, large datasets)
- ✅ Accessibility (semantic headings, ARIA attributes)
- ✅ Responsive design and styling
- ✅ Lifecycle (re-renders, unmount/remount)
- ✅ Tooltip behavior and formatting
- ✅ Real-world scenarios
The organization into logical describe blocks makes the suite maintainable and easy to navigate. Test data is realistic and covers boundary conditions effectively.
| // eslint-disable-next-line @typescript-eslint/no-require-imports | ||
| const { useTheme } = require('next-themes') |
There was a problem hiding this comment.
@Swyamk we do not allow using require in our code - this is why you were getting a linter error and it's a very bad practice to just disable it with the comment.
Please update these tests to import module properly.
Swyamk
force-pushed
the
test/contribution-heatmap-tests
branch
from
9a09bc5 to
bd983c8
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
frontend/__tests__/unit/components/ContributionHeatmap.test.tsx (1)
70-75:requireusage should be replaced with proper ES module imports.This was already flagged by the reviewer. The pattern is repeated throughout the file (lines 196, 296). Consider refactoring to use
jest.mocked()pattern:+import { useTheme } from 'next-themes' + +const mockedUseTheme = jest.mocked(useTheme) + const renderWithTheme = (ui: React.ReactElement, theme: 'light' | 'dark' = 'light') => { - // eslint-disable-next-line @typescript-eslint/no-require-imports - const { useTheme } = require('next-themes') - ;(useTheme as jest.Mock).mockReturnValue({ theme, setTheme: jest.fn() }) + mockedUseTheme.mockReturnValue({ theme, setTheme: jest.fn() }) return render(<ThemeProvider attribute="class">{ui}</ThemeProvider>) }Then update the other usages at lines 196-197 and 296-297 similarly.
🧹 Nitpick comments (3)
frontend/__tests__/unit/components/ContributionHeatmap.test.tsx (3)
261-265: Test doesn't actually verify SSR behavior.This test is identical to the minimal props rendering test. To properly test SSR/dynamic import behavior, you'd need to verify the component handles server-side rendering appropriately (e.g., checking for
typeof windowguards or Next.jsdynamic()configuration). Consider either removing this test or adding meaningful SSR-specific assertions.
267-316: Tooltip tests lack actual tooltip content assertions.These tests render the component but don't verify the tooltip output. The mock exercises the tooltip callback internally (lines 24-44), but the test assertions only check that the chart renders. Consider verifying the tooltip HTML output in the mock or adding data attributes to expose tooltip content for testing.
554-558: Test doesn't verify toolbar/legend configuration.The test name suggests verifying that toolbar and legend are disabled, but it only checks the chart renders. Consider exposing these options via data attributes in the mock or removing the misleading test name.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/__tests__/unit/components/ContributionHeatmap.test.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/__tests__/unit/pages/createProgram.test.tsx:70-86
Timestamp: 2025-07-12T17:36:57.255Z
Learning: When testing React page components that use mocked form components, validation logic should be tested at the form component level, not the page level. Page-level tests should focus on authentication, role checking, submission handling, and navigation logic.
📚 Learning: 2025-11-17T16:47:05.578Z
Learnt from: anurag2787
Repo: OWASP/Nest PR: 2671
File: frontend/__tests__/unit/components/MultiSearch.test.tsx:427-427
Timestamp: 2025-11-17T16:47:05.578Z
Learning: In the frontend test files for the OWASP/Nest repository, `expect(true).toBe(true)` no-op assertions may be intentionally added as workarounds when ESLint rule jest/expect-expect doesn't detect expectations inside helper functions or waitFor callbacks. These can be resolved by configuring the ESLint rule's assertFunctionNames option to recognize custom assertion helpers instead of flagging them as redundant.
Applied to files:
frontend/__tests__/unit/components/ContributionHeatmap.test.tsx
📚 Learning: 2025-11-17T17:30:42.139Z
Learnt from: anurag2787
Repo: OWASP/Nest PR: 2671
File: frontend/__tests__/unit/components/MultiSearch.test.tsx:169-171
Timestamp: 2025-11-17T17:30:42.139Z
Learning: In the OWASP/Nest frontend tests (PR #2671 context), wrapper functions like `expectChaptersCountEqualsThree` that simply call another helper with a fixed parameter (e.g., `expectChaptersCountEquals(3)`) are intentionally used to avoid arrow function callbacks in `waitFor` calls. This pattern prevents adding nesting depth that would trigger rule typescript:S2004. Example: `await waitFor(expectChaptersCountEqualsThree)` avoids the extra nesting from `await waitFor(() => expectChaptersCountEquals(3))`.
Applied to files:
frontend/__tests__/unit/components/ContributionHeatmap.test.tsx
📚 Learning: 2025-07-03T03:08:03.290Z
Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 1703
File: frontend/src/components/BarChart.tsx:33-46
Timestamp: 2025-07-03T03:08:03.290Z
Learning: In the OWASP Nest project's BarChart component (frontend/src/components/BarChart.tsx), the days and requirements arrays are guaranteed to always have the same length in their use cases, so input validation for array length matching is not needed.
Applied to files:
frontend/__tests__/unit/components/ContributionHeatmap.test.tsx
🔇 Additional comments (16)
frontend/__tests__/unit/components/ContributionHeatmap.test.tsx (16)
1-6: LGTM!Standard imports for React Testing Library tests with proper dependencies.
7-63: Well-structured mock for ApexCharts.The mock correctly exercises the tooltip callback for coverage while exposing useful test attributes. The ESLint disable for unused expressions is an acceptable workaround for ensuring branch coverage.
65-68: LGTM!Clean mock implementation for
next-themesthat allows controlled theme testing.
77-96: LGTM!Good test setup with representative mock data and proper test isolation via
beforeEach.
97-126: LGTM!Comprehensive coverage of rendering with minimal and optional props, verifying the 7-day series structure and chart attributes.
128-185: LGTM!Excellent edge case coverage including empty data, boundary values, various date ranges, and large datasets. The cleanup pattern with
unmount()ensures proper test isolation.
221-236: LGTM!Good accessibility coverage verifying semantic heading structure and proper ARIA roles.
318-340: LGTM as smoke tests.These tests verify rendering with specific date configurations. Week calculation logic verification would require inspecting the series data structure, which is handled internally by the mock.
342-382: LGTM!Good boundary value testing for color scale logic, covering all activity level thresholds.
384-434: LGTM!Excellent date edge case coverage including leap year handling, year boundary transitions, and unordered data input.
436-481: LGTM!Good robustness testing with negative values, floating-point numbers, and extremely large counts. These validate the component's resilience to unusual inputs.
483-510: LGTM!Good lifecycle testing covering multiple rapid updates and unmount/remount cycles.
512-545: LGTM!Good coverage of string formatting edge cases including non-standard date formats and special characters in unit strings.
560-573: LGTM!Good responsive design testing verifying CSS properties for aspect ratio and minimum height.
575-642: LGTM!Comprehensive integration tests covering realistic scenarios and combined edge cases. Good closing tests for the suite.
188-204: Verify the theme-specific class assertions against the component implementation.Both light and dark mode tests assert
text-gray-700(lines 193 and 203). Confirm whether the ContributionHeatmap component applies the same text color class regardless of theme or if different classes should be used for dark mode. If theme-dependent styling is intended, the assertions should differ.
…cessary comments and removing the use of require
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
frontend/__tests__/unit/components/ContributionHeatmap.test.tsx (1)
29-33: Thevoidoperator is still present and causing SonarCloud failure.This issue was flagged in a previous review and marked as addressed in commit 9ccb47d, but the code still contains the problematic
voidusage. SonarCloud is still reporting a failure on line 32.Apply this diff to fix the Sonar violation while preserving coverage intent:
if (typeof result === 'string') { - const hasLightMode = result.includes('#FFFFFF') || result.includes('#E5E7EB') - const hasDarkMode = result.includes('#1F2937') || result.includes('#374151') - void (hasLightMode || hasDarkMode) + // Force evaluation of light/dark mode template branches for coverage + expect( + result.includes('#FFFFFF') || + result.includes('#E5E7EB') || + result.includes('#1F2937') || + result.includes('#374151') + ).toBeDefined() }Alternatively, if you just need the side-effect of the includes calls without assertions:
if (typeof result === 'string') { - const hasLightMode = result.includes('#FFFFFF') || result.includes('#E5E7EB') - const hasDarkMode = result.includes('#1F2937') || result.includes('#374151') - void (hasLightMode || hasDarkMode) + // Force evaluation of both light and dark mode template branches + result.includes('#FFFFFF') || result.includes('#E5E7EB') + result.includes('#1F2937') || result.includes('#374151') }
🧹 Nitpick comments (1)
frontend/__tests__/unit/components/ContributionHeatmap.test.tsx (1)
257-302: Tooltip behavior tests lack meaningful assertions.These tests have descriptive names like "generates correct tooltip with date formatting" and "handles singular and plural unit labels" but only assert that the chart renders. While the mock does invoke
tooltip.custom()to exercise the code path, the actual tooltip output isn't verified.Consider capturing and asserting on the tooltip result in your mock:
// In the mock, store tooltip results for assertion let lastTooltipResult: string | undefined // ... inside MockChart: if (tooltip.custom) { lastTooltipResult = tooltip.custom({...}) as string } // Then in tests: it('generates correct tooltip with date formatting', () => { renderWithTheme(<ContributionHeatmap {...defaultProps} />) expect(lastTooltipResult).toContain('Jan 1, 2024') // or expected format })This would make the tests actually validate the described behavior rather than just exercising code paths.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/__tests__/unit/components/ContributionHeatmap.test.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/__tests__/unit/pages/createProgram.test.tsx:70-86
Timestamp: 2025-07-12T17:36:57.255Z
Learning: When testing React page components that use mocked form components, validation logic should be tested at the form component level, not the page level. Page-level tests should focus on authentication, role checking, submission handling, and navigation logic.
📚 Learning: 2025-11-17T16:47:05.578Z
Learnt from: anurag2787
Repo: OWASP/Nest PR: 2671
File: frontend/__tests__/unit/components/MultiSearch.test.tsx:427-427
Timestamp: 2025-11-17T16:47:05.578Z
Learning: In the frontend test files for the OWASP/Nest repository, `expect(true).toBe(true)` no-op assertions may be intentionally added as workarounds when ESLint rule jest/expect-expect doesn't detect expectations inside helper functions or waitFor callbacks. These can be resolved by configuring the ESLint rule's assertFunctionNames option to recognize custom assertion helpers instead of flagging them as redundant.
Applied to files:
frontend/__tests__/unit/components/ContributionHeatmap.test.tsx
📚 Learning: 2025-11-17T17:30:42.139Z
Learnt from: anurag2787
Repo: OWASP/Nest PR: 2671
File: frontend/__tests__/unit/components/MultiSearch.test.tsx:169-171
Timestamp: 2025-11-17T17:30:42.139Z
Learning: In the OWASP/Nest frontend tests (PR #2671 context), wrapper functions like `expectChaptersCountEqualsThree` that simply call another helper with a fixed parameter (e.g., `expectChaptersCountEquals(3)`) are intentionally used to avoid arrow function callbacks in `waitFor` calls. This pattern prevents adding nesting depth that would trigger rule typescript:S2004. Example: `await waitFor(expectChaptersCountEqualsThree)` avoids the extra nesting from `await waitFor(() => expectChaptersCountEquals(3))`.
Applied to files:
frontend/__tests__/unit/components/ContributionHeatmap.test.tsx
📚 Learning: 2025-07-03T03:08:03.290Z
Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 1703
File: frontend/src/components/BarChart.tsx:33-46
Timestamp: 2025-07-03T03:08:03.290Z
Learning: In the OWASP Nest project's BarChart component (frontend/src/components/BarChart.tsx), the days and requirements arrays are guaranteed to always have the same length in their use cases, so input validation for array length matching is not needed.
Applied to files:
frontend/__tests__/unit/components/ContributionHeatmap.test.tsx
🪛 GitHub Check: SonarCloud Code Analysis
frontend/__tests__/unit/components/ContributionHeatmap.test.tsx
[failure] 32-32: Remove this use of the "void" operator.
🔇 Additional comments (3)
frontend/__tests__/unit/components/ContributionHeatmap.test.tsx (3)
560-627: Good integration test coverage with realistic scenarios.The integration tests effectively combine edge cases (leap year, year boundaries, empty data) with realistic GitHub-like contribution patterns. This provides good confidence in real-world usage.
497-511: Non-standard date format may not match actual data contract.Line 501 uses
'2024-1-2': 10without leading zeros. If the component expects ISO 8601 format (YYYY-MM-DD), this data point may be silently ignored rather than processed. Verify this is testing intentional edge case handling and not accidentally passing invalid data.
181-194: Theme test may not properly verify theme-based styling.Both light mode (line 186) and dark mode (line 193) assertions check for
text-gray-700. If the component applies the same text color regardless of theme, clarify this in a comment or rename the test to reflect that it's verifying consistent styling across themes. If the component should apply different colors per theme, update the dark mode assertion to expect the appropriate dark mode class.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/__tests__/unit/components/ContributionHeatmap.test.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/__tests__/unit/pages/createProgram.test.tsx:70-86
Timestamp: 2025-07-12T17:36:57.255Z
Learning: When testing React page components that use mocked form components, validation logic should be tested at the form component level, not the page level. Page-level tests should focus on authentication, role checking, submission handling, and navigation logic.
📚 Learning: 2025-11-17T16:47:05.578Z
Learnt from: anurag2787
Repo: OWASP/Nest PR: 2671
File: frontend/__tests__/unit/components/MultiSearch.test.tsx:427-427
Timestamp: 2025-11-17T16:47:05.578Z
Learning: In the frontend test files for the OWASP/Nest repository, `expect(true).toBe(true)` no-op assertions may be intentionally added as workarounds when ESLint rule jest/expect-expect doesn't detect expectations inside helper functions or waitFor callbacks. These can be resolved by configuring the ESLint rule's assertFunctionNames option to recognize custom assertion helpers instead of flagging them as redundant.
Applied to files:
frontend/__tests__/unit/components/ContributionHeatmap.test.tsx
📚 Learning: 2025-11-17T17:30:42.139Z
Learnt from: anurag2787
Repo: OWASP/Nest PR: 2671
File: frontend/__tests__/unit/components/MultiSearch.test.tsx:169-171
Timestamp: 2025-11-17T17:30:42.139Z
Learning: In the OWASP/Nest frontend tests (PR #2671 context), wrapper functions like `expectChaptersCountEqualsThree` that simply call another helper with a fixed parameter (e.g., `expectChaptersCountEquals(3)`) are intentionally used to avoid arrow function callbacks in `waitFor` calls. This pattern prevents adding nesting depth that would trigger rule typescript:S2004. Example: `await waitFor(expectChaptersCountEqualsThree)` avoids the extra nesting from `await waitFor(() => expectChaptersCountEquals(3))`.
Applied to files:
frontend/__tests__/unit/components/ContributionHeatmap.test.tsx
📚 Learning: 2025-07-03T03:08:03.290Z
Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 1703
File: frontend/src/components/BarChart.tsx:33-46
Timestamp: 2025-07-03T03:08:03.290Z
Learning: In the OWASP Nest project's BarChart component (frontend/src/components/BarChart.tsx), the days and requirements arrays are guaranteed to always have the same length in their use cases, so input validation for array length matching is not needed.
Applied to files:
frontend/__tests__/unit/components/ContributionHeatmap.test.tsx
🪛 GitHub Check: SonarCloud Code Analysis
frontend/__tests__/unit/components/ContributionHeatmap.test.tsx
[warning] 33-33: The return value of "includes" must be used.
[warning] 31-31: The return value of "includes" must be used.
[warning] 30-30: The return value of "includes" must be used.
[warning] 32-32: The return value of "includes" must be used.
🔇 Additional comments (13)
frontend/__tests__/unit/components/ContributionHeatmap.test.tsx (13)
61-69: LGTM!The
next-themesmock andrenderWithThemehelper are well-structured for testing theme-dependent behavior.
71-89: LGTM!Good test fixture setup with representative data covering various scenarios (sequential dates, gaps, zero values, varying counts). Proper mock cleanup in
beforeEach.
91-120: LGTM!Good coverage of basic rendering: minimal props, optional props, day series rendering, and chart type verification. Proper use of
waitForfor async assertions.
122-179: LGTM!Comprehensive coverage of data processing edge cases: empty data, zero values, high values, various date ranges, sparse data, and large datasets (365 days). Proper cleanup with
unmount()in parameterized tests.
181-210: LGTM!Good theme testing with light/dark mode switching via
rerender. Style content assertions verify responsive breakpoints and container structure.
212-227: LGTM!Good accessibility testing: semantic HTML verification with
h4element check andgetByRole('heading', { level: 4 })query. Meets accessibility requirements from PR objectives.
258-303: Tooltip tests are limited by the mock approach.The tooltip tests verify that the chart renders but don't assert on actual tooltip content. The mock does call
tooltip.custom()to exercise the code path, but meaningful content assertions are limited. This is acceptable given the mock-based approach, but consider adding integration tests later for deeper tooltip validation.
305-369: LGTM!Good coverage of week number calculation and color scale logic scenarios. The tests verify the component renders correctly with various boundary values and date configurations.
371-421: LGTM!Excellent date edge case coverage: leap year dates, year boundaries, and reverse chronological order in data keys. These validate robustness against various date input scenarios.
423-468: LGTM!Good defensive testing for data quality edge cases: negative values, floating-point numbers, and extremely large counts. These ensure the component handles unexpected input gracefully.
470-496: LGTM!Good lifecycle testing: multiple consecutive updates validate rendering stability, and unmount/remount cycle verifies clean cleanup.
498-531: LGTM!Good string formatting tests: non-standard date format (
'2024-1-2') tests parser robustness, and special character unit strings (pull-request,PR's,contribution/day) validate proper handling.
561-629: LGTM!Excellent integration tests with realistic GitHub-like contribution data, empty data handling, and combined edge cases. These provide confidence in real-world usage scenarios.
frontend/__tests__/unit/components/ContributionHeatmap.test.tsx
Outdated
Show resolved
Hide resolved
…unnecessary checks
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
frontend/__tests__/unit/components/ContributionHeatmap.test.tsx (3)
157-173: Good large dataset test, but assertions could be more specific.The test verifies the component renders without crashing with 365 days of data, which is valuable for regression detection. Consider adding an assertion on the series data length to verify all data points are processed:
renderWithTheme( <ContributionHeatmap contributionData={largeData} startDate="2024-01-01" endDate="2024-12-31" /> ) expect(screen.getByTestId('mock-heatmap-chart')).toBeInTheDocument() + expect(screen.getByTestId('mock-heatmap-chart')).toHaveAttribute('data-series-length', '7') })
252-297: Tooltip tests provide coverage but lack behavioral assertions.The mock invokes
tooltip.custom()(lines 24-34), which exercises the tooltip code path, but the tests don't verify the actual tooltip output. While this achieves code coverage, consider capturing and asserting on the tooltip return value in critical cases:// In the mock, capture tooltip result for assertion +let lastTooltipResult: unknown = null if (tooltip.custom) { if (mockSeries[0]?.data.length > 0) { - tooltip.custom({ + lastTooltipResult = tooltip.custom({ seriesIndex: 0, dataPointIndex: 0, w: { config: { series: mockSeries } }, }) }Then export or expose
lastTooltipResultfor tests that need to verify tooltip content contains expected date formatting or unit labels.
468-479: Date string formatting produces non-ISO format for single digits.When
i=0, the template literal produces'2024-01-1'instead of'2024-01-01'. If consistent ISO format is expected, use padding:for (let i = 0; i < 5; i++) { - const newData = { [`2024-01-${i + 1}`]: i * 5 } + const newData = { [`2024-01-${String(i + 1).padStart(2, '0')}`]: i * 5 } rerender(If the component intentionally handles both formats, this is fine but should be consistent with the date format test intent.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/__tests__/unit/components/ContributionHeatmap.test.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-11-17T16:47:05.578Z
Learnt from: anurag2787
Repo: OWASP/Nest PR: 2671
File: frontend/__tests__/unit/components/MultiSearch.test.tsx:427-427
Timestamp: 2025-11-17T16:47:05.578Z
Learning: In the frontend test files for the OWASP/Nest repository, `expect(true).toBe(true)` no-op assertions may be intentionally added as workarounds when ESLint rule jest/expect-expect doesn't detect expectations inside helper functions or waitFor callbacks. These can be resolved by configuring the ESLint rule's assertFunctionNames option to recognize custom assertion helpers instead of flagging them as redundant.
Applied to files:
frontend/__tests__/unit/components/ContributionHeatmap.test.tsx
📚 Learning: 2025-11-17T17:30:42.139Z
Learnt from: anurag2787
Repo: OWASP/Nest PR: 2671
File: frontend/__tests__/unit/components/MultiSearch.test.tsx:169-171
Timestamp: 2025-11-17T17:30:42.139Z
Learning: In the OWASP/Nest frontend tests (PR #2671 context), wrapper functions like `expectChaptersCountEqualsThree` that simply call another helper with a fixed parameter (e.g., `expectChaptersCountEquals(3)`) are intentionally used to avoid arrow function callbacks in `waitFor` calls. This pattern prevents adding nesting depth that would trigger rule typescript:S2004. Example: `await waitFor(expectChaptersCountEqualsThree)` avoids the extra nesting from `await waitFor(() => expectChaptersCountEquals(3))`.
Applied to files:
frontend/__tests__/unit/components/ContributionHeatmap.test.tsx
📚 Learning: 2025-07-03T03:08:03.290Z
Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 1703
File: frontend/src/components/BarChart.tsx:33-46
Timestamp: 2025-07-03T03:08:03.290Z
Learning: In the OWASP Nest project's BarChart component (frontend/src/components/BarChart.tsx), the days and requirements arrays are guaranteed to always have the same length in their use cases, so input validation for array length matching is not needed.
Applied to files:
frontend/__tests__/unit/components/ContributionHeatmap.test.tsx
🔇 Additional comments (7)
frontend/__tests__/unit/components/ContributionHeatmap.test.tsx (7)
1-5: LGTM!Imports are well-organized and include all necessary testing utilities.
7-53: Well-designed mock implementation.The mock chart effectively exposes the chart configuration through data attributes and invokes the tooltip callback with both valid and invalid indices to exercise fallback behavior. This enables comprehensive testing without the overhead of real chart rendering.
55-63: LGTM!The theme mock and
renderWithThemehelper provide clean, reusable test setup for controlling theme state across test cases.
85-114: LGTM!Good coverage of rendering scenarios including minimal props, optional props, series verification, and conditional title rendering.
493-506: Clarify intent of non-standard date format test.The test uses
'2024-1-2'(line 496) alongside the ISO format'2024-01-01'. If the component intentionally supports non-ISO date formats, this is valid edge case coverage. However, if only ISO format (YYYY-MM-DD) is expected, this test documents potential undefined behavior rather than a feature. Consider adding a comment clarifying the intent or adjusting based on the component's actual contract.
555-622: LGTM!Integration tests provide excellent coverage of realistic usage patterns, including GitHub-like contribution data, empty datasets, and combined edge cases over a full year range.
176-189: Theme test may not be validating actual dark mode styling differences.Both light mode (line 181) and dark mode (line 188) assert the same
text-gray-700class. If the component actually applies different classes based on theme, this test won't catch regressions. If the component uses the same base class with CSS variables or Tailwind dark mode variants, consider verifying the actual visual difference via the chart options or adding a comment explaining the expectation.
frontend/__tests__/unit/components/ContributionHeatmap.test.tsx
Outdated
Show resolved
Hide resolved
|
2 participants
Summary
This PR expands the unit test coverage for the
ContributionHeatmapcomponent to ensure it behaves correctly across a wide range of real-world scenarios and edge cases.The original tests covered only basic rendering. This update adds deeper validation around data handling, date ranges, theming, accessibility, tooltips, responsive behavior, and component lifecycle. No production logic was changed as part of this PR.
Fixed: #2630
Key Areas Covered
Test Results