Skip to content
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

Merged
merged 7 commits into from
Jun 10, 2024

Conversation

galtm
Copy link
Collaborator

@galtm galtm commented Apr 26, 2024

traceableUQName sometimes doesn't reflect the node that coverage-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 both traceableUQName and traceableClassName 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 (UPDATE: but I will leave that removal/refactoring for a follow-up pull request; see #1918 (comment)).

Fixes #1917.

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.
@galtm galtm added this to the v3.1 milestone Apr 26, 2024
@galtm
Copy link
Collaborator Author

galtm commented Apr 26, 2024

@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

Installed distributions
  Trying to resolve the latest version from remote
  Error: Could not find satisfied version for SemVer '8'. 
  Available versions: 22.0.1+8, 22.0.0+36, 21.0.3+9.0.LTS, 21.0.2+13.0.LTS, 21.0.1+12.0.LTS, 21.0.0+35.0.LTS, 20.0.2+9, 20.0.1+9, 20.0.0+36, 19.0.2+7, 19.0.1+10, 19.0.0+36, 18.0.2+101, 18.0.2+9, 18.0.1+10, 18.0.0+36, 17.0.11+9, 17.0.10+7, 17.0.9+9, 17.0.8+101, 17.0.8+7, 17.0.7+7, 17.0.6+10, 17.0.5+8, 17.0.4+101, 17.0.4+8, 17.0.3+7, 17.0.2+8, 17.0.1+12, 17.0.0+35, 11.0.23+9, 11.0.22+7.1, 11.0.22+7, 11.0.21+9, 11.0.20+101, 11.0.20+8, 11.0.19+7, 11.0.18+10, 11.0.17+8, 11.0.16+101, 11.0.16+8, 11.0.15+10

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.

@cmarchand
Copy link
Collaborator

I've no idea on Why ! It seems that support for Java 8 has been dropped.
Where is the script source for this build ?

@galtm
Copy link
Collaborator Author

galtm commented Apr 26, 2024

I've no idea on Why ! It seems that support for Java 8 has been dropped.
Where is the script source for this build ?

While you were entering your comment, I was creating a separate issue: #1919 . Let's continue this discussion there.

@galtm
Copy link
Collaborator Author

galtm commented Jun 4, 2024

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 test/end-to-end/cases-coverage/expected/stylesheet between (a) the branch containing the tests written by @birdya22 from #1921 (comment) with some local modifications and additions, and (b) a branch with all the same test files but also the Java changes from this branch. 37 HTML reports had differences. In all cases, the changes in this PR improved the HTML reports, such as the content of a hit <xsl:text> element also being marked as hit.

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 test/ directory and diff'd the HTML reports. (BTW, this repo doesn't have a nifty way to automate that.)

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 <xsl:param> element. Presumably, that's why Saxon traced those <xsl:param> elements. With the change in this PR, the tracing made the elements get marked as "hit". Without the change, the tracing didn't make the elements get marked as "hit" because the trace element had uqname="Q{http://www.w3.org/1999/XSL/Transform}variable" which doesn't match xsl:param. Other logic in coverage-report.xsl unrelated to this PR caused the apparently-untraced <xsl:param> to be marked as ignored rather than missed.

galtm added 2 commits June 4, 2024 11:44
XML coverage data now includes class names.

Generated using Saxon 12.4.
@galtm
Copy link
Collaborator Author

galtm commented Jun 4, 2024

fee5255 adds end-to-end tests similar to #1917.

@galtm galtm marked this pull request as ready for review June 4, 2024 17:29
galtm added a commit to galtm/xspec that referenced this pull request Jun 5, 2024
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.
galtm added a commit to galtm/xspec that referenced this pull request Jun 5, 2024
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.
@galtm galtm requested a review from AirQuick June 5, 2024 14:20
@galtm galtm added the bug label Jun 5, 2024
@galtm
Copy link
Collaborator Author

galtm commented Jun 5, 2024

#1932 should be merged before this one. After updating this branch to obtain the new tests in that PR, their expected results in test/end-to-end/cases-coverage/expected/ will need to be regenerated to reflect the Java changes in this PR.

