Add testcase that references an Observation, add test that uses ELM . #1280
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ktarasenko
requested review from
aditya-07,
deepankarb,
jingtang10,
kevinmost,
omarismail94,
stevenckngaa,
vvikramraj and
yigit
as code owners
jingtang10
previously requested changes
Apr 7, 2022
There was a problem hiding this comment.
fantastic progress! thanks so much @ktarasenko
@f-odhiambo can you please also leave a review?
Comment on lines
84
to
85
| // FIXME | ||
| // filter(Patient.ACTIVE, { value = of(true) }) |
There was a problem hiding this comment.
do you need to uncommen this?
There was a problem hiding this comment.
I cleaned up filtering for now
Comment on lines
113
to
114
| // FIXME | ||
| // filter(Patient.ACTIVE, { value = of(true) }) |
| import org.opencds.cqf.cql.evaluator.measure.r4.R4MeasureProcessor | ||
|
|
||
| class FhirOperator(fhirContext: FhirContext, fhirEngine: FhirEngine) { | ||
| class FhirOperator(fhirContext: FhirContext, fhirEngine: FhirEngine, debugLogsOn: Boolean = false) { |
There was a problem hiding this comment.
have you looked into using Timber? wouldn't really want this to be in the public api i think.
There was a problem hiding this comment.
The logging of cql evaluator is a bit tricky. It's also doesn't really log anything, but rather dumps some debug information in memory. I will revert this part to avoid the confusion.
Comment on lines
67
to
70
| val measureReportJSON = | ||
| FhirContext.forR4().newJsonParser().encodeResourceToString(measureReport) | ||
|
|
||
| assertThat(measureReportJSON).isNotNull() |
There was a problem hiding this comment.
this is just testing the serialization? i feel we don't need this.
There was a problem hiding this comment.
right, there should be a special test case for it. Removing
| FhirContext.forR4().newJsonParser().encodeResourceToString(measureReport) | ||
|
|
||
| assertThat(measureReportJSON).isNotNull() | ||
| assertThat(measureReport).isNotNull() |
There was a problem hiding this comment.
this is subsumed by later lines. so i'd vote removing this line.
| assertThat(measureReportJSON).isNotNull() | ||
| assertThat(measureReport).isNotNull() | ||
| assertThat( | ||
| measureReport.evaluatedResource.any { it.reference == "Observation/anc-b6-de17-example" } |
There was a problem hiding this comment.
use of any seems very lenient in test cases. can we harden it?
same with below
ktarasenko
force-pushed
the
kt/evaluateMeasurewithObservation
branch
from
c4ce80f to
1506f92
Compare
ktarasenko
commented
Apr 12, 2022
Comment on lines
84
to
85
| // FIXME | ||
| // filter(Patient.ACTIVE, { value = of(true) }) |
There was a problem hiding this comment.
I cleaned up filtering for now
Comment on lines
113
to
114
| // FIXME | ||
| // filter(Patient.ACTIVE, { value = of(true) }) |
| @@ -21,57 +21,56 @@ import com.google.android.fhir.DatabaseErrorStrategy.UNSPECIFIED | |||
| import com.google.android.fhir.sync.Authenticator | |||
| import com.google.android.fhir.sync.DataSource | |||
|
|
|||
| /** The builder for [FhirEngine] instance */ | |||
| /** The builder for [FhirEngine] instance. */ | |||
There was a problem hiding this comment.
Having FhirEngineProvider as a singleton creates some problems in tests. @epicadk , @jingtang10 do you think it makes sense to convert it to a normal class and move the burden of keeping a single instance of it to clients?
There was a problem hiding this comment.
can you elaborate?
There was a problem hiding this comment.
Workflow library E2E tests require a real instance of the FhirEngine. Currently the only way to create the FhirEngine is through FhirEngineProvider, which maintains a single instance of the FhirServices.
To improve testability, we either need a way to clean up the FhirEngine instance or to have a fresh instance of it for every test (add a way to reset the FhirServices singleton, like i did in this PR).
I'd prefer the second option.
As a next step, given that singletons in android are generally frowned upon, I would suggest to get rid of memoisation part of the FhirEngineProvider and let it create a fresh instance of the FhirEngine every time. This way clients can chose their way of maintaining the single instance of the FhirEngine.
There was a problem hiding this comment.
@jingtang10 actually, the second Robolectric test doesn't work if the DB instance wasn't closed after every test case (in the @after method), so to be able to have all testcases in a single test file, this change is needed to be in this PR.
…irEngineProvider singleton
ktarasenko
force-pushed
the
kt/evaluateMeasurewithObservation
branch
from
e29b4c0 to
ac1218d
Compare
…ithObservation # Conflicts: # engine/src/main/java/com/google/android/fhir/FhirEngineProvider.kt # engine/src/main/java/com/google/android/fhir/db/Database.kt # workflow/src/test/java/com/google/android/fhir/workflow/FhirOperatorTest.kt
ktarasenko
force-pushed
the
kt/evaluateMeasurewithObservation
branch
from
de98911 to
747a3d1
Compare
ktarasenko
force-pushed
the
kt/evaluateMeasurewithObservation
branch
from
1cff4a0 to
46b6193
Compare
|
@jingtang10 @vitorpamplona @f-odhiambo Updated the CL, PTAL |
ktarasenko
force-pushed
the
kt/evaluateMeasurewithObservation
branch
from
46b6193 to
ab47c2e
Compare
f-odhiambo
approved these changes
May 5, 2022
There was a problem hiding this comment.
I run the test based on the newly added resources and the $evaluate_measure works for Individual measure reporting. There is a bug in Population measure reporting after adding multiple patients but I think it is more of a CQL than an API error. Can be sorted in a separate PR. I think we can merge this in now
|
I think this ticket can be merged in as the remaining issue is CQL related - Evaluation fails if we have more than 1 patient being evaluated for ANC Indicator 1 measure reporting. This occurs when you are trying to do a measure reporting evaluation for a population for more than 1 patient Good job @ktarasenko on the effort put on this |
|
@f-odhiambo is there an ETA by when this PR can be merged? |
|
I need access to push an update to Kostia's PR to be reviewed and merged. |
add assertions to the cql test disable populations test
…ithObservation # Conflicts: # workflow/src/main/java/com/google/android/fhir/workflow/FhirEngineRetrieveProvider.kt # workflow/src/main/java/com/google/android/fhir/workflow/FhirOperator.kt
ktarasenko
dismissed
jingtang10’s stale review
approved by f-odhiambo, changes are addessed
jingtang10
approved these changes
May 27, 2022
ktarasenko
changed the title
ktarasenko
force-pushed
the
kt/evaluateMeasurewithObservation
branch
from
4a706cc to
03d3d27
Compare
Codecov Report
@@ Coverage Diff @@
## master #1280 +/- ##
=========================================
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.
|
5 tasks
6 tasks
Tarun-Bhardwaj
added
type:bug
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #1107
Description
Add more complex test case for evaluateMeasure that refrerences an observation and an episode of care without the context value
Alternative(s) considered
Have you considered any alternatives? And if so, why have you chosen the approach in this PR?
Type
Bug fix| Testing
Screenshots (if applicable)
Checklist
./gradlew spotlessApplyand./gradlew spotlessCheckto check my code follows the style guide of this project../gradlew checkand./gradlew connectedCheckto test my changes locally.