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

Coverage 3.14.0 #314

Merged
merged 4 commits into from
Oct 22, 2024
Merged

Coverage 3.14.0 #314

merged 4 commits into from
Oct 22, 2024

Conversation

lmd59
Copy link
Contributor

@lmd59 lmd59 commented Oct 8, 2024

Summary

Implements an annotation-based coverage approach that only uses local ids that are also in the annotation (and therefore can be seen in the cql coverage highlighting). This mitigates updates in version 3.14.0 of the translator which added many additional local ids that could otherwise impact coverage.

New behavior

Coverage calculations will be approximately the same as for translator 3.3.2. There may be some very small differences in in coverage percentages since this is a new coverage approach, but any coverage gaps should be identifiable for the end user.

Code changes

  • Small README updates
  • ClauseResultsHelpers.ts Update to add findAnnotationLocalIds function that identifies all "r"-keyed local ids within an annotation structure. Uses this to limit the final localIds to the ones that can be found in the annotation.
  • ClauseResultsHelpers.test.ts Updates to "Not Equivalent" and "Not Equal" and addition of a "Not Null" unit test to align with new localid approach
  • Adds 3.15.0 folder for new translations of existing easily translatable cql unit test fixtures (some existing cql fixtures do not have the right dependency library versions to be easily re-translated)

Testing guidance

Copy link

github-actions bot commented Oct 8, 2024

Coverage report

St.
Category Percentage Covered / Total
🟢 Statements
85.52% (+0.11% 🔼)
2440/2853
🟡 Branches
73.26% (+0.08% 🔼)
2293/3130
🟢 Functions
87.75% (+0.07% 🔼)
437/498
🟢 Lines
85.82% (+0.1% 🔼)
2355/2744

Test suite run success

460 tests passing in 31 suites.

Report generated by 🧪jest coverage report action from 939ca5f

Copy link
Contributor

@hossenlopp hossenlopp left a comment

Choose a reason for hiding this comment

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

Small thought on typescript types.

src/helpers/ClauseResultsHelpers.ts Outdated Show resolved Hide resolved
Copy link
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.

I am not sure how much we care about this, but we should at the very least note it in the release notes. For CMS646, I noticed that the highlighting for v3.3.2 and v3.14.0 are slightly different. While the percentage is the same, looks like some aliases no longer get highlighted. Could be worth investigating if there is a fix and if not, at least noting it.

For example:
image

@lmd59
Copy link
Contributor Author

lmd59 commented Oct 16, 2024

I am not sure how much we care about this, but we should at the very least note it in the release notes. For CMS646, I noticed that the highlighting for v3.3.2 and v3.14.0 are slightly different. While the percentage is the same, looks like some aliases no longer get highlighted. Could be worth investigating if there is a fix and if not, at least noting it.

For example: image

So it looks like you're seeing a change that was caused by the translator update and not by this PR as 3.3.2-translated measure results before and after this PR change have the same highlighting (alias highlighted), and 3.14.0-translated measure results before and after this PR change have the same highlighting (alias un-highlighted).
Since this PR's task aims to resolve coverage and address the linked ticket, addressing highlighting may be out of scope (though the ticket does reference #285 as well, which is a highlighting issue).
I think we may also want to address highlighting separately because the fix is more likely to break backwards compatibility. Essentially, we have workarounds for highlighting aliases that use a plus 1 increment to identify the correct localid association (since aliases are identified within the annotation but not within the ELM). Initial testing shows that the relationships between the correct localids are at least sometimes a +2. Especially if we are aiming for backwards compatibility, this may take a new approach.

Copy link
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 small comments but functionality works great! Definitely want to wait to get the correct version of CMS145 to test.

src/helpers/ClauseResultsHelpers.ts Outdated Show resolved Hide resolved
src/helpers/ClauseResultsHelpers.ts Outdated Show resolved Hide resolved
src/helpers/ClauseResultsHelpers.ts Outdated Show resolved Hide resolved
test/unit/ClauseResultsHelper.test.ts Outdated Show resolved Hide resolved
Copy link
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.

Did a lot of testing on Friday (separate from the task that we have to do additional testing on this) and didn't find any inconsistencies.

Lgtm! 🥳

@hossenlopp hossenlopp self-requested a review October 22, 2024 17:15
@hossenlopp hossenlopp merged commit 6811b6c into master Oct 22, 2024
6 checks passed
@hossenlopp hossenlopp deleted the coverage-3.14.0 branch October 22, 2024 17:16
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