diff --git a/docs/adr/0040-testing-economics-and-priorities.md b/docs/adr/0040-testing-economics-and-priorities.md new file mode 100644 index 0000000..5f9b220 --- /dev/null +++ b/docs/adr/0040-testing-economics-and-priorities.md @@ -0,0 +1,214 @@ +# ADR-040: Testing Economics and Priorities + +**Date**: 2026-01-09 + +**Status**: Accepted + +## Context + +ADR-010 established the testing strategy and standards. However, it does not address the economic trade-offs of testing: when the cost of writing and maintaining tests exceeds their value. This document provides practical guidance on where to invest testing effort for maximum return. + +## Decision + +We adopt a **value-based testing approach** that prioritizes tests based on: + +1. Risk of the code path (what breaks if this fails?) +2. Stability of the code (how often does this change?) +3. Complexity of the logic (can a human easily verify correctness?) +4. Cost of the test (setup complexity, execution time, maintenance burden) + +## Testing Investment Matrix + +| Test Type | Investment Level | When to Write | When to Skip | +| --------------- | ------------------- | ------------------------------- | --------------------------------- | +| **E2E** | Minimal (5 tests) | Critical user flows only | Everything else | +| **Integration** | Moderate (17 tests) | API contracts, auth, DB queries | Internal service wiring | +| **Unit** | High (185+ tests) | Business logic, utilities | Defensive fallbacks, trivial code | + +## High-Value Tests (Always Write) + +### E2E Tests (Budget: 5-10 tests total) + +Write E2E tests for flows where failure means: + +- Users cannot sign up or log in +- Users cannot complete the core value proposition (upload flyer → see deals) +- Money or data is at risk + +**Current E2E coverage is appropriate:** + +- `auth.e2e.test.ts` - Registration, login, password reset +- `flyer-upload.e2e.test.ts` - Complete upload pipeline +- `user-journey.e2e.test.ts` - Full user workflow +- `admin-authorization.e2e.test.ts` - Admin access control +- `admin-dashboard.e2e.test.ts` - Admin operations + +**Do NOT add E2E tests for:** + +- UI variations or styling +- Edge cases (handle in unit tests) +- Features that can be tested faster at a lower level + +### Integration Tests (Budget: 15-25 tests) + +Write integration tests for: + +- Every public API endpoint (contract testing) +- Authentication and authorization flows +- Database queries that involve joins or complex logic +- Middleware behavior (rate limiting, validation) + +**Current integration coverage is appropriate:** + +- Auth, admin, user routes +- Flyer processing pipeline +- Shopping lists, budgets, recipes +- Gamification and notifications + +**Do NOT add integration tests for:** + +- Internal service-to-service calls (mock at boundaries) +- Simple CRUD operations (test the repository pattern once) +- UI components (use unit tests) + +### Unit Tests (Budget: Proportional to complexity) + +Write unit tests for: + +- **Pure functions and utilities** - High value, easy to test +- **Business logic in services** - Medium-high value +- **React components** - Rendering, user interactions, state changes +- **Custom hooks** - Data transformation, side effects +- **Validators and parsers** - Edge cases matter here + +## Low-Value Tests (Skip or Defer) + +### Tests That Cost More Than They're Worth + +1. **Defensive fallback code protected by types** + + ```typescript + // This fallback can never execute if types are correct + const name = store.name || 'Unknown'; // store.name is required + ``` + + - If you need `as any` to test it, the type system already prevents it + - Either remove the fallback or accept the coverage gap + +2. **Switch/case default branches for exhaustive enums** + + ```typescript + switch (status) { + case 'pending': + return 'yellow'; + case 'complete': + return 'green'; + default: + return ''; // TypeScript prevents this + } + ``` + + - The default exists for safety, not for execution + - Don't test impossible states + +3. **Trivial component variations** + - Testing every tab in a tab panel when they share logic + - Testing loading states that just show a spinner + - Testing disabled button states (test the logic that disables, not the disabled state) + +4. **Tests requiring excessive mock setup** + - If test setup is longer than test assertions, reconsider + - Per ADR-010: "Excessive mock setup" is a code smell + +5. **Framework behavior verification** + - React rendering, React Query caching, Router navigation + - Trust the framework; test your code + +### Coverage Gaps to Accept + +The following coverage gaps are acceptable and should NOT be closed with tests: + +| Pattern | Reason | Alternative | +| ------------------------------------------ | ------------------------- | ----------------------------- | +| `value \|\| 'default'` for required fields | Type system prevents | Remove fallback or accept gap | +| `catch (error) { ... }` for typed APIs | Error types are known | Test the expected error types | +| `default:` in exhaustive switches | TypeScript exhaustiveness | Accept gap | +| Logging statements | Observability, not logic | No test needed | +| Feature flags / environment checks | Tested by deployment | Config tests if complex | + +## Time Budget Guidelines + +For a typical feature (new API endpoint + UI): + +| Activity | Time Budget | Notes | +| --------------------------------------- | ----------- | ------------------------------------- | +| Unit tests (component + hook + utility) | 30-45 min | Write alongside code | +| Integration test (API contract) | 15-20 min | One test per endpoint | +| E2E test | 0 min | Only for critical paths | +| Total testing overhead | ~1 hour | Should not exceed implementation time | + +**Rule of thumb**: If testing takes longer than implementation, you're either: + +1. Testing too much +2. Writing tests that are too complex +3. Testing code that should be refactored + +## Coverage Targets + +We explicitly reject arbitrary coverage percentage targets. Instead: + +| Metric | Target | Rationale | +| ---------------------- | --------------- | -------------------------------------- | +| Statement coverage | No target | High coverage ≠ quality tests | +| Branch coverage | No target | Many branches are defensive/impossible | +| E2E test count | 5-10 | Critical paths only | +| Integration test count | 15-25 | API contracts | +| Unit test files | 1:1 with source | Colocated, proportional | + +## When to Add Tests to Existing Code + +Add tests when: + +1. **Fixing a bug** - Add a test that would have caught it +2. **Refactoring** - Add tests before changing behavior +3. **Code review feedback** - Reviewer identifies risk +4. **Production incident** - Prevent recurrence + +Do NOT add tests: + +1. To increase coverage percentages +2. For code that hasn't changed in 6+ months +3. For code scheduled for deletion/replacement + +## Consequences + +**Positive:** + +- Testing effort focuses on high-risk, high-value code +- Developers spend less time on low-value tests +- Test suite runs faster (fewer unnecessary tests) +- Maintenance burden decreases + +**Negative:** + +- Some defensive code paths remain untested +- Coverage percentages may not satisfy external audits +- Requires judgment calls that may be inconsistent + +## Key Files + +- `docs/adr/0010-testing-strategy-and-standards.md` - Testing mechanics +- `vitest.config.ts` - Coverage configuration +- `src/tests/` - Test utilities and setup + +## Review Checklist + +Before adding a new test, ask: + +1. [ ] What user-visible behavior does this test protect? +2. [ ] Can this be tested at a lower level (unit vs integration)? +3. [ ] Does this test require `as any` or mock gymnastics? +4. [ ] Will this test break when implementation changes (brittle)? +5. [ ] Is the test setup simpler than the code being tested? + +If any answer suggests low value, skip the test or simplify. diff --git a/docs/adr/index.md b/docs/adr/index.md index feab517..a7a82c3 100644 --- a/docs/adr/index.md +++ b/docs/adr/index.md @@ -60,6 +60,7 @@ This directory contains a log of the architectural decisions made for the Flyer **[ADR-010](./0010-testing-strategy-and-standards.md)**: Testing Strategy and Standards (Accepted) **[ADR-021](./0021-code-formatting-and-linting-unification.md)**: Code Formatting and Linting Unification (Accepted) **[ADR-027](./0027-standardized-naming-convention-for-ai-and-database-types.md)**: Standardized Naming Convention for AI and Database Types (Accepted) +**[ADR-040](./0040-testing-economics-and-priorities.md)**: Testing Economics and Priorities (Accepted) ## 9. Architecture Patterns diff --git a/src/components/FlyerCorrectionTool.test.tsx b/src/components/FlyerCorrectionTool.test.tsx index 720dc22..9aa42d2 100644 --- a/src/components/FlyerCorrectionTool.test.tsx +++ b/src/components/FlyerCorrectionTool.test.tsx @@ -48,7 +48,9 @@ describe('FlyerCorrectionTool', () => { }); it('should not render when isOpen is false', () => { - const { container } = renderWithProviders(); + const { container } = renderWithProviders( + , + ); expect(container.firstChild).toBeNull(); }); @@ -302,4 +304,45 @@ describe('FlyerCorrectionTool', () => { expect(clearRectSpy).toHaveBeenCalled(); }); + + it('should call rescanImageArea with "dates" type when Extract Sale Dates is clicked', async () => { + mockedAiApiClient.rescanImageArea.mockResolvedValue( + new Response(JSON.stringify({ text: 'Jan 1 - Jan 7' })), + ); + + renderWithProviders(); + + // Wait for image fetch to complete + await waitFor(() => expect(global.fetch).toHaveBeenCalledWith(defaultProps.imageUrl)); + + const canvas = screen.getByRole('dialog').querySelector('canvas')!; + const image = screen.getByAltText('Flyer for correction'); + + // Mock image dimensions + Object.defineProperty(image, 'naturalWidth', { value: 1000, configurable: true }); + Object.defineProperty(image, 'naturalHeight', { value: 800, configurable: true }); + Object.defineProperty(image, 'clientWidth', { value: 500, configurable: true }); + Object.defineProperty(image, 'clientHeight', { value: 400, configurable: true }); + + // Draw a selection + fireEvent.mouseDown(canvas, { clientX: 10, clientY: 10 }); + fireEvent.mouseMove(canvas, { clientX: 60, clientY: 30 }); + fireEvent.mouseUp(canvas); + + // Click the "Extract Sale Dates" button instead of "Extract Store Name" + fireEvent.click(screen.getByRole('button', { name: /extract sale dates/i })); + + await waitFor(() => { + expect(mockedAiApiClient.rescanImageArea).toHaveBeenCalledWith( + expect.any(File), + expect.objectContaining({ x: 20, y: 20, width: 100, height: 40 }), + 'dates', // This is the key difference - testing the 'dates' extraction type + ); + }); + + await waitFor(() => { + expect(mockedNotifySuccess).toHaveBeenCalledWith('Extracted: Jan 1 - Jan 7'); + expect(defaultProps.onDataExtracted).toHaveBeenCalledWith('dates', 'Jan 1 - Jan 7'); + }); + }); }); diff --git a/src/config/env.test.ts b/src/config/env.test.ts new file mode 100644 index 0000000..5630716 --- /dev/null +++ b/src/config/env.test.ts @@ -0,0 +1,432 @@ +// src/config/env.test.ts +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; + +describe('env config', () => { + const originalEnv = process.env; + + beforeEach(() => { + vi.resetModules(); + process.env = { ...originalEnv }; + }); + + afterEach(() => { + process.env = originalEnv; + }); + + /** + * Sets up minimal valid environment variables for config parsing. + */ + function setValidEnv(overrides: Record = {}) { + process.env = { + NODE_ENV: 'test', + // Database (required) + DB_HOST: 'localhost', + DB_PORT: '5432', + DB_USER: 'testuser', + DB_PASSWORD: 'testpass', + DB_NAME: 'testdb', + // Redis (required) + REDIS_URL: 'redis://localhost:6379', + // Auth (required - min 32 chars) + JWT_SECRET: 'this-is-a-test-secret-that-is-at-least-32-characters-long', + ...overrides, + }; + } + + describe('successful config parsing', () => { + it('should parse valid configuration with all required fields', async () => { + setValidEnv(); + + const { config } = await import('./env'); + + expect(config.database.host).toBe('localhost'); + expect(config.database.port).toBe(5432); + expect(config.database.user).toBe('testuser'); + expect(config.database.password).toBe('testpass'); + expect(config.database.name).toBe('testdb'); + expect(config.redis.url).toBe('redis://localhost:6379'); + expect(config.auth.jwtSecret).toBe( + 'this-is-a-test-secret-that-is-at-least-32-characters-long', + ); + }); + + it('should use default values for optional fields', async () => { + setValidEnv(); + + const { config } = await import('./env'); + + // Worker defaults + expect(config.worker.concurrency).toBe(1); + expect(config.worker.lockDuration).toBe(30000); + expect(config.worker.emailConcurrency).toBe(10); + expect(config.worker.analyticsConcurrency).toBe(1); + expect(config.worker.cleanupConcurrency).toBe(10); + expect(config.worker.weeklyAnalyticsConcurrency).toBe(1); + + // Server defaults + expect(config.server.port).toBe(3001); + expect(config.server.nodeEnv).toBe('test'); + expect(config.server.storagePath).toBe('/var/www/flyer-crawler.projectium.com/flyer-images'); + + // AI defaults + expect(config.ai.geminiRpm).toBe(5); + expect(config.ai.priceQualityThreshold).toBe(0.5); + + // SMTP defaults + expect(config.smtp.port).toBe(587); + expect(config.smtp.secure).toBe(false); + }); + + it('should parse custom port values', async () => { + setValidEnv({ + DB_PORT: '5433', + PORT: '4000', + SMTP_PORT: '465', + }); + + const { config } = await import('./env'); + + expect(config.database.port).toBe(5433); + expect(config.server.port).toBe(4000); + expect(config.smtp.port).toBe(465); + }); + + it('should parse boolean SMTP_SECURE correctly', async () => { + setValidEnv({ + SMTP_SECURE: 'true', + }); + + const { config } = await import('./env'); + + expect(config.smtp.secure).toBe(true); + }); + + it('should parse false for SMTP_SECURE when set to false', async () => { + setValidEnv({ + SMTP_SECURE: 'false', + }); + + const { config } = await import('./env'); + + expect(config.smtp.secure).toBe(false); + }); + + it('should parse worker concurrency values', async () => { + setValidEnv({ + WORKER_CONCURRENCY: '5', + WORKER_LOCK_DURATION: '60000', + EMAIL_WORKER_CONCURRENCY: '20', + ANALYTICS_WORKER_CONCURRENCY: '3', + CLEANUP_WORKER_CONCURRENCY: '15', + WEEKLY_ANALYTICS_WORKER_CONCURRENCY: '2', + }); + + const { config } = await import('./env'); + + expect(config.worker.concurrency).toBe(5); + expect(config.worker.lockDuration).toBe(60000); + expect(config.worker.emailConcurrency).toBe(20); + expect(config.worker.analyticsConcurrency).toBe(3); + expect(config.worker.cleanupConcurrency).toBe(15); + expect(config.worker.weeklyAnalyticsConcurrency).toBe(2); + }); + + it('should parse AI configuration values', async () => { + setValidEnv({ + GEMINI_API_KEY: 'test-gemini-key', + GEMINI_RPM: '10', + AI_PRICE_QUALITY_THRESHOLD: '0.75', + }); + + const { config } = await import('./env'); + + expect(config.ai.geminiApiKey).toBe('test-gemini-key'); + expect(config.ai.geminiRpm).toBe(10); + expect(config.ai.priceQualityThreshold).toBe(0.75); + }); + + it('should parse Google configuration values', async () => { + setValidEnv({ + GOOGLE_MAPS_API_KEY: 'test-maps-key', + GOOGLE_CLIENT_ID: 'test-client-id', + GOOGLE_CLIENT_SECRET: 'test-client-secret', + }); + + const { config } = await import('./env'); + + expect(config.google.mapsApiKey).toBe('test-maps-key'); + expect(config.google.clientId).toBe('test-client-id'); + expect(config.google.clientSecret).toBe('test-client-secret'); + }); + + it('should parse optional SMTP configuration', async () => { + setValidEnv({ + SMTP_HOST: 'smtp.example.com', + SMTP_USER: 'smtp-user', + SMTP_PASS: 'smtp-pass', + SMTP_FROM_EMAIL: 'noreply@example.com', + }); + + const { config } = await import('./env'); + + expect(config.smtp.host).toBe('smtp.example.com'); + expect(config.smtp.user).toBe('smtp-user'); + expect(config.smtp.pass).toBe('smtp-pass'); + expect(config.smtp.fromEmail).toBe('noreply@example.com'); + }); + + it('should parse optional JWT_SECRET_PREVIOUS for rotation', async () => { + setValidEnv({ + JWT_SECRET_PREVIOUS: 'old-secret-that-is-at-least-32-characters-long', + }); + + const { config } = await import('./env'); + + expect(config.auth.jwtSecretPrevious).toBe('old-secret-that-is-at-least-32-characters-long'); + }); + + it('should handle empty string values as undefined for optional int fields', async () => { + setValidEnv({ + GEMINI_RPM: '', + AI_PRICE_QUALITY_THRESHOLD: ' ', + }); + + const { config } = await import('./env'); + + // Should use defaults when empty + expect(config.ai.geminiRpm).toBe(5); + expect(config.ai.priceQualityThreshold).toBe(0.5); + }); + }); + + describe('convenience helpers', () => { + it('should export isProduction as false in test env', async () => { + setValidEnv({ NODE_ENV: 'test' }); + + const { isProduction } = await import('./env'); + + expect(isProduction).toBe(false); + }); + + it('should export isTest as true in test env', async () => { + setValidEnv({ NODE_ENV: 'test' }); + + const { isTest } = await import('./env'); + + expect(isTest).toBe(true); + }); + + it('should export isDevelopment as false in test env', async () => { + setValidEnv({ NODE_ENV: 'test' }); + + const { isDevelopment } = await import('./env'); + + expect(isDevelopment).toBe(false); + }); + + it('should export isSmtpConfigured as false when SMTP not configured', async () => { + setValidEnv(); + + const { isSmtpConfigured } = await import('./env'); + + expect(isSmtpConfigured).toBe(false); + }); + + it('should export isSmtpConfigured as true when all SMTP fields present', async () => { + setValidEnv({ + SMTP_HOST: 'smtp.example.com', + SMTP_USER: 'user', + SMTP_PASS: 'pass', + SMTP_FROM_EMAIL: 'noreply@example.com', + }); + + const { isSmtpConfigured } = await import('./env'); + + expect(isSmtpConfigured).toBe(true); + }); + + it('should export isAiConfigured as false when Gemini not configured', async () => { + setValidEnv(); + + const { isAiConfigured } = await import('./env'); + + expect(isAiConfigured).toBe(false); + }); + + it('should export isAiConfigured as true when Gemini key present', async () => { + setValidEnv({ + GEMINI_API_KEY: 'test-key', + }); + + const { isAiConfigured } = await import('./env'); + + expect(isAiConfigured).toBe(true); + }); + + it('should export isGoogleMapsConfigured as false when not configured', async () => { + setValidEnv(); + + const { isGoogleMapsConfigured } = await import('./env'); + + expect(isGoogleMapsConfigured).toBe(false); + }); + + it('should export isGoogleMapsConfigured as true when Maps key present', async () => { + setValidEnv({ + GOOGLE_MAPS_API_KEY: 'test-maps-key', + }); + + const { isGoogleMapsConfigured } = await import('./env'); + + expect(isGoogleMapsConfigured).toBe(true); + }); + }); + + describe('validation errors', () => { + it('should throw error when DB_HOST is missing', async () => { + setValidEnv(); + delete process.env.DB_HOST; + + await expect(import('./env')).rejects.toThrow('CONFIGURATION ERROR'); + }); + + it('should throw error when DB_USER is missing', async () => { + setValidEnv(); + delete process.env.DB_USER; + + await expect(import('./env')).rejects.toThrow('CONFIGURATION ERROR'); + }); + + it('should throw error when DB_PASSWORD is missing', async () => { + setValidEnv(); + delete process.env.DB_PASSWORD; + + await expect(import('./env')).rejects.toThrow('CONFIGURATION ERROR'); + }); + + it('should throw error when DB_NAME is missing', async () => { + setValidEnv(); + delete process.env.DB_NAME; + + await expect(import('./env')).rejects.toThrow('CONFIGURATION ERROR'); + }); + + it('should throw error when REDIS_URL is missing', async () => { + setValidEnv(); + delete process.env.REDIS_URL; + + await expect(import('./env')).rejects.toThrow('CONFIGURATION ERROR'); + }); + + it('should throw error when REDIS_URL is invalid', async () => { + setValidEnv({ + REDIS_URL: 'not-a-valid-url', + }); + + await expect(import('./env')).rejects.toThrow('CONFIGURATION ERROR'); + }); + + it('should throw error when JWT_SECRET is missing', async () => { + setValidEnv(); + delete process.env.JWT_SECRET; + + await expect(import('./env')).rejects.toThrow('CONFIGURATION ERROR'); + }); + + it('should throw error when JWT_SECRET is too short', async () => { + setValidEnv({ + JWT_SECRET: 'short', + }); + + await expect(import('./env')).rejects.toThrow('CONFIGURATION ERROR'); + }); + + it('should include field path in error message', async () => { + setValidEnv(); + delete process.env.DB_HOST; + + await expect(import('./env')).rejects.toThrow('database.host'); + }); + }); + + describe('environment modes', () => { + it('should set nodeEnv to development by default', async () => { + setValidEnv(); + delete process.env.NODE_ENV; + + const { config } = await import('./env'); + + expect(config.server.nodeEnv).toBe('development'); + }); + + it('should accept production as NODE_ENV', async () => { + setValidEnv({ + NODE_ENV: 'production', + }); + + const { config, isProduction, isDevelopment, isTest } = await import('./env'); + + expect(config.server.nodeEnv).toBe('production'); + expect(isProduction).toBe(true); + expect(isDevelopment).toBe(false); + expect(isTest).toBe(false); + }); + + it('should accept development as NODE_ENV', async () => { + setValidEnv({ + NODE_ENV: 'development', + }); + + const { config, isProduction, isDevelopment, isTest } = await import('./env'); + + expect(config.server.nodeEnv).toBe('development'); + expect(isProduction).toBe(false); + expect(isDevelopment).toBe(true); + expect(isTest).toBe(false); + }); + }); + + describe('server configuration', () => { + it('should parse FRONTEND_URL when provided', async () => { + setValidEnv({ + FRONTEND_URL: 'https://example.com', + }); + + const { config } = await import('./env'); + + expect(config.server.frontendUrl).toBe('https://example.com'); + }); + + it('should parse BASE_URL when provided', async () => { + setValidEnv({ + BASE_URL: '/api/v1', + }); + + const { config } = await import('./env'); + + expect(config.server.baseUrl).toBe('/api/v1'); + }); + + it('should parse STORAGE_PATH when provided', async () => { + setValidEnv({ + STORAGE_PATH: '/custom/storage/path', + }); + + const { config } = await import('./env'); + + expect(config.server.storagePath).toBe('/custom/storage/path'); + }); + }); + + describe('Redis configuration', () => { + it('should parse REDIS_PASSWORD when provided', async () => { + setValidEnv({ + REDIS_PASSWORD: 'redis-secret', + }); + + const { config } = await import('./env'); + + expect(config.redis.password).toBe('redis-secret'); + }); + }); +}); diff --git a/src/config/queryClient.test.tsx b/src/config/queryClient.test.tsx new file mode 100644 index 0000000..e17fc1c --- /dev/null +++ b/src/config/queryClient.test.tsx @@ -0,0 +1,98 @@ +// src/config/queryClient.test.ts +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { QueryClientProvider } from '@tanstack/react-query'; +import { renderHook, waitFor } from '@testing-library/react'; +import { useMutation } from '@tanstack/react-query'; +import type { ReactNode } from 'react'; +import { queryClient } from './queryClient'; +import * as loggerModule from '../services/logger.client'; + +vi.mock('../services/logger.client', () => ({ + logger: { + error: vi.fn(), + info: vi.fn(), + warn: vi.fn(), + debug: vi.fn(), + }, +})); + +const mockedLogger = vi.mocked(loggerModule.logger); + +describe('queryClient', () => { + beforeEach(() => { + vi.resetAllMocks(); + queryClient.clear(); + }); + + afterEach(() => { + queryClient.clear(); + }); + + describe('configuration', () => { + it('should have correct default query options', () => { + const defaultOptions = queryClient.getDefaultOptions(); + + expect(defaultOptions.queries?.staleTime).toBe(1000 * 60 * 5); // 5 minutes + expect(defaultOptions.queries?.gcTime).toBe(1000 * 60 * 30); // 30 minutes + expect(defaultOptions.queries?.retry).toBe(1); + expect(defaultOptions.queries?.refetchOnWindowFocus).toBe(false); + expect(defaultOptions.queries?.refetchOnMount).toBe(true); + expect(defaultOptions.queries?.refetchOnReconnect).toBe(false); + }); + + it('should have correct default mutation options', () => { + const defaultOptions = queryClient.getDefaultOptions(); + + expect(defaultOptions.mutations?.retry).toBe(0); + expect(defaultOptions.mutations?.onError).toBeDefined(); + }); + }); + + describe('mutation onError callback', () => { + const wrapper = ({ children }: { children: ReactNode }) => ( + {children} + ); + + it('should log Error instance message on mutation error', async () => { + const testError = new Error('Test mutation error'); + + const { result } = renderHook( + () => + useMutation({ + mutationFn: async () => { + throw testError; + }, + }), + { wrapper }, + ); + + result.current.mutate(); + + await waitFor(() => expect(result.current.isError).toBe(true)); + + expect(mockedLogger.error).toHaveBeenCalledWith('Mutation error', { + error: 'Test mutation error', + }); + }); + + it('should log "Unknown error" for non-Error objects', async () => { + const { result } = renderHook( + () => + useMutation({ + mutationFn: async () => { + throw 'string error'; + }, + }), + { wrapper }, + ); + + result.current.mutate(); + + await waitFor(() => expect(result.current.isError).toBe(true)); + + expect(mockedLogger.error).toHaveBeenCalledWith('Mutation error', { + error: 'Unknown error', + }); + }); + }); +}); diff --git a/src/features/charts/PriceChart.test.tsx b/src/features/charts/PriceChart.test.tsx index 29fbe6d..e184325 100644 --- a/src/features/charts/PriceChart.test.tsx +++ b/src/features/charts/PriceChart.test.tsx @@ -124,4 +124,59 @@ describe('PriceChart', () => { // Milk: $1.13/L (already metric) expect(screen.getByText('$1.13/L')).toBeInTheDocument(); }); + + it('should display N/A when unit_price is null or undefined', () => { + const dealsWithoutUnitPrice: DealItem[] = [ + { + item: 'Mystery Item', + master_item_name: null, + price_display: '$9.99', + price_in_cents: 999, + quantity: '1 pack', + storeName: 'Test Store', + unit_price: null, // No unit price available + }, + ]; + + mockedUseActiveDeals.mockReturnValue({ + activeDeals: dealsWithoutUnitPrice, + isLoading: false, + error: null, + totalActiveItems: dealsWithoutUnitPrice.length, + }); + + render(); + + expect(screen.getByText('Mystery Item')).toBeInTheDocument(); + expect(screen.getByText('$9.99')).toBeInTheDocument(); + expect(screen.getByText('N/A')).toBeInTheDocument(); + }); + + it('should not show master item name when it matches the item name (case insensitive)', () => { + const dealWithSameMasterName: DealItem[] = [ + { + item: 'Apples', + master_item_name: 'APPLES', // Same as item name, different case + price_display: '$2.99', + price_in_cents: 299, + quantity: 'per lb', + storeName: 'Fresh Mart', + unit_price: { value: 299, unit: 'lb' }, + }, + ]; + + mockedUseActiveDeals.mockReturnValue({ + activeDeals: dealWithSameMasterName, + isLoading: false, + error: null, + totalActiveItems: dealWithSameMasterName.length, + }); + + render(); + + expect(screen.getByText('Apples')).toBeInTheDocument(); + // The master item name should NOT be shown since it matches the item name + expect(screen.queryByText('(APPLES)')).not.toBeInTheDocument(); + expect(screen.queryByText('(Apples)')).not.toBeInTheDocument(); + }); }); diff --git a/src/features/flyer/AnalysisPanel.test.tsx b/src/features/flyer/AnalysisPanel.test.tsx index 4522333..8107ec9 100644 --- a/src/features/flyer/AnalysisPanel.test.tsx +++ b/src/features/flyer/AnalysisPanel.test.tsx @@ -301,4 +301,61 @@ describe('AnalysisPanel', () => { expect(screen.getByText('Some insights.')).toBeInTheDocument(); expect(screen.queryByText('Sources:')).not.toBeInTheDocument(); }); + + it('should display sources for Plan Trip analysis type', () => { + const { rerender } = render(); + fireEvent.click(screen.getByRole('tab', { name: /plan trip/i })); + + mockedUseAiAnalysis.mockReturnValue({ + results: { PLAN_TRIP: 'Here is your trip plan.' }, + sources: { + PLAN_TRIP: [{ title: 'Store Location', uri: 'https://maps.example.com/store1' }], + }, + loadingAnalysis: null, + error: null, + runAnalysis: mockRunAnalysis, + generatedImageUrl: null, + generateImage: mockGenerateImage, + }); + + rerender(); + + expect(screen.getByText('Here is your trip plan.')).toBeInTheDocument(); + expect(screen.getByText('Sources:')).toBeInTheDocument(); + expect(screen.getByText('Store Location')).toBeInTheDocument(); + }); + + it('should display sources for Compare Prices analysis type', () => { + const { rerender } = render(); + fireEvent.click(screen.getByRole('tab', { name: /compare prices/i })); + + mockedUseAiAnalysis.mockReturnValue({ + results: { COMPARE_PRICES: 'Price comparison results.' }, + sources: { + COMPARE_PRICES: [{ title: 'Price Source', uri: 'https://prices.example.com/compare' }], + }, + loadingAnalysis: null, + error: null, + runAnalysis: mockRunAnalysis, + generatedImageUrl: null, + generateImage: mockGenerateImage, + }); + + rerender(); + + expect(screen.getByText('Price comparison results.')).toBeInTheDocument(); + expect(screen.getByText('Sources:')).toBeInTheDocument(); + expect(screen.getByText('Price Source')).toBeInTheDocument(); + }); + + it('should show a loading spinner when loading watched items', () => { + mockedUseUserData.mockReturnValue({ + watchedItems: [], + isLoading: true, + error: null, + }); + render(); + expect(screen.getByRole('status')).toBeInTheDocument(); + expect(screen.getByText('Loading data...')).toBeInTheDocument(); + }); }); diff --git a/src/features/flyer/BulkImporter.test.tsx b/src/features/flyer/BulkImporter.test.tsx index f300a6b..e8222e8 100644 --- a/src/features/flyer/BulkImporter.test.tsx +++ b/src/features/flyer/BulkImporter.test.tsx @@ -112,6 +112,30 @@ describe('BulkImporter', () => { expect(dropzone).not.toHaveClass('border-brand-primary'); }); + it('should not call onFilesChange when files are dropped while isProcessing is true', () => { + render(); + const dropzone = screen.getByText(/processing, please wait.../i).closest('label')!; + const file = new File(['content'], 'flyer.pdf', { type: 'application/pdf' }); + + fireEvent.drop(dropzone, { + dataTransfer: { + files: [file], + }, + }); + + expect(mockOnFilesChange).not.toHaveBeenCalled(); + }); + + it('should handle file input change with null files', async () => { + render(); + const input = screen.getByLabelText(/click to upload/i); + + // Simulate a change event with null files (e.g., when user cancels file picker) + fireEvent.change(input, { target: { files: null } }); + + expect(mockOnFilesChange).not.toHaveBeenCalled(); + }); + describe('when files are selected', () => { const imageFile = new File(['image-content'], 'flyer.jpg', { type: 'image/jpeg' }); const pdfFile = new File(['pdf-content'], 'document.pdf', { type: 'application/pdf' }); diff --git a/src/features/flyer/ExtractedDataTable.test.tsx b/src/features/flyer/ExtractedDataTable.test.tsx index 28e48f1..dab9809 100644 --- a/src/features/flyer/ExtractedDataTable.test.tsx +++ b/src/features/flyer/ExtractedDataTable.test.tsx @@ -561,5 +561,67 @@ describe('ExtractedDataTable', () => { render(); expect(screen.getByText('(5)')).toBeInTheDocument(); }); + + it('should use fallback category when adding to watchlist for items without category_name', () => { + const itemWithoutCategory = createMockFlyerItem({ + flyer_item_id: 999, + item: 'Mystery Item', + master_item_id: 10, + category_name: undefined, + flyer_id: 1, + }); + + // Mock masterItems to include a matching item for canonical name resolution + vi.mocked(useMasterItems).mockReturnValue({ + masterItems: [ + createMockMasterGroceryItem({ + master_grocery_item_id: 10, + name: 'Canonical Mystery', + }), + ], + isLoading: false, + error: null, + }); + + render(); + + const itemRow = screen.getByText('Mystery Item').closest('tr')!; + const watchButton = within(itemRow).getByTitle("Add 'Canonical Mystery' to your watchlist"); + fireEvent.click(watchButton); + + expect(mockAddWatchedItem).toHaveBeenCalledWith('Canonical Mystery', 'Other/Miscellaneous'); + }); + + it('should not call addItemToList when activeListId is null and button is clicked', () => { + vi.mocked(useShoppingLists).mockReturnValue({ + activeListId: null, + shoppingLists: [], + addItemToList: mockAddItemToList, + setActiveListId: vi.fn(), + createList: vi.fn(), + deleteList: vi.fn(), + updateItemInList: vi.fn(), + removeItemFromList: vi.fn(), + isCreatingList: false, + isDeletingList: false, + isAddingItem: false, + isUpdatingItem: false, + isRemovingItem: false, + error: null, + }); + + render(); + + // Even with disabled button, test the handler logic by verifying no call is made + // The buttons are disabled but we verify that even if clicked, no action occurs + const addToListButtons = screen.getAllByTitle('Select a shopping list first'); + expect(addToListButtons.length).toBeGreaterThan(0); + + // Click the button (even though disabled) + fireEvent.click(addToListButtons[0]); + + // addItemToList should not be called because activeListId is null + expect(mockAddItemToList).not.toHaveBeenCalled(); + }); }); }); diff --git a/src/features/flyer/FlyerDisplay.test.tsx b/src/features/flyer/FlyerDisplay.test.tsx index 3012d0c..faaa917 100644 --- a/src/features/flyer/FlyerDisplay.test.tsx +++ b/src/features/flyer/FlyerDisplay.test.tsx @@ -65,6 +65,12 @@ describe('FlyerDisplay', () => { expect(screen.queryByAltText('SuperMart Logo')).not.toBeInTheDocument(); }); + it('should use fallback alt text when store has logo_url but no name', () => { + const storeWithoutName = { ...mockStore, name: undefined }; + render(); + expect(screen.getByAltText('Store Logo')).toBeInTheDocument(); + }); + it('should format a single day validity correctly', () => { render(); expect(screen.getByText('Valid on October 26, 2023')).toBeInTheDocument(); diff --git a/src/features/flyer/FlyerList.test.tsx b/src/features/flyer/FlyerList.test.tsx index 71016dd..0b9934d 100644 --- a/src/features/flyer/FlyerList.test.tsx +++ b/src/features/flyer/FlyerList.test.tsx @@ -322,6 +322,20 @@ describe('FlyerList', () => { expect(screen.getByText('• Expires in 6 days')).toBeInTheDocument(); expect(screen.getByText('• Expires in 6 days')).toHaveClass('text-green-600'); }); + + it('should show "Expires in 1 day" (singular) when exactly 1 day left', () => { + vi.setSystemTime(new Date('2023-10-10T12:00:00Z')); // 1 day left until Oct 11 + render( + , + ); + expect(screen.getByText('• Expires in 1 day')).toBeInTheDocument(); + expect(screen.getByText('• Expires in 1 day')).toHaveClass('text-orange-500'); + }); }); describe('Admin Functionality', () => { @@ -420,6 +434,29 @@ describe('FlyerList', () => { expect(mockedToast.error).toHaveBeenCalledWith('Cleanup failed'); }); }); + + it('should show generic error toast if cleanup API call fails with non-Error object', async () => { + vi.spyOn(window, 'confirm').mockReturnValue(true); + // Reject with a non-Error value (e.g., a string or object) + mockedApiClient.cleanupFlyerFiles.mockRejectedValue('Some non-error value'); + + render( + , + ); + + const cleanupButton = screen.getByTitle('Clean up files for flyer ID 1'); + fireEvent.click(cleanupButton); + + await waitFor(() => { + expect(mockedApiClient.cleanupFlyerFiles).toHaveBeenCalledWith(1); + expect(mockedToast.error).toHaveBeenCalledWith('Failed to enqueue cleanup job.'); + }); + }); }); }); diff --git a/src/features/flyer/ProcessingStatus.test.tsx b/src/features/flyer/ProcessingStatus.test.tsx index f4d91b9..01a618e 100644 --- a/src/features/flyer/ProcessingStatus.test.tsx +++ b/src/features/flyer/ProcessingStatus.test.tsx @@ -210,4 +210,60 @@ describe('ProcessingStatus', () => { expect(nonCriticalErrorStage).toHaveTextContent('(optional)'); }); }); + + describe('Edge Cases', () => { + it('should render null for unknown stage status icon', () => { + const stagesWithUnknownStatus: ProcessingStage[] = [ + createMockProcessingStage({ + name: 'Unknown Stage', + status: 'unknown-status' as any, + detail: '', + }), + ]; + render(); + + const stageIcon = screen.getByTestId('stage-icon-0'); + // The icon container should be empty (no SVG or spinner rendered) + expect(stageIcon.querySelector('svg')).not.toBeInTheDocument(); + expect(stageIcon.querySelector('.animate-spin')).not.toBeInTheDocument(); + }); + + it('should return empty string for unknown stage status text color', () => { + const stagesWithUnknownStatus: ProcessingStage[] = [ + createMockProcessingStage({ + name: 'Unknown Stage', + status: 'unknown-status' as any, + detail: '', + }), + ]; + render(); + + const stageText = screen.getByTestId('stage-text-0'); + // Should not have any of the known status color classes + expect(stageText.className).not.toContain('text-brand-primary'); + expect(stageText.className).not.toContain('text-gray-700'); + expect(stageText.className).not.toContain('text-gray-400'); + expect(stageText.className).not.toContain('text-red-500'); + expect(stageText.className).not.toContain('text-yellow-600'); + }); + + it('should not render page progress bar when total is 1', () => { + render( + , + ); + expect(screen.queryByText(/converting pdf/i)).not.toBeInTheDocument(); + }); + + it('should not render stage progress bar when total is 1', () => { + const stagesWithSinglePageProgress: ProcessingStage[] = [ + createMockProcessingStage({ + name: 'Extracting Items', + status: 'in-progress', + progress: { current: 1, total: 1 }, + }), + ]; + render(); + expect(screen.queryByText(/analyzing page/i)).not.toBeInTheDocument(); + }); + }); }); diff --git a/src/hooks/mutations/useAddShoppingListItemMutation.test.tsx b/src/hooks/mutations/useAddShoppingListItemMutation.test.tsx index 0122109..e498e42 100644 --- a/src/hooks/mutations/useAddShoppingListItemMutation.test.tsx +++ b/src/hooks/mutations/useAddShoppingListItemMutation.test.tsx @@ -60,7 +60,9 @@ describe('useAddShoppingListItemMutation', () => { await waitFor(() => expect(result.current.isSuccess).toBe(true)); - expect(mockedApiClient.addShoppingListItem).toHaveBeenCalledWith(1, { customItemName: 'Special Milk' }); + expect(mockedApiClient.addShoppingListItem).toHaveBeenCalledWith(1, { + customItemName: 'Special Milk', + }); }); it('should invalidate shopping-lists query on success', async () => { @@ -97,7 +99,7 @@ describe('useAddShoppingListItemMutation', () => { expect(mockedNotifications.notifyError).toHaveBeenCalledWith('Item already exists'); }); - it('should handle API error without message', async () => { + it('should handle API error when json parse fails', async () => { mockedApiClient.addShoppingListItem.mockResolvedValue({ ok: false, status: 500, @@ -114,6 +116,22 @@ describe('useAddShoppingListItemMutation', () => { expect(mockedNotifications.notifyError).toHaveBeenCalledWith('Request failed with status 500'); }); + it('should handle API error with empty message in response', async () => { + mockedApiClient.addShoppingListItem.mockResolvedValue({ + ok: false, + status: 400, + json: () => Promise.resolve({ message: '' }), + } as Response); + + const { result } = renderHook(() => useAddShoppingListItemMutation(), { wrapper }); + + result.current.mutate({ listId: 1, item: { masterItemId: 42 } }); + + await waitFor(() => expect(result.current.isError).toBe(true)); + + expect(result.current.error?.message).toBe('Failed to add item to shopping list'); + }); + it('should handle network error', async () => { mockedApiClient.addShoppingListItem.mockRejectedValue(new Error('Network error')); @@ -125,4 +143,18 @@ describe('useAddShoppingListItemMutation', () => { expect(result.current.error?.message).toBe('Network error'); }); + + it('should use fallback error message when error has no message', async () => { + mockedApiClient.addShoppingListItem.mockRejectedValue(new Error('')); + + const { result } = renderHook(() => useAddShoppingListItemMutation(), { wrapper }); + + result.current.mutate({ listId: 1, item: { masterItemId: 42 } }); + + await waitFor(() => expect(result.current.isError).toBe(true)); + + expect(mockedNotifications.notifyError).toHaveBeenCalledWith( + 'Failed to add item to shopping list', + ); + }); }); diff --git a/src/hooks/mutations/useAddWatchedItemMutation.test.tsx b/src/hooks/mutations/useAddWatchedItemMutation.test.tsx index 6550562..c0dc0a0 100644 --- a/src/hooks/mutations/useAddWatchedItemMutation.test.tsx +++ b/src/hooks/mutations/useAddWatchedItemMutation.test.tsx @@ -97,7 +97,7 @@ describe('useAddWatchedItemMutation', () => { expect(mockedNotifications.notifyError).toHaveBeenCalledWith('Item already watched'); }); - it('should handle API error without message', async () => { + it('should handle API error when json parse fails', async () => { mockedApiClient.addWatchedItem.mockResolvedValue({ ok: false, status: 500, @@ -112,4 +112,34 @@ describe('useAddWatchedItemMutation', () => { expect(result.current.error?.message).toBe('Request failed with status 500'); }); + + it('should handle API error with empty message in response', async () => { + mockedApiClient.addWatchedItem.mockResolvedValue({ + ok: false, + status: 400, + json: () => Promise.resolve({ message: '' }), + } as Response); + + const { result } = renderHook(() => useAddWatchedItemMutation(), { wrapper }); + + result.current.mutate({ itemName: 'Butter' }); + + await waitFor(() => expect(result.current.isError).toBe(true)); + + expect(result.current.error?.message).toBe('Failed to add watched item'); + }); + + it('should use fallback error message when error has no message', async () => { + mockedApiClient.addWatchedItem.mockRejectedValue(new Error('')); + + const { result } = renderHook(() => useAddWatchedItemMutation(), { wrapper }); + + result.current.mutate({ itemName: 'Yogurt' }); + + await waitFor(() => expect(result.current.isError).toBe(true)); + + expect(mockedNotifications.notifyError).toHaveBeenCalledWith( + 'Failed to add item to watched list', + ); + }); }); diff --git a/src/hooks/mutations/useCreateShoppingListMutation.test.tsx b/src/hooks/mutations/useCreateShoppingListMutation.test.tsx index e090703..41c0dba 100644 --- a/src/hooks/mutations/useCreateShoppingListMutation.test.tsx +++ b/src/hooks/mutations/useCreateShoppingListMutation.test.tsx @@ -81,7 +81,7 @@ describe('useCreateShoppingListMutation', () => { expect(mockedNotifications.notifyError).toHaveBeenCalledWith('List name already exists'); }); - it('should handle API error without message', async () => { + it('should handle API error when json parse fails', async () => { mockedApiClient.createShoppingList.mockResolvedValue({ ok: false, status: 500, @@ -96,4 +96,32 @@ describe('useCreateShoppingListMutation', () => { expect(result.current.error?.message).toBe('Request failed with status 500'); }); + + it('should handle API error with empty message in response', async () => { + mockedApiClient.createShoppingList.mockResolvedValue({ + ok: false, + status: 400, + json: () => Promise.resolve({ message: '' }), + } as Response); + + const { result } = renderHook(() => useCreateShoppingListMutation(), { wrapper }); + + result.current.mutate({ name: 'Empty Error' }); + + await waitFor(() => expect(result.current.isError).toBe(true)); + + expect(result.current.error?.message).toBe('Failed to create shopping list'); + }); + + it('should use fallback error message when error has no message', async () => { + mockedApiClient.createShoppingList.mockRejectedValue(new Error('')); + + const { result } = renderHook(() => useCreateShoppingListMutation(), { wrapper }); + + result.current.mutate({ name: 'New List' }); + + await waitFor(() => expect(result.current.isError).toBe(true)); + + expect(mockedNotifications.notifyError).toHaveBeenCalledWith('Failed to create shopping list'); + }); }); diff --git a/src/hooks/mutations/useDeleteShoppingListMutation.test.tsx b/src/hooks/mutations/useDeleteShoppingListMutation.test.tsx index 3907955..3a24fd1 100644 --- a/src/hooks/mutations/useDeleteShoppingListMutation.test.tsx +++ b/src/hooks/mutations/useDeleteShoppingListMutation.test.tsx @@ -81,7 +81,7 @@ describe('useDeleteShoppingListMutation', () => { expect(mockedNotifications.notifyError).toHaveBeenCalledWith('Shopping list not found'); }); - it('should handle API error without message', async () => { + it('should handle API error when json parse fails', async () => { mockedApiClient.deleteShoppingList.mockResolvedValue({ ok: false, status: 500, @@ -96,4 +96,32 @@ describe('useDeleteShoppingListMutation', () => { expect(result.current.error?.message).toBe('Request failed with status 500'); }); + + it('should handle API error with empty message in response', async () => { + mockedApiClient.deleteShoppingList.mockResolvedValue({ + ok: false, + status: 400, + json: () => Promise.resolve({ message: '' }), + } as Response); + + const { result } = renderHook(() => useDeleteShoppingListMutation(), { wrapper }); + + result.current.mutate({ listId: 456 }); + + await waitFor(() => expect(result.current.isError).toBe(true)); + + expect(result.current.error?.message).toBe('Failed to delete shopping list'); + }); + + it('should use fallback error message when error has no message', async () => { + mockedApiClient.deleteShoppingList.mockRejectedValue(new Error('')); + + const { result } = renderHook(() => useDeleteShoppingListMutation(), { wrapper }); + + result.current.mutate({ listId: 789 }); + + await waitFor(() => expect(result.current.isError).toBe(true)); + + expect(mockedNotifications.notifyError).toHaveBeenCalledWith('Failed to delete shopping list'); + }); }); diff --git a/src/hooks/mutations/useRemoveShoppingListItemMutation.test.tsx b/src/hooks/mutations/useRemoveShoppingListItemMutation.test.tsx index 37db03e..43e549d 100644 --- a/src/hooks/mutations/useRemoveShoppingListItemMutation.test.tsx +++ b/src/hooks/mutations/useRemoveShoppingListItemMutation.test.tsx @@ -44,7 +44,9 @@ describe('useRemoveShoppingListItemMutation', () => { await waitFor(() => expect(result.current.isSuccess).toBe(true)); expect(mockedApiClient.removeShoppingListItem).toHaveBeenCalledWith(42); - expect(mockedNotifications.notifySuccess).toHaveBeenCalledWith('Item removed from shopping list'); + expect(mockedNotifications.notifySuccess).toHaveBeenCalledWith( + 'Item removed from shopping list', + ); }); it('should invalidate shopping-lists query on success', async () => { @@ -81,7 +83,7 @@ describe('useRemoveShoppingListItemMutation', () => { expect(mockedNotifications.notifyError).toHaveBeenCalledWith('Item not found'); }); - it('should handle API error without message', async () => { + it('should handle API error when json parse fails', async () => { mockedApiClient.removeShoppingListItem.mockResolvedValue({ ok: false, status: 500, @@ -96,4 +98,34 @@ describe('useRemoveShoppingListItemMutation', () => { expect(result.current.error?.message).toBe('Request failed with status 500'); }); + + it('should handle API error with empty message in response', async () => { + mockedApiClient.removeShoppingListItem.mockResolvedValue({ + ok: false, + status: 400, + json: () => Promise.resolve({ message: '' }), + } as Response); + + const { result } = renderHook(() => useRemoveShoppingListItemMutation(), { wrapper }); + + result.current.mutate({ itemId: 88 }); + + await waitFor(() => expect(result.current.isError).toBe(true)); + + expect(result.current.error?.message).toBe('Failed to remove shopping list item'); + }); + + it('should use fallback error message when error has no message', async () => { + mockedApiClient.removeShoppingListItem.mockRejectedValue(new Error('')); + + const { result } = renderHook(() => useRemoveShoppingListItemMutation(), { wrapper }); + + result.current.mutate({ itemId: 555 }); + + await waitFor(() => expect(result.current.isError).toBe(true)); + + expect(mockedNotifications.notifyError).toHaveBeenCalledWith( + 'Failed to remove shopping list item', + ); + }); }); diff --git a/src/hooks/mutations/useRemoveWatchedItemMutation.test.tsx b/src/hooks/mutations/useRemoveWatchedItemMutation.test.tsx index 942a923..d77e7f8 100644 --- a/src/hooks/mutations/useRemoveWatchedItemMutation.test.tsx +++ b/src/hooks/mutations/useRemoveWatchedItemMutation.test.tsx @@ -44,7 +44,9 @@ describe('useRemoveWatchedItemMutation', () => { await waitFor(() => expect(result.current.isSuccess).toBe(true)); expect(mockedApiClient.removeWatchedItem).toHaveBeenCalledWith(123); - expect(mockedNotifications.notifySuccess).toHaveBeenCalledWith('Item removed from watched list'); + expect(mockedNotifications.notifySuccess).toHaveBeenCalledWith( + 'Item removed from watched list', + ); }); it('should invalidate watched-items query on success', async () => { @@ -81,7 +83,7 @@ describe('useRemoveWatchedItemMutation', () => { expect(mockedNotifications.notifyError).toHaveBeenCalledWith('Watched item not found'); }); - it('should handle API error without message', async () => { + it('should handle API error when json parse fails', async () => { mockedApiClient.removeWatchedItem.mockResolvedValue({ ok: false, status: 500, @@ -96,4 +98,34 @@ describe('useRemoveWatchedItemMutation', () => { expect(result.current.error?.message).toBe('Request failed with status 500'); }); + + it('should handle API error with empty message in response', async () => { + mockedApiClient.removeWatchedItem.mockResolvedValue({ + ok: false, + status: 400, + json: () => Promise.resolve({ message: '' }), + } as Response); + + const { result } = renderHook(() => useRemoveWatchedItemMutation(), { wrapper }); + + result.current.mutate({ masterItemId: 222 }); + + await waitFor(() => expect(result.current.isError).toBe(true)); + + expect(result.current.error?.message).toBe('Failed to remove watched item'); + }); + + it('should use fallback error message when error has no message', async () => { + mockedApiClient.removeWatchedItem.mockRejectedValue(new Error('')); + + const { result } = renderHook(() => useRemoveWatchedItemMutation(), { wrapper }); + + result.current.mutate({ masterItemId: 321 }); + + await waitFor(() => expect(result.current.isError).toBe(true)); + + expect(mockedNotifications.notifyError).toHaveBeenCalledWith( + 'Failed to remove item from watched list', + ); + }); }); diff --git a/src/hooks/mutations/useUpdateShoppingListItemMutation.test.tsx b/src/hooks/mutations/useUpdateShoppingListItemMutation.test.tsx index 3f59457..638d455 100644 --- a/src/hooks/mutations/useUpdateShoppingListItemMutation.test.tsx +++ b/src/hooks/mutations/useUpdateShoppingListItemMutation.test.tsx @@ -74,7 +74,9 @@ describe('useUpdateShoppingListItemMutation', () => { await waitFor(() => expect(result.current.isSuccess).toBe(true)); - expect(mockedApiClient.updateShoppingListItem).toHaveBeenCalledWith(42, { custom_item_name: 'Organic Milk' }); + expect(mockedApiClient.updateShoppingListItem).toHaveBeenCalledWith(42, { + custom_item_name: 'Organic Milk', + }); }); it('should update notes', async () => { @@ -89,7 +91,9 @@ describe('useUpdateShoppingListItemMutation', () => { await waitFor(() => expect(result.current.isSuccess).toBe(true)); - expect(mockedApiClient.updateShoppingListItem).toHaveBeenCalledWith(42, { notes: 'Get the 2% variety' }); + expect(mockedApiClient.updateShoppingListItem).toHaveBeenCalledWith(42, { + notes: 'Get the 2% variety', + }); }); it('should update multiple fields at once', async () => { @@ -104,7 +108,10 @@ describe('useUpdateShoppingListItemMutation', () => { await waitFor(() => expect(result.current.isSuccess).toBe(true)); - expect(mockedApiClient.updateShoppingListItem).toHaveBeenCalledWith(42, { quantity: 2, notes: 'Important' }); + expect(mockedApiClient.updateShoppingListItem).toHaveBeenCalledWith(42, { + quantity: 2, + notes: 'Important', + }); }); it('should invalidate shopping-lists query on success', async () => { @@ -141,7 +148,7 @@ describe('useUpdateShoppingListItemMutation', () => { expect(mockedNotifications.notifyError).toHaveBeenCalledWith('Item not found'); }); - it('should handle API error without message', async () => { + it('should handle API error when json parse fails', async () => { mockedApiClient.updateShoppingListItem.mockResolvedValue({ ok: false, status: 500, @@ -156,4 +163,34 @@ describe('useUpdateShoppingListItemMutation', () => { expect(result.current.error?.message).toBe('Request failed with status 500'); }); + + it('should handle API error with empty message in response', async () => { + mockedApiClient.updateShoppingListItem.mockResolvedValue({ + ok: false, + status: 400, + json: () => Promise.resolve({ message: '' }), + } as Response); + + const { result } = renderHook(() => useUpdateShoppingListItemMutation(), { wrapper }); + + result.current.mutate({ itemId: 99, updates: { notes: 'test' } }); + + await waitFor(() => expect(result.current.isError).toBe(true)); + + expect(result.current.error?.message).toBe('Failed to update shopping list item'); + }); + + it('should use fallback error message when error has no message', async () => { + mockedApiClient.updateShoppingListItem.mockRejectedValue(new Error('')); + + const { result } = renderHook(() => useUpdateShoppingListItemMutation(), { wrapper }); + + result.current.mutate({ itemId: 77, updates: { is_purchased: true } }); + + await waitFor(() => expect(result.current.isError).toBe(true)); + + expect(mockedNotifications.notifyError).toHaveBeenCalledWith( + 'Failed to update shopping list item', + ); + }); }); diff --git a/src/hooks/queries/useActivityLogQuery.test.tsx b/src/hooks/queries/useActivityLogQuery.test.tsx index 579b1f3..e46c3c8 100644 --- a/src/hooks/queries/useActivityLogQuery.test.tsx +++ b/src/hooks/queries/useActivityLogQuery.test.tsx @@ -87,6 +87,20 @@ describe('useActivityLogQuery', () => { expect(result.current.error?.message).toBe('Request failed with status 500'); }); + it('should use fallback message when error.message is empty', async () => { + mockedApiClient.fetchActivityLog.mockResolvedValue({ + ok: false, + status: 500, + json: () => Promise.resolve({ message: '' }), + } as Response); + + const { result } = renderHook(() => useActivityLogQuery(), { wrapper }); + + await waitFor(() => expect(result.current.isError).toBe(true)); + + expect(result.current.error?.message).toBe('Failed to fetch activity log'); + }); + it('should return empty array for no activity log entries', async () => { mockedApiClient.fetchActivityLog.mockResolvedValue({ ok: true, diff --git a/src/hooks/queries/useApplicationStatsQuery.test.tsx b/src/hooks/queries/useApplicationStatsQuery.test.tsx index a53c18d..ca5c0cd 100644 --- a/src/hooks/queries/useApplicationStatsQuery.test.tsx +++ b/src/hooks/queries/useApplicationStatsQuery.test.tsx @@ -75,4 +75,18 @@ describe('useApplicationStatsQuery', () => { expect(result.current.error?.message).toBe('Request failed with status 500'); }); + + it('should use fallback message when error.message is empty', async () => { + mockedApiClient.getApplicationStats.mockResolvedValue({ + ok: false, + status: 500, + json: () => Promise.resolve({ message: '' }), + } as Response); + + const { result } = renderHook(() => useApplicationStatsQuery(), { wrapper }); + + await waitFor(() => expect(result.current.isError).toBe(true)); + + expect(result.current.error?.message).toBe('Failed to fetch application stats'); + }); }); diff --git a/src/hooks/queries/useCategoriesQuery.test.tsx b/src/hooks/queries/useCategoriesQuery.test.tsx index 3571bf8..9120c30 100644 --- a/src/hooks/queries/useCategoriesQuery.test.tsx +++ b/src/hooks/queries/useCategoriesQuery.test.tsx @@ -73,6 +73,20 @@ describe('useCategoriesQuery', () => { expect(result.current.error?.message).toBe('Request failed with status 500'); }); + it('should use fallback message when error.message is empty', async () => { + mockedApiClient.fetchCategories.mockResolvedValue({ + ok: false, + status: 500, + json: () => Promise.resolve({ message: '' }), + } as Response); + + const { result } = renderHook(() => useCategoriesQuery(), { wrapper }); + + await waitFor(() => expect(result.current.isError).toBe(true)); + + expect(result.current.error?.message).toBe('Failed to fetch categories'); + }); + it('should return empty array for no categories', async () => { mockedApiClient.fetchCategories.mockResolvedValue({ ok: true, diff --git a/src/hooks/queries/useFlyerItemsQuery.test.tsx b/src/hooks/queries/useFlyerItemsQuery.test.tsx index d2cfeef..938500f 100644 --- a/src/hooks/queries/useFlyerItemsQuery.test.tsx +++ b/src/hooks/queries/useFlyerItemsQuery.test.tsx @@ -83,6 +83,33 @@ describe('useFlyerItemsQuery', () => { expect(result.current.error?.message).toBe('Request failed with status 500'); }); + it('should use fallback message when error.message is empty', async () => { + mockedApiClient.fetchFlyerItems.mockResolvedValue({ + ok: false, + status: 500, + json: () => Promise.resolve({ message: '' }), + } as Response); + + const { result } = renderHook(() => useFlyerItemsQuery(42), { wrapper }); + + await waitFor(() => expect(result.current.isError).toBe(true)); + + expect(result.current.error?.message).toBe('Failed to fetch flyer items'); + }); + + it('should throw error when refetch is called without flyerId', async () => { + // This tests the internal guard in queryFn that throws if flyerId is undefined + // We call refetch() manually to force the queryFn to execute even when disabled + const { result } = renderHook(() => useFlyerItemsQuery(undefined), { wrapper }); + + // Force the query to run by calling refetch + await result.current.refetch(); + + await waitFor(() => expect(result.current.isError).toBe(true)); + + expect(result.current.error?.message).toBe('Flyer ID is required'); + }); + it('should return empty array when API returns no items', async () => { mockedApiClient.fetchFlyerItems.mockResolvedValue({ ok: true, diff --git a/src/hooks/queries/useFlyersQuery.test.tsx b/src/hooks/queries/useFlyersQuery.test.tsx index b1095cf..bb0e104 100644 --- a/src/hooks/queries/useFlyersQuery.test.tsx +++ b/src/hooks/queries/useFlyersQuery.test.tsx @@ -87,6 +87,20 @@ describe('useFlyersQuery', () => { expect(result.current.error?.message).toBe('Request failed with status 500'); }); + it('should use fallback message when error.message is empty', async () => { + mockedApiClient.fetchFlyers.mockResolvedValue({ + ok: false, + status: 500, + json: () => Promise.resolve({ message: '' }), + } as Response); + + const { result } = renderHook(() => useFlyersQuery(), { wrapper }); + + await waitFor(() => expect(result.current.isError).toBe(true)); + + expect(result.current.error?.message).toBe('Failed to fetch flyers'); + }); + it('should return empty array for no flyers', async () => { mockedApiClient.fetchFlyers.mockResolvedValue({ ok: true, diff --git a/src/hooks/queries/useMasterItemsQuery.test.tsx b/src/hooks/queries/useMasterItemsQuery.test.tsx index c513db7..24ab76d 100644 --- a/src/hooks/queries/useMasterItemsQuery.test.tsx +++ b/src/hooks/queries/useMasterItemsQuery.test.tsx @@ -73,6 +73,20 @@ describe('useMasterItemsQuery', () => { expect(result.current.error?.message).toBe('Request failed with status 500'); }); + it('should use fallback message when error.message is empty', async () => { + mockedApiClient.fetchMasterItems.mockResolvedValue({ + ok: false, + status: 500, + json: () => Promise.resolve({ message: '' }), + } as Response); + + const { result } = renderHook(() => useMasterItemsQuery(), { wrapper }); + + await waitFor(() => expect(result.current.isError).toBe(true)); + + expect(result.current.error?.message).toBe('Failed to fetch master items'); + }); + it('should return empty array for no master items', async () => { mockedApiClient.fetchMasterItems.mockResolvedValue({ ok: true, diff --git a/src/hooks/queries/useShoppingListsQuery.test.tsx b/src/hooks/queries/useShoppingListsQuery.test.tsx index 1eb2770..49f8708 100644 --- a/src/hooks/queries/useShoppingListsQuery.test.tsx +++ b/src/hooks/queries/useShoppingListsQuery.test.tsx @@ -83,6 +83,20 @@ describe('useShoppingListsQuery', () => { expect(result.current.error?.message).toBe('Request failed with status 500'); }); + it('should use fallback message when error.message is empty', async () => { + mockedApiClient.fetchShoppingLists.mockResolvedValue({ + ok: false, + status: 500, + json: () => Promise.resolve({ message: '' }), + } as Response); + + const { result } = renderHook(() => useShoppingListsQuery(true), { wrapper }); + + await waitFor(() => expect(result.current.isError).toBe(true)); + + expect(result.current.error?.message).toBe('Failed to fetch shopping lists'); + }); + it('should return empty array for no shopping lists', async () => { mockedApiClient.fetchShoppingLists.mockResolvedValue({ ok: true, diff --git a/src/hooks/queries/useSuggestedCorrectionsQuery.test.tsx b/src/hooks/queries/useSuggestedCorrectionsQuery.test.tsx index e839395..9829785 100644 --- a/src/hooks/queries/useSuggestedCorrectionsQuery.test.tsx +++ b/src/hooks/queries/useSuggestedCorrectionsQuery.test.tsx @@ -72,6 +72,20 @@ describe('useSuggestedCorrectionsQuery', () => { expect(result.current.error?.message).toBe('Request failed with status 500'); }); + it('should use fallback message when error.message is empty', async () => { + mockedApiClient.getSuggestedCorrections.mockResolvedValue({ + ok: false, + status: 500, + json: () => Promise.resolve({ message: '' }), + } as Response); + + const { result } = renderHook(() => useSuggestedCorrectionsQuery(), { wrapper }); + + await waitFor(() => expect(result.current.isError).toBe(true)); + + expect(result.current.error?.message).toBe('Failed to fetch suggested corrections'); + }); + it('should return empty array for no corrections', async () => { mockedApiClient.getSuggestedCorrections.mockResolvedValue({ ok: true, diff --git a/src/hooks/queries/useWatchedItemsQuery.test.tsx b/src/hooks/queries/useWatchedItemsQuery.test.tsx index 0f3dae5..e84da9f 100644 --- a/src/hooks/queries/useWatchedItemsQuery.test.tsx +++ b/src/hooks/queries/useWatchedItemsQuery.test.tsx @@ -83,6 +83,20 @@ describe('useWatchedItemsQuery', () => { expect(result.current.error?.message).toBe('Request failed with status 500'); }); + it('should use fallback message when error.message is empty', async () => { + mockedApiClient.fetchWatchedItems.mockResolvedValue({ + ok: false, + status: 500, + json: () => Promise.resolve({ message: '' }), + } as Response); + + const { result } = renderHook(() => useWatchedItemsQuery(true), { wrapper }); + + await waitFor(() => expect(result.current.isError).toBe(true)); + + expect(result.current.error?.message).toBe('Failed to fetch watched items'); + }); + it('should return empty array for no watched items', async () => { mockedApiClient.fetchWatchedItems.mockResolvedValue({ ok: true, diff --git a/src/hooks/useDataExtraction.test.ts b/src/hooks/useDataExtraction.test.ts index 58b0f87..bac0447 100644 --- a/src/hooks/useDataExtraction.test.ts +++ b/src/hooks/useDataExtraction.test.ts @@ -1,22 +1,29 @@ // src/hooks/useDataExtraction.test.ts import { renderHook, act } from '@testing-library/react'; -import { describe, it, expect, vi, beforeEach } from 'vitest'; +import { describe, it, expect, vi, beforeEach, type Mock } from 'vitest'; import { useDataExtraction } from './useDataExtraction'; import type { Flyer } from '../types'; // Create a mock flyer for testing const createMockFlyer = (id: number, storeName: string = `Store ${id}`): Flyer => ({ flyer_id: id, - store: { store_id: id, name: storeName }, - start_date: '2024-01-01', - end_date: '2024-01-07', + store: { + store_id: id, + name: storeName, + created_at: '2024-01-01T00:00:00Z', + updated_at: '2024-01-01T00:00:00Z', + }, + file_name: `flyer${id}.jpg`, image_url: `https://example.com/flyer${id}.jpg`, + icon_url: `https://example.com/flyer${id}_icon.jpg`, status: 'processed', + item_count: 0, created_at: '2024-01-01T00:00:00Z', + updated_at: '2024-01-01T00:00:00Z', }); describe('useDataExtraction Hook', () => { - let mockOnFlyerUpdate: ReturnType; + let mockOnFlyerUpdate: Mock<(flyer: Flyer) => void>; beforeEach(() => { mockOnFlyerUpdate = vi.fn(); @@ -66,7 +73,7 @@ describe('useDataExtraction Hook', () => { expect(mockOnFlyerUpdate).toHaveBeenCalledTimes(1); const updatedFlyer = mockOnFlyerUpdate.mock.calls[0][0]; - expect(updatedFlyer.store.name).toBe('New Store Name'); + expect(updatedFlyer.store?.name).toBe('New Store Name'); // Ensure other properties are preserved expect(updatedFlyer.flyer_id).toBe(1); expect(updatedFlyer.image_url).toBe('https://example.com/flyer1.jpg'); @@ -86,7 +93,7 @@ describe('useDataExtraction Hook', () => { }); const updatedFlyer = mockOnFlyerUpdate.mock.calls[0][0]; - expect(updatedFlyer.store.store_id).toBe(42); + expect(updatedFlyer.store?.store_id).toBe(42); }); }); @@ -168,7 +175,7 @@ describe('useDataExtraction Hook', () => { it('should update handler when onFlyerUpdate changes', () => { const mockFlyer = createMockFlyer(1); - const mockOnFlyerUpdate2 = vi.fn(); + const mockOnFlyerUpdate2: Mock<(flyer: Flyer) => void> = vi.fn(); const { result, rerender } = renderHook( ({ onFlyerUpdate }) => diff --git a/src/hooks/useFlyerSelection.test.tsx b/src/hooks/useFlyerSelection.test.tsx index b6f8ba4..c4bfff8 100644 --- a/src/hooks/useFlyerSelection.test.tsx +++ b/src/hooks/useFlyerSelection.test.tsx @@ -20,12 +20,19 @@ vi.mock('../services/logger.client', () => ({ // Create mock flyers for testing const createMockFlyer = (id: number, storeName: string = `Store ${id}`): Flyer => ({ flyer_id: id, - store: { store_id: id, name: storeName }, - start_date: '2024-01-01', - end_date: '2024-01-07', + store: { + store_id: id, + name: storeName, + created_at: '2024-01-01T00:00:00Z', + updated_at: '2024-01-01T00:00:00Z', + }, + file_name: `flyer${id}.jpg`, image_url: `https://example.com/flyer${id}.jpg`, + icon_url: `https://example.com/flyer${id}_icon.jpg`, status: 'processed', + item_count: 0, created_at: '2024-01-01T00:00:00Z', + updated_at: '2024-01-01T00:00:00Z', }); const mockFlyers: Flyer[] = [ diff --git a/vitest.config.ts b/vitest.config.ts index 81bc4a4..b5c5174 100644 --- a/vitest.config.ts +++ b/vitest.config.ts @@ -13,5 +13,10 @@ export default defineConfig({ // This line is the key fix: it tells Vitest to include the type definitions include: ['src/**/*.test.{ts,tsx}'], + coverage: { + exclude: [ + '**/index.ts', // barrel exports don't need coverage + ], + }, }, });