Skip to content
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

Add tests for ProperIn #1394

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Add tests for ProperIn #1394

wants to merge 8 commits into from

Conversation

antvaset
Copy link
Contributor

@antvaset antvaset commented Jul 31, 2024

This PR adds tests for the ProperIn evaluator following the changes to ProperContains in #1393 to align it with the spec. Internally, ProperIn uses the ProperContains logic entirely because the operators are the same except for the order of the arguments.

Copy link

github-actions bot commented Jul 31, 2024

Formatting check succeeded!

Copy link

codecov bot commented Jul 31, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 63.72%. Comparing base (bc10dbe) to head (afe9392).

Additional details and impacted files
@@            Coverage Diff            @@
##             master    #1394   +/-   ##
=========================================
  Coverage     63.72%   63.72%           
  Complexity     2665     2665           
=========================================
  Files           493      493           
  Lines         27774    27774           
  Branches       5521     5521           
=========================================
  Hits          17700    17700           
  Misses         7831     7831           
  Partials       2243     2243           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@antvaset
Copy link
Contributor Author

Depends on cqframework/cql-tests#40

@antvaset antvaset marked this pull request as ready for review July 31, 2024 05:25
Base automatically changed from update-proper-contains-evaluator-implementation to master August 1, 2024 20:11
@JPercival JPercival enabled auto-merge (squash) August 1, 2024 20:18
</test>
<test name="ProperIn9">
<expression>'a' properly included in { 'a', null }</expression>
<output>null</output>
Copy link
Member

Choose a reason for hiding this comment

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

Seems like this should be true?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @brynrhodes my reading of the spec https://cql.hl7.org/04-logicalspecification.html#proper-in:

For the T, List overload, this operator returns [true] if the given element is in the given list, and it is not the only element in the list, using equality semantics, with the exception that null elements are considered equal. If the first argument is null, the result is true if the list contains any null elements and at least one other element, and false otherwise. If the second argument is null, the result is false.

is that:

null properly included in { 'a', null } = true // nulls are equal and there is at least one non-null in the list (spec is explicit)
null properly included in { null, null } = false // nulls are equal but there are no non-nulls in the list (spec is explicit)
'a' properly included in { 'a', 'b' } = true // the list has at least one element for which Equals(x, y) evaluates to true and at least one for which Equals(x, y) evaluates to false (spec is explicit)
'a' properly included in { 'a', 'b', null } = true // same as above
'a' properly included in { 'a', 'a' } = false // the list has at least one element for which Equals(x, y) evaluates to true, no elements for which Equals(x, y) evaluates to false, and no elements for which Equals(x, y) evaluates to null (spec is only explicit that the result is not true)
'a' properly included in { 'a', null } = null // the list has at least one element for which Equals(x, y) evaluates to true, no elements for which Equals(x, y) evaluates to false, but there are elements for which Equals(x, y) evaluates to null (spec is only explicit that the result is not true)

This is consistent with the existing null-valued ProperInTimeNull test in the same file that covers the ProperIn(T, Interval<T>) overload, indicating that we can return nulls from ProperIn, not just true and false.

What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also whichever we go with, I'll add the short explainers as comments to the XMLs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants