-
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
feat(xslt): external transformation #892
Conversation
# Conflicts: # src/common/xspec-utils.xsl # test/ant/worker/generate.xsl
# Conflicts: # src/compiler/generate-xspec-tests.xsl
# Conflicts: # src/compiler/generate-xspec-tests.xsl
# Conflicts: # src/compiler/generate-common-tests.xsl
@AirQuick , I just wanted you to know I started looking at this pull request. It might take me a while (e.g., 1-2 weeks) to chip away at the review, as I'd like to exercise it a reasonable amount in addition to looking at your code. So far, I'm finding the PR description and tests very helpful. |
Thanks @galtm. We have plenty of time. Because of its nature, this PR will be kept opened for weeks or even months. At first, I hoped |
# Conflicts: # src/compiler/generate-xspec-tests.xsl
# Conflicts: # src/compiler/generate-tests-utils.xsl
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 feature looks very promising and I think you've done a great job. I think it would be useful to get it out there in the field and let early adopters try it if they don't mind that it is experimental. You wrote that you're not sure if it is a viable solution; are there specific areas you'd like people (either code reviewers or early adopters) to explore and report back on?
Consider adding a test for a public packaged function (or other SUT code) that itself uses transform()
. Based on a small experiment, I believe it should work in this XSpec implementation as long as the underlying function works in XSLT, too.
Two documentation-related points:
a) Do you think the Compilation wiki should be enhanced soon to reflect this feature, or do you think its experimental status suggests we should wait? (If we update the wiki, we should probably make the functions public in the compilation-sut.xsl
file and maybe others in that folder.)
b) Users shouldn't think that run-as="external"
is universally better just because it's newer, and they might find that some of their existing tests cannot use this option. For instance, if the test uses x:call/@function
with functions that don't declare any visibility, the test will issue an error like XTDE0041: Cannot invoke function foo#1 externally, because it is not public
.
# Conflicts: # src/compiler/generate-common-tests.xsl
The behavior of the global context item seems a bit too unintuitive to me. That's why I wrote "I wonder if there is a finer way to control the global context item." In particular, I'm afraid apply-templates invocation and call-template invocation may be different from what the current XSpec users will expect. (I myself do not use XSpec in conjunction with global context, so I don't know exactly what they will expect.) |
f6a9d55
to
179f383
Compare
A very nice idea. |
The new tests look good. I like that you included the case of XQuery using |
Opened #1009.
Deferred it to #1010, because there are too many files (50 |
Looks like releasing this is better than waiting for more feedback. I'm merging this. |
I agree. I haven't had time to delve into your point about the global context item, but at some point I hope to do so, and I can always open a new issue if appropriate. Thanks for developing this feature! |
a) #1237 |
This pull request introduces a new
fn:transform()
-based test.Unlike the current
xsl:import
-based test, this new method is able to test XSLT packages.Possible use cases
Using this feature
Set
/x:description/@run-as
=external
.The XSpec schema annotates
@run-as
as EXPERIMENTAL, because I'm not sure if this entire feature is a viable solution.Examples
See
test/external_*.xspec
Limitations
vendor-options
offn:transform()
.xsl:stylesheet
orxsl:transform
)..xspec
files (x:expect/@select
etc.), you can't access variables/functions defined in the tested stylesheet.fn:load-xquery-module()
from an XSLT-based runner and simplify the XQuery implementation greatly. However,fn:load-xquery-module()
requires Saxon-EE.Global context item where global
xsl:param
andxsl:variable
are evaluatedfn:transform()
.x:context
withoutx:call[@template]
)x:context
is a single node, the root node of thex:context
tree becomes the global context item.x:context
is not a single node, the global context item is absent.x:call[@template]
)x:context
becomes not only the context item where the template is called but also the global context item.x:call[@function]
)source-location
, but I haven't implemented/tested it.XSpec global parameters and variables
/x:description/x:param
) take effect in the tested stylesheet. Their corresponding globalxsl:param
must be declared in the tested stylesheet./x:description/x:variable
) are not accessible from the tested stylesheet.Saxon configuration
$x:saxon-config
declared byx:variable
is passed tofn:transform()
asQName('http://saxon.sf.net/', 'configuration')
ofvendor-options
.x:variable
, but I found it handy for implementing this kind of optional/experimental feature. Thanks @galtm for completingx:variable
.$x:saxon-config
may need to be absolute.$x:saxon-config
, the tested stylesheet can't call a function passed as a parameter. (example)SAXON_CUSTOM_OPTIONS
(CLI) andsaxon.custom.options
(Ant) have no effect or partial effect on the tested stylesheet.Known issues
Probably Code Coverage fails for XSLT that calls transform() #746 needs to be fixed.(fixed by fix(xslt): fix issue 746 (transform() crashes Code Coverage) #1227)src/reporter/coverage-report.xsl
is not aware ofxsl:use-package
.Tests
test/external_*.xspec
.I have a few more tests in my local repository, but I didn't commit them to this branch for fear of cluttering this pull request prematurely. Once we agree on the outlook of this feature, I'll commit them to this branch.