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

Translator 3.7.1 fixes #296

Merged
merged 2 commits into from
Mar 13, 2024
Merged

Translator 3.7.1 fixes #296

merged 2 commits into from
Mar 13, 2024

Conversation

elsaperelli
Copy link
Contributor

Summary

The cql-to-elm translator was updated recently to version 3.7.1. The major change in this version is that there are now localIds on every statement in the ELM. While this is a good change, it requires us to do some updating in our clause coverage calculation due to workarounds and handling of specific cases that were present in previous translator versions that are now changed.

New behavior

Specifically, this PR adds handling in the findAllLocalIdsInStatement function for any TypeSpecifiers and aliases. Hoss detailed in comments in the code what exactly is being changed.

Code changes

  • src/helpers/ClauseResultsHelpers.ts - handling in findAllLocalIdsInStatement that reflects changes made in the translator

Testing guidance

  • npm run check
  • We were sent two measure bundles for MAT6725- one translated using the old translator version and the other using the new translator version. Run detailed results with the test cases they provided with the --debug flag and inspect the clause coverage output. The percentage BEFORE the translator changes on the master branch of fqm-execution should be 99.7%, the percentage AFTER the translator changes on the master branch of fqm-execution should be 57.9% and the percentage AFTER the translator changes on this branch should be 99.5%. Note that it is not back to the previous coverage because there are still issues that we are investigating.
  • Think of other ways we can test this. Using Dylan's updates to the cql-translation-service (Support for Translator 3.7.1 cqframework/cql-translation-service#40), I was able to run the translation service on his branch and retranslate some of the CQL in the unit tests (test/unit/elm/queries) and reran the unit tests with no issues. I also did this for the integration tests.
  • Also using Dylan's cql-translation-service branch and ecqm-bundler, I was able to retranslate CMS165 using the newest translator version. This branch was able to get coverage back to the same as it was pre translator update.

Copy link

github-actions bot commented Mar 8, 2024

Coverage report

St.
Category Percentage Covered / Total
🟢 Statements
85.46% (-0.05% 🔻)
2375/2779
🟡 Branches
73.25% (-0.01% 🔻)
2215/3024
🟢 Functions 88.15% 424/481
🟢 Lines
85.79% (-0.05% 🔻)
2295/2675
Show files with reduced coverage 🔻
St.
File Statements Branches Functions Lines
🟡
... / ClauseResultsHelpers.ts
77.3% (-0.8% 🔻)
86.67% (-1.19% 🔻)
84.62%
76.98% (-0.8% 🔻)

Test suite run success

451 tests passing in 31 suites.

Report generated by 🧪jest coverage report action from 784c804

@lmd59 lmd59 self-requested a review March 11, 2024 19:41
@lmd59 lmd59 self-assigned this Mar 11, 2024
@elsaperelli elsaperelli merged commit 91b3c7c into master Mar 13, 2024
6 checks passed
@elsaperelli elsaperelli deleted the translator-3.7.1-fixes branch March 13, 2024 13:37
elsaperelli added a commit that referenced this pull request Mar 13, 2024
@lmd59 lmd59 restored the translator-3.7.1-fixes branch March 14, 2024 15:20
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