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

Make highlighting backwards compatible with older translator versions #298

Merged
merged 2 commits into from
Mar 21, 2024

Conversation

elsaperelli
Copy link
Contributor

Summary

Our most recent PR addresses updates to clause coverage calculation and highlighting that were necessary to reflect the functionality of the 3.7.1 version of the cql to elm translator. Due to the nature of changes to localId assignment in the translator versions, this broke just the highlighting of some aliases (no affect to clause coverage calculation) for measures that use older translator versions. This PR makes sure that highlighting is backwards compatible with the older translator version (there should be no change to highlighting).

New behavior

I have a comment in the code that details these translator version quirks, but essentially we can't always use the statement.localId if it exists for measures that were translated using the other translator. I was able to narrow this down to just With statements.

Code changes

  • ClauseResultsHelpers.ts - ONLY use statement.localId if it is NOT a with statement. In the older translator version, with statements require the use of the statement.expression.localId + 1. In the new translator version, you will see that these aliases are not highlighted. This is because there is no longer a clear mapping to the localId in the ELM annotation that we want (pending translator fix).

Testing guidance

  • On the master branch, you will see that the MAT6725 Good (pre translator change) bundle and test cases produce 99.7% clause coverage highlighting HOWEVER there are aliases that are unhiglighted. On the 1.3.3 version of fqm-execution, MAT6725 Good (pre translator change) bundle and test cases produce 99.7% clause coverage highlighting with no highlighting issues. This is the desired behavior. On this branch (alias-highlighting-fix), the MAT6725 Good (pre translator change) bundle and test cases should produce the SAME results. as 1.3.3 (99.7% coverage, no highlighting issues).
  • On the master branch, you will see that the MAT6725 Bad (post translator change) bundle and test cases produce 99.5% clause coverage highlighting and there are aliases that are un highlighted. This is to be expected (need translator fix for this). On this branch, we want the same results.
  • Overall, the idea of this PR is that we want to keep the changes that we just merged so that fqm-execution is compatible with the new translator version but we also want results to be the same for measures that use the older translator. The best way to test this is to ensure that the new translator bundles have the fixes but also test a bunch of our older measures and make sure that their results are the same as results on fqm-execution version 1.3.3.

Copy link

github-actions bot commented Mar 13, 2024

Coverage report

St.
Category Percentage Covered / Total
🟢 Statements
85.47% (+0.01% 🔼)
2377/2781
🟡 Branches
73.28% (+0.04% 🔼)
2219/3028
🟢 Functions 88.15% 424/481
🟢 Lines
85.81% (+0.01% 🔼)
2297/2677

Test suite run success

451 tests passing in 31 suites.

Report generated by 🧪jest coverage report action from de0bba6

@elsaperelli elsaperelli requested a review from hossenlopp March 18, 2024 14:42
Copy link
Contributor

@lmd59 lmd59 left a comment

Choose a reason for hiding this comment

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

lgtm!

@hossenlopp hossenlopp merged commit 6c9a3a7 into master Mar 21, 2024
6 checks passed
@hossenlopp hossenlopp deleted the alias-highlighting-fix branch March 21, 2024 15:39
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