Skip to content

Update tests for Decimal successor/predecessor for 2.0.0 - #147

Open
dehall wants to merge 2 commits into
cqframework:mainfrom
dehall:decimal_successor_v20
Open

Update tests for Decimal successor/predecessor for 2.0.0#147
dehall wants to merge 2 commits into
cqframework:mainfrom
dehall:decimal_successor_v20

Conversation

@dehall

@dehall dehall commented Sep 2, 2026

Copy link
Copy Markdown

CQL 2.0.0 changed the behavior of successor of Decimal and predecessor of Decimal to be aware of the precision of the Decimal. Previously these operators always added/subtracted 0.00000001. This PR marks the existing tests with versionTo="1.5.3" and adds new tests with version="2.0" and the updated behavior.

Note: I looked for other instances of an operator's behavior changing across versions and didn't see anything obvious. The only other instance I see of versionTo is on "DateTimeComponentFromTimezoneOffset" where the operator name changed between 1.3 and 1.4 and so the test name changed.
I point that out to say, I think this PR could set a precedent for this sort of scenario so I encourage the maintainers to be nitpicky. 3 alternatives I considered:

  1. Only keeping one test with the latest version, ie setting version="2.0" and updating the expected result
  2. Two tests, but the tests should use the same name instead of different names
  3. Two tests, but the test names should indicate the version, eg "PredecessorOf1D_v200" or something along those lines

@dehall

dehall commented Sep 2, 2026

Copy link
Copy Markdown
Author

I suppose there's also option 4, add versioning info to the output instead, and the tests could then look something like this:

<test name="PredecessorOf1D" version="1.0">
  <expression>predecessor of 1.0</expression>
  <output versionTo="1.5.3">0.99999999</output>
  <output version="2.0">0.9</output>
</test>

But that's not currently supported by the schema and so is a larger change with more downstream impact

@bryantaustin13 bryantaustin13 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@cmoesel
cmoesel self-requested a review September 9, 2026 14:57

@cmoesel cmoesel left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Approved (on behalf of MITRE's JS cql-execution team).

That said, I noticed that there are predecessor tests for quantities but there are no successor tests for quantities. I'd accept this PR as-is because it is an improvement, but it would be a bonus if you could add successor tests for quantities as well.

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