diff --git a/docs/WORKLOG.md b/docs/WORKLOG.md index 67677d0..e3a71ec 100644 --- a/docs/WORKLOG.md +++ b/docs/WORKLOG.md @@ -1,3 +1,11 @@ +2025-12-24 11:15 UTC - Fix diffResults import + test hardening +- Files: services/db/operations/imports.ts; tests/current-system/export-import.test.ts; tests/services/comparisonService.test.ts; tests/adapters/providers/OpenAIAdapter.test.ts; tests/contracts/provider.contract.test.ts; tests/hooks/useChapterTelemetry.test.tsx; docs/WORKLOG.md +- Why: Imported diffResults could throw `DataError` because export emits `fanVersionId: null` but IndexedDB keys must be valid strings; plus expand coverage for provider/adversarial parsing paths. +- Details: + - Normalized diffResults records during full-session import (coerce `fanVersionId` to `''`, fill hash nulls) so composite keys remain valid. + - Strengthened tests around diffResults export/import, OpenAI adapter error paths, comparison JSON extraction, and chapter telemetry perf/logging. +- Tests: `npx tsc --noEmit`; `npx vitest run tests/current-system/export-import.test.ts`; `npx vitest run tests/services/comparisonService.test.ts`; `npx vitest run tests/adapters/providers/OpenAIAdapter.test.ts`; `npx vitest run tests/hooks/useChapterTelemetry.test.tsx` + 2025-11-21 01:00 UTC - TranslationStatusPanel spacing tweak - Files: components/chapter/TranslationStatusPanel.tsx (wrapper div class) - Why: Chapter body sat flush against the translation/image metric lines; user requested extra spacing after the status rows. diff --git a/services/db/operations/imports.ts b/services/db/operations/imports.ts index ba77ab1..b829645 100644 --- a/services/db/operations/imports.ts +++ b/services/db/operations/imports.ts @@ -9,6 +9,7 @@ import type { ExportedImageAsset, } from '../types'; import type { StableSessionData } from '../../stableIdService'; +import type { DiffResult } from '../../diff/types'; import { getConnection } from '../core/connection'; import { STORE_NAMES } from '../core/schema'; import { ImageCacheStore } from '../../imageCacheService'; @@ -25,6 +26,16 @@ const BATCH_SIZE = 50; const nowIso = () => new Date().toISOString(); +type StoredDiffResult = DiffResult & { fanVersionId: string }; + +const prepareDiffResultForStorage = (record: DiffResult): StoredDiffResult => ({ + ...record, + fanVersionId: record.fanVersionId ?? '', + aiHash: record.aiHash ?? null, + fanHash: record.fanHash ?? null, + rawHash: record.rawHash ?? record.rawVersionId, +}); + const putSettingsRecord = ( store: IDBObjectStore, key: string, @@ -223,8 +234,8 @@ export class ImportOps { tx.oncomplete = () => resolve(); tx.onerror = () => reject(tx.error); const store = tx.objectStore(STORE_NAMES.DIFF_RESULTS); - for (const record of diffResults) { - store.put(record); + for (const record of diffResults as DiffResult[]) { + store.put(prepareDiffResultForStorage(record) as StoredDiffResult); } }); } diff --git a/tests/adapters/providers/OpenAIAdapter.test.ts b/tests/adapters/providers/OpenAIAdapter.test.ts index 382fceb..2fb1ac5 100644 --- a/tests/adapters/providers/OpenAIAdapter.test.ts +++ b/tests/adapters/providers/OpenAIAdapter.test.ts @@ -166,3 +166,87 @@ describe('OpenAIAdapter translate() parameter handling', () => { expect(recordMetricMock).toHaveBeenCalledWith(expect.objectContaining({ success: true })); }); }); + +describe('OpenAIAdapter adversarial scenarios', () => { + beforeEach(() => { + openAiMocks.create.mockReset(); + openAiMocks.ctor.mockClear(); + recordMetricMock.mockClear(); + supportsStructuredOutputsMock.mockResolvedValue(false); + }); + + const makeRequest = (): TranslationRequest => ({ + title: 'Test', + content: 'Content', + settings: baseSettings, + history: [], + }); + + it('handles rate limit (429) errors with informative message', async () => { + const rateLimitError = new Error('Rate limit exceeded'); + (rateLimitError as any).status = 429; + (rateLimitError as any).headers = { 'retry-after': '30' }; + + openAiMocks.create.mockRejectedValueOnce(rateLimitError); + + const adapter = new OpenAIAdapter(); + + await expect(adapter.translate(makeRequest())).rejects.toThrow('Rate limit exceeded'); + expect(recordMetricMock).toHaveBeenCalledWith( + expect.objectContaining({ success: false }) + ); + }); + + it('handles network timeout errors', async () => { + const timeoutError = new Error('Request timed out'); + (timeoutError as any).code = 'ETIMEDOUT'; + + openAiMocks.create.mockRejectedValueOnce(timeoutError); + + const adapter = new OpenAIAdapter(); + + await expect(adapter.translate(makeRequest())).rejects.toThrow('timed out'); + expect(recordMetricMock).toHaveBeenCalledWith( + expect.objectContaining({ success: false }) + ); + }); + + it('handles empty response gracefully', async () => { + openAiMocks.create.mockResolvedValueOnce({ + choices: [], + usage: { prompt_tokens: 0, completion_tokens: 0 }, + }); + + const adapter = new OpenAIAdapter(); + + await expect(adapter.translate(makeRequest())).rejects.toThrow(); + }); + + it('handles null message content', async () => { + openAiMocks.create.mockResolvedValueOnce({ + choices: [{ finish_reason: 'stop', message: { content: null } }], + usage: { prompt_tokens: 10, completion_tokens: 0 }, + }); + + const adapter = new OpenAIAdapter(); + + await expect(adapter.translate(makeRequest())).rejects.toThrow(); + }); + + it('handles response with missing required fields', async () => { + // JSON is valid but missing translatedTitle + openAiMocks.create.mockResolvedValueOnce({ + choices: [{ + finish_reason: 'stop', + message: { content: '{"translation": "content only"}' }, + }], + usage: { prompt_tokens: 10, completion_tokens: 5 }, + }); + + const adapter = new OpenAIAdapter(); + const result = await adapter.translate(makeRequest()); + + // Should not crash - missing fields should be handled gracefully + expect(result.translation).toBe('content only'); + }); +}); diff --git a/tests/contracts/provider.contract.test.ts b/tests/contracts/provider.contract.test.ts index 73e4ece..58929f0 100644 --- a/tests/contracts/provider.contract.test.ts +++ b/tests/contracts/provider.contract.test.ts @@ -1,20 +1,25 @@ /** * Provider Contract Tests * - * TEST-QUALITY: 8.5/10 (Target: High, integration-style) + * STATUS: SCAFFOLD - All tests skipped pending VCR infrastructure + * TEST-QUALITY: 2/10 (current) → 8.5/10 (target when implemented) * - * Construct: "Given a prompt + text, provider returns well-formed TranslationResult - * with correct token accounting and typed errors within timeout." + * This file defines the STRUCTURE for provider contract tests but the VCR + * (Video Cassette Recording) infrastructure for deterministic replay is not built. + * All tests are currently skipped. * - * Ecological validity: Uses VCR-style replay for determinism, but tests real adapter logic. - * Can optionally run live with LIVE_API_TEST=1 env var. + * Target construct: "Given a prompt + text, provider returns well-formed TranslationResult + * with correct token accounting and typed errors within timeout." * - * Decision-useful: Catches adapter bugs, API contract changes, token counting errors. + * NOTE: Adversarial tests (rate limits, timeouts, malformed responses) are now + * implemented in the individual adapter test files where they can be properly tested: + * - tests/adapters/providers/OpenAIAdapter.test.ts (adversarial scenarios section) + * - tests/adapters/providers/GeminiAdapter.test.ts * - * Gaps addressed from audit: - * - Mock overuse: Tests real adapter code (only network is replayed) - * - No ground truth: Validates token counts, cost calculations - * - No adversarial: Tests rate limits, timeouts, unknown models + * To make these contract tests real: + * 1. Implement VCR cassette recording/replay (see TODO at bottom) + * 2. Hook up actual adapters instead of inline mock responses + * 3. Remove .skip from test cases */ import { describe, it, expect, beforeEach, vi } from 'vitest'; @@ -278,49 +283,11 @@ describe('Provider Contract: Gemini', () => { }); }); -// Adversarial contract tests -describe('Provider Contract: Adversarial Cases', () => { - it.skip('[Adversarial] rate limit: backs off correctly', async () => { - // TODO: Implement rate limit testing - // Should test: - // 1. Adapter receives 429 response - // 2. Waits appropriate time (respecting Retry-After header) - // 3. Retries with exponential backoff - // 4. Eventually succeeds or returns typed error - - expect(true).toBe(true); // Placeholder - }); - - it.skip('[Adversarial] timeout: returns partial result or error', async () => { - // TODO: Test timeout handling - // Should test: - // 1. Request exceeds timeout threshold - // 2. Adapter cancels request - // 3. Returns typed error with timeout info - - expect(true).toBe(true); // Placeholder - }); - - it.skip('[Adversarial] unknown model: maps or fails predictably', async () => { - // TODO: Test unknown model handling - // Should test: - // 1. Adapter receives unknown model name - // 2. Either maps to known model (with warning log) - // 3. Or returns typed error (not crash) - - expect(true).toBe(true); // Placeholder - }); - - it.skip('[Adversarial] malformed API response: handles gracefully', async () => { - // TODO: Test malformed response handling - // Should test: - // 1. API returns invalid JSON - // 2. API returns unexpected structure - // 3. Adapter returns typed error (not crash) - - expect(true).toBe(true); // Placeholder - }); -}); +// NOTE: Adversarial contract tests (rate limits, timeouts, malformed responses) +// have been moved to individual adapter test files where they can be properly tested: +// - tests/adapters/providers/OpenAIAdapter.test.ts → "adversarial scenarios" describe block +// - tests/adapters/providers/GeminiAdapter.test.ts +// This avoids placeholder stubs that inflate test counts without testing behavior. /** * Implementation TODO (for full 8.5/10 score): diff --git a/tests/current-system/export-import.test.ts b/tests/current-system/export-import.test.ts index 3efe3db..09967eb 100644 --- a/tests/current-system/export-import.test.ts +++ b/tests/current-system/export-import.test.ts @@ -130,6 +130,24 @@ describe('Session export/import', () => { }); it('should restore diffResults from imported session data', async () => { + const testDiffResult: DiffResult = { + chapterId: 'stable-import', + aiVersionId: '9876543210', + fanVersionId: null, + rawVersionId: 'xyz98765', + algoVersion: '1.0.0', + markers: [{ + chunkId: 'para-0-xyz', + colors: ['orange'], + reasons: ['raw-divergence'], + aiRange: { start: 0, end: 20 }, + position: 0 + }], + analyzedAt: Date.now(), + costUsd: 0.002, + model: 'gpt-4o-mini' + }; + const sessionData = { metadata: { format: 'lexiconforge-full-1', exportedAt: new Date().toISOString() }, settings: null, @@ -138,42 +156,21 @@ describe('Session export/import', () => { novels: [], chapters: [], promptTemplates: [], - diffResults: [ - { - chapterId: 'stable-import', - aiVersionId: '9876543210', - fanVersionId: null, - rawVersionId: 'xyz98765', - algoVersion: '1.0.0', - markers: [{ - chunkId: 'para-0-xyz', - colors: ['orange'], - reasons: ['raw-divergence'], - aiRange: { start: 0, end: 20 }, - position: 0 - }], - analyzedAt: Date.now(), - costUsd: 0.002, - model: 'gpt-4o-mini' - } - ] satisfies DiffResult[] + diffResults: [testDiffResult] }; - // Import should succeed or gracefully skip if diffResults store doesn't exist - // (older DB versions may not have the store) - try { - await ImportOps.importFullSessionData(sessionData); - // If we get here, import succeeded - expect(true).toBe(true); - } catch (error: any) { - // If error is because diffResults store doesn't exist, that's acceptable in tests - // Otherwise, rethrow - if (!error.message?.includes('diffResults') && !error.message?.includes('Data provided')) { - throw error; - } - // Store doesn't exist in test DB - this is OK - expect(true).toBe(true); - } + // Import the session data + await ImportOps.importFullSessionData(sessionData); + + // Verify the diffResult was actually imported by reading it back + const allDiffResults = await DiffOps.getAll(); + const imported = allDiffResults.find(r => r.chapterId === 'stable-import'); + + expect(imported).toBeDefined(); + expect(imported?.aiVersionId).toBe('9876543210'); + expect(imported?.model).toBe('gpt-4o-mini'); + expect(imported?.markers).toHaveLength(1); + expect(imported?.markers[0].reasons).toContain('raw-divergence'); }); it('should handle export when no diffResults exist', async () => { diff --git a/tests/hooks/useChapterTelemetry.test.tsx b/tests/hooks/useChapterTelemetry.test.tsx index 9cf2a8c..db0bbfc 100644 --- a/tests/hooks/useChapterTelemetry.test.tsx +++ b/tests/hooks/useChapterTelemetry.test.tsx @@ -1,36 +1,228 @@ -import { describe, expect, it, vi } from 'vitest'; -import { render } from '@testing-library/react'; -import React from 'react'; +/** + * Tests for useChapterTelemetry hook + * + * This hook captures UX performance metrics: + * 1. Component mount time + * 2. Chapter ready time (after loading completes) + * 3. Selection state changes (logged for debugging) + */ + +import { describe, expect, it, vi, beforeEach, afterEach } from 'vitest'; +import { render, act } from '@testing-library/react'; +import React, { useState } from 'react'; import { useChapterTelemetry } from '../../hooks/useChapterTelemetry'; +// Mock telemetryService +const capturePerformanceMock = vi.fn(); vi.mock('../../services/telemetryService', () => ({ telemetryService: { - capturePerformance: vi.fn(), + capturePerformance: (...args: unknown[]) => capturePerformanceMock(...args), }, })); -const TestComponent: React.FC<{ selection?: { text: string; rect: DOMRect } | null }> -= ({ selection = null }) => { +// Mock debugLog +const debugLogMock = vi.fn(); +vi.mock('../../utils/debug', () => ({ + debugLog: (...args: unknown[]) => debugLogMock(...args), +})); + +interface TestComponentProps { + selection?: { text: string; rect: DOMRect } | null; + currentChapterId?: string | null; + chapters?: Map; + chapter?: { id: string } | null; + translationResult?: { translation: string } | null; + isLoading?: { fetching: boolean }; + translationInProgress?: boolean; + isHydratingCurrent?: boolean; + viewMode?: 'original' | 'fan' | 'english'; + feedbackCount?: number; +} + +const TestComponent: React.FC = ({ + selection = null, + currentChapterId = 'ch-1', + chapters, + chapter = { id: 'ch-1' }, + translationResult = { translation: 'text' }, + isLoading = { fetching: false }, + translationInProgress = false, + isHydratingCurrent = false, + viewMode = 'english' as const, + feedbackCount = 0, +}) => { useChapterTelemetry({ selection, - currentChapterId: 'c1', - chapters: new Map(), - chapter: { id: 'c1' }, - translationResult: { translation: 'text' }, - isLoading: { fetching: false }, - translationInProgress: false, - isHydratingCurrent: false, - viewMode: 'english', - feedbackCount: 0, + currentChapterId, + chapters: + chapters ?? + (currentChapterId && chapter ? new Map([[currentChapterId, chapter]]) : new Map()), + chapter, + translationResult, + isLoading, + translationInProgress, + isHydratingCurrent, + viewMode, + feedbackCount, }); - return null; + return
Rendered
; }; describe('useChapterTelemetry', () => { - it('logs selection updates', () => { - const rect = new DOMRect(); - render(); - // nothing to assert beyond no crashes; log goes through debugLog - expect(true).toBe(true); + beforeEach(() => { + vi.clearAllMocks(); + }); + + afterEach(() => { + vi.clearAllMocks(); + }); + + describe('mount performance', () => { + it('captures mount time on initial render', () => { + render(); + + expect(capturePerformanceMock).toHaveBeenCalledWith( + 'ux:component:ChapterView:mount', + expect.any(Number), + expect.objectContaining({ + chapterId: 'ch-1', + hasChapter: true, + }) + ); + }); + + it('reports hasChapter=false when chapter not in map initially', () => { + render( + + ); + + expect(capturePerformanceMock).toHaveBeenCalledWith( + 'ux:component:ChapterView:mount', + expect.any(Number), + expect.objectContaining({ + chapterId: 'missing-chapter', + hasChapter: false, + }) + ); + }); + }); + + describe('chapter ready performance', () => { + it('captures ready time when chapter finishes loading', () => { + // Start with loading state + const { rerender } = render( + + ); + + // Clear the mount call + capturePerformanceMock.mockClear(); + + // Simulate loading complete + rerender( + + ); + + expect(capturePerformanceMock).toHaveBeenCalledWith( + 'ux:component:ChapterView:ready', + expect.any(Number), + expect.objectContaining({ + chapterId: 'ch-1', + hasTranslation: true, + viewMode: 'english', + feedbackCount: 0, + }) + ); + }); + + it('does not capture ready if still loading', () => { + const { rerender } = render( + + ); + + capturePerformanceMock.mockClear(); + + // Still loading - should not capture ready + rerender(); + + expect(capturePerformanceMock).not.toHaveBeenCalledWith( + 'ux:component:ChapterView:ready', + expect.any(Number), + expect.anything() + ); + }); + + it('does not capture ready if translation in progress', () => { + const { rerender } = render( + + ); + + capturePerformanceMock.mockClear(); + + rerender(); + + expect(capturePerformanceMock).not.toHaveBeenCalledWith( + 'ux:component:ChapterView:ready', + expect.any(Number), + expect.anything() + ); + }); + }); + + describe('selection logging', () => { + it('logs selection updates via debugLog', () => { + const rect = new DOMRect(100, 200, 50, 20); + const selection = { text: 'Selected text for comparison', rect }; + + render(); + + expect(debugLogMock).toHaveBeenCalledWith( + 'comparison', + 'summary', + '[ChapterView] Selection state updated', + expect.objectContaining({ + text: 'Selected text for comparison', + textLength: 28, + rectTop: 200, + rectLeft: 100, + }) + ); + }); + + it('truncates long selection text in log', () => { + const longText = 'A'.repeat(100); + const rect = new DOMRect(); + const selection = { text: longText, rect }; + + render(); + + expect(debugLogMock).toHaveBeenCalledWith( + 'comparison', + 'summary', + '[ChapterView] Selection state updated', + expect.objectContaining({ + text: 'A'.repeat(50) + '...', + textLength: 100, + }) + ); + }); + + it('does not log when selection is null', () => { + render(); + + expect(debugLogMock).not.toHaveBeenCalled(); + }); }); }); diff --git a/tests/integration/registry.test.ts b/tests/integration/registry.test.ts index 5aec7a7..d12b59a 100644 --- a/tests/integration/registry.test.ts +++ b/tests/integration/registry.test.ts @@ -1,10 +1,23 @@ +/** + * RegistryService Unit Tests + * + * NOTE: Despite being in the 'integration' folder, these are UNIT tests. + * They mock `fetch` to test the service's logic in isolation: + * - Multi-fetch aggregation (registry → individual novel metadata) + * - Partial failure handling (some novels fail, others succeed) + * - URL routing (custom registry URLs, individual metadata URLs) + * + * For true integration tests against a real registry server, + * see tests/e2e/ or run with LIVE_REGISTRY_TEST=1. + */ + import { describe, it, expect, vi, beforeEach } from 'vitest'; import { RegistryService } from '../../services/registryService'; -// Mock fetch globally +// Mock fetch globally - this makes these UNIT tests, not integration tests global.fetch = vi.fn(); -describe('RegistryService Integration', () => { +describe('RegistryService (unit tests with mocked fetch)', () => { beforeEach(() => { vi.clearAllMocks(); }); diff --git a/tests/services/comparisonService.test.ts b/tests/services/comparisonService.test.ts index 95a0e44..fb0b15e 100644 --- a/tests/services/comparisonService.test.ts +++ b/tests/services/comparisonService.test.ts @@ -1,15 +1,29 @@ import { describe, it, expect, beforeEach, vi } from 'vitest'; import type { AppSettings } from '../../types'; -const { createMock, openAiCtor } = vi.hoisted(() => { +// Create mock using hoisted pattern with proper class structure +const openAiMocks = vi.hoisted(() => { const createMock = vi.fn(); - const openAiCtor = vi.fn(); - return { createMock, openAiCtor }; + const ctorMock = vi.fn(); + + // Must be a real class for `new OpenAI()` to work + class MockOpenAI { + chat = { + completions: { + create: (...args: any[]) => createMock(...args), + }, + }; + constructor(...args: any[]) { + ctorMock(...args); + } + } + + return { createMock, ctorMock, MockOpenAI }; }); vi.mock('openai', () => ({ __esModule: true, - OpenAI: openAiCtor, + OpenAI: openAiMocks.MockOpenAI, })); import { ComparisonService } from '../../services/comparisonService'; @@ -38,37 +52,211 @@ const baseArgs = { }; beforeEach(() => { - createMock.mockReset(); - openAiCtor.mockReset(); - openAiCtor.mockImplementation(() => ({ - chat: { - completions: { - create: createMock, - }, - }, - })); + openAiMocks.createMock.mockReset(); + openAiMocks.ctorMock.mockClear(); }); describe('ComparisonService.requestFocusedComparison', () => { - it('throws when fan translation is missing', async () => { - await expect( - ComparisonService.requestFocusedComparison({ + describe('validation', () => { + it('throws when fan translation is missing', async () => { + await expect( + ComparisonService.requestFocusedComparison({ + ...baseArgs, + fullFanTranslation: ' ', + settings: buildSettings(), + }), + ).rejects.toThrow('Fan translation is required for comparison.'); + }); + + it('throws when API key is missing', async () => { + delete (process.env as Record).VITE_OPENAI_API_KEY; + delete (process.env as Record).OPENAI_API_KEY; + + await expect( + ComparisonService.requestFocusedComparison({ + ...baseArgs, + settings: buildSettings({ apiKeyOpenAI: undefined }), + }), + ).rejects.toThrow('API key for OpenAI is missing.'); + }); + }); + + describe('happy path', () => { + it('returns parsed comparison result from LLM response', async () => { + const mockResponse = { + fanExcerpt: 'The hero stood tall.', + fanContextBefore: 'Before the battle...', + fanContextAfter: 'After the dust settled...', + rawExcerpt: '영웅이 우뚝 섰다.', + rawContextBefore: '전투 전에...', + rawContextAfter: '먼지가 가라앉은 후...', + confidence: 0.85, + }; + + openAiMocks.createMock.mockResolvedValueOnce({ + choices: [{ + message: { + content: JSON.stringify(mockResponse), + }, + }], + }); + + const result = await ComparisonService.requestFocusedComparison({ + ...baseArgs, + settings: buildSettings(), + }); + + expect(result).toEqual({ + fanExcerpt: 'The hero stood tall.', + fanContextBefore: 'Before the battle...', + fanContextAfter: 'After the dust settled...', + rawExcerpt: '영웅이 우뚝 섰다.', + rawContextBefore: '전투 전에...', + rawContextAfter: '먼지가 가라앉은 후...', + confidence: 0.85, + }); + + // Verify OpenAI was called correctly + expect(openAiMocks.createMock).toHaveBeenCalledTimes(1); + expect(openAiMocks.createMock).toHaveBeenCalledWith( + expect.objectContaining({ + model: 'gpt-4o-mini', + temperature: 0, + messages: expect.arrayContaining([ + expect.objectContaining({ role: 'user' }), + ]), + }), + ); + }); + + it('handles response wrapped in markdown code fences', async () => { + const mockResponse = { + fanExcerpt: 'Extracted from fenced block.', + fanContextBefore: null, + fanContextAfter: null, + rawExcerpt: null, + rawContextBefore: null, + rawContextAfter: null, + }; + + openAiMocks.createMock.mockResolvedValueOnce({ + choices: [{ + message: { + content: '```json\n' + JSON.stringify(mockResponse) + '\n```', + }, + }], + }); + + const result = await ComparisonService.requestFocusedComparison({ ...baseArgs, - fullFanTranslation: ' ', settings: buildSettings(), - }), - ).rejects.toThrow('Fan translation is required for comparison.'); + }); + + expect(result.fanExcerpt).toBe('Extracted from fenced block.'); + }); + + it('handles response with extra text around JSON', async () => { + const mockResponse = { + fanExcerpt: 'Parsed from noisy response.', + fanContextBefore: null, + fanContextAfter: null, + rawExcerpt: null, + rawContextBefore: null, + rawContextAfter: null, + }; + + openAiMocks.createMock.mockResolvedValueOnce({ + choices: [{ + message: { + content: 'Here is the comparison:\n' + JSON.stringify(mockResponse) + '\nHope that helps!', + }, + }], + }); + + const result = await ComparisonService.requestFocusedComparison({ + ...baseArgs, + settings: buildSettings(), + }); + + expect(result.fanExcerpt).toBe('Parsed from noisy response.'); + }); + + it('throws when response contains no valid JSON', async () => { + openAiMocks.createMock.mockResolvedValueOnce({ + choices: [{ + message: { + content: 'I cannot perform this comparison because...', + }, + }], + }); + + await expect( + ComparisonService.requestFocusedComparison({ + ...baseArgs, + settings: buildSettings(), + }), + ).rejects.toThrow('Comparison response did not contain valid JSON.'); + }); + + it('handles partial/optional fields gracefully', async () => { + // LLM returns only some fields + openAiMocks.createMock.mockResolvedValueOnce({ + choices: [{ + message: { + content: JSON.stringify({ + fanExcerpt: 'Just the excerpt.', + // All other fields missing + }), + }, + }], + }); + + const result = await ComparisonService.requestFocusedComparison({ + ...baseArgs, + settings: buildSettings(), + }); + + expect(result.fanExcerpt).toBe('Just the excerpt.'); + expect(result.fanContextBefore).toBeNull(); + expect(result.fanContextAfter).toBeNull(); + expect(result.rawExcerpt).toBeNull(); + expect(result.confidence).toBeUndefined(); + }); }); - it('throws when API key is missing', async () => { - delete (process.env as Record).VITE_OPENAI_API_KEY; - delete (process.env as Record).OPENAI_API_KEY; + describe('provider routing', () => { + it('uses correct baseURL for OpenAI provider', async () => { + openAiMocks.createMock.mockResolvedValueOnce({ + choices: [{ message: { content: '{"fanExcerpt": "test"}' } }], + }); - await expect( - ComparisonService.requestFocusedComparison({ + await ComparisonService.requestFocusedComparison({ ...baseArgs, - settings: buildSettings({ apiKeyOpenAI: undefined }), - }), - ).rejects.toThrow('API key for OpenAI is missing.'); + settings: buildSettings({ provider: 'OpenAI' }), + }); + + expect(openAiMocks.ctorMock).toHaveBeenCalledWith( + expect.objectContaining({ + baseURL: 'https://api.openai.com/v1', + }), + ); + }); + + it('uses correct baseURL for DeepSeek provider', async () => { + openAiMocks.createMock.mockResolvedValueOnce({ + choices: [{ message: { content: '{"fanExcerpt": "test"}' } }], + }); + + await ComparisonService.requestFocusedComparison({ + ...baseArgs, + settings: buildSettings({ provider: 'DeepSeek', apiKeyDeepSeek: 'deepseek-key' }), + }); + + expect(openAiMocks.ctorMock).toHaveBeenCalledWith( + expect.objectContaining({ + baseURL: 'https://api.deepseek.com/v1', + }), + ); + }); }); });