From 2663620d88972a4327320ab1fa27417b830231ff Mon Sep 17 00:00:00 2001 From: Aditya Date: Fri, 26 Dec 2025 00:02:23 +0530 Subject: [PATCH] test: steel-man test suites to prevent Goodharting MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit MOTIVATION: - Several tests were mocking the very functions they were supposed to test - Tests only verified partial fields, missing serialization bugs - Registry tests lacked error handling coverage for real-world failures - ComparisonService tests didn't cover malformed LLM responses APPROACH: - Converted mock-based tests to true integration tests using fake-indexeddb - Added full-field assertions for round-trip fidelity (all DiffResult fields) - Added error handling tests (HTTP 404/500, timeouts, malformed JSON) - Added resilience tests for incomplete/null data in responses CHANGES: - tests/current-system/export-import.test.ts: - P0: Replaced mocked import test with true integration test - P0: Added full-field assertions for diffResults round-trip - P1: Converted diffResults export test to integration - P1: Converted amendmentLogs tests to integration with round-trip - tests/integration/registry.test.ts: - P1: Added HTTP error response tests (404, 500) - P1: Added network timeout test - P1: Added malformed JSON response test - P2: Added incomplete data handling tests (missing fields, null values) - tests/services/comparisonService.test.ts: - P2: Added malformed response tests (empty choices, null content, etc.) - P2: Improved provider config verification (full client config) IMPACT: - Tests now catch real serialization bugs, not just "mock was called" - Coverage for edge cases and error paths - Higher confidence in export/import fidelity TESTING: - All 552 tests pass - Verified each test file independently before full suite run πŸ€– Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 --- tests/current-system/export-import.test.ts | 457 ++++++++++++++++----- tests/integration/registry.test.ts | 189 +++++++++ tests/services/comparisonService.test.ts | 153 ++++++- 3 files changed, 681 insertions(+), 118 deletions(-) diff --git a/tests/current-system/export-import.test.ts b/tests/current-system/export-import.test.ts index 09967eb..fcf0f62 100644 --- a/tests/current-system/export-import.test.ts +++ b/tests/current-system/export-import.test.ts @@ -5,13 +5,17 @@ import type { TranslationResult } from '../../types'; import type { DiffResult } from '../../services/diff/types'; import { AmendmentOps, + ChapterOps, DiffOps, ImportOps, SessionExportOps, SettingsOps, + TemplatesOps, + TranslationOps, } from '../../services/db/operations'; import * as RenderingOps from '../../services/db/operations/rendering'; import { normalizeUrlAggressively } from '../../services/stableIdService'; +import type { DiffMarker } from '../../services/diff/types'; const resetStore = () => { useAppStore.setState({ @@ -63,35 +67,77 @@ describe('Session export/import', () => { }); describe('diffResults export/import', () => { - it('[Unit] should include diffResults in exported session data (mocked)', async () => { - // Unit test: Mock DiffOps.getAll() to test export logic in isolation - const mockDiffResults = [ - { - chapterId: 'stable-1', - aiVersionId: '1234567890', - fanVersionId: null, - rawVersionId: 'abc12345', - algoVersion: '1.0.0', - markers: [{ - chunkId: 'para-0-abc', - colors: ['grey'], - reasons: ['stylistic-choice'], - aiRange: { start: 0, end: 10 }, - position: 0 - }], - analyzedAt: Date.now(), - costUsd: 0.001, - model: 'gpt-4o-mini' - } - ] satisfies DiffResult[]; + it('[Integration] should serialize diffResults with full field fidelity on export', async () => { + // Integration test: Save real data with ALL fields, then verify export captures everything + const fixedTimestamp = 1703520000000; - vi.spyOn(DiffOps, 'getAll').mockResolvedValue(mockDiffResults); + const testDiffResult: DiffResult = { + chapterId: 'stable-export-fidelity', + aiVersionId: 'ai-export-123', + fanVersionId: 'fan-export-456', // Non-null fan version + rawVersionId: 'raw-export-789', + algoVersion: '2.1.0', + aiHash: 'exporthash1', + fanHash: 'exporthash2', + rawHash: 'exporthash3', + markers: [ + { + chunkId: 'para-0-export', + colors: ['grey', 'blue'], // Multiple colors + reasons: ['stylistic-choice', 'fan-divergence'], + aiRange: { start: 0, end: 50 }, + position: 0, + explanations: ['Style differs', 'Fan had different wording'], + confidence: 0.92 + }, + { + chunkId: 'para-1-export', + colors: ['purple'], + reasons: ['sensitivity-filter'], + aiRange: { start: 51, end: 100 }, + position: 1 + } + ], + analyzedAt: fixedTimestamp, + costUsd: 0.00345, + model: 'gpt-4o' + }; + + // Save to actual DB (no mocks) + await DiffOps.save(testDiffResult); + // Export should retrieve from DB with full fidelity const exported = await SessionExportOps.exportFullSession(); expect(exported.diffResults).toBeDefined(); - expect(exported.diffResults).toHaveLength(1); - expect(exported.diffResults[0].chapterId).toBe('stable-1'); + const found = exported.diffResults.find((r: DiffResult) => r.chapterId === 'stable-export-fidelity'); + expect(found).toBeDefined(); + + // Verify ALL top-level fields survive export + expect(found).toMatchObject({ + chapterId: 'stable-export-fidelity', + aiVersionId: 'ai-export-123', + fanVersionId: 'fan-export-456', + rawVersionId: 'raw-export-789', + algoVersion: '2.1.0', + costUsd: 0.00345, + model: 'gpt-4o' + }); + + // Verify hash fields + expect(found?.aiHash).toBe('exporthash1'); + expect(found?.fanHash).toBe('exporthash2'); + expect(found?.rawHash).toBe('exporthash3'); + expect(found?.analyzedAt).toBe(fixedTimestamp); + + // Verify markers structure + expect(found?.markers).toHaveLength(2); + expect(found?.markers[0].colors).toEqual(['grey', 'blue']); + expect(found?.markers[0].reasons).toEqual(['stylistic-choice', 'fan-divergence']); + expect(found?.markers[0].explanations).toEqual(['Style differs', 'Fan had different wording']); + expect(found?.markers[0].confidence).toBe(0.92); + expect(found?.markers[1].colors).toEqual(['purple']); + expect(found?.markers[1].reasons).toEqual(['sensitivity-filter']); }); it('[Integration] should include diffResults from real DB', async () => { @@ -129,22 +175,40 @@ describe('Session export/import', () => { expect(found?.model).toBe('gpt-4o-mini'); }); - it('should restore diffResults from imported session data', async () => { + it('should restore diffResults from imported session data with full field fidelity', async () => { + // Use fixed timestamp for deterministic testing + const fixedTimestamp = 1703520000000; + + // Create a comprehensive test case with ALL fields populated, including edge cases const testDiffResult: DiffResult = { - chapterId: 'stable-import', + chapterId: 'stable-import-fidelity', aiVersionId: '9876543210', - fanVersionId: null, + fanVersionId: null, // Edge case: null fan version should survive round-trip 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, + algoVersion: '2.0.0', + aiHash: 'abc12345', + fanHash: null, // Should survive as null + rawHash: 'def67890', + markers: [ + { + chunkId: 'para-0-xyz', + colors: ['orange', 'grey'], // Multiple colors + reasons: ['raw-divergence', 'stylistic-choice'], // Multiple reasons + aiRange: { start: 0, end: 20 }, + position: 0 + }, + { + chunkId: 'para-1-abc', + colors: ['red'], + reasons: ['missing-context'], + aiRange: { start: 21, end: 50 }, + position: 1, + explanations: ['Context was omitted from source'], + confidence: 0.85 + } + ], + analyzedAt: fixedTimestamp, + costUsd: 0.00234, model: 'gpt-4o-mini' }; @@ -162,15 +226,50 @@ describe('Session export/import', () => { // Import the session data await ImportOps.importFullSessionData(sessionData); - // Verify the diffResult was actually imported by reading it back + // Verify the diffResult was actually imported with ALL fields preserved const allDiffResults = await DiffOps.getAll(); - const imported = allDiffResults.find(r => r.chapterId === 'stable-import'); + const imported = allDiffResults.find(r => r.chapterId === 'stable-import-fidelity'); 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'); + + // Verify ALL top-level fields + expect(imported).toMatchObject({ + chapterId: 'stable-import-fidelity', + aiVersionId: '9876543210', + fanVersionId: null, // null should survive round-trip + rawVersionId: 'xyz98765', + algoVersion: '2.0.0', + costUsd: 0.00234, + model: 'gpt-4o-mini' + }); + + // Verify hash fields survive + expect(imported?.aiHash).toBe('abc12345'); + expect(imported?.fanHash).toBeNull(); + expect(imported?.rawHash).toBe('def67890'); + + // Verify timestamp (allow small variance for serialization) + expect(imported?.analyzedAt).toBe(fixedTimestamp); + + // Verify markers array structure preserved completely + expect(imported?.markers).toHaveLength(2); + + // First marker: multiple colors/reasons + const marker0 = imported?.markers[0] as DiffMarker; + expect(marker0.chunkId).toBe('para-0-xyz'); + expect(marker0.colors).toEqual(['orange', 'grey']); + expect(marker0.reasons).toEqual(['raw-divergence', 'stylistic-choice']); + expect(marker0.aiRange).toEqual({ start: 0, end: 20 }); + expect(marker0.position).toBe(0); + + // Second marker: with optional fields (explanations, confidence) + const marker1 = imported?.markers[1] as DiffMarker; + expect(marker1.chunkId).toBe('para-1-abc'); + expect(marker1.colors).toEqual(['red']); + expect(marker1.reasons).toEqual(['missing-context']); + expect(marker1.position).toBe(1); + expect(marker1.explanations).toEqual(['Context was omitted from source']); + expect(marker1.confidence).toBe(0.85); }); it('should handle export when no diffResults exist', async () => { @@ -200,60 +299,122 @@ describe('Session export/import', () => { }); describe('amendmentLogs export/import', () => { - it('should include amendment logs in exported session data', async () => { - const logs = [ - { - id: 'log-1', - timestamp: Date.now(), - chapterId: 'stable-123', - proposal: { - observation: 'Note', - currentRule: 'Rule', - proposedChange: 'Change', - reasoning: 'Because', - }, - action: 'accepted' as const, - finalPromptChange: 'Updated prompt', - notes: 'Imported', + it('[Integration] should export amendment logs with full field fidelity', async () => { + // Integration test: Store real data, verify export captures everything + const fixedTimestamp = 1703520000000; + + const testLog = { + id: 'log-export-fidelity', + timestamp: fixedTimestamp, + chapterId: 'stable-amendment-export', + proposal: { + observation: 'The AI translation uses "color" instead of "colour"', + currentRule: 'Use British English spelling throughout', + proposedChange: 'Add explicit rule: Prefer British spelling (colour, favour, honour)', + reasoning: 'Consistency with target audience preferences' }, - ]; - vi.spyOn(AmendmentOps, 'getLogs').mockResolvedValue(logs); + action: 'accepted' as const, + finalPromptChange: 'Use British English spelling including: colour, favour, honour, etc.', + notes: 'Approved after user discussion' + }; + + // Save to actual DB (no mocks) + await AmendmentOps.storeRecord(testLog); + // Export should capture the log with full fidelity const exported = await SessionExportOps.exportFullSession(); - expect(exported.amendmentLogs).toEqual(logs); + const found = exported.amendmentLogs.find((l: any) => l.id === 'log-export-fidelity'); + + expect(found).toBeDefined(); + + // Verify ALL fields are preserved + expect(found).toMatchObject({ + id: 'log-export-fidelity', + timestamp: fixedTimestamp, + chapterId: 'stable-amendment-export', + action: 'accepted', + finalPromptChange: 'Use British English spelling including: colour, favour, honour, etc.', + notes: 'Approved after user discussion' + }); + + // Verify nested proposal object + expect(found?.proposal).toMatchObject({ + observation: 'The AI translation uses "color" instead of "colour"', + currentRule: 'Use British English spelling throughout', + proposedChange: 'Add explicit rule: Prefer British spelling (colour, favour, honour)', + reasoning: 'Consistency with target audience preferences' + }); }); - it('imports amendment logs when present in payload', async () => { - const logs = [ + it('[Integration] should round-trip amendment logs with full fidelity', async () => { + // Integration test: Import β†’ verify in DB β†’ export β†’ verify matches original + const fixedTimestamp = 1703520000000; + + const testLogs = [ { - id: 'log-2', - timestamp: Date.now(), - chapterId: 'stable-456', + id: 'log-roundtrip-1', + timestamp: fixedTimestamp, + chapterId: 'stable-roundtrip', proposal: { - observation: 'Detail', - currentRule: 'Old', - proposedChange: 'New', - reasoning: 'Better', + observation: 'AI added explanatory text not in source', + currentRule: 'Translate faithfully without additions', + proposedChange: 'Allow brief clarifications in [brackets]', + reasoning: 'Some cultural context needs explanation' }, action: 'modified' as const, - finalPromptChange: 'Changed prompt', - notes: 'Imported', + finalPromptChange: 'Allow [bracketed clarifications] for cultural context only', + notes: 'Compromised with user' }, + { + id: 'log-roundtrip-2', + timestamp: fixedTimestamp + 1000, + chapterId: 'stable-roundtrip', + proposal: { + observation: 'Names are inconsistently romanized', + currentRule: 'Use consistent romanization', + proposedChange: 'Prefer revised romanization for Korean names', + reasoning: 'More accurate phonetically' + }, + action: 'rejected' as const, + finalPromptChange: null, + notes: 'User prefers McCune-Reischauer' + } ]; + // Import the logs await ImportOps.importFullSessionData({ metadata: { format: 'lexiconforge-full-1' }, - amendmentLogs: logs, + amendmentLogs: testLogs }); - // Verify amendment logs were written to DB (integration test) + // Verify logs were written to DB with all fields const retrievedLogs = await AmendmentOps.getLogs(); - const importedLog = retrievedLogs.find(l => l.id === 'log-2'); - expect(importedLog).toBeDefined(); - expect(importedLog?.chapterId).toBe('stable-456'); - expect(importedLog?.action).toBe('modified'); - expect(importedLog?.finalPromptChange).toBe('Changed prompt'); + const log1 = retrievedLogs.find(l => l.id === 'log-roundtrip-1'); + expect(log1).toBeDefined(); + expect(log1).toMatchObject({ + id: 'log-roundtrip-1', + chapterId: 'stable-roundtrip', + action: 'modified', + finalPromptChange: 'Allow [bracketed clarifications] for cultural context only', + notes: 'Compromised with user' + }); + expect(log1?.proposal.observation).toBe('AI added explanatory text not in source'); + expect(log1?.proposal.reasoning).toBe('Some cultural context needs explanation'); + + const log2 = retrievedLogs.find(l => l.id === 'log-roundtrip-2'); + expect(log2).toBeDefined(); + expect(log2?.action).toBe('rejected'); + expect(log2?.finalPromptChange).toBeNull(); + expect(log2?.notes).toBe('User prefers McCune-Reischauer'); + + // Now export and verify round-trip fidelity + const exported = await SessionExportOps.exportFullSession(); + const exportedLog1 = exported.amendmentLogs.find((l: any) => l.id === 'log-roundtrip-1'); + const exportedLog2 = exported.amendmentLogs.find((l: any) => l.id === 'log-roundtrip-2'); + + expect(exportedLog1).toMatchObject(testLogs[0]); + expect(exportedLog2).toMatchObject(testLogs[1]); }); }); @@ -284,48 +445,116 @@ describe('Session export/import', () => { }); describe('importSessionData()', () => { - it('imports the full session format via IndexedDB', async () => { + it('[Integration] imports full session and hydrates store correctly', async () => { + // True integration test: NO mocks on ImportOps or DB operations + // Tests entire import pipeline from payload β†’ DB β†’ store hydration + const chapterId = 'stable-import-integration'; + const url = 'https://example.com/test/chapter/integration'; + const importPayload = { - metadata: { format: 'lexiconforge-full-1' }, - }; - const rendering = [ - { - stableId: 'stable-1', - url: 'https://example.com/chapters/1', - chapterNumber: 1, - data: { - chapter: { - title: 'Imported title', - content: 'Imported content', - nextUrl: null, - prevUrl: null, - }, - translationResult: sampleChapter('stable-1', 'https://example.com/chapters/1').translationResult, - }, + metadata: { format: 'lexiconforge-full-1', exportedAt: new Date().toISOString() }, + settings: { fontSize: 18, fontStyle: 'serif' }, + navigation: { + history: [chapterId], + lastActive: { id: chapterId, url } }, - ]; - - const importSpy = vi.spyOn(ImportOps, 'importFullSessionData').mockResolvedValue(); - vi.spyOn(RenderingOps, 'fetchChaptersForReactRendering').mockResolvedValue(rendering as any); - vi.spyOn(SettingsOps, 'getKey').mockImplementation(async (key: string) => { - if (key === 'navigation-history') return { stableIds: ['stable-1'] }; - if (key === 'lastActiveChapter') return { id: 'stable-1', url: rendering[0].url }; - return null; - }); + urlMappings: [ + { url, stableId: chapterId, isCanonical: true, dateAdded: new Date().toISOString() } + ], + novels: [], + chapters: [{ + stableId: chapterId, + canonicalUrl: url, + title: 'Integration Test Chapter', + content: '

