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

Separate common tests (for XSLT and XQuery) from generate-tests-utils.xspec (only for XSLT) #83

Merged
merged 2 commits into from Apr 9, 2017

Conversation

AirQuick
Copy link
Member

Currently test/generate-tests-utils.xspec, despite its filename, has scenarios both for XSLT (src/compiler/generate-tests-utils.xsl) and XQuery (src/compiler/generate-query-utils.xql). So three kinds of scenarios coexist in test/generate-tests-utils.xspec:

  • (a) Scenarios which should be Success both on XSLT and XQuery
  • (b) Scenarios which should be Success on XSLT and Failure on XQuery (Some features/fixes not implemented yet in src/compiler/generate-query-utils.xql)
  • (c) Scenarios which do work on XSLT but somehow do not work on XQuery (XPDY0050 when running generate-tests-utils.xspec for XQuery #82)

This pull request does two things:

  • Moves (a) to a new file test/generate-x-utils.xspec
  • Adds TODO comments to test/generate-tests-utils.xspec

In consequence,

@cirulls cirulls self-assigned this Apr 8, 2017
@cirulls cirulls self-requested a review April 8, 2017 12:28
@cirulls cirulls added this to the v0.6.0 milestone Apr 8, 2017
<!-- Copyright (c) 2010 Jeni Tennsion (see end of file.) -->
<!-- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -->
<t:description xmlns:t="http://www.jenitennison.com/xslt/xspec" xmlns:xs="http://www.w3.org/2001/XMLSchema" xmlns:test="http://www.jenitennison.com/xslt/unit-test" query="http://www.jenitennison.com/xslt/unit-test" query-at="../src/compiler/generate-query-utils.xql" stylesheet="../src/compiler/generate-tests-utils.xsl">
<t:description xmlns:t="http://www.jenitennison.com/xslt/xspec" xmlns:xs="http://www.w3.org/2001/XMLSchema" xmlns:test="http://www.jenitennison.com/xslt/unit-test" stylesheet="../src/compiler/generate-tests-utils.xsl">
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest to reinstate the copyright at the beginning and the end of the file. I would personally prefer to have all these copyright infos consolidated in the license file rather than at the beginning of each script but it may be complicated or long winded to get the consent from the original developers. Feel free to add your nickname as reviewer in the author comment or add a comment extended by.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point.

But, it may not be appropriate to simply reinstate the original copyright description. Neither Florent nor Jeni wrote the code in this file. In this pull request, the original code has been migrated to generate-x-utils.xspec.
So this file is essentially derived from 86e9bbf and 9b94529. What to do in this case?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. Then it's fine not to add the copyright as what it is left here is only the original filename. I'll merge the pull request then.

Copy link
Member

@cirulls cirulls left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me and I approve it in principle. I just added a comment for a trivial change. If you agree with it, please add it to your pull request and I will merge the code into master.

I tested this pull request by running locally both generate-tests-utils.xspec and generate-x-utils.xspec and inspecting the code. When running these XSpec tests, I got this warning:

Warning: at xsl:stylesheet on line 20 column 75 of generate-tests-utils.xsl:
  Stylesheet module file:/tmp/xspec/src/compiler/generate-tests-utils.xsl is included or
  imported more than once. This is permitted, but may lead to errors or unexpected behavior

I remember seeing this warning before so this has not been introduced in this pull request. I had a quick look at generate-tests-utils.xsl but I could not see any obvious case of double import (I have not looked at this thoroughly though). This is not a blocker for merging this pull request but I wanted to report it.

@cirulls cirulls assigned AirQuick and unassigned cirulls Apr 8, 2017
@AirQuick
Copy link
Member Author

AirQuick commented Apr 9, 2017

Regarding the warning, it is observed on v0.4-RC, too.
#69 addresses it (mostly as a side effect). I'll update #69 to make it more clear.

AirQuick referenced this pull request in AirQuick/xspec Apr 9, 2017
* 'master' of git://github.com/xspec/xspec:
  Separate common tests (for XSLT and XQuery) from generate-tests-utils.xspec (only for XSLT)  (#83)
  Make XProc testing command lines more portable (xspec#98)
  Update Saxon versions (xspec#116)
  Display result.log on test failure (xspec#110)

# Conflicts:
#	test/generate-tests-utils.xspec
AirQuick referenced this pull request in AirQuick/xspec Apr 16, 2017
* 'master' of git://github.com/xspec/xspec:
  Do not copy unused namespaces from format utils to output (xspec#91)
  Stop using functx namespace (xspec#104)
  Migrate test:tests to test/*.xspec (xspec#118)
  Added documentation for attributes in RNG schema (xspec#115)
  Separate common tests (for XSLT and XQuery) from generate-tests-utils.xspec (only for XSLT)  (#83)
  Make XProc testing command lines more portable (xspec#98)
  Update Saxon versions (xspec#116)
  Display result.log on test failure (xspec#110)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants