[NicoPiel]

Adds unit tests for the HL7v2 and NCPDP readers, the HL7v2 response validator and the digest authenticator, and brings over the data type tests from #449 so they can land sooner.

New unit tests

What the tests cover:

• Readers: separators and encoding characters, empty fields, repetitions and subcomponents, NCPDP transaction groups, count and counter fields, and field names that differ between versions.
• Validator: ER7 and XML ACKs, custom ACK codes, control-ID lookups from the encoded message or from a map variable, and rejection of a DOCTYPE in an XML response.
• Digest: the full challenge and response flow for MD5, MD5-sess, auth and auth-int, plus stale nonces, replayed and too-high nonce counts, and each invalid-directive path.

Class	Before (Sonar)	After (local JaCoCo)	Test file
ER7Reader	0%	99.5% lines / 92.7% branches	ER7ReaderTest.java (new)
NCPDPReader	0%	99.4% / 91.1%	NCPDPReaderTest.java (new)
HL7v2ResponseValidator	2.8%	99.4% / 92.0%	HL7v2ResponseValidatorTest.java (new)
DigestAuthenticator	7.4%	91.5% / 86.5%	DigestAuthenticatorTest.java (15 tests added)

Data type tests from #449

Cherry-picked from @mgaffigan's maint/restore-tests with -x, authorship and sign-off kept:

• Restore some of the disabled *Tests (HL7 serializer, password and value replacer tests renamed to *Test so the build picks them up)
• Re-add the EDI serializer tests using new fixtures
• Re-add the NCPDP tests, expanded to include a D.0 response and checks that converting back gives the original message
• Re-add the DICOM tests with sample instances
• Drop the dead main()-style test entrypoints

These sample-message tests pin realistic round trips, including the XML-to-message direction. The reader tests above cover edge cases the samples don't reach. Four reader tests that only repeated what the fixtures already check were dropped in the last commit. testTransactionGroups stays, because every NCPDP fixture has a single transaction.

142 data type, digest, password and value-replacer tests pass locally on JDK 17.

[github-actions]

Test Results

  138 files  +  9    138 suites  +9   3m 46s ⏱️ + 1m 4s
  897 tests +108    897 ✅ +108  0 💤 ±0  0 ❌ ±0 
1 023 runs  +108  1 017 ✅ +108  6 💤 ±0  0 ❌ ±0 

Results for commit 3a19db3. ± Comparison against base commit f2ac43d.

♻️ This comment has been updated with latest results.

[mgaffigan]

@NicoPiel, seems like a good aim for a core use case. You might cherry-pick and de-dupe the related tests from #449 to this branch if you are trying to get these merged sooner. That branch includes sample messages and pinning of parsing. Same for DICOM.

[NicoPiel]

You might cherry-pick and de-dupe the related tests from #449 to this branch

I'll try and do that.

[jonbartels]

Review notes

Overall this is a net gain. Coverage goes up, the restored tests are real tests now, and nothing that used to run was lost. There's one test failure to look at and a few fixture items.

Renames, deletions, coverage

• Renames: PasswordRequirements, Pre22PasswordChecker, TemplateValueReplacer and ValueReplacer change only the class name.
• HL7SerializerTest: all 15 methods are kept. It now actually applies the repetition/subcomponent settings each fixture was written for; before, those lived in commented-out code and every test ran with the defaults (both true). The XML comparisons also ignore CRLF vs LF now. That one is necessary, because the HL7 fixtures are CRLF.
  • Side effect: no toXML test runs with the product default (both on) any more.
• Deleted *Tests: every one of the 16 @Test methods (DICOM 3, EDI 4, NCPDP 9) has a replacement with the same input, the same direction, and an equal or stricter assertion. On main they could never have passed, because their fixtures were never committed: running them gives 13× NoSuchFileException, and HL7SerializerTests fails 5 toXml assertions.
• Deleted main() harnesses: never automated. These areas are still untested, which is fine for this PR:
  • DICOM fromXML
  • NCPDP strict schema validation
  • NCPDP 5.1 transaction types other than B1
  • JAXB loading of the X12 transaction definitions