Original Korean content ν•œκΈ€

', + nextUrl: 'https://example.com/test/chapter/2', + prevUrl: null, + translations: [{ + id: 'trans-integration-1', + version: 1, + translatedTitle: 'Translated Integration Chapter', + translation: '

Translated English content

', + footnotes: [{ id: 'fn-1', marker: '[1]', content: 'A test footnote' }], + suggestedIllustrations: [{ marker: '{{IMG:test}}', description: 'Test illustration' }], + provider: 'OpenAI', + model: 'gpt-4o', + temperature: 0.3, + isActive: true, + usageMetrics: { + totalTokens: 1500, + promptTokens: 1000, + completionTokens: 500, + estimatedCost: 0.002, + requestTime: 2.5 + } + }], + feedback: [{ + id: 'fb-integration-1', + type: 'correction', + selection: 'Translated', + comment: 'Could be more natural' + }] + }], + promptTemplates: [{ + id: 'template-integration-1', + name: 'Integration Test Template', + description: 'A template for testing', + content: 'Translate: {{content}}', + isDefault: false + }], + diffResults: [], + amendmentLogs: [] + }; + // Perform actual import (NO MOCKS) await useAppStore.getState().importSessionData(importPayload); - // Verify import was called (relax assertion - extra undefined param is ok) - expect(importSpy).toHaveBeenCalled(); - expect(importSpy.mock.calls[0][0]).toEqual(importPayload); - - // Verify the outcome - state was updated correctly + // Verify store state was hydrated const state = useAppStore.getState(); expect(state.chapters.size).toBe(1); - expect(state.currentChapterId).toBe('stable-1'); - expect(state.navigationHistory).toContain('stable-1'); - const normalized = normalizeUrlAggressively(rendering[0].url)!; - expect(state.urlIndex.get(normalized)).toBe('stable-1'); + expect(state.currentChapterId).toBe(chapterId); + expect(state.navigationHistory).toContain(chapterId); + + // Verify chapter data made it to store + const chapter = state.chapters.get(chapterId); + expect(chapter).toBeDefined(); + expect(chapter?.title).toBe('Integration Test Chapter'); + expect(chapter?.content).toContain('ν•œκΈ€'); + + // Verify translation result is present + expect(chapter?.translationResult?.translatedTitle).toBe('Translated Integration Chapter'); + expect(chapter?.translationResult?.footnotes).toHaveLength(1); + expect(chapter?.translationResult?.footnotes?.[0]?.content).toBe('A test footnote'); + + // Verify URL index was built + const normalized = normalizeUrlAggressively(url); + expect(normalized).toBeDefined(); + expect(state.urlIndex.get(normalized!)).toBe(chapterId); + + // Verify data actually persisted to DB (not just in-memory) + const dbChapter = await ChapterOps.getByStableId(chapterId); + expect(dbChapter).toBeDefined(); + expect(dbChapter?.title).toBe('Integration Test Chapter'); + expect(dbChapter?.nextUrl).toBe('https://example.com/test/chapter/2'); + + // Verify translations persisted to DB + const translations = await TranslationOps.getVersionsByStableId(chapterId); + expect(translations.length).toBeGreaterThanOrEqual(1); + const activeTranslation = translations.find(t => t.isActive); + expect(activeTranslation?.translatedTitle).toBe('Translated Integration Chapter'); + expect(activeTranslation?.provider).toBe('OpenAI'); + expect(activeTranslation?.model).toBe('gpt-4o'); + + // Verify settings persisted + const settings = await SettingsOps.getKey('app-settings'); + expect(settings?.fontSize).toBe(18); + expect(settings?.fontStyle).toBe('serif'); + + // Verify prompt template persisted + const templates = await TemplatesOps.getAll(); + const importedTemplate = templates.find(t => t.id === 'template-integration-1'); + expect(importedTemplate).toBeDefined(); + expect(importedTemplate?.name).toBe('Integration Test Template'); }); it('raises errors for malformed payloads', async () => { diff --git a/tests/integration/registry.test.ts b/tests/integration/registry.test.ts index d12b59a..5dd94a9 100644 --- a/tests/integration/registry.test.ts +++ b/tests/integration/registry.test.ts @@ -204,4 +204,193 @@ describe('RegistryService (unit tests with mocked fetch)', () => { expect(novel.versions).toHaveLength(1); expect(novel.versions![0].versionId).toBe('v1'); }); + + describe('error handling', () => { + it('should throw on HTTP 404 Not Found response', async () => { + (global.fetch as any).mockResolvedValueOnce({ + ok: false, + status: 404, + statusText: 'Not Found' + }); + + await expect(RegistryService.fetchRegistry()).rejects.toThrow(); + }); + + it('should throw on HTTP 500 Internal Server Error', async () => { + (global.fetch as any).mockResolvedValueOnce({ + ok: false, + status: 500, + statusText: 'Internal Server Error' + }); + + await expect(RegistryService.fetchRegistry()).rejects.toThrow(); + }); + + it('should throw on network timeout', async () => { + (global.fetch as any).mockRejectedValueOnce(new Error('Network timeout')); + + await expect(RegistryService.fetchRegistry()).rejects.toThrow('Network timeout'); + }); + + it('should throw on malformed JSON response', async () => { + (global.fetch as any).mockResolvedValueOnce({ + ok: true, + json: async () => { throw new SyntaxError('Unexpected token < in JSON'); } + }); + + await expect(RegistryService.fetchRegistry()).rejects.toThrow(); + }); + + it('should throw on individual novel metadata 404', async () => { + (global.fetch as any).mockResolvedValueOnce({ + ok: false, + status: 404, + statusText: 'Not Found' + }); + + await expect( + RegistryService.fetchNovelMetadata('https://example.com/nonexistent.json') + ).rejects.toThrow(); + }); + }); + + describe('resilience to imperfect data', () => { + it('should handle registry with missing optional fields', async () => { + const mockRegistry = { + // Missing version and lastUpdated - only required field is novels + novels: [ + { id: 'minimal', metadataUrl: 'https://example.com/minimal.json' } + ] + }; + const mockNovel = { + id: 'minimal', + title: 'Minimal Novel' + // Missing metadata, versions - should still work + }; + + (global.fetch as any) + .mockResolvedValueOnce({ ok: true, json: async () => mockRegistry }) + .mockResolvedValueOnce({ ok: true, json: async () => mockNovel }); + + const novels = await RegistryService.fetchAllNovelMetadata(); + + expect(novels).toHaveLength(1); + expect(novels[0].title).toBe('Minimal Novel'); + expect(novels[0].metadata).toBeUndefined(); + expect(novels[0].versions).toBeUndefined(); + }); + + it('should skip novels with empty metadataUrl', async () => { + const mockRegistry = { + novels: [ + { id: 'valid', metadataUrl: 'https://example.com/valid.json' }, + { id: 'empty-url', metadataUrl: '' }, // Empty URL should be skipped + ] + }; + const mockValidNovel = { + id: 'valid', + title: 'Valid Novel', + metadata: { originalLanguage: 'Korean' } + }; + + (global.fetch as any) + .mockResolvedValueOnce({ ok: true, json: async () => mockRegistry }) + .mockResolvedValueOnce({ ok: true, json: async () => mockValidNovel }); + + const novels = await RegistryService.fetchAllNovelMetadata(); + + // Should only return the valid novel, not crash on empty URL + expect(novels).toHaveLength(1); + expect(novels[0].id).toBe('valid'); + }); + + it('should skip novels with missing metadataUrl', async () => { + const mockRegistry = { + novels: [ + { id: 'valid', metadataUrl: 'https://example.com/valid.json' }, + { id: 'no-url' }, // No metadataUrl field at all + ] + }; + const mockValidNovel = { + id: 'valid', + title: 'Valid Novel' + }; + + (global.fetch as any) + .mockResolvedValueOnce({ ok: true, json: async () => mockRegistry }) + .mockResolvedValueOnce({ ok: true, json: async () => mockValidNovel }); + + const novels = await RegistryService.fetchAllNovelMetadata(); + + expect(novels).toHaveLength(1); + expect(novels[0].id).toBe('valid'); + }); + + it('should handle novel metadata with null/undefined values gracefully', async () => { + const mockNovelMetadata = { + id: 'partial-novel', + title: 'Partial Novel', + metadata: { + originalLanguage: 'Korean', + targetLanguage: null, // null value + chapterCount: undefined, // undefined value + genres: [], // empty array + description: '' // empty string + }, + versions: null // null versions array + }; + + (global.fetch as any).mockResolvedValueOnce({ + ok: true, + json: async () => mockNovelMetadata + }); + + const novel = await RegistryService.fetchNovelMetadata('https://example.com/partial.json'); + + expect(novel.title).toBe('Partial Novel'); + expect(novel.metadata?.originalLanguage).toBe('Korean'); + expect(novel.metadata?.targetLanguage).toBeNull(); + expect(novel.versions).toBeNull(); + }); + + it('should process custom registry URL and return correct response structure', async () => { + const customRegistryUrl = 'https://custom.example.com/registry.json'; + const mockRegistry = { + version: '2.0', + lastUpdated: '2025-12-01', + novels: [ + { id: 'custom-novel', metadataUrl: 'https://custom.example.com/novel.json' } + ] + }; + const mockNovelMetadata = { + id: 'custom-novel', + title: 'Custom Novel', + metadata: { + originalLanguage: 'Chinese', + targetLanguage: 'English', + chapterCount: 200, + genres: ['Cultivation'], + description: 'A custom novel' + }, + versions: [{ + versionId: 'custom-v1', + displayName: 'Custom Translation' + }] + }; + + (global.fetch as any) + .mockResolvedValueOnce({ ok: true, json: async () => mockRegistry }) + .mockResolvedValueOnce({ ok: true, json: async () => mockNovelMetadata }); + + // Verify both registry and novel metadata are fetched and processed + const novels = await RegistryService.fetchAllNovelMetadata(customRegistryUrl); + + expect(global.fetch).toHaveBeenCalledWith(customRegistryUrl); + expect(global.fetch).toHaveBeenCalledWith('https://custom.example.com/novel.json'); + expect(novels).toHaveLength(1); + expect(novels[0].title).toBe('Custom Novel'); + expect(novels[0].metadata?.originalLanguage).toBe('Chinese'); + expect(novels[0].versions?.[0]?.versionId).toBe('custom-v1'); + }); + }); }); diff --git a/tests/services/comparisonService.test.ts b/tests/services/comparisonService.test.ts index fb0b15e..f22ea03 100644 --- a/tests/services/comparisonService.test.ts +++ b/tests/services/comparisonService.test.ts @@ -225,38 +225,183 @@ describe('ComparisonService.requestFocusedComparison', () => { }); describe('provider routing', () => { - it('uses correct baseURL for OpenAI provider', async () => { + it('configures OpenAI client correctly for OpenAI provider', async () => { openAiMocks.createMock.mockResolvedValueOnce({ choices: [{ message: { content: '{"fanExcerpt": "test"}' } }], }); await ComparisonService.requestFocusedComparison({ ...baseArgs, - settings: buildSettings({ provider: 'OpenAI' }), + settings: buildSettings({ + provider: 'OpenAI', + apiKeyOpenAI: 'sk-test-key-12345', + model: 'gpt-4o-mini' + }), }); + // Verify full client configuration expect(openAiMocks.ctorMock).toHaveBeenCalledWith( expect.objectContaining({ + apiKey: 'sk-test-key-12345', baseURL: 'https://api.openai.com/v1', + dangerouslyAllowBrowser: true }), ); + + // Verify request parameters + expect(openAiMocks.createMock).toHaveBeenCalledWith( + expect.objectContaining({ + model: 'gpt-4o-mini', + temperature: 0, // Comparison should use 0 for determinism + messages: expect.arrayContaining([ + expect.objectContaining({ + role: 'user', + content: expect.stringContaining(baseArgs.selectedTranslation) + }) + ]) + }) + ); }); - it('uses correct baseURL for DeepSeek provider', async () => { + it('configures OpenAI client correctly for DeepSeek provider', async () => { openAiMocks.createMock.mockResolvedValueOnce({ choices: [{ message: { content: '{"fanExcerpt": "test"}' } }], }); await ComparisonService.requestFocusedComparison({ ...baseArgs, - settings: buildSettings({ provider: 'DeepSeek', apiKeyDeepSeek: 'deepseek-key' }), + settings: buildSettings({ + provider: 'DeepSeek', + model: 'deepseek-chat', + apiKeyDeepSeek: 'ds-test-key' + }), }); + // Verify full client configuration for DeepSeek expect(openAiMocks.ctorMock).toHaveBeenCalledWith( expect.objectContaining({ + apiKey: 'ds-test-key', baseURL: 'https://api.deepseek.com/v1', + dangerouslyAllowBrowser: true }), ); + + // Verify the model is passed correctly + expect(openAiMocks.createMock).toHaveBeenCalledWith( + expect.objectContaining({ + model: 'deepseek-chat' + }) + ); + }); + }); + + describe('malformed response handling', () => { + it('throws on empty choices array', async () => { + openAiMocks.createMock.mockResolvedValueOnce({ + choices: [] + }); + + await expect( + ComparisonService.requestFocusedComparison({ + ...baseArgs, + settings: buildSettings(), + }) + ).rejects.toThrow(); + }); + + it('throws on null message content', async () => { + openAiMocks.createMock.mockResolvedValueOnce({ + choices: [{ message: { content: null } }] + }); + + await expect( + ComparisonService.requestFocusedComparison({ + ...baseArgs, + settings: buildSettings(), + }) + ).rejects.toThrow(); + }); + + it('throws on undefined message', async () => { + openAiMocks.createMock.mockResolvedValueOnce({ + choices: [{ message: undefined }] + }); + + await expect( + ComparisonService.requestFocusedComparison({ + ...baseArgs, + settings: buildSettings(), + }) + ).rejects.toThrow(); + }); + + it('throws on truncated JSON response', async () => { + openAiMocks.createMock.mockResolvedValueOnce({ + choices: [{ message: { content: '{"fanExcerpt": "test", "confidence":' } }] // Truncated + }); + + await expect( + ComparisonService.requestFocusedComparison({ + ...baseArgs, + settings: buildSettings(), + }) + ).rejects.toThrow('Comparison response did not contain valid JSON'); + }); + + it('propagates API error responses', async () => { + openAiMocks.createMock.mockRejectedValueOnce(new Error('Rate limit exceeded')); + + await expect( + ComparisonService.requestFocusedComparison({ + ...baseArgs, + settings: buildSettings(), + }) + ).rejects.toThrow('Rate limit exceeded'); + }); + + it('propagates network errors', async () => { + openAiMocks.createMock.mockRejectedValueOnce(new Error('Network request failed')); + + await expect( + ComparisonService.requestFocusedComparison({ + ...baseArgs, + settings: buildSettings(), + }) + ).rejects.toThrow('Network request failed'); + }); + + it('returns default values for JSON string instead of object', async () => { + // Response is valid JSON but not an object - service normalizes to default + openAiMocks.createMock.mockResolvedValueOnce({ + choices: [{ message: { content: '"just a string"' } }] + }); + + const result = await ComparisonService.requestFocusedComparison({ + ...baseArgs, + settings: buildSettings(), + }); + + // Service gracefully handles non-object JSON by returning defaults + expect(result.fanExcerpt).toBe(''); + expect(result.fanContextBefore).toBeNull(); + expect(result.rawExcerpt).toBeNull(); + }); + + it('returns default values for array instead of object', async () => { + // Array is valid JSON but not an object - service normalizes to default + openAiMocks.createMock.mockResolvedValueOnce({ + choices: [{ message: { content: '[1, 2, 3]' } }] + }); + + const result = await ComparisonService.requestFocusedComparison({ + ...baseArgs, + settings: buildSettings(), + }); + + // Service gracefully handles array JSON by returning defaults + expect(result.fanExcerpt).toBe(''); + expect(result.fanContextBefore).toBeNull(); + expect(result.rawExcerpt).toBeNull(); }); }); });