fix(report): coverage for variables, their descendants, and others

[galtm]

This pull request does more than I originally set out to do, but I didn't want to leave things in a worse state than before. I'll try to explain what the pieces are to make them clearer.

Original goal: Remove special-case coverage logic for xsl:variable that either masked coverage treatment of descendants or produced incorrect coverage status for global variables. @birdya22, you also noticed that they are problematic or suspicious, in comment https://github.com/xspec/xspec/issues/1921#issuecomment-2085595862 ("2 variable tests").

Commits in this PR:

• ccb999a4231f9c344c7ec3b6c17bbb357fb2a57b removes the special-case logic that considers all descendants of a "hit" variable to be hit, too. Removing this logic affected several coverage reports in the test suite. The changes in the report for the xsl:variable test are welcome corrections. The other changes in the coverage test reports are incorrect status values due to unrelated known issues; that's what the next 5 commits in this pull request are trying to correct.
• b191a87d5a1d060cb47066625b8e5455d2daeade applies the Use Child Data rule to xsl:perform-sort and the Use Parent Status rule to xsl:sort. The xslt-code-coverage-by-element.md document doesn't have a specific proposal for xsl:perform-sort, so this is a new proposal. For xsl:sort, the document proposes the Use Parent Data rule for children of xsl:apply-templates, xsl:for-each, and xsl-for-each-group. Because these parent elements are traced, Use Parent Data and Use Parent Status produce the same effects. When xsl:sort is a child of xsl:perform-sort, which is not traced, the Use Parent Status rule aligns the status of xsl:perform-sort and xsl:sort. For examples, see line 54 in xsl-sort-01-coverage.html (executed xsl:sort element is marked "unknown" rather than "missed") and line 23 in xsl-perform-sort-01-coverage.html (executed xsl:sort element is marked "hit" rather than "missed").
• 6c53b6fff470dbf51296cd773e6b9081fccd8c3d applies the Unknown rule for xsl:map and xsl:map-entry as tentatively suggested in xslt-code-coverage-by-element.md, but then 76d97f4caac3577c78e87009a091572d9edf8027 changes them to Use Child Data instead. The difference occurs when one of these elements has a sequence constructor that includes a node that has a hit in the trace. It should be safe to conclude from such a hit that the ancestor xsl:map or ancestor xsl:map-entry was also executed. For examples, see lines 18 and 23 in xsl-map-01-coverage.html. (Lines 20 and 21 still say "unknown" and I'm not sure that trying to change that is worth the added complexity.)
• 9a0b7deaa70960099d60149c244b4bac4e8cdf10 applies the Use Child Data rule to xsl:evaluate. This rule usually yields 'unknown', but if xsl:evaluate/xsl:with-param has a sequence constructor with a hit in the trace, xsl:evaluate will get the 'hit' status. (I hesitate to work around the column 0 in Saxon 12.4 by making assumptions about how many instructions are on the same line of the file, plus Saxon 12.5 doesn't trace xsl:evaluate at all.) For examples, see lines 31 and 32 in xsl-evaluate-01-coverage.html, where xsl:evaluate and xsl:with-param are marked as "hit" because their descendant xsl:for-each element has a hit in the trace. In xsl-with-param-01-coverage.html, lines 69 and 70 show both elements marked as "unknown" because there's no descendant data, while lines 46-48 show both elements marked as "missed" because the only traceable descendant is a text node that doesn't have a hit in the trace (Aside: it should have a hit, and maybe will have one when we switch to the linked tree model).
• fb6851b032491a284f9aedd1725a31e8f97d0ae6 applies the Use Parent Status to xsl:with-param. This is slightly different from Use Parent Data (suggested in #1947) when the parent is the untraced xsl:evaluate element. Examples are as in the preceding list item.
• 5ab2feb3275355e08227beb14b07483e66c732f8 removes special-case treatment of global variables, causing them to follow the Use Trace Data rule instead of using the status of a sibling. Note: For now, I left in place the special-case treatment of local variables because when I removed that behavior, in all 7 of the test cases that showed a change, the change was from "hit" to "missed" for a variable that was, in fact, used.

@birdya22, @cmarchand, and any other reviewers: Let me know what you think of not only the code implementation, but any design changes that aren't what the xslt-code-coverage-by-element.md document or open issues currently propose.

---

Fixes #1947

[galtm]

Saxon 12.5 with these changes

On a separate local branch, I tried the following to see how these changes worked with Saxon 12.5:

1. Started from the same commit as this pull request, based on #1975
2. Bumped Saxon from 12.4 to 12.5 in test\ci\env\saxon-12.env and ran test\ci\install-deps.cmd
3. Made the Java change that I think #1971 is suggesting, to add the getLocation method, and recompiled
4. Regenerated all the coverage test expected results (Saxon 12.5 baseline)
5. Cherry-picked the commits from this pull request to that other branch
6. Regenerated all the coverage test expected results to see what effect this PR's changes have

There are differences in 5 HTML coverage reports, but I don't think any of them make the XSLT or design changes in this pull request less valid.

• In xsl-evaluate-01-coverage.html, xsl:for-each on line 33 is missed instead of hit, while its child xsl:sequence is hit instead of missed. Trading one issue for another just happens to preserve what this part of the test is trying to do.
• In several spots among xsl-merge-01-coverage.html, xsl-next-match-01-coverage.html, and xsl-variable-01-coverage.html, text nodes are missed instead of hit. Maybe using the linked tree model will help? I didn't try it on this branch.
• In xsl-map-01-coverage.html, some untraced xsl:copy instructions (a known 12.5 issue) cause ancestor xsl:map and xsl:map-entry elements to show the wrong status.

[birdya22]

A few points: Point 1) It will take me a couple of days to digest all of this and have a look at it.

In several spots among xsl-merge-01-coverage.html, xsl-next-match-01-coverage.html, and xsl-variable-01-coverage.html, text nodes are missed instead of hit. Maybe using the linked tree model will help? I didn't try it on this branch.

I would expect it to. You could try my tip from PR1936 to check. Duplicated here: Using the LinkedTree causes correct column numbers to be produced in the coverage-report.xsl stylesheet. As a check I added the following attribute where the span element is created to check the column number as it appeared in coverage-report.xsl: title="{name($node) || ':' || x:line-number($node) || ':' || x:column-number($node)}"

Point 2)

In xsl-map-01-coverage.html, some untraced xsl:copy instructions (a known 12.5 issue) cause ancestor xsl:map and xsl:map-entry elements to show the wrong status.

Related to this, I wrote the following at the top of the xsl-map-01.xsl test:

Copy this variable as an alternative to using xsl:sequence inside a xsl:map-entry. This is on the basis that xsl:sequence is not traced in Saxon 12.4 so it isn't possible to tell if child nodes are traced inside xsl:map-entry.

This was fine for Saxon 12.4 where xsl:copy was traced, but turns out to be a bad decision for Saxon 12.5. It might be worth choosing another element that isn't xsl:sequence or xsl:copy within the test.

Point 3) In Issue #1958 I shared a diagnostic version of the coverage-report I'd produced. It required a little tweak to be able to copy and paste most of the code into the standard coverage-repport.xsl to get details of the trace file to appear on the right of the stylesheet text. If you would like, I can provide my updated version and explain what to copy and paste into your local coverage-report.xsl if you would like to see a summary of the trace details in your local coverage-report.

