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

Deduplicate the conditional and date/time operator tests #1399

Merged
merged 2 commits into from
Aug 15, 2024
Merged

Conversation

antvaset
Copy link
Contributor

@antvaset antvaset commented Aug 12, 2024

This PR deduplicates the conditional and date/time operator tests in this codebase.

Conditional operator tests

Removed engine/.../CqlConditionalOperatorsTest.cql, all tests already exist in CqlConditionalOperatorsTest.xml.

Date/time operator tests

Merged all tests from engine/.../CqlDateTimeOperatorsTest.cql into CqlDateTimeOperatorsTest.xml.

Note that:

  1. I updated the DateTimeComponentFromTimezone test to expect it to raise a translation error due to the use of the timezone keyword which is no longer supported. Added the new DateTimeComponentFromTimezone2 test which uses the new timezoneoffset keyword. The CQL library was using the timezoneoffset keyword.
  2. I updated the TimeDurationBetweenHourDiffPrecision test to expect it to raise a translation error due to the syntax error at Z. Added the new TimeDurationBetweenHourDiffPrecision21 test which does not have Z`, as in the CQL library.
  3. The TimeSameOrBeforeHourFalse0 and TimeSameOrBeforeMillisTrue2 already existed in CqlDateTimeOperatorsTest.xml as TimeSameOrBeforeHourTrue2 and TimeSameOrBeforeMillisFalse0 respectively.
  4. I didn't touch the Issue34A, TimeOfDayTest, and Issue34B tests because they should remain as Java-native tests.

Note also that some of the library-based tests used the equivalence semantics when comparing the evaluation result against the expected result. E.g. for this define in the CQL library:

define DateTimeAdd5Years: DateTime(2005, 10, 10) + 5 years

the Java test case was:

var value = engine.expression(library, "DateTimeAdd5Years").value();
assertTrue(EquivalentEvaluator.equivalent(value, new DateTime(bigDecimalZoneOffset, 2010, 10, 10)));

However the corresponding XML test case:

<test name="DateTimeAdd5Years">
    <expression>DateTime(2005, 10, 10) + 5 years</expression>
    <output>@2010-10-10T</output>
</test>

tests that this is CQL evaluates to true which uses the = operator`:

(DateTime(2005, 10, 10) + 5 years) = (@2010-10-10T)

I still consider these tests to be duplicates because the XML tests that we keep use the narrower semantics.

For all the tests that I added to the XML file that were missing, I used CVL in the output unless the test wasn't passing. For example this:

<test name="DateTimeAddThreeWeeks">
    <expression>DateTime(2018, 5, 2) + 3 weeks</expression>
    <output>@2018-05-23</output>
</test>

didn't pass but this did (both with the = and ~ operators):

<test name="DateTimeAddThreeWeeks">
    <expression>DateTime(2018, 5, 2) + 3 weeks = DateTime(2018, 5, 23)</expression>
    <output>true</output>
</test>

so I used the equality operator in the expression for these tests (also for consistency with other existing XML tests).

Also, these tests exist both in the CQL library and XML file but have different expected values:

DateTimeDurationBetweenYear
DateTimeDurationBetweenUncertainInterval
DateTimeDurationBetweenUncertainInterval2
DateTimeDurationBetweenUncertainAdd
DateTimeDurationBetweenUncertainSubtract
DateTimeDurationBetweenUncertainMultiply
DurationInDaysA
DurationInDaysAA

and the XML versions of these tests are currently skipped - something to look into in the future.

All additions to CqlDateTimeOperatorsTest.xml are mirrored in cqframework/cql-tests#47.

Copy link

Formatting check succeeded!

@JPercival JPercival merged commit bc10dbe into master Aug 15, 2024
4 checks passed
@JPercival JPercival deleted the merge-tests branch August 15, 2024 22:04
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