Code Review Phase 13: Tests & Quality Assurance

[jitu5]

Code Review Phase 13: Test Infrastructure & Coverage Assessment

Purpose: Review the shared test infrastructure, contract tests, and assess overall test coverage gaps. By now, reviewers have already seen all implementation test files alongside their respective phases — this phase focuses on the testing foundation and cross-cutting quality.

Prerequisites: All prior phases (0-12)
Estimated time: 1 hour
Files: 7

---

Current Test Metrics (Updated 2026-02-13)

Metric	Value
Test files	46
Total tests	376
Pass rate	100%
Coverage	64.06% overall
Test types	Unit, contract, integration, component

---

Files to Review (in order)

Test Infrastructure

• src/test/setup.ts — Vitest setup: localStorage mock, ResizeObserver mock, IntersectionObserver mock
• src/test/fixtures/mockData.ts — Comprehensive mock data (nodes, datasets, connections)
• src/test/utils/mockStore.ts — Redux store test utilities
• src/test/utils/testUtils.tsx — Render utilities with Provider wrapping

Contract Tests (cross-cutting invariants)

• src/test/contracts/events.contracts.test.ts — 11 tests: custom window event format contracts
• src/test/contracts/idFormats.contracts.test.ts — 25 tests: ID format invariants (prefixes, type guards, connection IDs)
• src/test/contracts/localStorage.contracts.test.ts — 12 tests: localStorage key format and STORAGE_KEYS consistency

---

Key Concepts

• Contract tests verify that string formats, event shapes, and storage keys remain stable — preventing silent cross-module breakage when constants change
• Mock data in mockData.ts represents a complete ML pipeline for realistic test scenarios
• Tests use Vitest (Vite-native) with jsdom for DOM simulation
• Test suite was trimmed from 482 to 340 tests with zero coverage impact, then expanded to 376 tests with 23 new component test files

---

Action Items

Run these commands to assess the current state:

npm run test           # Run all 376 tests
npm run test:coverage  # Generate coverage report
npm run lint           # Check linting
npm run build          # Verify production build

---

Focus Areas

• Coverage assessment: Run npm run test:coverage — current coverage is 64.06%. What are the biggest gaps?
• Canvas hooks are now tested with smoke tests (API shape verification + key behaviors)
• React components now have 23 test files covering UI primitives, Canvas system, Export wizard, and Code viewer
• Do contract tests cover ALL constants from Phase 2 (storage keys, event names, ID formats)?
• Are mock store utilities consistent with the actual store configuration from Phase 4?
• Is the Vitest configuration properly set up for coverage thresholds?
• Are test fixtures (mockData.ts) consistent with current type definitions?

---

Review complete! All findings should be collected into a single follow-up PR for code improvements.

[ravi-kumar-pilla]

Issue 1 — createMockState / createTestState initialize tutorialStep: 0 and walkthroughStep: 0 — diverges from uiSlice initial state of 1, crashing tests that render tutorial/walkthrough

Files:

• src/test/utils/mockStore.ts (lines 61, 64, 138, 141)
• src/features/ui/uiSlice.ts (lines 8, 14)

Severity: High — any test that renders TutorialModal or WalkthroughOverlay via these helpers with showTutorial: true / showWalkthrough: true will crash with a TypeError

Description:

Both createMockState (returned from the helper) and createTestState (used for pure-function tests) hard-code step values of 0:

// mockStore.ts lines 61–64  (createMockState)
tutorialStep: 0,          // ← wrong
tutorialCompleted: false,
showWalkthrough: false,
walkthroughStep: 0,       // ← wrong

// mockStore.ts lines 138–141  (createTestState)
tutorialStep: 0,          // ← wrong
...
walkthroughStep: 0,       // ← wrong

The uiSlice initial state (which represents the true production default) uses 1 for both:

// uiSlice.ts line 8, 14
tutorialStep: 1,
walkthroughStep: 1,

