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 aggregate function tests. Use CVL for expected output. #41

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

antvaset
Copy link
Contributor

@antvaset antvaset commented Aug 1, 2024

This PR adds new conformance tests for aggregate functions. I picked all the ones from https://github.com/cqframework/clinical_quality_language/blob/7e91478/Src/java/engine/src/test/resources/org/opencds/cqf/cql/engine/execution/CqlAggregateFunctionsTest.cql which didn't exist yet here.

Using CVL (literals only) for the expected output values, building on top of #29. Focusing only on the tests/cql/CqlAggregateFunctionsTest.xml tests.

Copy link
Contributor

@suyashkumar suyashkumar left a comment

Choose a reason for hiding this comment

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

LGTM, thanks! Just had a couple random comments.

</group>
<group name="Avg">
<test name="AvgTest1">
<expression>Avg({ 1.0, 2.0, 3.0, 6.0 })</expression>
<output>3.0</output>
</test>
</group>
<group name="Product">
<test name="Product_Long">
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I don't think underscores are used anywhere else for test names in this repo, so consider remaining consistent

Suggested change
<test name="Product_Long">
<test name="ProductLong">

I personally don't feel that strongly though.

<test name="Product_Long">
<expression>Product({5L, 4L, 5L})</expression>
<output>100L</output>
</test>
Copy link
Contributor

Choose a reason for hiding this comment

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

should we add other product tests (e.g. for other overloads, null input, etc) or should that come in another PR? (same comment for the other operators)

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 here if we're going to add this test group we should have a few more edge cases covered

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.

4 participants