• Coverage: JaCoCo, :server:test -Pcoverage=true, JDK 17, merge base vs PR head.
  • Lines 20.4% → 22.5% (+1,164), branches 15.3% → 17.7% (+471).
  • No class goes down.
  • The four numbers in the description match.
  • The old *Tests, run on main, would have added only about 5 lines over this PR.

Should fix

1. TemplateValueReplacerTest.testReplaceValues is order-dependent.
   • In a full local :server:test run (macOS, Zulu 17) it fails with an NPE: ConfigurationController.getInstance() returns null in getConfigurationMap().
   • It passes run alone and passed in CI.
   • It looks like an earlier test in the same fork leaves a mocked ControllerFactory behind.
   • The old TemplateValueReplacerTests fails the same way, so the rename turns on a test that was already broken.
2. The DICOM samples carry about 2.6 MB of pixel data the test never reads. DICOMSerializerTest calls removePixelData before comparing. Header-only .dcm files would cut about 97% of the size and remove any burned-in-annotation risk, unless a pixel or fromXML test is planned.
3. server/tests/dicom/README.md attribution. CC BY, and TCIA's data usage policy, expect for each collection: the license version, the collection DOI/citation, the original source (TCIA/IDC), and a note on whether the files were modified. At the moment it only says "CC-BY … by way of saga-it.com". The de-identification itself looks fine: patient identity fields are pseudonyms, empty or removed, dates are shifted or placeholders, and PatientIdentityRemoved is set where applicable.
4. The EDI tests would break on a Windows checkout.
   • test-edi-input.txt and test-1597-output.txt aren't pinned in .gitattributes. With core.autocrlf=true they become CRLF.
   • EDISerializer's delimiter inference only handles \n straight after the segment terminator, so ~\r\n would break those tests.
   • Suggest -text or eol=lf for them. CI is Linux-only, so this is latent.

Worth discussing

5. DigestAuthenticatorTest.testNonceWithoutCountCanBeReused pins RFC 2069-style unlimited nonce reuse when qop is absent.
   • Related production behaviour, pre-existing and out of scope for this PR: in DigestAuthenticator, a missing algorithm falls back to MD5 even when MD5 is disabled, and a missing qop is accepted even when QOP modes are restricted.
   • A comment saying this is deliberate legacy behaviour, or a follow-up issue, would keep the test from reading as an endorsement.
6. HL7v2ResponseValidatorTest.testXmlUnknownAckCodeLeavesStatusUnchanged pins an inconsistency. An unknown ACK code becomes QUEUED/"Invalid" for ER7 responses but stays SENT for XML ones. Is that intended, or a bug being characterized?

Nits

• ER7ReaderTest, NCPDPReaderTest and HL7v2ResponseValidatorTest lack the SPDX-License-Identifier header that recent test files carry.
• test-1597-input.xml and test-1597-output.txt are byte-identical to test-edi-output.xml and test-edi-input.txt, so testIssue1597toXML repeats testToXml.
• binary on the NCPDP inputs also sets -diff. -text would protect them and keep reviewable diffs.
• Nonce expiry by age (60 s) isn't covered. testDisabledAlgorithmAndQopAreRejected would be stronger with a positive control for the enabled combination.

Provenance

The five cherry-picks match #449 except for the (cherry picked from …) line, and all commits carry DCO sign-offs. Heads-up: later commits in #449 also touch TemplateValueReplacerTest, so expect a conflict there.

[mgaffigan]

The TemplateValueReplacerTest thing sounds similar to #519.

The NCPDP thing should stay binary - they only casually look like text. Line endings do not have semantic meaning in an NCPDP file and would corrupt things. They also contain control characters that may anger terminals when diff is run.

Unless a DICOM expert can tell me the pixel data is useless I would recommend leaving it. 2.6MB is negligible and ensures we don't have to later add samples that do have pixel data. It also avoids creating a derive work of the NCI dataset - though they may be available without the pixel data - not sure.