Project: doctolib-doctolib · Branch: .invalid · Mode:  · Model: claude-sonnet-4-6

Stats: 1 user messages, 7 tool calls, tools used: Read, Bash.

Conversation

Turn 1 — User

Analyze test coverage for PR #242170 in the monolith repo at /Users/USER/workspace/doctolib/doctolib, branch fhir-2245_observability-search-documents.

The PR adds observability to the NHR adapter layer. The new/modified test files are:

1. packages/@doctolib/national-health-records/src/business/adapters/NhrAdapterWithObservability.test.ts — 15 unit tests for the new decorator
2. packages/@doctolib/national-health-records/src/hooks/useDmpNhr.test.ts — 8 tests (1 new assertion added)

The source files being tested:
1. packages/@doctolib/national-health-records/src/business/adapters/NhrAdapterWithObservability.ts — decorator implementing NhrAdapter, wraps searchDocuments with observability, getDocumentsContent and updateDocumentMetadata are transparent stubs
2. packages/@doctolib/national-health-records/src/business/adapters/nhrMonitoring.ts — types and sendNhrRequest/sendNhrResponse helpers
3. packages/@doctolib/national-health-records/src/hooks/useDmpNhr.ts — wiring change only

Analyze whether the test coverage is complete, identifies critical gaps, and check whether the test cases match the requirements. Report specific missing test cases with recommendations.

Turn 1 — Assistant

I'll analyze the test coverage by reading the source files and test files in parallel.

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/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<typeof sendMonitoringEvent
…(truncated, 9995 more chars)

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/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)

Tools used:

- Read: /Users/USER/workspace/doctolib/doctolib/packages/@doctolib/national-health-records/src/hooks/useDmpNhr.test.ts

Tool results:

  → result (ok): 1	import { renderHook } from '@testing-library/react'
2	import { useDmpSetUp } from '@doctolib/dmp'
3	import { createNhrService } from '@doctolib/tpf-national-health-records'
4	import { useDocumentCategorizationFacetsIndex } from 'engines/document_categorization/queries/document_categorization/facets'
5	import type { Patient } from 'monolith_DEPRECATED/types/doctor/api/Patient'
6	import { DmpNhrAdapter } from '../business/adapters/DmpNhrAdapter'
7	import { NhrAdapterWithObservability } from '../
…(truncated, 4038 more chars)

Now let me look at the DmpNhrAdapter to understand the full context, and also check if there are adjacent tests or the getDocumentsContent return type more carefully.

Tools used:

- Read: /Users/USER/workspace/doctolib/doctolib/packages/@doctolib/national-health-records/src/business/adapters/DmpNhrAdapter.ts

Tool results:

  → result (ok): 1	import type {
2	  DmpApi,
3	  DmpAuthor,
4	  DmpDocument,
5	  DmpDocumentContent,
6	  SearchProps,
7	  SearchResult as DmpSearchResult,
8	} from '@doctolib/dmp'
9	import { DmpApiError, DOCUMENT_STATUS, DOCUMENT_VISIBILITY, getPatientInsForDMP } from '@doctolib/dmp'
10	import { isUnstructured } from '@doctolib/cda/types/UnstructuredBody'
11	import { isStructured } from '@doctolib/cda/types/StructuredBody'
12	import type { Patient } from 'monolith_DEPRECATED/types/doctor/api/Patient'
13	import t
…(truncated, 9720 more chars)

Tools used:

- Bash: find /Users/USER/workspace/doctolib/doctolib/packages/@doctolib/national-health-records/src -name "*.ts" | sort

Tool results:

  → result (ok): /Users/USER/workspace/doctolib/doctolib/packages/@doctolib/national-health-records/src/business/adapters/DmpNhrAdapter.test.ts
/Users/USER/workspace/doctolib/doctolib/packages/@doctolib/national-health-records/src/business/adapters/DmpNhrAdapter.ts
/Users/USER/workspace/doctolib/doctolib/packages/@doctolib/national-health-records/src/business/adapters/NhrAdapterWithObservability.test.ts
/Users/USER/workspace/doctolib/doctolib/packages/@doctolib/national-health-records/src/business/adapters/NhrAd
…(truncated, 5330 more chars)

Now I have a thorough picture of the PR. Here is my analysis.

---

Summary

The test coverage for this PR is strong and well-structured. The 15 unit tests for NhrAdapterWithObservability cover the primary observability contract with good depth. The useDmpNhr hook test is adequate as a wiring test. There are no critical production-breaking gaps, but several meaningful cases are either under-specified or missing entirely.

---

Critical Gaps

None rated 9-10. The core error-reporting contract is covered.

---

Important Improvements

1. has_date_filters logic — only creationDate is exercised (Rating: 7)

The buildSuccessFields callback in searchDocuments computes has_date_filters from four independent fields: creationDate, serviceStartDate, serviceStopDate, and submissionDate. The existing test "sends correct filter counts and flags" only sets creationDate. If any one of the other three date fields were accidentally dropped from the OR expression (a trivial one-line refactor mistake), every test would still pass.

