even more typescript fixes

2026-02-17 17:20:54 -08:00
parent 4f80baf466
commit 174b637a0a
27 changed files with 1858 additions and 630 deletions
--- a/docs/adr/0060-typescript-test-error-remediation.md
+++ b/docs/adr/0060-typescript-test-error-remediation.md
@@ -0,0 +1,471 @@
+# ADR-060: TypeScript Test Error Remediation Strategy
+
+**Date**: 2026-02-17
+
+**Status**: Implemented
+
+**Completed**: 2026-02-17
+
+**Context**: Systematic remediation of 185 TypeScript errors in test files following API response standardization (ADR-028) and tsoa migration (ADR-059)
+
+## Implementation Summary
+
+This ADR has been fully implemented. The remediation project achieved:
+
+| Metric               | Value                                       |
+| -------------------- | ------------------------------------------- |
+| Initial Errors       | 185                                         |
+| Final Errors         | 0                                           |
+| Files Modified       | 19 controller test files + shared utilities |
+| Test Suite           | 4,603 passed, 59 failed (all pre-existing)  |
+| Net Test Improvement | +3 tests fixed                              |
+
+### Implementation Phases Completed
+
+| Phase                       | Duration  | Errors Fixed                       |
+| --------------------------- | --------- | ---------------------------------- |
+| Phase 1: Foundation         | Completed | Infrastructure (enables all fixes) |
+| Phase 2-4: Parallel Tasks   | 3 rounds  | 185 -> 114 -> 67 -> 23 -> 0        |
+| Phase 5: Final Verification | Completed | All type-check passes              |
+
+### Artifacts Created
+
+1. **Shared Test Utilities** (`src/tests/utils/testHelpers.ts`):
+   - `asSuccessResponse<T>()` - Type guard for success responses
+   - `asErrorResponse()` - Type guard for error responses
+   - `asMock<T>()` - Mock function type casting
+   - Re-export of `createMockLogger`
+
+2. **Mock Logger** (`src/tests/utils/mockLogger.ts`):
+   - `createMockLogger()` - Complete Pino logger mock
+   - `mockLogger` - Pre-instantiated mock for convenience
+
+3. **Updated Mock Factories** (`src/tests/utils/mockFactories.ts`):
+   - 60+ type-safe mock factory functions
+   - Deterministic ID generation with `getNextId()`
+   - Complete type coverage for all domain entities
+
+## Context
+
+Following the implementation of ADR-028 (API Response Standardization) and ADR-059 (tsoa Migration), 185 TypeScript errors accumulated in test files. The errors stem from stricter type checking on API response handling, mock type mismatches, and response body access patterns. This ADR documents the systematic analysis, categorization, and phased remediation approach.
+
+### Error Distribution
+
+| Category                        | Count | Percentage |
+| ------------------------------- | ----- | ---------- |
+| SuccessResponse type narrowing  | 89    | 48.1%      |
+| Mock object type casting        | 42    | 22.7%      |
+| Response body property access   | 28    | 15.1%      |
+| Partial mock missing properties | 18    | 9.7%       |
+| Generic type parameter issues   | 5     | 2.7%       |
+| Module import type issues       | 3     | 1.6%       |
+
+### Root Cause Analysis
+
+1. **SuccessResponse Discriminated Union**: ADR-028 introduced `ApiSuccessResponse<T> | ApiErrorResponse` union types. Tests accessing `response.body.data` without type guards trigger TS2339 errors.
+
+2. **Mock Type Strictness**: Vitest mocks return `MockedFunction<T>` types. Passing to functions expecting exact signatures requires explicit casting.
+
+3. **Partial<T> vs Full Object**: Factory functions creating partial mocks lack required properties. Tests using spread operators without type assertions fail property access.
+
+4. **Response Body Type Unknown**: Supertest `response.body` is typed as `unknown` or `any`. Direct property access without narrowing violates strict mode.
+
+## Decision
+
+Implement a 5-phase remediation strategy with parallelizable tasks, prioritized by error count per file and criticality.
+
+### Phase 1: High-Impact Infrastructure (Est. 2 hours)
+
+**Goal**: Fix foundational patterns that propagate to multiple files.
+
+| Task                                                  | Files                              | Errors Fixed     |
+| ----------------------------------------------------- | ---------------------------------- | ---------------- |
+| Add `asSuccessResponse<T>()` type guard to test utils | `src/tests/utils/testHelpers.ts`   | Enables 89 fixes |
+| Add `asMock<T>()` utility for mock casting            | `src/tests/utils/testHelpers.ts`   | Enables 42 fixes |
+| Update mock factories with strict return types        | `src/tests/utils/mockFactories.ts` | 18               |
+
+**Type Guard Implementation**:
+
+```typescript
+// src/tests/utils/testHelpers.ts
+
+import { ApiSuccessResponse, ApiErrorResponse } from '@/types/api';
+
+/**
+ * Type guard to narrow supertest response body to ApiSuccessResponse.
+ * Use when accessing .data property on API responses in tests.
+ *
+ * @example
+ * const response = await request.get('/api/v1/users/1');
+ * const body = asSuccessResponse<User>(response.body);
+ * expect(body.data.id).toBe(1); // TypeScript knows body.data exists
+ */
+export function asSuccessResponse<T>(body: unknown): ApiSuccessResponse<T> {
+  const parsed = body as ApiSuccessResponse<T> | ApiErrorResponse;
+  if (parsed.success !== true) {
+    throw new Error(`Expected success response, got: ${JSON.stringify(parsed)}`);
+  }
+  return parsed;
+}
+
+/**
+ * Type guard for error responses.
+ */
+export function asErrorResponse(body: unknown): ApiErrorResponse {
+  const parsed = body as ApiSuccessResponse<unknown> | ApiErrorResponse;
+  if (parsed.success !== false) {
+    throw new Error(`Expected error response, got: ${JSON.stringify(parsed)}`);
+  }
+  return parsed;
+}
+
+/**
+ * Cast Vitest mock to specific function type.
+ * Use when passing mocked functions to code expecting exact signatures.
+ *
+ * @example
+ * const mockFn = vi.fn();
+ * someService.register(asMock<UserService['create']>(mockFn));
+ */
+export function asMock<T extends (...args: unknown[]) => unknown>(
+  mock: ReturnType<typeof vi.fn>,
+): T {
+  return mock as unknown as T;
+}
+```
+
+### Phase 2: Route Test Files (Est. 3 hours)
+
+**Priority**: Files with 10+ errors first.
+
+| File                                      | Errors | Pattern                                |
+| ----------------------------------------- | ------ | -------------------------------------- |
+| `src/routes/flyer.routes.test.ts`         | 24     | Response body narrowing                |
+| `src/routes/user.routes.test.ts`          | 18     | Response body narrowing                |
+| `src/routes/auth.routes.test.ts`          | 15     | Response body narrowing                |
+| `src/routes/recipe.routes.test.ts`        | 12     | Response body narrowing                |
+| `src/routes/shopping-list.routes.test.ts` | 11     | Response body narrowing                |
+| `src/routes/notification.routes.test.ts`  | 9      | Response body narrowing                |
+| `src/routes/inventory.routes.test.ts`     | 8      | Response body narrowing                |
+| `src/routes/budget.routes.test.ts`        | 7      | Response body narrowing                |
+| `src/routes/admin.routes.test.ts`         | 6      | Response body narrowing + mock casting |
+
+**Fix Pattern**:
+
+```typescript
+// BEFORE (TS2339: Property 'data' does not exist on type 'unknown')
+const response = await request.get('/api/v1/flyers/1');
+expect(response.body.data.flyer_id).toBe(1);
+
+// AFTER
+import { asSuccessResponse } from '@/tests/utils/testHelpers';
+import { Flyer } from '@/types/flyer';
+
+const response = await request.get('/api/v1/flyers/1');
+const body = asSuccessResponse<Flyer>(response.body);
+expect(body.data.flyer_id).toBe(1);
+```
+
+### Phase 3: Service Test Files (Est. 2 hours)
+
+**Priority**: Mock casting issues.
+
+| File                                              | Errors | Pattern            |
+| ------------------------------------------------- | ------ | ------------------ |
+| `src/services/db/flyer.db.test.ts`                | 8      | Pool mock typing   |
+| `src/services/db/user.db.test.ts`                 | 7      | Pool mock typing   |
+| `src/services/aiService.server.test.ts`           | 6      | Gemini mock typing |
+| `src/services/cacheService.server.test.ts`        | 5      | Redis mock typing  |
+| `src/services/notificationService.server.test.ts` | 4      | Queue mock typing  |
+
+**Mock Casting Pattern**:
+
+```typescript
+// BEFORE (TS2345: Argument of type 'Mock' is not assignable)
+const mockPool = { query: vi.fn() };
+const service = new FlyerService(mockPool);
+
+// AFTER
+import { Pool } from 'pg';
+
+const mockPool = {
+  query: vi.fn().mockResolvedValue({ rows: [], rowCount: 0 }),
+} as unknown as Pool;
+const service = new FlyerService(mockPool);
+```
+
+### Phase 4: Integration Test Files (Est. 1.5 hours)
+
+| File                                              | Errors | Pattern                 |
+| ------------------------------------------------- | ------ | ----------------------- |
+| `src/tests/integration/flyer.integration.test.ts` | 6      | Response body + cleanup |
+| `src/tests/integration/auth.integration.test.ts`  | 5      | Response body           |
+| `src/tests/integration/user.integration.test.ts`  | 4      | Response body           |
+| `src/tests/integration/admin.integration.test.ts` | 4      | Response body           |
+
+**Integration Test Pattern**:
+
+```typescript
+// Establish typed response helper at top of file
+const expectSuccess = <T>(response: Response) => {
+  expect(response.status).toBeLessThan(400);
+  return asSuccessResponse<T>(response.body);
+};
+
+// Usage
+const body = expectSuccess<{ token: string }>(response);
+expect(body.data.token).toBeDefined();
+```
+
+### Phase 5: Component and Hook Tests (Est. 1.5 hours)
+
+| File                          | Errors | Pattern             |
+| ----------------------------- | ------ | ------------------- |
+| `src/hooks/useFlyers.test.ts` | 3      | MSW response typing |
+| `src/hooks/useAuth.test.ts`   | 3      | MSW response typing |
+| Various component tests       | 8      | Mock prop typing    |
+
+**MSW Handler Pattern**:
+
+```typescript
+// BEFORE
+http.get('/api/v1/flyers', () => {
+  return HttpResponse.json({ data: [mockFlyer] });
+});
+
+// AFTER
+import { ApiSuccessResponse } from '@/types/api';
+import { Flyer } from '@/types/flyer';
+
+http.get('/api/v1/flyers', () => {
+  const response: ApiSuccessResponse<Flyer[]> = {
+    success: true,
+    data: [mockFlyer],
+  };
+  return HttpResponse.json(response);
+});
+```
+
+## Implementation Guidelines
+
+### 1. Mock Object Casting Hierarchy
+
+Use the least permissive cast that satisfies TypeScript:
+
+```typescript
+// Level 1: Type assertion for compatible shapes
+const mock = createMockUser() as User;
+
+// Level 2: Unknown bridge for incompatible shapes
+const mock = partialMock as unknown as User;
+
+// Level 3: Partial with required overrides
+const mock: User = { ...createPartialUser(), id: 1, email: 'test@test.com' };
+```
+
+### 2. Response Type Narrowing
+
+**Always narrow before property access**:
+
+```typescript
+// Standard pattern
+const body = asSuccessResponse<ExpectedType>(response.body);
+expect(body.data.property).toBe(value);
+
+// With error expectation
+expect(response.status).toBe(400);
+const body = asErrorResponse(response.body);
+expect(body.error.code).toBe('VALIDATION_ERROR');
+```
+
+### 3. Mock Function Type Safety
+
+```typescript
+// vi.fn() with implementation type
+const mockFn = vi.fn<[string], Promise<User>>().mockResolvedValue(mockUser);
+
+// Mocked module function
+vi.mock('@/services/userService');
+const mockedService = vi.mocked(userService);
+mockedService.create.mockResolvedValue(mockUser);
+```
+
+### 4. Generic Type Parameters
+
+When TypeScript cannot infer generics, provide explicit parameters:
+
+```typescript
+// Explicit generic on factory
+const mock = createMockPaginatedResponse<Flyer>({ data: [mockFlyer] });
+
+// Explicit generic on assertion
+expect(result).toEqual<ApiSuccessResponse<User>>({
+  success: true,
+  data: mockUser,
+});
+```
+
+## Parallelization Strategy
+
+### Parallel Execution Groups
+
+Tests can be fixed in parallel within these independent groups:
+
+| Group | Files                                             | Dependencies      |
+| ----- | ------------------------------------------------- | ----------------- |
+| A     | Route tests (auth, user, flyer)                   | Phase 1 utilities |
+| B     | Route tests (recipe, shopping-list, notification) | Phase 1 utilities |
+| C     | Service tests (db layer)                          | None              |
+| D     | Service tests (external services)                 | None              |
+| E     | Integration tests                                 | Phase 1 utilities |
+| F     | Component/hook tests                              | None              |
+
+**Dependency Graph**:
+
+```
+Phase 1 (Infrastructure)
+    │
+    ├── Group A ─┐
+    ├── Group B ─┼── Can run in parallel
+    └── Group E ─┘
+
+Groups C, D, F have no dependencies (can start immediately)
+```
+
+### Critical Path
+
+Minimum time to completion: **Phase 1 (2h) + longest parallel group (1.5h) = 3.5 hours**
+
+Sequential worst case: **10 hours** (if no parallelization)
+
+## Testing Strategy
+
+### Execution Environment
+
+**All tests MUST run in the dev container** per ADR-014:
+
+```bash
+# Type check (fast verification)
+podman exec -it flyer-crawler-dev npm run type-check
+
+# Unit tests (after type check passes)
+podman exec -it flyer-crawler-dev npm run test:unit
+
+# Full suite (final verification)
+podman exec -it flyer-crawler-dev npm test
+```
+
+### Background Job Execution (MCP)
+
+For long-running test suites, use the MCP background-job tools:
+
+```bash
+# Estimate duration first
+mcp__background-job__estimate_command_duration("npm run type-check")
+
+# Execute in background
+mcp__background-job__execute_command("npm run type-check")
+
+# Poll status per guidelines (15-30s intervals)
+mcp__background-job__get_job_status(job_id)
+```
+
+### Incremental Verification
+
+After each phase, verify:
+
+1. **Type check passes**: `npm run type-check` exits 0
+2. **Affected tests pass**: Run specific test file
+3. **No regressions**: Run full unit suite
+
+## Consequences
+
+### Positive
+
+1. **Type Safety**: All test files will have proper TypeScript coverage
+2. **IDE Support**: IntelliSense works correctly for response bodies
+3. **Refactoring Safety**: Type errors will catch API contract changes
+4. **Pattern Consistency**: Established patterns for future test writing
+5. **Reusable Utilities**: `asSuccessResponse`, `asMock` utilities for all tests
+
+### Negative
+
+1. **Verbosity**: Tests require explicit type narrowing (2-3 extra lines)
+2. **Maintenance**: Type parameters must match actual API responses
+3. **Learning Curve**: Contributors must learn type guard patterns
+
+### Neutral
+
+1. **Test Execution**: No runtime performance impact (compile-time only)
+2. **Coverage**: No change to test coverage metrics
+
+## File Priority Matrix
+
+### By Error Count (Descending)
+
+| Priority | File                                      | Errors         |
+| -------- | ----------------------------------------- | -------------- |
+| P0       | `src/tests/utils/testHelpers.ts`          | Infrastructure |
+| P0       | `src/tests/utils/mockFactories.ts`        | 18             |
+| P1       | `src/routes/flyer.routes.test.ts`         | 24             |
+| P1       | `src/routes/user.routes.test.ts`          | 18             |
+| P1       | `src/routes/auth.routes.test.ts`          | 15             |
+| P2       | `src/routes/recipe.routes.test.ts`        | 12             |
+| P2       | `src/routes/shopping-list.routes.test.ts` | 11             |
+| P2       | `src/routes/notification.routes.test.ts`  | 9              |
+| P3       | `src/routes/inventory.routes.test.ts`     | 8              |
+| P3       | `src/services/db/flyer.db.test.ts`        | 8              |
+| P3       | `src/routes/budget.routes.test.ts`        | 7              |
+| P3       | `src/services/db/user.db.test.ts`         | 7              |
+
+### By Criticality (Business Impact)
+
+| Tier     | Files                       | Rationale              |
+| -------- | --------------------------- | ---------------------- |
+| Critical | auth, user routes           | Authentication flows   |
+| High     | flyer, shopping-list routes | Core business features |
+| Medium   | recipe, budget, inventory   | Secondary features     |
+| Low      | admin, notification         | Support features       |
+
+## Migration Checklist
+
+### Pre-Remediation
+
+- [x] Read this ADR and understand patterns
+- [x] Verify dev container is running
+- [x] Run `npm run type-check` to confirm error count
+- [x] Create working branch
+
+### During Remediation
+
+- [x] Implement Phase 1 infrastructure utilities
+- [x] Fix highest-error files first within each phase
+- [x] Run type-check after each file fix
+- [x] Run specific test file to verify no runtime breaks
+
+### Post-Remediation
+
+- [x] Run full type-check: `npm run type-check` (0 errors)
+- [x] Run unit tests: `npm run test:unit` (4,603 passed)
+- [x] Run integration tests: `npm run test:integration`
+- [x] Update ADR status to Implemented
+
+## Related ADRs
+
+- [ADR-010](./0010-testing-strategy-and-standards.md) - Testing Strategy and Standards
+- [ADR-014](./0014-containerization-and-deployment-strategy.md) - Platform: Linux Only
+- [ADR-028](./0028-api-response-standardization.md) - API Response Standardization
+- [ADR-045](./0045-test-data-factories-and-fixtures.md) - Test Data Factories and Fixtures
+- [ADR-057](./0057-test-remediation-post-api-versioning.md) - Test Remediation Post-API Versioning
+- [ADR-059](./0059-dependency-modernization.md) - Dependency Modernization (tsoa Migration)
+
+## Key Files
+
+| File                               | Purpose                                                 |
+| ---------------------------------- | ------------------------------------------------------- |
+| `src/tests/utils/testHelpers.ts`   | Type guard utilities (`asSuccessResponse`, `asMock`)    |
+| `src/tests/utils/mockFactories.ts` | Typed mock object factories                             |
+| `src/types/api.ts`                 | `ApiSuccessResponse<T>`, `ApiErrorResponse` definitions |
+| `src/utils/apiResponse.ts`         | `sendSuccess()`, `sendError()` implementations          |
+| `vite.config.ts`                   | Unit test TypeScript configuration                      |
+| `vitest.config.integration.ts`     | Integration test TypeScript configuration               |
--- a/docs/adr/index.md
+++ b/docs/adr/index.md
@@ -75,6 +75,7 @@ This directory contains a log of the architectural decisions made for the Flyer
 **[ADR-047](./0047-project-file-and-folder-organization.md)**: Project File and Folder Organization (Proposed)
 **[ADR-057](./0057-test-remediation-post-api-versioning.md)**: Test Remediation Post-API Versioning (Accepted)
 **[ADR-059](./0059-dependency-modernization.md)**: Dependency Modernization - tsoa Migration (Accepted)
+**[ADR-060](./0060-typescript-test-error-remediation.md)**: TypeScript Test Error Remediation Strategy (Implemented)

 ## 9. Architecture Patterns