Open nmorasb opened 2 months ago
There isn't a 1:1 correspondence between ELM nodes and annotation nodes, the chunks are collapsed. So although not every ELM node will have an entry in the annotation, it should be the case that the annotation nodes cover all the ELM nodes in the chunks. The parent localId 398 in this case is the chunk, and so "includes" the operand localId 397. Does that work to indicate coverage?
Thanks for taking a look @brynrhodes! Sounds like the Annotations are functioning as intended and adjustments should be made in cqm-execution for overly funky cases like this one. We will move forward with making adjustments to cqm-execution.
Here's some additional background on the specific scenario that resulted in this Issue:
The parent localId 398 in this case is the chunk, and so "includes" the operand localId 397. Does that work to indicate coverage?
Not completely. Cqm-execution considers most clauses with a localId property to be a unique clause. Because 398 ("Not") and 397 ("IsNull") both have local Ids, cqm-execution considers both independently when calculating coverage. Hence, Test Case data that results in the target value being not null will count as 398 being covered, but not 397.
The Annotations have "IsNull" (397) collapsed under "Not" (398), so when the "Not" logic is covered, 397 and 398 are highlighted. There is no clear indication in our highlighting that "IsNull" (397) has not been covered except for the <100% coverage value. Adding another test case will complete the coverage, but doing so feels awkward, especially since highlighting will be unchanged.
I see, that makes sense, and yes, that's isn't ideal. Is there a listing of which clauses Cqm-execution does consider unique clauses? Or a principle that we could use to base that decision on?
We have been looking at this same issue from the fqm-execution
side of things. Our current plan to resolve this coverage issue is to calculate coverage by only using localId
s from the statement expression tree that are also found in the annotations. There has always been some disparity between the annotations and the expression tree. Only using the annotation localId
s completely resolves the issue of having invisible coverage deficiencies. Our rationale is that only the visible CQL is able to be acted upon by the tester, and they shouldn’t be docked for something they cannot see.
Hi! We're hoping to get a little clarification on why not all localIds appear in the annotations. In MADiE, we rely on the annotations to re-construct CQL for our execution highlighting feature (for QDM). We finally got around to incorporating a newer version of cqframework and cql-to-elm, and noticed that there are now many more localIds in the ELM than previously. However, only a portion of those appear in the annotations.
For example, for this group:
MADiE looks at the associated annotations:
Because localId 397 does not appear in the annotations, the statement associated with it is identified by the parent localId. When generating our coverage highlighting and comparing against the covered/executed clauses (identified by localId) from the execution output, we're ending up with a handful of uncovered clauses that can't be identified within our highlighting.
So, we'd like to request an enhancement (if feasible) to have the annotations updated so clauses are broken down to the same granular level to include all localIds, or we'd appreciate some feedback on why that may not be possible. Thanks!