-
Notifications
You must be signed in to change notification settings - Fork 6
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
Uncoverage #299
Uncoverage #299
Conversation
Coverage report
Show files with reduced coverage 🔻
Test suite run success451 tests passing in 31 suites. Report generated by 🧪jest coverage report action from 3c91aa0 |
export interface ClauseCoverageDetails { | ||
totalClauses: number; | ||
coveredClauses: number; | ||
uncoveredClauses: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because the other two clauses details are numbers, this naming seems a a bit incongruous. It would lead me to expect all of them to be the same type. Maybe we change the other two to totalClausCount
and coveredClauseCount
or something like that so that it doesn't lead to the expectation that uncoveredClauses
also has a count?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Renamed in ca98110 and added uncovered count
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Woo uncoverage!!! Super excited for this to be a new part of fqm-e :) Code all looked good to me, I would just suggest adding a section about this to the README.
Another suggestion although I don't know how doable/worth it it would be is to include some sort of "hidden" uncovered clause count? For example, when I run this branch on the MAT6725 measure bundle that was created with the new translator, the uncoverage HTML says 2 uncovered clauses but only one clause is highlighted in red. This is because the other uncovered clause is not a part of the HTML and therefore "hidden". Is there a way we could make this known in our debug output somehow? I think it would be extremely helpful in our future work with coverage highlighting and calculation with the new translator. Also happy to make this an additional task if out of scope.
… coverage issues.
…n uncoverage. made it default in cli
…ing to have it's own strategy for highlighting
…b clauses that may have been covered
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still want to do some testing and actually look at the new complementary uncoverage HTML, but I figured I would put up the few comments I have on README wording :)
Yay uncoverage!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an incredible addition to fqm-execution. I did a LOT of testing to look for any discrepancies in the complementary highlighting and to make sure the regular clause coverage highlighting and calculation continued to work the same way. Noting the measures that I tested here for future reference (mostly taken from fqm-execution coverage issues since those measures are the ones that would have benefitted from this feature): CMS104, CMS108, CMS135, CMS144, CMS145, CMS159, CMS165, CMS190, CMS22, CMS334, CMS645, CMS646, CMS69, CMS72, MAT6725.
The only one in which I found an issue with the complementary highlighting was CMS144 and it was the NoBetaBlockerOrdered.medication
piece of CQL which we currently have a backlog task to address and therefore I do not think it is an issue in the scope of this PR (just noting for completeness).
Everything makes sense to me! 👏 🥳
Does the CLI have an option for uncoverage? Can README say something about that? |
This does run in the CLI when you use |
Summary
New option to show which clauses are uncovered as a new set of output. Also adds details on coverage including the list of clauses that were not coverage.
Additionally coverage html calculation was made considerably faster (~200x). This should be VERY noticeable when run with larger sets of patients.
New behavior
New options in calculator that are false by default:
calculateClauseUncoverage
: Calculates HTML that highlights clauses that are not covered by the set of patients.calculateCoverageDetails
: Calculates the total count of covered and uncovered clauses and lists the location of the uncovered clauses.These new options will run in the CLI and the coverageDetails will be output as it's own debug output file. This is intended to help with pinpointing coverage issues as it can tell you exactly where in the ELM there are gaps in the coverage.
Code changes
HTMLBuilder.ts
- Reworked logic for calculating coverage to collect unique lists of covered and uncovered clause results. Reworked coverage highlighting to work off these smaller lists to vastly speed up HTML generation. Added a helper to highlight specifically for uncoverage.Calculator.ts
- Now can handle the two new options and writes out the coverageDetails to a new debug file.types/Calculator.ts
- New types for the new options and output.cli.ts
- DefaultscalculateClauseUncoverage
andcalculateCoverageDetails
to true.Testing guidance
Test with any measures that feel appropriate and look for any differences in coverage highlighting or percentage. Check to make sure that uncoverage highlighting looks sane.