fix(xslt): include class in XSLT code coverage data #1918
Conversation
traceableUQName is sometimes an ancestor of the node that coverage-report.xsl expects to see (xspec#1917), whereas traceableClassName works more reliably. Therefore, include the class in the coverage data even if uqname is available.
|
@cmarchand , by any chance do you know why macOS fails on Java 8? https://github.com/xspec/xspec/actions/runs/8841495386/job/24278687107?pr=1918 The same thing just started happening in my own fork earlier this week, when I was merely bringing it up to date with this repo. |
|
I've no idea on Why ! It seems that support for Java 8 has been dropped. |
While you were entering your comment, I was creating a separate issue: #1919 . Let's continue this discussion there. |
|
Here are some notes about local qualification of this change. Tests in test/end-to-end/cases-coverage/Today, I diff'd the generated HTML reports from Top level of test/Around the time I first created this PR, I didn't have a lot of coverage tests to run, so I generated coverage reports with and without this PR's Java changes, for files at the top level of the Bottom line: The changes in this PR helped in some cases. I found no cases where the changes did any harm, such as marking something as "hit" that was really "missed." Details on two tricky cases (https://github.com/xspec/xspec/blob/master/test/issue-1564.xspec and https://github.com/xspec/xspec/blob/master/test/global-override.xspec): This PR's changes caused a total of three global XSLT parameters to be reported as "hit" instead of "ignored". In all three cases, the global XSLT parameter was not overridden (i.e., did not have a value provided) from XSpec, so Saxon had to evaluate the default value provided in the |
XML coverage data now includes class names. Generated using Saxon 12.4.
| <xsl:for-each select="$hits"> | ||
| <xsl:variable name="hit-traceable-uqname" as="xs:string?" | ||
| select="exactly-one(key('traceables', @traceableId, $trace))/@uqname" /> | ||
| <xsl:variable name="hit-traceable-class" as="xs:string?" | ||
| select="exactly-one(key('traceables', @traceableId, $trace))/@class" /> | ||
| <xsl:if test="($node-uqname eq $hit-traceable-uqname) or | ||
| empty($hit-traceable-uqname)"> | ||
| exists($hit-traceable-class)"> | ||
| <xsl:sequence select="." /> | ||
| </xsl:if> | ||
| </xsl:for-each> |
There was a problem hiding this comment.
I am pretty sure this block of code can be simpler, maybe even removing the conditional logic. But it might be useful to do such refactoring in a follow-up pull request to make it easier to see whether any of the expected HTML coverage reports in test/end-to-end/cases-coverage/ actually change as a result of simplifying the XSLT.
traceableUQNamesometimes doesn't reflect the node thatcoverage-report.xslexpects to see, which can lead to nodes being marked in the XSLT coverage report as missed when they were, in fact, hit. This change causes the coverage data to include bothtraceableUQNameandtraceableClassNameif not null, and causes the XSLT to be less strict about when it considers a hit to be relevant to the node it's looking at.This pull request is not complete, as I plan to add tests and add some notes about my local qualification. It's also possible that the XSLT has some unneeded code that can be removed.
Fixes #1917.