[galtm]

Point 1) It will take me a couple of days to digest all of this and have a look at it.

No problem. :) Likewise, it will take me some time to digest some of your recent work (including Point 3), especially since I have some non-code-coverage things coming up soon.

Re point 2, I should be able to find something that will accomplish the same testing purpose in this file with both 12.4 and 12.5.

[birdya22]

A comment on xsl:sort. Lines 19, 35 and 48 don't look good (xsl:value-of within a xsl:sort as 'missed'). The xslt-code-coverage-by-element.md document says 'When using a sequence constructor with xsl:sort, the children are not traced.' I checked this again with something more than a single element and it appears the sequence constructor isn't traced if it is a single element, but more complex element are traced:

         <xsl:sort>
          <xsl:value-of>
            <xsl:text>100</xsl:text> 'Traced'
          </xsl:value-of>
        </xsl:sort>

and

          <xsl:choose>                               'Traced'
            <xsl:when test="1 eq 2">
              <xsl:value-of select="." />
            </xsl:when>
            <xsl:otherwise>
              <xsl:value-of select="." />    'Traced'
            </xsl:otherwise>
          </xsl:choose>

There needs to be a rule if there is a single child element of xsl:sort - should it be 'unknown' or could it be 'hit'? Note: In xsl-evaluate-01.xsl line 20 there is a xsl:evaluate element as the single element of xsl:sort.

