-
Notifications
You must be signed in to change notification settings - Fork 31
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
fix(xslt): include class in XSLT code coverage data #1918
base: master
Are you sure you want to change the base?
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Children of xsl:comment are traced differently depending on the order of `<node>` elements in XSLT; without xspec#1918 change, the xsl:value-of child is incorrectly marked as missed.
Mimics the difference that the Java change in xspec#1918 makes in test/global-override.xspec. The overridden global xsl:param is marked as ignored, but an open design question is whether it should instead be marked as missed. The comment in this revision says "Expected miss" in anticipation of changing the treatment of global params.
#1932 should be merged before this one. After updating this branch to obtain the new tests in that PR, their expected results in |
traceableUQName
sometimes doesn't reflect the node thatcoverage-report.xsl
expects 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 bothtraceableUQName
andtraceableClassName
if 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.