Skip to content
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

Add tests to schema config reader and adjust tests for merging schema… #8741

Merged
merged 4 commits into from
Mar 16, 2023

Conversation

jimmyfagan
Copy link
Collaborator

This PR adds a missing piece of the merge between two schemas. Previously, the merge would pull child schema elements into the parent schema, either by adding them straight in, or by merging them into an element within the parent schema, but the name of the resulting merged schema would still reflect the base schema and the constants from the child schema would be ignored. A couple lines of code was enough to fix that behavior.

As part of this, I adjust the unit tests to make sure we're checking the constants and the name of a schema post-merge.

Test Steps:

  • Run fhirdata --input-file src/testIntegration/resources/datatests/FHIR_to_HL7/sample_SR_1_20230302-0001.fhir -s .yml --output-format HL7 --output-file src/testIntegration/resources/datatests/FHIR_to_HL7/sample_SR_1_20230302-0001.hl7 using a yml file that extends another yml file with constants defined at the top level of both and using those constants as part of the conversion.
  • Observe the results to see that the constant values from both the child schema and the parent schema were used.

or

  • Run test test read from file with extends

Changes

  • Edit the merge function in ConfigSchema.kt to allow merging of two schemas' constants and name values
  • Add to ConfigSchemaReaderTests.kt ConverterSchemaTests.kt and FhirTransformSchemaTests.kt to exercise new code and to more closely verify behavior of inherited constants/names.

Checklist

Testing

  • Tested locally?
  • Ran ./prime test or ./gradlew testSmoke against local Docker ReportStream container?
  • Added tests?

Linked Issues

…s; account for constants and names when merging schemas
@jimmyfagan jimmyfagan temporarily deployed to staging March 15, 2023 23:07 — with GitHub Actions Inactive
@github-actions
Copy link
Contributor

github-actions bot commented Mar 15, 2023

Test Results

764 tests  +2   760 ✔️ +2   1m 24s ⏱️ ±0s
  94 suites ±0       4 💤 ±0 
  94 files   ±0       0 ±0 

Results for commit cc3d55d. ± Comparison against base commit 08d683c.

♻️ This comment has been updated with latest results.

@github-actions
Copy link
Contributor

github-actions bot commented Mar 15, 2023

Integration Test Results

124 tests   124 ✔️  2m 26s ⏱️
  12 suites      0 💤
  12 files        0

Results for commit cc3d55d.

♻️ This comment has been updated with latest results.

@jimmyfagan jimmyfagan temporarily deployed to staging March 16, 2023 13:17 — with GitHub Actions Inactive
@jimmyfagan jimmyfagan marked this pull request as ready for review March 16, 2023 13:58
Copy link
Collaborator

@victor-chaparro victor-chaparro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested this with my branch and constants are working, nice! By the way something we tried to do to suppress AOEs was to override a constant in the extension schema. I tested that and it works too, but it may be worth adding a unit test for that, in case we need it in the future.

@jimmyfagan jimmyfagan temporarily deployed to staging March 16, 2023 18:06 — with GitHub Actions Inactive
@jimmyfagan jimmyfagan temporarily deployed to staging March 16, 2023 18:09 — with GitHub Actions Inactive
@jimmyfagan
Copy link
Collaborator Author

Covered Victor's suggestion in this commit, just waiting on another approval to merge.

@jimmyfagan jimmyfagan merged commit a9cab5c into master Mar 16, 2023
@jimmyfagan jimmyfagan deleted the jimmy/8723-fix-merge-schema branch March 16, 2023 19:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants