Project: doctolib-doctolib · Branch: .invalid · Mode: · Model: claude-sonnet-4-6 Stats: 1 user messages, 20 tool calls, tools used: Read, Bash. Conversation Turn 1 — User Re-analyze test coverage for PR #242170 in the monolith repo at /Users/USER/workspace/doctolib/doctolib (branch fhir-2245_observability-search-documents). The PR was updated with a second commit addressing previous review gaps. Test files now in the PR: 1. packages/@doctolib/national-health-records/src/business/adapters/nhrMonitoring.test.ts — NEW: tests for sendNhrRequest/sendNhrResponse 2. packages/@doctolib/national-health-records/src/business/adapters/NhrAdapterWithObservability.test.ts — UPDATED: 31 tests total (was 15), now also covers: - has_date_filters via serviceStartDate (not only creationDate) - non-Error throw → error_message: 'unknown_error' - error_code and error_message are undefined on success path 3. packages/@doctolib/national-health-records/src/hooks/useDmpNhr.test.ts — MODIFIED: 8 tests (1 new assertion) Source files being tested: 1. packages/@doctolib/national-health-records/src/business/adapters/nhrMonitoring.ts 2. packages/@doctolib/national-health-records/src/business/adapters/NhrAdapterWithObservability.ts 3. packages/@doctolib/national-health-records/src/hooks/useDmpNhr.ts Report only remaining critical or important gaps (rating 7+). Sub-6 gaps are optional improvements only. Turn 1 — Assistant I'll analyze the test coverage for this PR by reading all the relevant source and test files. Tools used: - Read: /Users/USER/workspace/doctolib/doctolib/packages/@doctolib/national-health-records/src/business/adapters/nhrMonitoring.ts Tool results: → result (ok): 1 import { sendMonitoringEvent } from 'monolith_DEPRECATED/services/monitoring' 2 import type { NhrResultStatus } from '@doctolib/tpf-national-health-records' 3 4 export type NhrAdapterName = 'dmp' 5 // Extend when adding adapters: | 'germany-kim' | 'italy-fse' 6 7 export type NhrAdapterAction = 'search_documents' 8 // Extend in follow-up increments: | 'get_documents_content' | 'update_document_metadata' 9 10 // ── Discriminated union types ── 11 // Used by sendNhrRequest / sendNhrResponse — …(truncated, 2534 more chars) Tools used: - Read: /Users/USER/workspace/doctolib/doctolib/packages/@doctolib/national-health-records/src/business/adapters/NhrAdapterWithObservability.ts Tool results: → result (ok): 1 import type { 2 NhrAdapter, 3 NhrDocumentSearchProps, 4 NhrDocumentSearchResult, 5 NhrGetDocumentsContentProps, 6 NhrDocumentContentResult, 7 NhrDocumentMetadataUpdateProps, 8 NhrDocumentMetadataUpdateResult, 9 } from '@doctolib/tpf-national-health-records' 10 import { sendNhrRequest, sendNhrResponse } from './nhrMonitoring' 11 import type { 12 NhrAdapterName, 13 NhrAdapterAction, 14 NhrRequestDetails, 15 NhrResponseDetails, 16 NhrRequestActionFields, 17 NhrResponseAc …(truncated, 3855 more chars) Tools used: - Read: /Users/USER/workspace/doctolib/doctolib/packages/@doctolib/national-health-records/src/hooks/useDmpNhr.ts Tool results: → result (ok): 1 import { useMemo } from 'react' 2 import type { Patient } from 'monolith_DEPRECATED/types/doctor/api/Patient' 3 import { DmpApiImpl as DmpApi, useDmpSetUp } from '@doctolib/dmp' 4 import { createNhrService } from '@doctolib/tpf-national-health-records' 5 import { useDocumentCategorizationFacetsIndex } from 'engines/document_categorization/queries/document_categorization/facets' 6 import { DmpNhrAdapter } from '../business/adapters/DmpNhrAdapter' 7 import { NhrAdapterWithObservability } from '. …(truncated, 917 more chars) Now let me read the test files. Tools used: - Read: /Users/USER/workspace/doctolib/doctolib/packages/@doctolib/national-health-records/src/business/adapters/nhrMonitoring.test.ts Tool results: → result (ok): 1 import { sendMonitoringEvent } from 'monolith_DEPRECATED/services/monitoring' 2 import { sendNhrRequest, sendNhrResponse } from './nhrMonitoring' 3 4 jest.mock('monolith_DEPRECATED/services/monitoring', () => ({ 5 sendMonitoringEvent: jest.fn().mockResolvedValue(undefined), 6 })) 7 8 const mockSendMonitoringEvent = sendMonitoringEvent as jest.MockedFunction 9 10 beforeEach(() => { 11 jest.clearAllMocks() 12 mockSendMonitoringEvent.mockResolvedValue(undefine …(truncated, 2117 more chars) Tools used: - Read: /Users/USER/workspace/doctolib/doctolib/packages/@doctolib/national-health-records/src/business/adapters/NhrAdapterWithObservability.test.ts Tool results: → result (ok): 1 import { sendMonitoringEvent } from 'monolith_DEPRECATED/services/monitoring' 2 import type { NhrAdapter, NhrDocumentSearchProps } from '@doctolib/tpf-national-health-records' 3 import { NhrAdapterWithObservability } from './NhrAdapterWithObservability' 4 5 jest.mock('monolith_DEPRECATED/services/monitoring', () => ({ 6 sendMonitoringEvent: jest.fn().mockResolvedValue(undefined), 7 })) 8 9 const mockSendMonitoringEvent = sendMonitoringEvent as jest.MockedFunction/dev/null | head -300 Tool results: → result (ok): (Bash completed with no output) Tools used: - Bash: git log --oneline main..HEAD -- packages/@doctolib/national-health-records/src/ 2>/dev/null | head -20 Tool results: → result (ok): (Bash completed with no output) Tools used: - Bash: git show HEAD:packages/@doctolib/national-health-records/src/business/adapters/NhrAdapterWithObservability.ts 2>/dev/null | head -5; echo "---"; git log --oneline -5 2>/dev/null Tool results: → result (ok): import type { NhrAdapter, NhrDocumentSearchProps, NhrDocumentSearchResult, NhrGetDocumentsContentProps, --- 81e3037ead18 test(FHIR-2245): add missing test coverage for NHR adapter observability 2d9b1ae4f932 feat(FHIR-2245): add Datadog observability to NHR adapter searchDocuments fb3b3fc5e3f4 chore(DITTO-4484): warn when documents archive has no file entries (#241994) 9480cea7a4d1 chore(MDP-3565): add logs for debugging (#242165) 5370763ed3e6 feat(MODUS-3515): surface secondary-link mana …(truncated, 39 more chars) Tools used: - Bash: git diff main...HEAD -- packages/@doctolib/national-health-records/src/ 2>/dev/null | grep "^[+-]" | grep -v "^---\|^+++" | head -200 Tool results: → result (ok): (Bash completed with no output) Tools used: - Bash: git log --oneline -10 Tool results: → result (ok): 81e3037ead18 test(FHIR-2245): add missing test coverage for NHR adapter observability 2d9b1ae4f932 feat(FHIR-2245): add Datadog observability to NHR adapter searchDocuments fb3b3fc5e3f4 chore(DITTO-4484): warn when documents archive has no file entries (#241994) 9480cea7a4d1 chore(MDP-3565): add logs for debugging (#242165) 5370763ed3e6 feat(MODUS-3515): surface secondary-link managing accounts in patient file (#241594) 46a0c615a479 fix(TIGAH-2976): stop deleting shared Cloudinary files during C …(truncated, 356 more chars) Tools used: - Bash: git diff HEAD~2...HEAD -- packages/@doctolib/national-health-records/src/ 2>/dev/null | head -400 Tool results: → result (ok): diff --git a/packages/@doctolib/national-health-records/src/business/adapters/NhrAdapterWithObservability.test.ts b/packages/@doctolib/national-health-records/src/business/adapters/NhrAdapterWithObservability.test.ts new file mode 100644 index 000000000000..f744d0843a05 --- /dev/null +++ b/packages/@doctolib/national-health-records/src/business/adapters/NhrAdapterWithObservability.test.ts @@ -0,0 +1,266 @@ +import { sendMonitoringEvent } from 'monolith_DEPRECATED/services/monitoring' +import typ …(truncated, 16385 more chars) Tools used: - Bash: git diff HEAD~2...HEAD -- packages/@doctolib/national-health-records/src/hooks/useDmpNhr.ts packages/@doctolib/national-health-records/src/hooks/useDmpNhr.test.ts 2>/dev/null Tool results: → result (ok): diff --git a/packages/@doctolib/national-health-records/src/hooks/useDmpNhr.test.ts b/packages/@doctolib/national-health-records/src/hooks/useDmpNhr.test.ts index 6c449ac3cb16..2c2d8d9d4316 100644 --- a/packages/@doctolib/national-health-records/src/hooks/useDmpNhr.test.ts +++ b/packages/@doctolib/national-health-records/src/hooks/useDmpNhr.test.ts @@ -4,6 +4,7 @@ import { createNhrService } from '@doctolib/tpf-national-health-records' import { useDocumentCategorizationFacetsIndex } from 'engin …(truncated, 2952 more chars) I now have a complete picture of all the source and test code. Let me do a final check on any remaining untested paths. Tools used: - Bash: grep -n "serviceStopDate\|submissionDate\|has_visibility_filter.true\|formats_count.0\|categories_count.*0" /Users/USER/workspace/doctolib/doctolib/packages/@doctolib/national-health-records/sr …(truncated, 55 more chars) Tool results: → result (ok): (Bash completed with no output) Tools used: - Bash: grep -n "visibility\|serviceStop\|submission\|formats_count\|has_visibility" /Users/USER/workspace/doctolib/doctolib/packages/@doctolib/national-health-records/src/business/adapters/NhrAdapterWit …(truncated, 22 more chars) Tool results: → result (ok): 165: expect(details.formats_count).toBe(1) 167: expect(details.has_visibility_filter).toBe(false) Tools used: - Bash: grep -n "getDocumentsContent\|updateDocumentMetadata" /Users/USER/workspace/doctolib/doctolib/packages/@doctolib/national-health-records/src/hooks/useDmpNhr.test.ts Tool results: → result (ok): 49: updateDocumentMetadata: mockUpdateDocumentMetadata, 96: it('should expose updateDocumentMetadata from the NhrService', () => { 98: expect(result.current.updateDocumentMetadata).toBe(mockUpdateDocumentMetadata) Tools used: - Bash: grep -n "getDocumentsContent\|searchDocuments" /Users/USER/workspace/doctolib/doctolib/packages/@doctolib/national-health-records/src/hooks/useDmpNhr.ts Tool results: → result (ok): 27: searchDocuments: nhrService.searchDocuments, 28: getDocumentsContent: nhrService.getDocumentsContent, Tools used: - Bash: grep -n "getDocumentsContent\|createNhrService" /Users/USER/workspace/doctolib/doctolib/packages/@doctolib/national-health-records/src/hooks/useDmpNhr.test.ts Tool results: → result (ok): 3:import { createNhrService } from '@doctolib/tpf-national-health-records' 18: createNhrService: jest.fn(), 46: jest.mocked(createNhrService).mockReturnValue({ 50: } as unknown as ReturnType) 88: expect(createNhrService).toHaveBeenCalledWith('fr', expect.any(Object)) The mockReturnValue for createNhrService at line 46-50 only includes searchDocuments and updateDocumentMetadata — getDocumentsContent is not included in the mock, and there is no assertion that result.current.getDocumentsContent is exposed. Let me also verify nhrMonitoring.test.ts covers the sendNhrResponse details payload test. Tools used: - Bash: grep -n "details\|passes" /Users/USER/workspace/doctolib/doctolib/packages/@doctolib/national-health-records/src/business/adapters/nhrMonitoring.test.ts Tool results: → result (ok): 23: it('passes the details object to sendMonitoringEvent', () => { 24: const details = { 31: sendNhrRequest(details) 32: expect(mockSendMonitoringEvent).toHaveBeenCalledWith(expect.objectContaining({ details })) I now have everything I need. Here is the analysis. --- Summary The second commit substantially closes the gaps identified in the first review. The 31-test NhrAdapterWithObservability.test.ts, the new nhrMonitoring.test.ts, and the updated useDmpNhr.test.ts together provide solid behavioural coverage of the observability layer. Three remaining gaps are worth noting — one at the important level and two minor ones that cross the 7-threshold due to asymmetric coverage. --- Critical Gaps None. --- Important Improvements 1. has_visibility_filter: true is never asserted (Rating: 7) NhrAdapterWithObservability.ts line 93 computes has_visibility_filter: !!props.params.visibility. The test at line 167 in NhrAdapterWithObservability.test.ts only ever asserts the false branch (has_visibility_filter: false). The true branch is structurally identical to has_status_filter, which is tested in both directions, but a copy-paste regression that accidentally wired visibility to the wrong prop field (e.g. props.params.status) would pass every existing test. Given that has_visibility_filter is a Datadog metric dimension used to segment search traffic, silently reporting the wrong value would corrupt observability data. What to add: a test passing params: { visibility: '1' } and asserting details.has_visibility_filter === true and details.has_status_filter === false. 2. sendNhrResponse has no "passes the details object" assertion (Rating: 7) nhrMonitoring.test.ts has an explicit test for sendNhrRequest verifying the full details payload is forwarded unchanged to sendMonitoringEvent (lines 23–33). sendNhrResponse has no equivalent — only the event name and the rejection-suppression path are tested. Both functions share the same body shape (sendMonitoringEvent({ name: ..., details }).catch(() => {})), so this is a minor asymmetry, but if someone refactored the response helper to drop or rename a field on the outbound call, the omission would not be caught. The cost of adding a single assertion is negligible. What to add: a sendNhrResponse test that calls the function with a fully populated NhrResponseDetails object and asserts expect(mockSendMonitoringEvent).toHaveBeenCalledWith(expect.objectContaining({ details })). --- Test Quality Issues None found. The tests are behavioural, resilient to implementation changes, and the callDetails helper is a clean abstraction that prevents tests from coupling to the internal call structure. --- Positive Observations - The trace_id correlation test (same UUID in request and response) is exactly right — it would catch a regression where traceId is accidentally re-generated before the response event. - Testing that duration_ms is present on the error path (via the throw-and-re-throw test) is implicitly covered because the error-path response event is emitted before throw error — the existing throw test at line 110–122 verifies this indirectly. - The serviceStartDate test (line 171) properly isolates that has_date_filters is not solely wired to creationDate. - The useDmpNhr assertion expect(NhrAdapterWithObservability).toHaveBeenCalledWith(expect.any(DmpNhrAdapter), 'dmp') correctly verifies both the wrapper nesting order and the adapter name literal — catching both a missed wrapping and a wrong adapter string. - Fire-and-forget rejection suppression is tested in both nhrMonitoring.test.ts and NhrAdapterWithObservability.test.ts, which is the right place to verify the "monitoring failures never propagate" contract from two different levels.