-
Notifications
You must be signed in to change notification settings - Fork 122
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
Null handling by ProperContains, ProperIncludedIn, ProperIncludes, ProperIn evaluators. More tests for arithmetic functions. #1370
Conversation
…evaluators handling nulls.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1370 +/- ##
============================================
+ Coverage 63.69% 63.74% +0.04%
Complexity 2666 2666
============================================
Files 492 492
Lines 27738 27718 -20
Branches 5511 5503 -8
============================================
- Hits 17669 17668 -1
+ Misses 7828 7820 -8
+ Partials 2241 2230 -11 ☔ View full report in Codecov by Sentry. |
@antvaset - We have a goal of >=80% test coverage for new/updated code. Can you add tests for the Evaluators that gets the patch up to 80%? |
Hi @JPercival, I've squeezed in some more coverage. This is the most we can do I believe. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new ArithmeticFunctionsTests are good, but the changes to the "proper" operators are incorrect. List and interval membership operations throughout the language use "equality semantics, with the exception that nulls are considered equal for the purposes of membership". See the "ProperContains" and "ProperIn" operator descriptions in the Logical Specification: https://cql.hl7.org/04-logicalspecification.html#proper-contains
Thanks for the clarification @brynrhodes. I'm closing this because the fixes to the evaluator logic are incorrect. Also linking here the issue in JIRA https://jira.hl7.org/browse/FHIR-46283 and the pull request with the salvaged arithmetic function tests https://github.com/cqframework/cql-tests/pull/33/files. |
This fixes how the ProperContains, ProperIncludedIn, ProperIncludes, ProperIn evaluators handle nulls. The CQL specification states that:
T properly included in List<T>
expression (ProperIn(T, List<T>)
in ELM) evaluates to null if either argument is null (link).List<T> properly included in List<T>
(ProperIncludedIn(List<T>, List<T>)
in ELM).List<T> properly includes T
(ProperContains(List<T>, T)
in ELM) evaluates to null if either argument is null (link).List<T> properly includes List<T>
(ProperIncludes(List<T>, List<T>)
in ELM).Also adding new tests for arithmetic functions.