-
Notifications
You must be signed in to change notification settings - Fork 4
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
base: main
Are you sure you want to change the base?
Conversation
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.
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"> |
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.
nit: I don't think underscores are used anywhere else for test names in this repo, so consider remaining consistent
<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> |
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.
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)
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.
+1 here if we're going to add this test group we should have a few more edge cases covered
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.