TutorialModal.tsx unconditionally accesses tutorialSteps[currentStep - 1]. With currentStep = 0, this evaluates to tutorialSteps[-1] = undefined. The very next line:

const Icon = typeof step.icon === 'string' ? null : step.icon;  // TypeError: Cannot read properties of undefined

crashes the render.

Current test gap: No test calls createMockState() with showTutorial: true and then renders TutorialModal — so this crash is latent but undetected. The moment someone adds such a test (or uses showWalkthrough: true with WalkthroughOverlay), it will fail with a TypeError, not with a meaningful assertion failure.

Secondary consequence — nextTutorialStep reducer also has this hardcoded cap:

// uiSlice.ts line 51
nextTutorialStep: (state) => {
  if (state.tutorialStep < 7) {   // ← same magic number as TutorialModal.tsx
    state.tutorialStep += 1;
  }
},

At step 0, the reducer increments to 1 (a real step), but the guard caps advancement at 7. This is a second instance of the Phase 12 Issue 1 magic number. With the mock starting at step 0, the step sequence under test is 0 → 1 → 2 → ... → 7 (8 total states), not the real 1 → 2 → ... → 7 (7 steps). Any test counting dispatches to reach the final step will be off by one.

Fix:

// createMockState and createTestState — align with uiSlice initial state
tutorialStep: 1,    // was 0
walkthroughStep: 1, // was 0

Separately, fix uiSlice.ts to derive the step cap from a shared constant (same fix as Phase 12 Issue 1):

import { tutorialSteps } from '../../components/Tutorial/tutorialSteps';

nextTutorialStep: (state) => {
  if (state.tutorialStep < tutorialSteps.length) {  // derived, not magic
    state.tutorialStep += 1;
  }
},

---

Issue 2 — testUtils.tsx configures the store without serializableCheck: false — middleware diverges from mockStore.ts, making the two render utilities behave differently

File: src/test/utils/testUtils.tsx (lines 29–43)

Severity: High — tests that dispatch non-serializable values via renderWithProviders silently fail with Redux middleware warnings that don't appear in tests using createMockStore

Description:

mockStore.ts explicitly disables Redux Toolkit's serializable check:

// mockStore.ts lines 31–34
middleware: (getDefaultMiddleware) =>
  getDefaultMiddleware({
    serializableCheck: false,
  }),

testUtils.tsx (used by all 23 component test files via renderWithProviders) passes no middleware override:

// testUtils.tsx lines 29–42
store = configureStore({
  reducer: { ... } as any,
  preloadedState: preloadedState as any,
  // ← default middleware, serializableCheck is ON
}),

Redux Toolkit's serializableCheck middleware throws a console.error for any non-serializable value in state or dispatched actions. The production configureStore (in the actual app) also disables this check. So the test utility that most closely mimics production (renderWithProviders) is actually more strict than production, while createMockStore matches production.

Concrete failure mode: Any component test that dispatches an action containing a Date object, a Map, or a function (e.g., in mock callbacks or project state with Date.now() stored as a number but the pattern is fragile) will print Redux warnings. If these warnings were ever promoted to errors (e.g., vi.spyOn(console, 'error').mockImplementation(() => { throw }) in a test), renderWithProviders tests would fail while createMockStore tests pass.

The two utilities are also inconsistent in a subtler way: renderWithProviders creates a new store per test call (inline configureStore in the default parameter) while mockStore.ts returns a reusable store. Tests that test multi-action sequences must use mockStore.ts; tests that want a fresh store use testUtils.tsx. This split is intentional but the middleware gap is not.

Fix:

// testUtils.tsx — add middleware override to match mockStore.ts and production
store = configureStore({
  reducer: { ... } as any,
  preloadedState: preloadedState as any,
  middleware: (getDefaultMiddleware) =>
    getDefaultMiddleware({ serializableCheck: false }),
}),

---

Issue 3 — setup.ts global localStorage mock returns undefined (not null) for missing keys — safeGetItem contract breaks in all component tests

File: src/test/setup.ts (lines 11–18)