Missing test: a case where creationDate is absent but one of serviceStartDate, serviceStopDate, or submissionDate is set, asserting has_date_filters === true.

Relevant source lines in /Users/USER/workspace/doctolib/doctolib/packages/@doctolib/national-health-records/src/business/adapters/NhrAdapterWithObservability.ts (lines 95–100):
has_date_filters: !!(
  props.params.creationDate ||
  props.params.serviceStartDate ||
  props.params.serviceStopDate ||
  props.params.submissionDate
),

2. error_message for non-Error thrown values (Rating: 7)

The catch branch sets error_message to 'unknown_error' when the thrown value is not an instanceof Error (line 74). The existing "re-throws" test only exercises an Error instance. If the string branch ('unknown_error') were accidentally removed, no test would catch it.

Missing test: throw a plain string or a non-Error object, assert error_message === 'unknown_error' in the response event details.

3. formats_count zero path is not explicitly asserted (Rating: 6)

The filter-counts test sets formats: ['fmt1'] (count 1) and verifies formats_count === 1. The zero case (formats absent) is only implicitly covered through categories_count in a different test. While categories_count: 0 is asserted in the non-throwing error test (no filters), formats_count is never independently asserted as zero. This is a minor gap but worth noting because the default for ?? on an undefined array is 0.

4. error_code and error_message are undefined in the success response event (Rating: 6)

The success path in buildSuccessFields sets error_code and error_message to undefined when status !== 'error' (lines 88–89). No test asserts these fields are absent/undefined on a successful response event. If the condition were accidentally inverted to always populate them, no existing test would catch it.

Missing test: on a successful result, assert callDetails(1).error_code is undefined and callDetails(1).error_message is undefined.

5. adapterName passed as non-'dmp' value is never used in any test (Rating: 5)

All 15 tests use new NhrAdapterWithObservability(mockAdapter, 'dmp'). The type NhrAdapterName is a union that currently only contains 'dmp', but if it is ever extended (the comment on line 5 of nhrMonitoring.ts already anticipates 'germany-kim' and 'italy-fse'), the adapter name propagation to monitoring events should be verified. This is low risk right now but the test suite effectively only covers one concrete value of the constructor parameter.

---

Test Quality Issues

6. "does not propagate monitoring failures to the caller" tests the wrong layer (Rating: 5, quality issue)

The test at line 124 mocks sendMonitoringEvent to reject and asserts the searchDocuments call still resolves. However, sendNhrRequest and sendNhrResponse in nhrMonitoring.ts already call .catch(() => {}) on the promise before it ever reaches the observe() code — the fire-and-forget pattern is in nhrMonitoring.ts, not in NhrAdapterWithObservability. This test therefore passes vacuously: even if the .catch(() => {}) were removed from nhrMonitoring.ts, the test would still pass because the rejection is swallowed before observe() awaits anything. The test is testing behaviour that belongs to nhrMonitoring.ts, but nhrMonitoring.ts has no tests at all.

The actual contract that should be tested is in nhrMonitoring.ts: sendNhrRequest and sendNhrResponse must not throw even when sendMonitoringEvent rejects. A unit test for those helpers in their own test file would be the correct location.

7. callDetails helper assumes call ordering but tests don't reset between describe blocks (minor)

beforeEach clears mocks at the top level, so this is fine in practice. But the helper callDetails(n) relies on index-based ordering of sendMonitoringEvent calls. Any test that adds an extra sendMonitoringEvent call — for example if observe() were refactored to emit a third event — would silently produce wrong indices in assertions. Tests that assert specific call counts first (like expect(mockSendMonitoringEvent).toHaveBeenCalledTimes(2)) before reading details mitigate this. All tests that use callDetails do check call count first, so this is not a defect, but worth noting as a maintenance fragility.

---

Positive Observations

- The test file is cleanly partitioned into three describe blocks matching the three logical concerns: invariants, searchDocuments-specific fields, and stub delegation. This makes the intent of each test group immediately clear.
- Testing both the throwing error path and the non-throwing error result (DMP returning status: 'error' without throwing) is excellent — this is a real and important distinction in the DMP adapter contract.
- The trace_id correlation test (same UUID in request and response) directly prevents a class of silent observability bugs where request/response events cannot be joined in a dashboard.
- The useDmpNhr test correctly asserts the wrapping order: DmpNhrAdapter is constructed first, then passed into NhrAdapterWithObservability with the 'dmp' adapter name, then passed to createNhrService. This verifies the decorator wiring is correct and would catch someone accidentally bypassing the decorator.
- The missing getDocumentsContent return value exposure from useDmpNhr is not tested (only searchDocuments and updateDocumentMetadata are asserted as being the same function reference), but this is an existing gap, not introduced by this PR.