Add comprehensive unit tests and test infrastructure #110
Conversation
…ducing shared MSW server/polyfill modules, stubbing Azure env vars globally, cleaning up mocks each test.
WalkthroughAdds Vitest testing infrastructure, MSW mock server, polyfills, many new unit/integration tests and fixtures, CI steps to run tests and upload coverage, package.json test scripts/deps, a Vitest config, test utilities/setup, and a small Search URL pagination behavior change. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Runner as Test runner (Vitest)
participant Setup as Test setup (src/__tests__/setup.ts)
participant MSW as MSW server (`server`)
participant Component as Component under test
participant API as Search API (mock /search)
Runner->>Setup: beforeAll -> start server, stub env
Setup->>MSW: server.listen()
Runner->>Component: render(component)
Component->>API: fetch POST /search
API->>MSW: request intercepted
MSW->>Component: respond with `mockSearchResponse` or error
Component->>Component: update state, render results
Runner->>Component: user interactions / assertions
Runner->>Setup: afterEach -> resetHandlers, cleanup, clear storage
Runner->>Setup: afterAll -> server.close()
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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: 5
🧹 Nitpick comments (13)
src/serialization/DateConverter.test.ts (1)
17-25: Consider using UTC methods for consistency.When parsing date strings without a timezone, JavaScript's behavior is implementation-specific. While this test should pass (both parsing and extraction use local time), using UTC methods would make the test more robust and explicit.
Consider this alternative approach:
- it('should deserialize date string without timezone', () => { - const dateString = '2024-01-15T10:30:00'; + it('should deserialize date string and handle as local time', () => { + const dateString = '2024-01-15T10:30:00Z'; // Use explicit UTC const result = converter.deserialize(dateString); expect(result).toBeInstanceOf(Date); - expect(result.getFullYear()).toBe(2024); - expect(result.getMonth()).toBe(0); // January = 0 - expect(result.getDate()).toBe(15); + expect(result.getUTCFullYear()).toBe(2024); + expect(result.getUTCMonth()).toBe(0); // January = 0 + expect(result.getUTCDate()).toBe(15); });.github/workflows/build-deploy.yml (1)
41-45: Consider removing redundant test execution.The workflow runs tests twice:
- Line 42:
pnpm test:run- Line 45:
pnpm test:coverageSince
test:coveragealready executes all tests while collecting coverage, the separatetest:runstep is redundant and increases CI time unnecessarily.Apply this diff to remove the redundant step:
- - name: Run tests - run: pnpm test:run - - name: Generate coverage report run: pnpm test:coverageIf you want to fail fast without coverage overhead, you could keep
test:runand make coverage optional, but for most cases, running coverage alone is sufficient.src/App.test.tsx (1)
1-33: App test is clear; consider reusing shared test render utilitiesThe test correctly verifies that
Apprenders theRootlayout under aHelmetProvider. For consistency with the rest of the suite, you might consider importing the sharedrenderhelper from yoursrc/__tests__/test-utils.tsxso all component tests are wrapped with the same providers by default, but the current approach is perfectly fine functionally.src/components/NavBar.test.tsx (1)
12-29: TypecontextOverridesto avoid implicitanyand strengthen checks
renderNavBarcurrently takescontextOverrides = {}without a type, which will becomeanyundernoImplicitAny: falseand a compiler error undernoImplicitAny: true. You can tighten this and get better autocomplete by typing it against the context value:// Assuming you have something like IColorSchemeContext exported: const defaultContextValue: IColorSchemeContext = { preferedColorScheme: ColorSchemeType.Auto, browserColorScheme: ColorSchemeType.Light, isDarkMode: false, toggleColorScheme: mockToggleColorScheme, }; const renderNavBar = (contextOverrides: Partial<IColorSchemeContext> = {}) => { const contextValue = { ...defaultContextValue, ...contextOverrides }; // ... };(or derive the type via
React.ContextType<typeof ColorSchemeContext>).src/components/SearchBar.test.tsx (1)
27-76: Debounce tests are accurate; consider hardening fake timer teardownThe debounce and cleanup behavior are well covered and correctly wrapped in
act. To make the suite more robust, you might centralize timer management so they’re always restored even if an assertion fails, e.g.:describe('SearchBar', () => { beforeEach(() => { vi.useFakeTimers(); }); afterEach(() => { vi.useRealTimers(); }); // tests can now omit the per-test useFakeTimers/useRealTimers calls });(or use try/finally inside each test). Not required, but it reduces the risk of leaking fake timers into other tests.
src/components/Footer.test.tsx (1)
28-32: Version/commit assertion is broad; tighten if the UI text is known
getByText(/\//)will pass on any text node containing/, not necessarily the version/commit label if other content with slashes appears. If the footer renders a more specific pattern (e.g.v1.2.3/abcdef), you could assert on a more constrained regex or role/text combination. If the exact format is intentionally loose, this is fine to keep.src/components/Buckets.test.tsx (1)
41-56: Consider more explicit filter checking for maintainability.The MSW handler distinguishes official vs community buckets using
body.filter?.includes('1')on line 48. While functional, this magic string check is fragile and may break if the component's filter format changes.Consider a more explicit check, such as:
- if (body.filter?.includes('1')) { + if (body.filter?.includes('OfficialRepository eq true')) { return HttpResponse.json(mockOfficialBucketsResponse); }Or add a comment explaining the '1' check.
vitest.config.ts (1)
1-41: Solid Vitest configuration with comprehensive coverage settings!The configuration properly:
- Merges with existing Vite config to preserve build settings
- Sets up jsdom environment with necessary resources for React component testing
- Configures appropriate coverage reporting (text, json, html, lcov) for CI integration
- Excludes tests, configs, and build artifacts from coverage
- Uses setup file for global test initialization
The 80% coverage thresholds across all metrics are ambitious. If you encounter difficulties meeting these thresholds initially, consider starting with lower values (e.g., 70%) and gradually increasing them:
thresholds: { - lines: 80, - functions: 80, - branches: 80, - statements: 80, + lines: 70, + functions: 70, + branches: 70, + statements: 70, },src/colorscheme/ColorSchemeProvider.test.tsx (1)
407-427: Consider adding cleanup verification.The test verifies
addEventListeneris called but doesn't verify thatremoveEventListeneris called on unmount. This could help catch memory leaks.it('should register media query change listener', () => { const mockAddEventListener = vi.fn(); + const mockRemoveEventListener = vi.fn(); mockMatchMedia.mockReturnValue({ matches: false, media: '(prefers-color-scheme: dark)', addEventListener: mockAddEventListener, - removeEventListener: vi.fn(), + removeEventListener: mockRemoveEventListener, dispatchEvent: vi.fn(), }); - renderWithoutProviders( + const { unmount } = renderWithoutProviders( <ColorSchemeProvider> <TestComponent /> </ColorSchemeProvider> ); expect(mockMatchMedia).toHaveBeenCalledWith('(prefers-color-scheme: dark)'); expect(mockAddEventListener).toHaveBeenCalledWith('change', expect.any(Function)); + + unmount(); + expect(mockRemoveEventListener).toHaveBeenCalledWith('change', expect.any(Function)); });src/components/SearchProcessor.test.tsx (1)
340-357: Abort controller test doesn't verify abort behavior.The test name suggests it verifies that previous requests are aborted, but it only confirms that
onResultsChangeis called again after the query changes. This doesn't prove the previous request was actually aborted.To properly test abort behavior, consider verifying the abort signal:
describe('abort controller', () => { it('should abort previous request when query changes', async () => { + let abortSignals: AbortSignal[] = []; + + server.use( + http.post(`${SEARCH_API_URL}/search`, async ({ request }) => { + abortSignals.push(request.signal); + return HttpResponse.json(mockSearchResponse); + }) + ); + const { rerender } = render(<SearchProcessor {...defaultProps} query="git" />); await waitFor(() => { expect(mockOnResultsChange).toHaveBeenCalled(); }); mockOnResultsChange.mockClear(); // Change query rerender(<SearchProcessor {...defaultProps} query="nodejs" />); await waitFor(() => { expect(mockOnResultsChange).toHaveBeenCalled(); + // First request should have been aborted + expect(abortSignals[0]?.aborted).toBe(true); }); }); });src/components/Home.test.tsx (1)
117-131: Color scheme tests don't verify style application.The test names "applies light style in light mode" and "applies dark style in dark mode" are misleading - they only verify the component renders without error, not that any specific styles are applied. The comments acknowledge this, but the test names should reflect actual behavior.
Consider renaming for clarity:
describe('color scheme', () => { - it('applies light style in light mode', () => { + it('renders without error in light mode', () => { renderHome({ isDarkMode: false }); - // Component should render without error in light mode expect(screen.getByText('Scoop')).toBeInTheDocument(); }); - it('applies dark style in dark mode', () => { + it('renders without error in dark mode', () => { renderHome({ isDarkMode: true }); - // Component should render without error in dark mode expect(screen.getByText('Scoop')).toBeInTheDocument(); }); });src/components/Search.test.tsx (2)
95-113: Consider centralizingrenderSearchwiring into shared test utilities
renderSearchwiresHelmetProvider,MemoryRouter, routes, andLocationDisplay. If other tests need a similar environment, consider moving this into your sharedtest-utilsto avoid duplication and keep setup consistent. This is optional but can simplify future test maintenance.
132-250: Tests cover key behaviors well; a couple of small cleanups are possibleThe three tests exercise URL/state initialization, query updates with debounced submit, and result → modal flow very thoroughly. A couple of minor tidy-ups you might consider later:
- In the last test,
vi.useRealTimers()is called twice (Lines 219 and 249); the second call is redundant and can be dropped.- If other tests in the suite start relying on
localStorage, adding alocalStorage.clear()in a globalbeforeEach/afterEachcould help avoid subtle cross-test interactions.These are non-blocking polish items; the current tests are otherwise in good shape.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (32)
.github/workflows/build-deploy.yml(1 hunks)package.json(3 hunks)src/App.test.tsx(1 hunks)src/__tests__/mocks/fixtures.ts(1 hunks)src/__tests__/polyfills.ts(1 hunks)src/__tests__/server.ts(1 hunks)src/__tests__/setup.ts(1 hunks)src/__tests__/test-utils.tsx(1 hunks)src/colorscheme/ColorSchemeProvider.test.tsx(1 hunks)src/components/BucketTypeIcon.test.tsx(1 hunks)src/components/Buckets.test.tsx(1 hunks)src/components/CopyToClipboardButton.test.tsx(1 hunks)src/components/Footer.test.tsx(1 hunks)src/components/Home.test.tsx(1 hunks)src/components/NavBar.test.tsx(1 hunks)src/components/Root.test.tsx(1 hunks)src/components/Search.test.tsx(1 hunks)src/components/Search.tsx(1 hunks)src/components/SearchBar.test.tsx(1 hunks)src/components/SearchPagination.test.tsx(1 hunks)src/components/SearchProcessor.test.tsx(1 hunks)src/components/SearchResult.test.tsx(1 hunks)src/components/SearchStatus.test.tsx(1 hunks)src/serialization/BucketsResultsFacetJson.test.ts(1 hunks)src/serialization/BucketsResultsJson.test.ts(1 hunks)src/serialization/DateConverter.test.ts(1 hunks)src/serialization/ManifestJson.test.ts(1 hunks)src/serialization/MetadataJson.test.ts(1 hunks)src/serialization/SearchResultsJson.test.ts(1 hunks)src/setupTests.ts(0 hunks)src/utils.test.ts(1 hunks)vitest.config.ts(1 hunks)
💤 Files with no reviewable changes (1)
- src/setupTests.ts
🧰 Additional context used
🧬 Code graph analysis (11)
src/components/Buckets.test.tsx (1)
src/__tests__/server.ts (2)
server(21-21)SEARCH_API_URL(5-5)
src/serialization/BucketsResultsJson.test.ts (1)
src/__tests__/mocks/fixtures.ts (1)
mockBucketsResponse(90-118)
src/serialization/SearchResultsJson.test.ts (1)
src/__tests__/mocks/fixtures.ts (3)
mockSearchResponse(6-58)mockEmptySearchResponse(61-64)createMockSearchResponse(155-160)
src/utils.test.ts (1)
src/utils.ts (1)
extractPathFromUrl(1-3)
src/components/SearchResult.test.tsx (1)
src/__tests__/mocks/fixtures.ts (1)
mockManifest(67-87)
src/__tests__/server.ts (1)
src/__tests__/mocks/fixtures.ts (1)
mockSearchResponse(6-58)
src/components/SearchProcessor.test.tsx (3)
src/__tests__/server.ts (2)
server(21-21)SEARCH_API_URL(5-5)src/__tests__/mocks/fixtures.ts (1)
mockSearchResponse(6-58)src/components/SearchProcessor.tsx (1)
sortModes(37-80)
src/components/NavBar.test.tsx (1)
src/colorscheme/ColorSchemeContext.ts (1)
ColorSchemeContext(19-19)
src/__tests__/setup.ts (1)
src/__tests__/server.ts (2)
server(21-21)SEARCH_API_URL(5-5)
src/colorscheme/ColorSchemeProvider.test.tsx (1)
src/colorscheme/ColorSchemeContext.ts (1)
ColorSchemeContext(19-19)
src/components/Home.test.tsx (1)
src/colorscheme/ColorSchemeContext.ts (1)
ColorSchemeContext(19-19)
🪛 GitHub Actions: Build and deploy Website
src/components/Search.test.tsx
[error] 125-125: Cannot find namespace 'vi'.
🪛 GitHub Check: Build ⚙️📦
src/components/Search.test.tsx
[failure] 125-125:
Cannot find namespace 'vi'.
🔇 Additional comments (33)
src/serialization/DateConverter.test.ts (4)
1-5: LGTM!Clean test setup with proper imports and converter instance initialization.
8-15: LGTM!Test correctly handles ISO 8601 format with explicit UTC timezone, ensuring timezone-independent behavior.
37-51: LGTM!Both tests are well-designed:
- The various formats test appropriately validates multiple date string formats with explicit timezone information.
- The invalid date test correctly verifies error handling behavior.
54-59: LGTM!Correctly tests that serialization throws an appropriate error with a clear message.
src/__tests__/polyfills.ts (1)
1-53: LGTM! Comprehensive test polyfills.The polyfills provide complete implementations of browser APIs needed for testing:
matchMediawith all standard listener methodslocalStoragewith proper IIFE encapsulation and complete API surfaceclipboard.writeTextmockThese will ensure tests run reliably in the jsdom environment.
src/serialization/BucketsResultsFacetJson.test.ts (1)
1-73: LGTM! Comprehensive test coverage for facet deserialization.The test suite thoroughly covers:
- Standard value types (boolean, string)
- Edge cases (zero count, large count, empty string)
- Proper json2typescript integration
All assertions are clear and verify both count and value properties correctly.
src/serialization/BucketsResultsJson.test.ts (2)
1-132: LGTM! Comprehensive test suite for BucketsResultsJson.The tests thoroughly cover:
- Basic deserialization with facets
- Facet result structure and ordering
- Empty responses
- Multiple facet types
- Single facet values
- Edge cases (zero counts, large counts, special characters)
All test assertions are clear and validate the expected behavior correctly.
30-40: No inconsistency found. The codebase consistently uses the correct OData property name.The verification confirms:
mockBucketsResponsein fixtures.ts uses'@odata.count'BucketsResultsJsonimplementation uses@JsonProperty('@odata.count', Number)- The test snippet uses
'@odata.count': 0- No usage of
'@data.count'exists anywhere in the codebaseThe original review comment's concern about inconsistent property names is unfounded. All code and tests correctly use the Azure Search API standard
'@odata.count'property name.src/__tests__/mocks/fixtures.ts (1)
1-160: LGTM! Well-structured test fixtures and helpers.The fixtures provide comprehensive mock data for testing:
- Complete search responses with realistic Azure Search structure
- Empty response handling
- Detailed manifest examples
- Buckets responses with facets
- Error responses
- Helper functions for customizing fixtures
The
createMockManifestandcreateMockSearchResponsehelpers are particularly useful for test scenarios requiring variations..github/workflows/build-deploy.yml (1)
47-51: LGTM! Coverage artifact upload configured correctly.The coverage report upload will provide visibility into test coverage metrics in CI, which is valuable for maintaining code quality.
src/components/Search.tsx (1)
201-209: LGTM! Correct UX behavior for query changes.Resetting the page parameter to 1 when the query changes ensures users see results from the beginning of the new search, which is the expected behavior. This prevents showing an empty or incorrect page when the new query has fewer results.
src/__tests__/setup.ts (1)
1-23: LGTM! Proper test lifecycle management.The test setup provides:
- MSW server initialization with strict unhandled request handling
- Environment variable stubbing for Azure Search credentials
- Comprehensive cleanup after each test (handlers, DOM, storage, mocks)
- Proper teardown on completion
This ensures tests run in isolation with a clean, predictable environment.
package.json (1)
49-65: All specified test dependency versions are current stable releases with no known security advisories.The packages you listed are at their latest versions as of November 2025:
@testing-library/[email protected]✓@vitest/[email protected]✓@vitest/[email protected]✓[email protected]✓[email protected]✓[email protected]✓No known security vulnerabilities or advisories have been reported for these versions.
src/components/CopyToClipboardButton.test.tsx (1)
1-55: CopyToClipboardButton timer behavior tests look solidThe tests thoroughly cover the disabled window and subsequent clicks with fake timers, and the helper pattern with
ComponentProps<typeof CopyToClipboardButton>keeps props flexible. No changes needed from my side.src/serialization/MetadataJson.test.ts (1)
5-134: MetadataJson deserialization tests provide good coverageThe tests exercise full vs partial metadata, date conversion, official flag, and star count edge cases using
JsonConvert, which should catch most mapping regressions inMetadataJson. Looks good as-is.src/components/SearchStatus.test.tsx (1)
5-104: SearchStatus test coverage is comprehensive and focusedThese tests nicely cover searching, results, no-results (with and without query),
officialOnlybehavior, and query formatting edge cases. The expectations are clear and map well to the component props; I don’t see any changes needed here.src/components/BucketTypeIcon.test.tsx (1)
1-91: Well-structured test suite with comprehensive coverage!The test organization is clear and covers all major rendering paths:
- Icon type variations based on official flag and star counts
- Tooltip configuration with wrapper verification
- Props spreading validation
The tests effectively verify color attributes, SVG presence, and prop forwarding.
src/serialization/SearchResultsJson.test.ts (1)
1-170: Comprehensive deserialization test coverage!The test suite thoroughly validates SearchResultsJson.Create across multiple scenarios:
- Standard and empty responses
- Single and multiple results
- Count vs results length mismatches (pagination scenario)
- Nested property preservation including Date conversion
- Edge cases with missing optional fields and large datasets
The use of mock fixtures and helper functions promotes maintainability.
src/__tests__/server.ts (1)
1-21: Clean MSW test server setup!The server configuration is well-structured with:
- Reusable URL constant for consistency across tests
- Default handler supporting both success and error scenarios
- Clear separation between test conditions (
search === 'error')This provides a solid foundation for API mocking across the test suite.
src/utils.test.ts (1)
1-167: Excellent comprehensive test coverage for the utility function!The test suite thoroughly documents extractPathFromUrl behavior across:
- Basic URL and path extraction
- Custom separator support
- Edge cases including trailing slashes, empty strings, encoded characters
- Real-world scenarios from GitHub and Azure Search
The edge case tests (lines 72-136) are particularly valuable for documenting the function's actual behavior with unusual inputs.
src/__tests__/test-utils.tsx (1)
1-33: Excellent test utility following React Testing Library best practices!The custom render function provides:
- Consistent provider wrapping (ColorSchemeProvider, MemoryRouter) across all tests
- Flexible
withProvidersflag for edge cases where providers aren't needed- Full re-export of Testing Library utilities for convenience
This pattern significantly reduces boilerplate in test files while ensuring consistent context setup.
src/components/SearchResult.test.tsx (1)
27-299: Comprehensive test coverage for SearchResult component!The test suite thoroughly validates:
- Core rendering (name, description, repository, version, homepage, notes)
- License handling (SPDX links vs plain text)
- Command generation for official and community repositories
- Copy-to-clipboard interactions including "Copy all" functionality
- Result selection behavior
- Homepage formatting (protocol removal, trailing slash handling)
- Official vs community bucket differences
- Date formatting and commit links
The test organization and assertions are clear and maintainable, pending resolution of the prop name typo.
src/components/SearchPagination.test.tsx (1)
1-34: Well-structured visibility tests.Good coverage of edge cases: single page, zero results, and multiple pages. The test organization with describe blocks is clean.
src/colorscheme/ColorSchemeProvider.test.tsx (2)
26-77: Thorough browser API mocking setup.The mocks for
document.getElementsByTagName,window.matchMedia, andlocalStorageare well-structured. ThebeforeEach/afterEachlifecycle properly clears state between tests.
233-251: <function_calls>
#!/bin/bashRead the test file to understand structure and the toggle test in question
cat -n src/colorscheme/ColorSchemeProvider.test.tsx | sed -n '220,260p'
</function_calls>src/components/SearchProcessor.test.tsx (1)
50-146: Comprehensive API interaction tests.Good coverage of query parameters, filters, and error handling using MSW. The pattern of capturing request bodies for verification is effective.
src/serialization/ManifestJson.test.ts (3)
1-62: Solid deserialization test coverage.The tests properly verify JSON deserialization including optional fields and metadata mapping. Good use of the
mockManifestfixture with spread operator for creating test variants.
64-159: Thorough highlight getter testing.Excellent coverage of highlight scenarios including fallbacks, multiple highlight sources, and joining multiple values. Tests align well with the expected behavior described in the summary.
161-200: Good favicon edge case coverage.Tests cover homepage with subpaths, ports, and missing homepage scenarios. The URL origin extraction is properly validated.
src/components/Home.test.tsx (2)
9-21: Good mocking strategy for complex dependencies.Mocking
AsciinemaCaststo avoid complex dependencies anduseNavigatefor navigation verification is appropriate. The mock preserves the rest of react-router-dom's exports correctly.
92-106: Search navigation test is well-structured.Properly simulates user typing and form submission, then verifies navigation call with correct pathname and search params.
src/components/Search.test.tsx (2)
15-60: Mocking child components viavi.mockand capturing props looks solidUsing lightweight mocks for
SearchBar,SearchProcessor,SearchResult, andSearchPaginationwhile forwarding props intomockSearch*fns gives you good introspection without over-coupling to implementation details. This is a clean pattern for keeping the tests focused onSearch’s behavior.
115-130: The review comment is incorrect – the code already uses the properMocktype importThe review claims line 125 contains
(globalThis.fetch as unknown as vi.Mock)causing a "Cannot find namespace 'vi'" error, but the actual code on line 125 is:(globalThis.fetch as unknown as Mock) = vi.fn().mockResolvedValue({The
Mocktype is correctly imported as a type import fromviteston line 5:import { beforeAll, beforeEach, describe, expect, it, type Mock, vi } from 'vitest';The current code is already fixed and does not have the issue described. The suggested change to remove the type assertion entirely would actually reduce type safety unnecessarily.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/components/Search.test.tsx (3)
115-130: Globals setup is fine; consider centralizing and adding cleanupStubbing
cancelIdleCallback,fetch, andscrollTo, plus clearing all component mocks inbeforeEach, is reasonable here, but you may want to (a) move shared polyfills likecancelIdleCallbackinto your global test setup and (b) add a matchingafterEachthat restores or clears global state (e.g.,localStorage.clear(),vi.restoreAllMocks()) to avoid subtle cross‑test coupling as the suite grows.
198-250: Result rendering + modal flow looks good; drop duplicateuseRealTimersThe test cleanly simulates a results payload, verifies list and pagination updates, and drives the modal open/close cycle including URL
idchanges. There’s a minor nit:vi.useRealTimers()is called twice (once afterrunAllTimersand again at the end); the second call is redundant and can be removed for clarity.If you update this, just ensure no other parts of the test rely on fake timers after the first
useRealTimers()call.
62-88: Perfect! I've completed the verification. Here are my findings:Verification Results:
Current Modal Mock Limitations Confirmed: The mock only accepts
show,onHide, andchildren, but the actualSearchcomponent passes additional props:restoreFocus,size,centered, andclassName.Modal.Header/Footer Usage: Not currently used anywhere in the codebase, confirming the "future-proofing" aspect of the suggestion.
Props Not Being Forwarded: Unlike the
MockWrappercomponents that properly forward extra props with...rest, theModalmock component ignores all undestructured props.Tests Still Pass: The tests only verify visibility and the close button, so the missing props don't cause test failures—but they could matter if future tests need to assert on modal styling or behavior.
The review comment's suggestion is valid and appropriately scoped as optional. The recommendation to forward extra props and add stub
Modal.Header/Modal.Footercomponents would indeed make the mock more resilient to future changes without adding significant complexity.
Consider slightly future-proofing the Modal mock
The mock currently only supports
show,onHide,children, andModal.Body. However, the actualSearchcomponent passes additional props (restoreFocus,size,centered,className) that the mock silently ignores. While this doesn't break current tests, you might optionally forward extra props to the Modal wrapper and add stubbedModal.Header/Modal.Footercomponents to make the mock more resilient to future changes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/Search.test.tsx(1 hunks)
🔇 Additional comments (8)
src/components/Search.test.tsx (8)
1-14: Imports and typing are consistent and minimalThe import set and use of
type-only imports keep runtime surface small and typings clear; nothing problematic here.
15-23: SearchBar mock pattern is clear and type‑safeCapturing props via
ComponentPropsand a dedicatedmockSearchBargives you good visibility into interactions while keeping the rendered output trivial.
24-36: SearchProcessor mock correctly preserves named exportsRe‑exporting
SortDirectionandsortModeswhile stubbing only the default component is a good balance between isolation and realism.
38-51: SearchResult mock nicely distinguishes list vs modal contextDeriving a stable test id from
result.idandonResultSelectedand wiringonClickthroughonResultSelectedgives you expressive, robust selectors for the tests.
53-60: SearchPagination mock is sufficient for prop inspectionLightweight markup plus
mockSearchPaginationcaptures all the behavior the tests need without coupling to the real pagination UI.
90-113: Test harness around Search is well‑structuredThe
renderSearchhelper that wrapsSearchwith routing and aLocationDisplaygives each test a clean, reusable way to assert on URL search params.
132-157: Initialization test covers URL + stored preferences wellThis test exercises the interaction between query params, stored sort settings, and pagination props; the assertions on bar, processor, and pagination props look consistent with the intended behavior.
158-197: Query‑change behavior and debounced submit are exercised thoroughlyThe test validates URL updates, page reset, pagination sync, and the debounced
onSubmitpath via fake timers; the sequencing withwaitForand timer toggling looks sound.
gpailler
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
src/components/Footer.test.tsx (1)
5-26: Footer creator and link tests are clear and robustThese tests cover the key visible text and link targets the footer exposes and use RTL queries appropriately. If you want to reduce a bit of repetition, you could move
render(<Footer />);into a small helper orbeforeEach, but it’s not required.src/colorscheme/ColorSchemeProvider.test.tsx (2)
183-221: Strengthen localStorage persistence assertions to cover write/remove behaviorBoth localStorage tests currently start from a state that makes them pass even if the persistence logic were partially missing:
- “save user preference” pre-populates
preferred-color-schemewithDarkand only checks it remainsDark, which would still pass ifsetItemweren’t called or the key changed.- “remove localStorage item when set to Auto” starts from empty storage and only asserts
getItemisnull, which would also be true ifremoveItemwere never called.To better exercise persistence, consider:
- Starting from an empty localStorage and asserting that
localStorage.setItemis called with the expected key/value when a non‑Auto preference is active (spying onlocalStorage.setItem).- Starting from a non‑empty value and asserting that
localStorage.removeItemis invoked and that the key is cleared.This would make these tests more effective at catching regressions in the persistence logic.
223-405: Toggle and media-query behavior tests are thorough; minor simplification possibleThe toggle and media-query suites cover the full matrix of Auto/Light/Dark vs. browser preference and validate both context values and
isDarkMode, plus verifying listener registration and update on change. From a behavior standpoint this is very solid.You could simplify a bit by relying on React Testing Library’s normal rerendering after state updates (the
rerendercalls after button clicks aren’t strictly necessary), but that’s optional and doesn’t affect correctness.src/components/SearchProcessor.test.tsx (1)
9-30: Clear shared mocks between tests for stronger isolationThe shared mocks (
mockOnResultsChange,mockOnSortChange, etc.) are defined once at the top of the suite and (apart from the abort tests) never explicitly cleared. IfclearMocksisn’t enabled in the Vitest config, earlier calls could satisfy latertoHaveBeenCalledWithassertions and slightly weaken test isolation.Consider adding a
beforeEachthat callsmockOnResultsChange.mockClear()(and similarly for the other shared mocks) so each test only observes its own interactions.src/components/SearchResult.test.tsx (1)
9-25: Consider clearing shared mocks between tests for stricter isolation
mockOnCopyToClipboardandmockOnResultSelectedare shared across the suite and aren’t reset between tests. Since the expectations only usetoHaveBeenCalledWith, earlier calls could satisfy later tests if Vitest isn’t configured withclearMocks: true.It’s not breaking, but adding a simple
beforeEachthat callsmockOnCopyToClipboard.mockClear()andmockOnResultSelected.mockClear()would make each copy/selection test self‑contained.Also applies to: 165-208
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
.github/workflows/build-deploy.yml(1 hunks)src/colorscheme/ColorSchemeProvider.test.tsx(1 hunks)src/colorscheme/ColorSchemeProvider.tsx(1 hunks)src/components/Buckets.test.tsx(1 hunks)src/components/Footer.test.tsx(1 hunks)src/components/Home.test.tsx(1 hunks)src/components/Root.test.tsx(1 hunks)src/components/Search.test.tsx(1 hunks)src/components/Search.tsx(3 hunks)src/components/SearchBar.test.tsx(1 hunks)src/components/SearchPagination.test.tsx(1 hunks)src/components/SearchProcessor.test.tsx(1 hunks)src/components/SearchProcessor.tsx(3 hunks)src/components/SearchResult.test.tsx(1 hunks)src/components/SearchResult.tsx(1 hunks)src/serialization/DateConverter.test.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/components/SearchBar.test.tsx
🚧 Files skipped from review as they are similar to previous changes (5)
- src/components/Buckets.test.tsx
- src/components/SearchPagination.test.tsx
- src/serialization/DateConverter.test.ts
- src/components/Home.test.tsx
- .github/workflows/build-deploy.yml
🧰 Additional context used
🧬 Code graph analysis (3)
src/components/SearchProcessor.test.tsx (3)
src/__tests__/server.ts (2)
server(21-21)SEARCH_API_URL(5-5)src/__tests__/mocks/fixtures.ts (1)
mockSearchResponse(6-58)src/components/SearchProcessor.tsx (1)
sortModes(37-80)
src/colorscheme/ColorSchemeProvider.test.tsx (1)
src/colorscheme/ColorSchemeContext.ts (1)
ColorSchemeContext(19-19)
src/components/SearchResult.test.tsx (1)
src/__tests__/mocks/fixtures.ts (1)
mockManifest(67-87)
🔇 Additional comments (10)
src/components/Footer.test.tsx (1)
28-38: Version/commit test is valuable; ensure globals are wired into the Vitest envThe checks for
APP_VERSION/APP_COMMIT_HASHbeing defined and rendered are useful to catch build/config issues, and the substringgetByTextusage is reasonable. Just make sure these globals are also defined in the Vitest environment (e.g., viadefine/globals in your test config), otherwise this test will throw aReferenceErrorwhen run in isolation.src/colorscheme/ColorSchemeProvider.tsx (1)
87-101: Media query listener wiring and cleanup look correctUsing a named
handleChangeclosure andaddEventListener/removeEventListenerwith the same function reference ensures updates and cleanup behave correctly, and the effect’s empty dependency array keeps the handler identity stable. No issues from this change.src/components/Root.test.tsx (1)
1-58: Root composition tests are clear and well‑isolatedThe mocking strategy for
NavBar,Footer, andColorSchemeProviderplus wrappingRootinMemoryRoutergives focused coverage of Root’s wiring without over-coupling to child internals. Type-onlyReactNodeusage keeps the mock typed without pulling in a runtime React import. This file looks good as-is.src/colorscheme/ColorSchemeProvider.test.tsx (1)
31-73: I'll begin by verifying the concerns raised in the review comment. Let me start by examining the full test file and understanding the actual test implementation, particularly around mock restoration and localStorage assertions.
<function_calls>
#!/bin/bashFirst, find and read the test file to understand full context
fd "ColorSchemeProvider.test.tsx" --type f
</function_calls>
<function_calls>
#!/bin/bashRead the full test file to understand structure and mock restoration
cat src/colorscheme/ColorSchemeProvider.test.tsx
</function_calls>src/components/SearchProcessor.tsx (1)
147-242: AbortController refactor correctly scopes cancellation to each effect runCreating the
AbortControllerinside the effect and aborting it in the cleanup neatly ties each request to a single render cycle and preserves the previous cancellation and error‑handling semantics without introducing new races or leaks. Looks good.src/components/SearchResult.tsx (1)
20-39: Copy‑to‑clipboard prop rename is consistent and correctly wiredThe
onCopyToClipboardprop name, handler, and hook dependencies are now aligned across the component and callers; no behavioral regressions here.src/components/SearchProcessor.test.tsx (1)
336-378: Abort controller behavior is well‑specified in testsThe abort‑controller tests correctly distinguish initial render (no abort on mount) from subsequent query changes (cleanup abort invoked), and the comments acknowledge the extra abort calls from undici’s internal propagation. This gives good coverage over the new per‑effect
AbortControllerlogic.src/components/Search.tsx (2)
201-207: Resetting page to 1 on query change is correct and consistentUpdating both the
psearch param andcurrentPageto1when the query changes keeps URL state and component state in sync and avoids “stale page” scenarios when starting a new search. This matches the tests inSearch.test.tsxand is a sensible UX.
257-263: Copy‑to‑clipboard plumbing between Search and SearchResult is aligned
handleCopyToClipboardwrites to the clipboard and is passed asonCopyToClipboardin both the list and modal usages ofSearchResult, matching the renamed prop and the SearchResult tests. No issues here.Also applies to: 323-357
src/components/Search.test.tsx (1)
95-113: Search tests provide good coverage of URL state, debouncing, and modal flowThe mocked child components, router wrapper, and
LocationDisplayhelper work well together to assert URL/search‑param behavior, debounced query submission, pagination updates, and the selected‑result modal lifecycle. Timer usage (useFakeTimers/useRealTimers) is scoped and correctly drives the debounced query andrequestIdleCallbackpath. This looks solid.Also applies to: 134-251
| it('should render version with "v" prefix for numeric versions', () => { | ||
| const numericVersionManifest = { | ||
| ...mockManifest, | ||
| Version: '1.0.0', | ||
| }; | ||
| const result = jsonConvert.deserializeObject(numericVersionManifest, ManifestJsonClass); | ||
|
|
||
| render(<SearchResult {...defaultProps} result={result} />); | ||
|
|
||
| // Check that version is displayed | ||
| expect(screen.getByText('1.0.0')).toBeInTheDocument(); | ||
| }); |
There was a problem hiding this comment.
Align test assertion with the “v”‑prefix behavior
The test name says “should render version with 'v' prefix for numeric versions”, but the assertion only checks that 1.0.0 is rendered, not that it is actually prefixed with v (i.e., v1.0.0).
Either:
- Strengthen the assertion to check for the full prefixed string, or
- Relax the test name to reflect that it only verifies that the version text is rendered.
Right now, the test wouldn’t fail if the prefix behavior regressed.
🤖 Prompt for AI Agents
In src/components/SearchResult.test.tsx around lines 53 to 64, the test title
expects the version to be rendered with a "v" prefix but the assertion only
checks for "1.0.0"; update the test to assert the prefixed string by replacing
the expectation to look for "v1.0.0" (or a regex that includes the leading "v")
so the test will fail if the "v" prefix is not rendered. Ensure the rendered
component still receives the same numeric Version value and adjust the assertion
only.
Summary by CodeRabbit
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.