[birdya22]

A separate comment on text nodes and a bit of a change in direction from me, which I have not tested. For completeness I'm including a description of text node handling, including details on how code coverage works.

When testing a stylesheet the XSLTCoverageTraceListener is used to produce a file, coverage.xml, of the elements Saxon traces. Saxon does not trace text nodes, so they don't appear in the coverage.xml file.

When formatting the coverage report the coverage-report.xsl iterates through the stylesheet being tested, and using the line and column data from Saxon (via the XSpec extension functions) checks to see if there is a hit element in the coverage.xml file that matches the current node's line and column values (this is ignoring any element specific rules).

In Saxon 12.4 there was an issue that Saxon didn't always report the correct column number for a text node within coverage-report.xsl. Generally it reported the column number as the start of the text node, but in some circumstances it didn't. Saxonica issue 6405 was raised which resulted in a change in Saxon 12.5 to report the column number at the end of the text node (which is not useful for XSpec). This was when using the Tiny tree method.

Within that Saxonica issue there was discussion about the Linked tree model with Michael Kay saying:

Currently with the linked tree, the getLineNumber() and getColumnNumber() methods for a text node return the line number of the parent element, which in principle can be a long distance away.

Because the Linked tree provided more consistent results in 12.4 PR #1936 was raised to change XSpec to use the Linked tree when running the coverage-repoprt.xsl stylesheet. This required a change to the saxon-config.xml used by XSpec.

My suggestion is to stick with the Tiny tree, abandon PR #1936, and implement a rule for text nodes which is Use Parent Status/Data (not sure which yet). This will basically follow what the Linked tree does i.e. use the details from the parent node and not use the column number for text nodes.

[galtm]

A comment on xsl:sort. Lines 19, 35 and 48 don't look good (xsl:value-of within a xsl:sort as 'missed').

I agree.

it appears the sequence constructor isn't traced if it is a single element, but more complex element are traced:

I am having trouble discerning the logic for hits of descendants of xsl:sort. Sometimes, the sequence constructor is traced when it's a single element.

<xsl:apply-templates mode="sortMode">
  <xsl:sort>
    <xsl:apply-templates select="nonexistent" /><!-- hit in trace -->
  </xsl:sort>
</xsl:apply-templates>

Sometimes, when the sequence constructor is multiple sibling elements, some are traced and some aren't.

<xsl:apply-templates mode="sortMode">
  <xsl:sort>
    <xsl:value-of select="." /><!-- hit in trace -->
    <xsl:copy-of select="()" /><!-- no hit in trace -->
    <xsl:copy-of select="()" /><!-- no hit in trace -->
  </xsl:sort>
</xsl:apply-templates>
<xsl:apply-templates mode="sortMode">
  <xsl:sort>
    <xsl:copy-of select="()" /><!-- hit in trace -->
    <xsl:copy-of select="()" /><!-- hit in trace -->
    <xsl:value-of select="." /><!-- hit in trace -->
    <xsl:copy-of select="()" /><!-- hit in trace -->
    <xsl:copy-of select="()" /><!-- hit in trace -->
  </xsl:sort>
</xsl:apply-templates>

I don't understand why cases that appear similar come out differently, such as the pair above and the pair below. Maybe there is some optimization effect?

