Skip to content

Tests pin emitted FHIR by string only, so invalid output passes #1508

Description

@alexzautke

Summary

Tests covering the CQL→FHIR converter assert the emitted value with an exact-string comparison and nothing else. A string assertion pins whatever the converter currently produces, so it cannot distinguish a correct value from a lexically invalid one — it only detects change. As a result, invalid FHIR reached develop with a full green suite, and in one case a test was written that asserted the invalid output was correct.

Impact

Two concrete instances, both found by review rather than by the test suite (see #1506 and PR #1458):

  1. FHIR.time carrying a timezone offset. Convert<Time> emitted "10:30:00Z" and "10:30:00+02:00". FHIR R4 time states "A time zone SHALL NOT be present"; Hl7.Fhir.Model.Time.IsValidValue returns false for both. A test — ConvertCqlTime_Time_PassesAVestigialOffsetThrough — pinned exactly these two strings, so the suite actively defended the invalid output.
  2. Time-bearing dateTime with no offset. "2014-02-01T10:30:00" was emitted for an offset-less CQL value. FHIR R4 requires an offset once hours and minutes are present; FhirDateTime.IsValidValue returns false. Several tests pinned the offset-less strings.

Both were lexically invalid FHIR that a receiving server can reject, and in both cases the suite was green.

Root Cause

Hl7.Fhir.Model.<Type>.IsValidValue is a public static on Hl7.Fhir.Base, which the test projects already reference, but it is not used anywhere in the test suite. Nothing in CLAUDE.md or .github/copilot-instructions/ asks for it, so each new converter test reproduces the pattern: assert the string, assume validity.

A string pin is a regression oracle. It answers "did the output change?", never "is the output correct?" — and when the first-written expectation is wrong, it converts a bug into a defended invariant.

Expected Behavior

Every test pinning the text of an emitted FHIR primitive also asserts IsValidValue on that text, so a malformed value fails on the validity assertion regardless of what the string assertion says. Documented as a convention in both instruction files, since Copilot and Claude both author converter tests in this repo.

Separately: "pre-existing" and "out of scope" are legitimate reasons not to fix invalid output in a given change, but never reasons to pin it as correct or to document an invariant the code does not hold. Instance 1 above was initially triaged that way before being corrected.

Acceptance Criteria

  • CLAUDE.md states that a test pinning emitted FHIR primitive text must also assert Hl7.Fhir.Model.<Type>.IsValidValue, with the two real failures as motivation.
  • The equivalent rule exists in .github/copilot-instructions/, per the sync requirement that a universal convention not live in only one of the two.
  • Both state that invalid output may be left unfixed but must never be pinned as correct, nor described by a comment asserting an invariant the code does not hold.
  • The existing converter tests already carrying IsValidValue assertions (added in PR Pad partial-precision times/dateTimes for FHIR #1458) match the documented convention.

Metadata

Metadata

Assignees

No one assigned

    Labels

    EnhancementNew feature or request

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions