Closed elsaperelli closed 1 year ago
St.:grey_question: |
Category | Percentage | Covered / Total |
---|---|---|---|
🟢 | Statements | 85.04% (+0.04% 🔼) |
1996/2347 |
🟡 | Branches | 73.79% (+0.14% 🔼) |
1768/2396 |
🟢 | Functions | 87.13% (+0.03% 🔼) |
352/404 |
🟢 | Lines | 85.33% (+0.04% 🔼) |
1926/2257 |
366 tests passing in 28 suites.
Report generated by 🧪jest coverage report action from fd67bf485ff3cf4a409cc66e45120c181353b814
I'm not an official reviewer on this, but I wanted to leave a comment to give kudos for such a well-written PR description. The testing guidance is very detailed, and linking all the related Github issues will make it easy for us to use this as a reference for why we implemented this workaround. Good work!
Summary
Fixes #202 Even though we deduced that this error was the result of how the cql-to-elm translator works (see cql-to-elm translator issue here), this PR provides a work around for this issue in fqm-execution (see Matt's explanation on the fqm-execution issue here).
New behavior
The cql-to-elm translator causes the ELM Binary Expressions where one side of the comparison is of type Literal to result in the operator (in the case of the issue,
<=
) being grouped in with the literal it is being compared to. Regardless of whether or not this is intentional or a mistake, this PR creates a workaround that makes sure the expression with the literal and comparison operator in it does not use its localId, but rather the localId of the actual comparison expression itself. Doing so makes sure that the highlighting is correct. In order to do this, special handling is added for cql comparison operations.Code changes
src/calculation/ClauseResultsHelpers.ts
- Special case handling for when the statement is a ELM Binary Expression with a Comparison Operator infindAllLocalIdsInStatement
that calls a new functionfindLocalIdsForComparisonOperators
that handles this case.test/unit/ClauseResultsHelper.test.ts
- Two new unit tests to test the new special case handling.test/unit/fixtures/elm/ComparisonWithLiteral.json
/test/unit/fixtures/elm/ComparisonWithoutLiteral.json
- Two new fixtures that are used in the new unit tests. Are these okay to include? If not, is there a better way to function test this function?Testing guidance
npm run check
fqm-execution
. It includes ahighlighting-issue
folder,age-highlighting.ts
file, and arun.sh
script. Thehighlighting-issue
folder contains two patient bundles,patient-bundle-fail.json
has a patient who is younger than 66 andpatient-bundle-pass.json
has a patient who is older than 66,literal-on-left.json
elm json file where the CQL comparison expression has the literal on the left side,literal-on-right.json
elm json file where the CQL comparison expression has the literal on the right side (the example in the issue),highlightIssue.cql
file that contains minimum cql to replicate the issue, andFHIRHelpers.cql
. I included the last two files incase anyone wanted to test another situation and bundle it using ecqm-bundler../run.sh
This runs the detailed calculation on four different measure/patient bundle combinations and puts the resulting logic highlighting html in a folder called html.cd html
right-fail.html
left-fail.html
These both make sense because the patient in
patient-bundle-fail.json
is younger than 66, so we want to make sure that the comparison is highlighted in red. This is the solution that was agreed upon in the issue thread.right-pass.html
&left-pass.html
This makes sense because the patient in
patient-bundle-pass.json
is older than 66 so it should be highlighted green.npm run cli -- detailed -m highlight-issue/literal-on-right.json -p highlight-issue/patient-bundle-fail.json -o --debug -s 2023-01-01 -e 2023-12-31
age-highlighting-testing.zip