Skip to content

Conversation

@vitorpamplona
Copy link
Contributor

@vitorpamplona vitorpamplona commented Apr 23, 2022

Description

Implemented FhirOperator.evaluateLibrary function
Connected the LibraryProcessor to the FhirEngine's local DB, matching evaluateMeasure
Generalized FhirEngineRetrieveProvider to work with any FHIR Model
Added the first androidTests with a simple COVID Immunization check

IMPORTANT: All PRs must be linked to an issue (except for extremely trivial and straightforward changes).

Fixes #[issue number]

Clear and concise code change description.

Alternative(s) considered
Have you considered any alternatives? And if so, why have you chosen the approach in this PR?

Type
Choose one: Feature

Screenshots (if applicable)

Checklist

  • I have read and acknowledged the Code of conduct.
  • I have read the Contributing page.
  • I have signed the Google Individual CLA, or I am covered by my company's Corporate CLA.
  • I have discussed my proposed solution with code owners in the linked issue(s) and we have agreed upon the general approach.
  • I have run ./gradlew spotlessApply and ./gradlew spotlessCheck to check my code follows the style guide of this project.
  • I have run ./gradlew check and ./gradlew connectedCheck to test my changes locally.
  • I have built and run the demo app(s) to verify my change fixes the issue and/or does not break the demo app(s).

@google-cla
Copy link

google-cla bot commented Apr 23, 2022

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

For more information, open the CLA check for this pull request.

Copy link
Contributor

@ktarasenko ktarasenko left a comment

Choose a reason for hiding this comment

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

Amazing, thanks for adding this!

@codecov
Copy link

codecov bot commented Apr 29, 2022

Codecov Report

Merging #1326 (029062e) into master (d806535) will not change coverage.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master    #1326   +/-   ##
=========================================
  Coverage     85.72%   85.72%           
  Complexity      716      716           
=========================================
  Files           149      149           
  Lines         10760    10760           
  Branches        858      858           
=========================================
  Hits           9224     9224           
  Misses         1095     1095           
  Partials        441      441           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d806535...029062e. Read the comment docs.

@vitorpamplona vitorpamplona requested a review from jingtang10 May 16, 2022 20:27
Copy link
Collaborator

@jingtang10 jingtang10 left a comment

Choose a reason for hiding this comment

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

thanks @vitorpamplona - around tests what i'm really concerned about is just that if the tests start failing, will someone be able to step in and debug the test. if the tests are super complex with a lot of artifacts, it'll be super hard. that's why i'm always pushing for minimum tests. thanks for making that change.

additionally, can you add a little more comment on where the cql lib came from and how that's generated etc in the code comment? i know you can't add comment in json, so adding explanatino in the test file is good enough.

otherwise looking good. just have another small nit.

happy to merge if these are addressed.

@vitorpamplona
Copy link
Contributor Author

around tests what i'm really concerned about is just that if the tests start failing, will someone be able to step in and debug the test. if the tests are super complex with a lot of artifacts, it'll be super hard. that's why i'm always pushing for minimum tests. thanks for making that change.

I completely agree. Test structures for most of these CQL evaluation tasks are quite complex. I added PR #1395 to help understand this Test Case. It goes through the steps of Compiling CQL, generating the FHIR Library and making the bundle, which are necessary to call evaluate functions. As you can probably see, these are not really Unit tests. It's more of an integration test than anything else.

I think moving forward, when the CQL compilation becomes available in the SDK, we can redesign these tests to go from reading CQL directly and running on it - masking all the intermediary processes and hopefully facilitating the understanding of what's happening.

Once CQL compilation is available, we can also start breaking tests down to individual evaluation features. Right now it would be too much complexity and duplicated test resources to handle.

* and not exists(I.protocolApplied.seriesDoses.value)
* ```
*/
@Test
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jingtang10 see if this description for the LibraryEvaluate test makes it clear.

Copy link
Collaborator

Choose a reason for hiding this comment

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

thanks this is helpful.

auto-merge was automatically disabled May 23, 2022 13:49

Head branch was pushed to by a user without write access

@jingtang10 jingtang10 enabled auto-merge (squash) May 23, 2022 14:26
auto-merge was automatically disabled May 23, 2022 17:40

Head branch was pushed to by a user without write access

@jingtang10 jingtang10 enabled auto-merge (squash) May 24, 2022 09:05
@jingtang10 jingtang10 merged commit e8bd02a into google:master May 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

5 participants