test(data): add dataviewer custom hook tests with consolidated infra#606
test(data): add dataviewer custom hook tests with consolidated infra#606WilliamBerryiii wants to merge 4 commits intomainfrom
Conversation
- Add renderHookWithProviders shared test utility - Add Vitest tests for 13 previously untested hooks - Swallow ECONNREFUSED unhandledRejections from background refetches - Document use-object-detection useMemo side-effect bug (follow-up) - Refs #214 🧪 - Generated by Copilot
- migrate 7 hook test files to shared renderHookWithProviders helper - consolidate fetch mocks (installFetchMock, jsonResponse, mockMutationFetch) in src/test-utils - add unmount-during-pending coverage (DR-04) to use-annotations, use-ai-analysis, use-export, use-toast - raise vitest coverage thresholds 45/35/35/45 -> 55/55/40/55 with safety margin - document --no-file-parallelism requirement for local coverage (happy-dom races) 🔒 - Generated by Copilot
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Snapshot WarningsEnsure that dependencies are being submitted on PR branches and consider enabling retry-on-snapshot-warnings. See the documentation for more information and troubleshooting advice. Scanned FilesNone |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #606 +/- ##
==========================================
+ Coverage 65.16% 70.76% +5.60%
==========================================
Files 251 265 +14
Lines 15597 16844 +1247
Branches 2152 2293 +141
==========================================
+ Hits 10164 11920 +1756
+ Misses 5142 4607 -535
- Partials 291 317 +26
*This pull request uses carry forward flags. Click here to find out more.
🚀 New features to boost your workflow:
|
- align resolveFetch annotation in use-annotations.test.ts with the JsonResponseLike helper used elsewhere in the file - resolves CI tsc TS2345 (jsonResponse return not assignable to DOM Response) on PR #606 🩹 - Generated by Copilot
🔒 - Generated by Copilot
katriendg
left a comment
katriendg
left a comment
There was a problem hiding this comment.
Good test infrastructure PR — all 15 custom hooks now have coverage, the shared renderHookWithProviders and fetch mock utilities are clean, and the setup.ts ECONNREFUSED handler is well-scoped. The WI-01 documentation for the useMemo anti-pattern is appreciated.
Key findings:
- 🔒 High: Label mutation hooks (
use-labels.ts) missing CSRF tokens — pre-existing production gap faithfully mirrored by tests ⚠️ Medium:cancelExport()sets error state when idle — production bug surfaced by tests- 💡 Low:
as nevercasts reduce type safety;setTimeout(0)disabled-query assertions are fragile; some hooks lack error-path and DR-04 unmount coverage
None of these are blockers for the test PR itself — the tests accurately reflect production behavior. The CSRF gap and cancelExport bug should be tracked as follow-ups.
| episodes: { '0': ['SUCCESS'], '1': ['CUSTOM'] }, | ||
| }), | ||
| ) | ||
|
|
||
| selectDataset('ds-1') | ||
|
|
||
| const { result } = renderHookWithProviders(() => useDatasetLabels()) | ||
|
|
||
| await waitFor(() => expect(result.current.isSuccess).toBe(true)) | ||
|
|
||
| expect(mockFetch).toHaveBeenCalledTimes(1) | ||
| expect(mockFetch.mock.calls[0][0]).toBe('/api/datasets/ds-1/labels') |
There was a problem hiding this comment.
🔒 Security: Label mutations missing CSRF tokens
setEpisodeLabels, addLabelOption, and removeLabelOption perform PUT/POST/DELETE requests without CSRF tokens. Compare with use-annotations.ts which correctly uses mutationHeaders() from api-client.ts to include X-CSRF-Token.
The tests accurately reflect this gap (no _resetCsrfToken() or mockMutationFetch calls), but the production code needs fixing — these mutation endpoints should include CSRF protection consistent with the rest of the app.
This is not a test issue — it's a pre-existing production gap that the tests faithfully mirror. Consider filing a follow-up to add mutationHeaders() to all three functions.
|
|
||
| expect(mockCancel).not.toHaveBeenCalled() | ||
| expect(result.current.error).toBe('Export cancelled') | ||
| }) | ||
|
|
||
| it('fetchPreview populates previewStats on success', async () => { | ||
| mockGetExportPreview.mockResolvedValueOnce(samplePreview) | ||
| const { result } = renderHook(() => useExport({ datasetId: 'ds-1' })) | ||
|
|
||
| await act(async () => { | ||
| await result.current.fetchPreview([0, 1], [5]) |
There was a problem hiding this comment.
cancelExport sets error when no export is in flight
This test documents a production bug — cancelExport() sets error to 'Export cancelled' even when isExporting is false. The assertion confirms it:
expect(result.current.error).toBe('Export cancelled')Calling cancelExport() from a clean/idle state should be a true no-op (no error set). Consumers reading error to display alerts will show a spurious "Export cancelled" message. Consider filing a follow-up to guard cancelExport with an isExporting check before setting error state.
| [1, 1, 1], | ||
| [2, 2, 2], | ||
| ] |
There was a problem hiding this comment.
💡 Anti-pattern: pervasive as never type casts
as never casts appear throughout the test suite (here, use-annotation-workflow, use-labels, use-object-detection). This completely disables type checking and can mask breaking interface changes — if a hook's parameter shape changes, these tests still compile but test the wrong shapes silently.
Consider using test-specific factory functions that return the correct type, or targeted as unknown as TrajectoryData casts that at least name the target type so refactoring tools can detect breakage.
|
|
||
| it('is disabled when fewer than 3 positions are provided', async () => { | ||
| renderHookWithProviders(() => | ||
| useAISuggestion({ |
There was a problem hiding this comment.
💡 Fragile timing: setTimeout(resolve, 0) for disabled-query assertions
Multiple tests across the suite assert disabled-query behavior using:
await new Promise((resolve) => setTimeout(resolve, 0))
expect(mockFetch).not.toHaveBeenCalled()A zero-delay setTimeout is timing-dependent — a query that fires after one additional microtask could still pass this check. More robust alternative: assert result.current.fetchStatus === 'idle' (TanStack Query's disabled state signal), which is deterministic and doesn't rely on microtask ordering.
Affected files: use-ai-analysis, use-annotations, use-datasets, use-labels, use-episodes.
There was a problem hiding this comment.
💡 Missing error-path and DR-04 unmount coverage
Several query hooks lack error-path tests: useAISuggestion, useTrajectoryAnalysis, useAnomalyDetection, useDatasetLabels, useCapabilities. Since these use handleResponse which throws ApiClientError, verifying error propagation increases confidence.
Also, DR-04 unmount-during-pending coverage is selective — useDashboardStats with its refetchInterval: 60_000 and useOfflineAnnotations.sync() are not covered. The refetch interval case is particularly relevant since a background refetch firing into a dead mount is a common source of test flakiness and "state update on unmounted component" warnings.
3 participants
Description
Adds Vitest unit test coverage for 13 previously untested dataviewer custom hooks and establishes shared frontend testing infrastructure. Introduces the
renderHookWithProvidershelper and a consolidated fetch mocking module (installFetchMock,jsonResponse,mockMutationFetch) undersrc/test-utils/, adds unmount-during-pending coverage (DR-04) for async hooks, and raises Vitest coverage thresholds from45/35/35/45to55/55/40/55with safety margin. Also documents the--no-file-parallelismrequirement for local coverage runs (happy-dom races under parallel workers).Closes #214
Type of Change
Component(s) Affected
infrastructure/terraform/prerequisites/- Azure subscription setupinfrastructure/terraform/- Terraform infrastructureinfrastructure/setup/- OSMO control plane / Helmworkflows/- Training and evaluation workflowstraining/- Training pipelines and scriptsdocs/- Documentationdata-management/viewer/frontend/(test-only changes; not on the predefined list).Testing Performed
planreviewed (no unexpected changes)applytested in dev environmentsmoke_test_azure.py)cd data-management/viewer/frontend && npm run validate— 832 passing, 1 skipped; coverage 61.08% lines / 46.82% branches / 62.07% functions / 61.41% statements (above the new 55/55/40/55 thresholds).Documentation Impact
Bug Fix Checklist
Skipped — test infrastructure addition, not a bug fix.
Checklist