<xsl:apply-templates mode="sortMode">
  <xsl:sort>
    <xsl:value-of>               <!-- no hit in trace -->
      <xsl:value-of select="." /><!-- hit in trace -->
      <xsl:copy-of select="()" /><!-- no hit in trace -->
    </xsl:value-of>
  </xsl:sort>
</xsl:apply-templates>
<xsl:apply-templates mode="sortMode">
  <xsl:sort>
    <xsl:value-of>                        <!-- no hit in trace -->
      <xsl:value-of select="." />         <!-- hit in trace -->
      <xsl:copy-of select="()" />         <!-- hit in trace -->
      <xsl:copy-of select="nonexistent" /><!-- hit in trace -->
    </xsl:value-of>
  </xsl:sort>
</xsl:apply-templates>

What if the rule for child elements of xsl:sort is Use Parent Status, which has the same effect as using the status of the xsl:sort element's own parent? Would that still make sense if the child element of xsl:sort is one that normally uses the Use Descendant Data rule?

As for deeper descendants of xsl:sort, using the status of the ancestor xsl:sort or its parent seems incorrect because the descendant could be within xsl:if or xsl:choose. How about:

• If the trace has a hit, mark it 'hit'.
• Otherwise, mark it 'unknown'.

That seems to stick close to the data that Saxon gives us, though we might end up marking some nodes as 'unknown' when they should really be 'missed'.

[galtm]

My suggestion is to stick with the Tiny tree, abandon PR #1936, and implement a rule for text nodes which is Use Parent Status/Data (not sure which yet). This will basically follow what the Linked tree does i.e. use the details from the parent node and not use the column number for text nodes.

Sounds promising and worth testing when one of us gets a chance. My hunch is that Use Parent Status will be better than Use Parent Data wherever they return different status values.

[galtm]

Congratulations

I must share any congratulations with @birdya22, who has made outstanding contributions in the code coverage area. Thanks!

[galtm]

My suggestion is to stick with the Tiny tree, abandon PR #1936, and implement a rule for text nodes which is Use Parent Status/Data (not sure which yet). This will basically follow what the Linked tree does i.e. use the details from the parent node and not use the column number for text nodes.

Sounds promising and worth testing when one of us gets a chance. My hunch is that Use Parent Status will be better than Use Parent Data wherever they return different status values.

If the text node's status is based on parent hits or parent data, how do we distinguish between the following?

<xsl:if test="1 eq 1">executed</xsl:if>
<xsl:if test="1 eq 2">unexecuted</xsl:if>

Should we use the parent element's line/column information but look for a particular class name, not the one corresponding to the parent element?

[birdya22]

If the text node's status is based on parent hits or parent data, how do we distinguish between the following?

executed unexecuted

The current solution doesn't. Using the tiny tree both text nodes are marked 'missed'. Using the Linked tree both text nodes are marked 'hit' (in both 12.4 and 12.5). There needs to be an additional test in the text-node-01.xsl test case and a rule created for text nodes in xslt-code-coverage-by-element.md. The coverage-report.xsl is currently using Use Trace Data isn't it, but text nodes aren't traced.

The rule needs to say mark as 'unknown' if parent is xsl:if - not sure about the rest yet, but it will only ever match the hit of its parent in the trace file. Are there other test cases where the parent of a text node is traced, but the text node isn't output?

I don't see any way of distinguishing the 2 cases.

[cmarchand]

You both have done a lot on this subject, but as far as Saxon reporting is not precise enough, it'll be difficult to use code coverage with XSpec as a test-quality reliable indicator.

My opinion is you've done best efforts, and we should entitled the reporting with something like 'test coverage reporting is given "as is", it is not perfect, and should be use with caution'.

I've worked hard before on Java code coverage to be able to distinguish between two expressions on the same line, and I know compilers and runners reporting are very difficult to perfectly understand, mainly when compilers make optimizations.

I think you've reached a good reporting level, and it is much enough for me, much enough for a CI quality gate analysis where the 100% goal is never required.