Severity: High — components and hooks that check safeGetItem(key) === null behave incorrectly in all tests using the global mock

Description:

The global mock assigns vi.fn() instances for all Storage methods:

const localStorageMock = {
  getItem: vi.fn(),   // ← default return value is undefined
  setItem: vi.fn(),
  removeItem: vi.fn(),
  clear: vi.fn(),
};
global.localStorage = localStorageMock as unknown as Storage;

The real localStorage.getItem returns null for missing keys. The vi.fn() default returns undefined. This creates a gap for any code using strict null checks:

// safeGetItem (storage.ts line 65)
return localStorage.getItem(key);  // returns undefined in tests, null in production

Any consumer doing:

if (safeGetItem(STORAGE_KEYS.CURRENT_PROJECT) === null) {
  // first-run logic — NEVER executes in tests using setup.ts mock
}

gets undefined !== null → the condition is always false → first-run branches (tutorial auto-start, empty state initialization) are never exercised in component tests.

useAppInitialization uses truthy checks (if (savedItem)) which happen to work because both null and undefined are falsy. But the contract for StorageKey → string | null (the declared return type) is violated, and tests asserting specific return values from safeGetItem would fail.

The storage.test.ts file explicitly documents this problem:

// storage.test.ts line 5-6
// The global test setup (test/setup.ts) replaces localStorage with vi.fn() stubs
// that silently swallow every call -- exactly the over-mocking problem this test exists to fix.

The workaround is local to storage.test.ts. All other files using localStorage in component tests receive the broken mock.

Additional problem — mock state is not reset between tests:

There is no afterEach(() => vi.clearAllMocks()) in setup.ts, and vite.config.ts does not set clearMocks: true. Mock call counts from Test A accumulate into Test B. A test that asserts:

expect(localStorage.setItem).toHaveBeenCalledTimes(1);

...may fail if a previous test in the same file also called setItem.

Fix:

Option A — make getItem return null by default and reset between tests:

const localStorageMock = {
  getItem: vi.fn().mockReturnValue(null),   // match real Storage contract
  setItem: vi.fn(),
  removeItem: vi.fn(),
  clear: vi.fn(),
};
global.localStorage = localStorageMock as unknown as Storage;

afterEach(() => {
  vi.clearAllMocks();  // reset call counts and mock implementations
  cleanup();
});

Option B — set clearMocks: true globally in vite.config.ts:

test: {
  globals: true,
  environment: 'jsdom',
  setupFiles: './src/test/setup.ts',
  clearMocks: true,   // ← add this
  ...
}

Option A is preferred; it also fixes the undefined vs null return value issue which clearMocks: true alone does not address.

---

Issue 4 — No coverage thresholds in vite.config.ts — 64.06% coverage can silently regress to 0% without a build failure

File: vite.config.ts (lines 27–38)

Severity: High — a PR that deletes test files or breaks branch coverage passes CI with no indication of regression

Description:

The coverage config specifies reporters but no enforcement thresholds:

// vite.config.ts lines 27–38
coverage: {
  provider: 'v8',
  reporter: ['text', 'json', 'html'],
  exclude: [
    'node_modules/',
    'src/test/',
    '**/*.d.ts',
    '**/*.config.*',
    '**/mockData',
    'dist/',
  ],
  // ← no thresholds block
},

Without thresholds, the npm run test:coverage command always exits 0 regardless of coverage percentage. The stated metric of 64.06% is informational only — it has no enforcement mechanism.

This is particularly risky given the history: the suite was actively trimmed from 482 → 340 → 376 tests. A future trimming pass could drop coverage substantially without detection.

Fix:

Add thresholds at the current baseline (or conservatively below it to allow minor variance):

coverage: {
  provider: 'v8',
  reporter: ['text', 'json', 'html'],
  exclude: [...],
  thresholds: {
    lines: 64,
    branches: 60,
    functions: 60,
    statements: 64,
  },
},

This causes npm run test:coverage to exit non-zero if coverage drops below the floor, making coverage regression a CI-detectable failure.