-
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 normalization of XQuery test result HTML in end-to-end test #293
Conversation
…185_cumulative # Conflicts: # build.xml
…-tests # Conflicts: # build.xml # test/run-xspec-tests-ant.cmd # test/run-xspec-tests-ant.sh
…-tests # Conflicts: # test/end-to-end/generate-expected.cmd # test/end-to-end/generate-expected.sh # test/end-to-end/run-e2e-tests.cmd # test/end-to-end/run-e2e-tests.sh
…aramater # Conflicts: # build.xml # test/end-to-end/generate-expected.cmd # test/end-to-end/generate-expected.sh # test/end-to-end/run-e2e-tests.cmd # test/end-to-end/run-e2e-tests.sh # test/run-xspec-tests-ant.cmd # test/run-xspec-tests-ant.sh
…st-xquery-norm # Conflicts: # build.xml # test/end-to-end/generate-expected.cmd # test/end-to-end/generate-expected.sh # test/end-to-end/processor/html/_normalizer.xsl # test/end-to-end/run-e2e-tests.cmd # test/end-to-end/run-e2e-tests.sh # test/run-xspec-tests-ant.cmd # test/run-xspec-tests-ant.sh
…185_cumulative # Conflicts: # test/xspec-bat.cmd # test/xspec.bats
…-tests # Conflicts: # test/xspec-bat.cmd # test/xspec.bats
…aramater # Conflicts: # test/xspec-bat.cmd # test/xspec.bats
…st-xquery-norm # Conflicts: # test/xspec-bat.cmd # test/xspec.bats
…k/xspec into align-bats-tests # Conflicts: # test/xspec-bat.cmd # test/xspec.bats
…-tests # Conflicts: # test/xspec-bat.cmd # test/xspec.bats
…into fix_typo-Paramater
…into fix_e2e-test-xquery-norm
…st-xquery-norm # Conflicts: # test/win-bats/collection.xml # test/xspec.bats
@cirulls |
@AirQuick: sorry for the wait. I'll review this over the weekend and merge it if everything is fine (I'm sure it is). When I do the review I will restart the AppVeyor tests, hopefully the network issue will be fixed. |
@@ -1,12 +1,14 @@ | |||
<?xml version="1.0" encoding="UTF-8"?><html xmlns:test="http://www.jenitennison.com/xslt/unit-test" xmlns="http://www.w3.org/1999/xhtml"> | |||
<head> | |||
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8" /> | |||
<title>Test Report for www.functx.com (passed: 1 / pending: 0 / failed: 0 / total: 1)</title> | |||
<title>Test Report for http://www.functx.com (passed: 1 / pending: 0 / failed: 0 / total: | |||
1) |
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.
This is a minor issue: it seems to me that this extra formatting with whitespace is unnecessary and makes the title less readable - maybe it was due to automatic formatting? I think keeping the original version would be better.
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.
We need to keep the extra whitespace, because that's what Saxon 9.8 and 9.7 actually serialize when handling the title
element. You can see it with the following dumb stylesheet.
<?xml version="1.0" encoding="UTF-8"?>
<xsl:stylesheet xmlns:xsl="http://www.w3.org/1999/XSL/Transform" version="3.0">
<xsl:template name="main">
<xsl:message select="('xsl:product-name', 'xsl:product-version') ! system-property(.)" />
<html xmlns:test="http://www.jenitennison.com/xslt/unit-test" xmlns="http://www.w3.org/1999/xhtml">
<head>
<title>Test Report for http://www.functx.com (passed: 1 / pending: 0 / failed: 0 / total: 1)</title>
</head>
</html>
</xsl:template>
</xsl:stylesheet>
C:\>java -jar saxon9ee.jar -it:main -xsl:test.xsl
SAXON EE 9.7.0.21
<?xml version="1.0" encoding="UTF-8"?><html xmlns:test="http://www.jenitennison.com/xslt/unit-test" xmlns="http://www.w3.org/1999/xhtml">
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8" />
<title>Test Report for http://www.functx.com (passed: 1 / pending: 0 / failed: 0 / total:
1)
</title>
</head>
</html>
It seems fixed on 9.9. (Bug 3842 perhaps?)
P:\>java -jar saxon9ee.jar -it:main -xsl:test.xsl
SAXON EE 9.9.0.2
<?xml version="1.0" encoding="UTF-8"?><html xmlns:test="http://www.jenitennison.com/xslt/unit-test" xmlns="http://www.w3.org/1999/xhtml">
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8" />
<title>Test Report for http://www.functx.com (passed: 1 / pending: 0 / failed: 0 / total: 1)</title>
</head>
</html>
One day when 9.9 is considered as the mainstream, we can probably update the HTML files in the repository by regenerating them using 9.9. But it's not now, so the extra whitespace is there.
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 see. That's absolutely fine, thank for the explanation. I'm merging this pull request into master
.
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.
In general, this looks good to me (apart from the minor issue related to formatting). I reviewed it by:
- inspecting the changes and the new test.
- running the generated-exspected.sh and then the end-to-end test suite locally on Linux, all tests green.
- restarting the build for this pull request on AppVeyor, everything is green now (it was previously red because of a temporary network issue).
@AirQuick: let me know about the comment related to formatting. If the extra formatting is unnecessary please remove it from this pull request and I'll merge the pull request. If the extra formatting is important or you have a different opinion, please explain it further. Thanks!
This pull request derives from #292. So needs to be handled after that.
In the XQuery test result HTML report, URL in
/html/head/title
and/html/body/p[1]
is a namespace.But the end-to-end test normalizes it as if it were a file. (
title
,p
)This pull request fixes it. (
title
,p
)