Skip to content

Update Spec Tests and Fix Newly Found Issues#348

Merged
hossenlopp merged 15 commits into
masterfrom
update-spec-tests
Aug 13, 2025
Merged

Update Spec Tests and Fix Newly Found Issues#348
hossenlopp merged 15 commits into
masterfrom
update-spec-tests

Conversation

@cmoesel
Copy link
Copy Markdown
Member

@cmoesel cmoesel commented Aug 9, 2025

This PR updates the spec-tests to mirror the latest tests in cqframework/cql-tests and fixes some of the issues that these tests reported. This brings us in closer alignment with other engines, surfaces possible areas of disagreement (see skip-list.txt), and highlights unimplemented features (also see skip-list.txt). Changes include:

  • Add a README.md to test/spec-tests.
  • Copy over all test cases and the test schema from cqframework/cql-tests.
  • Update the test builder to handle the new <library> tag gracefully. It currently skips the one test that has a <library> tag since we haven't implemented that yet.
  • Update skip-list.txt to remove skips that are now passing, add new skips as needed, and re-categorize other skips as appropriate.
  • Update AllTrue logic to return true if the items list is null.
    • E.g., AllTrue(null) is now true.
  • Update AnyTrue logic to return false if the items list is null.
    • E.g., AnyTrue(null) is now false.
  • Update In and Contains overloads to treat null items as equal.
    • E.g., null in {null} and {null} contains null are now true.
  • Update Includes and IncludedIn point/list overloads to treat null items as equal.
    • E.g., {null} includes null and null included in {null} are now true.
  • Update Includes and IncludedIn list/list overloads to treat null items inside of lists as equal.
    • E.g., {null} includes {null} and {null} included in {null} are now true.
  • Update Includes and IncludedIn list/list overloads to return null if either list argument is null.
    • E.g., null as List<String> includes {'a'}, {'a'} includes null as List<String>, null as List<String> included in {'a'}, {'a'} included in null as List<String> are now all null.
  • Update ReplaceMatches to support escaping $ in the substitution by using \$.
    • E.g., ReplaceMatches('All that glitters is not gold', '\\s', '\\$') is 'All$that$glitters$is$not$gold'.
  • Update aggregation queries to not be distinct by default.
    • E.g., ({1, 2, 3, 3, 4}) L aggregate A starting 1: A * L is 72 (not 24).

To test:

  • Run npm run build:all and verify there are no changes
  • Run npm test and verify tests pass

Fixes #318

Submitter:

  • This pull request describes why these changes were made
  • Code diff has been done and been reviewed (it does not contain: additional white space, not applicable code changes, debug statements, etc.)
  • Tests are included and test edge cases
  • Tests have been run locally and pass
  • Code coverage has not gone down and all code touched or added is covered.
  • Code passes lint and prettier (hint: use npm run test:plus to run tests, lint, and prettier)
  • [N/A] All dependent libraries are appropriately updated or have a corresponding PR related to this change
  • cql4browsers.js built with npm run build:browserify if source changed.

Reviewer:

Name: @hossenlopp

  • Code is maintainable and reusable, reuses existing code and infrastructure where appropriate, and accomplishes the task’s purpose
  • The tests appropriately test the new code, including edge cases
  • You have tried to break the code

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Aug 9, 2025

Codecov Report

❌ Patch coverage is 90.90909% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 87.47%. Comparing base (88c80e7) to head (31ff87d).
⚠️ Report is 9 commits behind head on master.

Files with missing lines Patch % Lines
src/elm/list.ts 66.66% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #348      +/-   ##
==========================================
+ Coverage   87.40%   87.47%   +0.06%     
==========================================
  Files          52       52              
  Lines        4558     4566       +8     
  Branches     1288     1291       +3     
==========================================
+ Hits         3984     3994      +10     
+ Misses        361      358       -3     
- Partials      213      214       +1     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@cmoesel cmoesel changed the title Update Spec Tests and Fix Issues Update Spec Tests and Fix Newly Found Issues Aug 9, 2025
cmoesel added 15 commits August 11, 2025 12:11
The spec-tests runner was never updated to use the async exec. As a result, it always passed all tests (because, apparently, unresolved promises are always equal). This updates the runner to properly await each exec.

This also skips tests that are now failing due to translator issues or unimplemented CQL features. It also fixes the implementation of includes/included in for the overload with two lists, which was returning the wrong result when the second list was null.
Update the spec-tests based on the latest versions at cqframework/cql-tests. Also update the generator to more gracefully handle the new <library> tag.

Note that there are now broken tests that will be addressed in a future commit.
Remove any skipped tests that are now passing and recategorize any that fail for different reasons.
Copy link
Copy Markdown
Contributor

@elsaperelli elsaperelli left a comment

Choose a reason for hiding this comment

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

Just a few comments to get input from @hossenlopp on, otherwise looks great!

Comment thread src/elm/aggregate.ts
Comment thread src/elm/aggregate.ts
Comment thread test/spec-tests/cql/CqlAggregateFunctionsTest.cql
Comment thread test/spec-tests/cql/CqlAggregateFunctionsTest.cql
@hossenlopp hossenlopp merged commit 9672631 into master Aug 13, 2025
8 checks passed
@cmoesel cmoesel deleted the update-spec-tests branch August 19, 2025 18:44
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.

Integrate updated CQL tests and fix bugs as necessary

4 participants