Thanks a lot for your work, Christophe

[birdya22]

I've done some testing for text nodes using the Use Parent Data rule and compared the results locally to using the Linked tree (I didn't see a Use Parent Status in the copy I have).

I removed text() from the match="element() | text() template and added it into the one commented Use Parent Data that includes XSLT:context-item | XSLT:merge-action etc.

I had 8 differences which were all due to the ancestor::XSLT:variable handling present in the match="element() | text() template that isn't in the Use Parent Data. The differences were (line numbers match items in the master repo when I took a copy and may differ if you've updated the files): xsl-evaluate-01.xsl line containing 'parameter not used in evaluation' (line 32) xsl-with-param-01.xsl line containing 'parameter not used in evaluation' (line 48) xsl-variable-01.xsl multiple lines - 20, 21, 25, 47, 48 and 52

The 8 differences are entirely explainable because of the XSLT:variable handling not being present in my Use Parent Data template.

Personally, I'm content that the same effect can be achieved through a rule for text() as can be achieved by changing to the Linked tree when generating the coverage report.

[birdya22]

For the discussion on xsl:sort.

What if the rule for child elements of xsl:sort is Use Parent Status, which has the same effect as using the status of the xsl:sort element's own parent? Would that still make sense if the child element of xsl:sort is one that normally uses the Use Descendant Data rule?

As for deeper descendants of xsl:sort, using the status of the ancestor xsl:sort or its parent seems incorrect because the descendant could be within xsl:if or xsl:choose. How about:

If the trace has a hit, mark it 'hit'. Otherwise, mark it 'unknown'. That seems to stick close to the data that Saxon gives us, though we might end up marking some nodes as 'unknown' when they should really be 'missed'.

Looking at your examples, I think some of the differences are due to using an empty sequence. Where it is at the end, I think Saxon is basically throwing it away because it does nothing, however that's not true in the case where it starts with an empty sequence and ends with an empty sequence.

I tried your examples in 12.5 and none of the descendants of xsl:sort are traced, even when using -Tlevel:high. I didn't see this before because no descendants are traced in the test cases in 12.4.

If you have multiple elements in the sequence constructor that all add data to the sort key you get a Saxon error:

100 001 XTTE1020 A sequence of more than one item is not allowed as the @select attribute of xsl:sort (text{100}, text{}, text{001})

So, I think your suggestion is ok. In 12.4 there may be some hits for deeper descendants but in 12.5 they will all be 'unknown`, but that might change in future releases.

[galtm]

My opinion is you've done best efforts, and we should entitled the reporting with something like 'test coverage reporting is given "as is", it is not perfect, and should be use with caution'.

@cmarchand : Thank you for your perspective and reality check. I think there may be some more that we can do, but you are right that it's never going to be perfect with the data we have (and people who use code coverage metrics might not be striving to reach 100% anyway, because the last N percentage points are either impossible or not worth the effort). We should add something to https://github.com/xspec/xspec/wiki/Code-Coverage, maybe in the Limitations section, and I linked your comment to #1844 as a reminder.

[galtm]

For the discussion on xsl:sort:

I pushed 45c0874602c76250f98160fbd1bda529b244215f with some XSLT, test, and Markdown updates. I wasn't sure whether the rule for children of xsl:sort should include text nodes, so I left that out. Can you even have a non-whitespace-only text node as a child of xsl:sort without getting a warning or error from Saxon?

[galtm]

@birdya22 : Should we continue the discussion of text nodes in #1920? Or make a new issue? I don't want that discussion to get lost whenever this pull request gets merged, but I also don't think this pull request itself should aim to address text nodes.

[galtm]

Re point 2, I should be able to find something that will accomplish the same testing purpose in this file with both 12.4 and 12.5.

c6b39f608aab2bbded9377413f7a21b56d5ce7bf uses a contrived combination of elements but appears to do the trick as far as illustrating the coverage rules for xsl:map and xsl:map-entry with 12.4 and 12.5 goes.