galtm added a commit that referenced this pull request Jun 10, 2024
…#1932)

* Coverage tests from birdya22 ZIP-file

Tests written by user birdya22 (Adrian Bird).
From https://github.com/xspec/xspec/files/15447320/20240526Update.zip

* Conventions, Markdown based on .txt from birdya22

In the ZIP-file from user birdya22 (Adrian Bird) is a readme.txt
file that, among other things, documents some conventions used in
the per-element coverage tests. This Markdown file is based on that
documentation.

* Generated expected results

Using Saxon 12.4 and XSpec master branch as of 2 June 2024

* In xsl:apply-templates test, use mode attribute

* In xsl:assert test, remove try/catch and add -ea

1. xsl:try/xsl:catch structure has its own test, so simplify this test
by using catch="yes" in XSpec
2. Add Saxon custom option to enable assertions

* Remove unneeded namespace declaration

* In xsl:attribute test, add case for child element

Trace treats a child element and its text child differently
from a child text node, so both patterns are worth capturing.

* In xsl:global-context-item test, use XSpec context

In external transformations, XSpec passes x:context to
XSLT as the global context item, so XSLT can get it using
"." syntax. In external transformations, XSpec x:param
does not get passed to XSLT processor.

* Make xsl-accumulator-01.xspec run w/ external

run-as="external" requires an additional xsl:mode
element with use-accumulators, for the default mode.
This change facilitates importing xsl-accumulator-01.xspec
into a parent XSpec file that has run-as="external".

* Add to xsl:on-non-empty test

In case Saxon optimizes away a case where
static analysis of XSLT shows the sequence constructor
is always empty, provide a case that requires the
source document in addition to the XSLT.

* In xsl:comment test, add more cases

Children of xsl:comment are traced differently depending on
the order of `<node>` elements in XSLT; without #1918 change,
the xsl:value-of child is incorrectly marked as missed.

* Misc. non-substantive tweaks

Typos, comments, code formatting

* Example for xsl:evaluate with parameter

* In xsl:map test, add more cases

Trace behavior for xsl:variable is complicated, so judging
trace behavior of xsl:map and xsl:map-entry might be
easier with additional data points. Use a template parameter
and function body as additional contexts for xsl:map.

* In xsl:message test, add cases that terminate

* In xsl:mode test, add a case without name attribute

* In xsl:next-match test, include more cases

- Case with xsl:with-param child
- Case where the next match is built-in template

* In xsl:where-populated test, consistent structure

For testing, it shouldn't matter whether `<node type="where-populated">`
is the child or the parent of xsl:where-populated. This commit makes the
structure consistent between the two cases.

* In xsl:catch test, add more cases

Design document suggests an "unknown" status for xsl:catch
coverage reporting, which is used when xsl:catch has no children.
Include example of two no-children cases (select attribute and
empty) for executed xsl:catch and non-executed xsl:catch
situations.

* In xsl:with-param test, add xsl:evaluate case

* In xsl:param test, include global not overridden

Mimics the difference that the Java change in #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.

* Fix lint issues

* Regenerate expected results

* Address review comments from birdya22

Also, regenerate the expected results corresponding to
the modified .xsl files.
@galtm
Copy link
Collaborator Author

galtm commented Jun 10, 2024

#1932 should be merged before this one. After updating this branch to obtain the new tests in that PR, their expected results in test/end-to-end/cases-coverage/expected/ will need to be regenerated to reflect the Java changes in this PR.

Done; 3c9aa00 has the regenerated files.

@galtm
Copy link
Collaborator Author

galtm commented Jun 10, 2024

I'll merge this change after the checks are complete. Code coverage reporting needs further improvement beyond this PR, but I believe this PR is a step forward.

@galtm galtm merged commit d0cab21 into xspec:master Jun 10, 2024
36 checks passed
@galtm galtm deleted the issue1917-coverage branch June 10, 2024 12:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Issue with code Coverage Report in 3.0.3
2 participants