Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Create integration test for the FHIR converter #826

Merged
merged 51 commits into from
Sep 13, 2023

Conversation

emmastephenson
Copy link
Collaborator

@emmastephenson emmastephenson commented Sep 12, 2023

PULL REQUEST

Summary

Add a new integration test for the FHIR converter, and update our test workflows to ensure integration tests run on CI.

Related Issue

Fixes #438

Additional Information

This integration test uses a library called testcontainers, which has a bunch of different options, but the one we care about is the docker-compose module. This lets you spin up a docker-compose script inside a test, which you can then hit with requests. This does appear to cache the container between runs, at least locally, so the first test takes a while but they get faster subsequently assuming you don't change the container.

There's also some fun tricks happening with pytest here. One is marking the tests so they don't need to be executed per run, and the other is the setup/teardown aspect. They're currently configured to set up once, at the beginning of the test suite, and tear down only after all the tests have executed.

Finally, there's a new step of the testing process - integration-test-python-containers. A fun quirk of the way we run pytest is that if there are no tests found, the Github check fails. The way I'm getting around this is to specify that integration tests need to be in an integration folder.

For local testing - if you just run pytest everything will execute. If you run pytest -m "not integration" integration tests won't run; if you run pytest -m "integration" only integration tests will execute. (Will put this info in the wiki)

Background

@emmastephenson emmastephenson changed the base branch from main to emma/737-part-2 September 12, 2023 21:28
@emmastephenson emmastephenson changed the base branch from emma/737-part-2 to main September 12, 2023 21:29
}
vxu_conversion_response = httpx.post(CONVERT_TO_FHIR, json=request)

assert vxu_conversion_response.status_code == 200
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

On all these integration tests, I'm not doing anything except verifying that the status is 200 and that there's a bundle in the body. I had originally hoped to create golden files for these tests and just assert response == golden, but each of the IDs in the bundle will be different on each run :/ Might be something to solve with the FHIR resource library.

Copy link
Contributor

Choose a reason for hiding this comment

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

We should have 'golden' files for the ECR conversions already. I think this will be sufficient for now.

@emmastephenson emmastephenson changed the base branch from main to emma/737-part-2 September 13, 2023 19:14
@emmastephenson emmastephenson changed the base branch from emma/737-part-2 to main September 13, 2023 19:14
Copy link
Collaborator

@DanPaseltiner DanPaseltiner left a comment

Choose a reason for hiding this comment

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

Really exciting to see us finally implementing this kind of testing! 🚀

Copy link
Contributor

@BradySkylight BradySkylight left a comment

Choose a reason for hiding this comment

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

This is awesome! We do have the before and after for Eicr conversion though. Both the RR and the EICR and the EICR with the RR added in, as well as the expected converted JSON file. This way you can test both steps the rr extraction and input into eicr as well as the full conversion.

@emmastephenson
Copy link
Collaborator Author

This is awesome! We do have the before and after for Eicr conversion though. Both the RR and the EICR and the EICR with the RR added in, as well as the expected converted JSON file. This way you can test both steps the rr extraction and input into eicr as well as the full conversion.

@BradySkylight yeah, I was really hoping to make this approach work - my original plan was to use the original EICR/RR files and the expected conversions as the goldens and just verify that the response matched. Unfortunately, the FHIR conversion doesn't generate consistent outputs each time - the Patient ID and timestamps change across each execution, so the comparisons always fail. I'm hoping there's some kind of pytest tooling that allows you to ignore certain variables in a golden, but I think that might be better suited for a followup PR.

@BradySkylight
Copy link
Contributor

This is awesome! We do have the before and after for Eicr conversion though. Both the RR and the EICR and the EICR with the RR added in, as well as the expected converted JSON file. This way you can test both steps the rr extraction and input into eicr as well as the full conversion.

@BradySkylight yeah, I was really hoping to make this approach work - my original plan was to use the original EICR/RR files and the expected conversions as the goldens and just verify that the response matched. Unfortunately, the FHIR conversion doesn't generate consistent outputs each time - the Patient ID and timestamps change across each execution, so the comparisons always fail. I'm hoping there's some kind of pytest tooling that allows you to ignore certain variables in a golden, but I think that might be better suited for a followup PR.

That is strange. I may need to look into that...perhaps because it's making a UUID each time? Once I get this MPI stuff worked out, maybe we can look into this together.

@emmastephenson
Copy link
Collaborator Author

This is awesome! We do have the before and after for Eicr conversion though. Both the RR and the EICR and the EICR with the RR added in, as well as the expected converted JSON file. This way you can test both steps the rr extraction and input into eicr as well as the full conversion.

@BradySkylight yeah, I was really hoping to make this approach work - my original plan was to use the original EICR/RR files and the expected conversions as the goldens and just verify that the response matched. Unfortunately, the FHIR conversion doesn't generate consistent outputs each time - the Patient ID and timestamps change across each execution, so the comparisons always fail. I'm hoping there's some kind of pytest tooling that allows you to ignore certain variables in a golden, but I think that might be better suited for a followup PR.

That is strange. I may need to look into that...perhaps because it's making a UUID each time? Once I get this MPI stuff worked out, maybe we can look into this together.

@BradySkylight yeah, I think it is the Patient UUID in particular making it non-idempotent. Would be happy to look at it with you later!

@emmastephenson emmastephenson merged commit 1ff2cdf into main Sep 13, 2023
@emmastephenson emmastephenson deleted the emma/438-fhir-converter branch September 13, 2023 22:15
@nickclyde nickclyde mentioned this pull request Sep 26, 2023
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[SPIKE] Integration tests for containers
3 participants