-
Notifications
You must be signed in to change notification settings - Fork 123
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
Question about differences between localIds in ELM and annotations #1417
Comments
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:
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 |
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!
The text was updated successfully, but these errors were encountered: