[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

  141 files  +  7    141 suites  +7   4m 0s ⏱️ +47s
  919 tests +100    919 ✅ +100  0 💤 ±0  0 ❌ ±0 
1 051 runs  +100  1 045 ✅ +100  6 💤 ±0  0 ❌ ±0 

Results for commit 95d4503. ± Comparison against base commit 67d2cc9.

♻️ 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.

[NicoPiel]

@jonbartels

1. TemplateValueReplacerTest order dependence (3d8dd2c): reproduced it locally. A full ./gradlew test with the default four forks fails exactly as you saw, and a single fork running TcpReceiverTest, TcpDispatcherTest and ServerSettingsTest first fails every time. Those tests statically inject a mock ControllerFactory with no ConfigurationController and never reset it. That's the shared-JVM problem Remove Guice to fix flaky tests #519 fixes at the root by removing Guice and restoring the mocks. So this PR no longer re-enables the test: it is back to TemplateValueReplacerTests, unchanged from main, and Restore some of disabled *Tests #449/Remove Guice to fix flaky tests #519 can turn it on together with the fix. A Guice-based workaround here would stop compiling once Remove Guice to fix flaky tests #519 lands.

2. DICOM pixel data: keeping it. As Mitch points out, 2.6 MB is small, keeping the full instances means later samples with pixel data won't need a different layout, and it avoids making a derived work of the NCI data.

3. DICOM attribution (a46e17a): the README now lists license version and collection DOI per sample, taken from the IDC index by each sample's SeriesInstanceUID (five CC BY 3.0, two CC BY 4.0). It links the TCIA data usage policy and states that we haven't modified the files.

4. EDI line endings (4e8db28): test-edi-input.txt is now text eol=lf. The duplicate test-1597-input.xml / test-1597-output.txt are gone, and testIssue1597fromXML now compares against test-edi-input.txt. The NCPDP inputs stay binary: they only look like text, line endings carry no meaning in NCPDP and would corrupt the files, and the control characters don't belong in a terminal diff.

5. Digest nonce reuse (1f87dab): added a comment marking testNonceWithoutCountCanBeReused as characterizing RFC 2069 behaviour, not endorsing it. The MD5 and qop fallbacks you point out are worth a follow-up issue.

6. XML unknown ACK (1f87dab): added a comment that it characterizes current behaviour and differs from the ER7 path. I think it's a bug, but I'd rather fix it in its own PR than change behaviour in a test PR.

Nits (1f87dab): added SPDX headers. Added a positive control to testDisabledAlgorithmAndQopAreRejected. That also showed the qop case could pass on a nonce-count rejection, because it reused the nonce from the first attempt, so each attempt now gets its own nonce. Nonce expiry by age isn't covered. It would need a clock seam in DigestAuthenticator, so I've left it out of this PR.