-
Notifications
You must be signed in to change notification settings - Fork 16
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[DPC-4171] Upgrade HAPI FHIR #2214
Conversation
dpc-common/src/test/java/gov/cms/dpc/fhir/validations/AttestationValidationTest.java
Show resolved
Hide resolved
@@ -50,7 +53,22 @@ static void setup() { | |||
void definitionIsValid() { | |||
final StructureDefinition patientDefinition = dpcModule.fetchStructureDefinition(PatientProfile.PROFILE_URI); | |||
final ValidationResult result = fhirValidator.validateWithResult(patientDefinition); | |||
assertTrue(result.isSuccessful(), "Should have passed"); | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand why going from success to 2 errors is not a problem?
|
||
List<String> expectedMessages = List.of( | ||
"Found # expecting a token name", | ||
" The slice definition for Patient.identifier has a minimum of 0 but the slices add up to a minimum of 1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For this particular error, we can fix it by adding "min": 1
to DPCPatient.json in the Patient.identifier block.
I'm still trying to figure out the one about "Found # expecting a token name". It seems like the HAPI FHIR validator is having a problem with the StructureDefinition provided by the HAPI FHIR library, but I'm not 100% sure yet.
Either way, we should probably fix the validations that we can fix.
@@ -69,7 +86,7 @@ void testHasReason() { | |||
final ValidationResult result = fhirValidator.validateWithResult(provenance); | |||
|
|||
assertAll(() -> assertFalse(result.isSuccessful(), "Should not have passed"), | |||
() -> assertEquals(1, result.getMessages().size(), "Should have a single message")); | |||
() -> assertEquals(1, result.getMessages().size(), "Should have two messages for reason slice")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This went from one to two errors because the validation now accounts for missing nested values, so the two messages are for Provenance.reason and Provenance.reason.provenance-reason.
dpc-common/src/test/java/gov/cms/dpc/fhir/validations/PatientValidationTest.java
Show resolved
Hide resolved
dpc-common/src/test/java/gov/cms/dpc/fhir/validations/AttestationValidationTest.java
Show resolved
Hide resolved
@@ -63,12 +63,11 @@ void definitionIsValid() { | |||
|
|||
List<String> expectedMessages = List.of( | |||
"Element Provenance.target: derived min (0) cannot be less than the base min (1)", | |||
"The slice definition for Provenance.reason has a minimum of 0 but the slices add up to a minimum of 1", | |||
"The repeating element has a pattern. The pattern will apply to all the repeats (this has not been clear to all users)", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one I don't think is a big deal. Maybe we should exclude errors where the severity is INFORMATION and only consider the ones marked ERROR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
🎫 Ticket
https://jira.cms.gov/browse/DPC-4171
🛠 Changes
The upgrade to 6.x only required an update to one test, so this PR covers two major version upgrades.
ℹ️ Context
🧪 Validation