Fix/revert revert reverting

[fgeorges]

No description provided.

[cirulls]

@fgeorges: Thanks for your pull request.

I started reviewing it and have several questions, I hope you can provide us with the information we were not able to find out when we reverted to v0.3.0 for attributes assert and context and the XProc harnesses.

First of all, I listed all the files that have changed. There are lots of changed files and unfortunately the files changed view in GitHub is not very helpful in this case as it doesn't not remove trivial changes like whitespaces and carriage returns.

Files that have been added or changed

bin/xspec.sh
src/compiler/generate-xspec-tests.xsl
src/harnesses/basex/basex-server-xquery-harness.xproc
src/harnesses/basex/basex-standalone-xquery-harness.xproc
src/harnesses/exist/exist-xquery-harness.xproc
src/harnesses/harness-lib.xpl
src/harnesses/marklogic/ml-xquery-harness.xproc
src/harnesses/saxon/saxon-xquery-harness.xproc
src/harnesses/saxon/saxon-xslt-harness.xproc
src/harnesses/zorba/zorba-xquery-harness.xproc
test/unit-expect-xsl.xspec
test/xspec-bat.cmd
test/xspec.bats

Files that have not changed or where the change is trivial (e.g. whitespace)

src/compiler/generate-query-helper.xsl
src/compiler/generate-query-tests.xsl
src/compiler/generate-query-utils.xql
src/compiler/generate-tests-helper.xsl
src/compiler/generate-tests-utils.xsl
test/generate-xspec-tests.xspec
test/xspec-26.xsl
test/xspec-26.xspec

Files deleted

src/harnesses/basex/basex-xquery-harness.xproc

Questions

• (a) In src/compiler/generate-xspec-tests.xsl there is a template x:output-expect-FIXME-TOREMOVE and a template x:output-expect. We were a bit puzzled by the name of the first template and noticed that in v0.3.0 only x:output-expect existed. What is the purpose of template x:output-expect-FIXME-TOREMOVE? Should it be removed or renamed to something more meaningful?

• (b) I can read some documentation in the files related to the XProc harnesses (especially parameters and ports) but it was not clear to me how to run the XProc harnesses for BaseX (server and standalone), eXist-db, Marklogic, and Zorba as I'm not familiar with all the tools. Are these harnesses executed directly from the GUI in the relevant tools? Or is there a command to run the XProc harnesses for these tools? Which dependencies should be installed beforehand? I would like to add a test for each harness like we did for the Saxon harness so that we can make sure that they always run correctly.

• (c) In src/harnesses/harness-lib.xpl there is a comment at the beginning of the file saying TODO: Does not work as is.... This was also one of the reasons we reverted to v0.3.0 as we suspected the implementation was not finished due to this comment. Is this comment still relevant? I can see harness-lib.xpl is imported in the other harnesses - is it used to run all the XProc harnesses? What is the command to execute it?

• (d) Would it be possible to provide examples of tests with attributes assert and context? In particular, what is the difference between the use of attribute test and the use of attribute assert? An XSpec example would clarify and it would be a good starting point to write a test and documentation for these attributes.

• (e) The RelaxNG schema xspec.rnc currently does not contain these new attributes. Would it be possible to add them?

• (f) In xspec.bats there are several lines where the test checks two conditions with an OR statement (e.g. ||) where there used to be only one condition:

  [[ "${lines[3]}" =~ "Testing with"
      || "${lines[7]}" =~ "Testing with" ]]
  

  Is this due to local testing/the introduction of the variable SAXON_SCRIPT in xspec.sh?

Review

There is lots to review as these are actually two pull requests in one (attributes assert and context and XProc harnesses). Plus, I would like to get this done sooner rathen than later as I really want to see the test badge green again on the master branch.

@AirQuick: would you have time to review one part of this pull request? I'm happy to review the XProc harness part as I remember you previously mentioned you are not familiar with all the tools (I'm not either but I can give it a go with help from Florent).

[fgeorges]

(a) Leave it as it is for now. There are still some work to be done on @assert and @context, this is the old implementation for reference during development.

(b) They are XProc pipeline to drive executing XSpec on different processors. Assuming only Saxon is wrong. So they are supposed to be invoked from command line using Calabash (the same way the harness for Saxon is invoked). Each pipeline contains documentation on how it should be invoked, its options, etc.

The level of maturity of these harnesses varies from each other. They at least have the merit to exist, instead of having nothing at all for other processors, and give as a minimum one way to use XSpec on these processors.

(c) This line explains why the step next line is commented out. The step is supposed to provide extra checks. It is not implemented yet. Not a big deal.

(d) There is more than an example to had. It should be documented, tested, etc. There are emails on the XSpec group discussing all this. One of the primary reasons for introducing @assert was that @test can do several aspects at the same time (test assertion expressions, select pieces of XML to be compared, etc.) Which led to errors in some cases, where the test writer intend something, but in some circumstances the other aspect of @test was picked. The new attributes each implements a different aspect of @test.

This is a "beta feature". We want to keep the possibility to change it slightly in a future release based on feedback.

(e) I guess so. I don't know who/what that schema is used by/for. Probably best to wait for a "definitive" version of them before adding them. Especially if that schema is used for auto-completion, e.g. in oXygen.

(f) In some case (when using a repository with the XAR version of XSpec), some extra lines are output. The test is not about checking that a piece of text is absolutely on a specific line, but that it appears in a way that confirms all is OK.

[cirulls]

Thanks @fgeorges for your replies and for marking the questions. I think I have enough information to review the XProc harness part. I agree that it is good to have something for processors other than Saxon. I'll try to install these processors and run the XProc harnesses. I may ask you some additional questions if I get stuck.

If @AirQuick can review the assert and context attributes part, it would be grand.

[AirQuick]

In the present situation I'm not joining this activity, sorry...

[cirulls]

@fgeorges: I started reviewing your pull request. As there are several things to review, I'm going to do it little by little so that parts of your pull request can be immediately pushed to master and integrated with documentation and tests.

I started by reviewing the XProc harness for Saxon. The thing that I missed out when we reverted to v0.3.0 was the -p option to put in front of xspec-home, this was the reason why I couldn't make it work (I saw @AirQuick tried this out in #74 (comment)). Looking at your log commits and how you implemented the bats test was very useful to understand how the harness works. Using the library harness-lib.xpl and import it into all the harnesses is also a good approach to modularize the code and add the logging.

I had a look at the XProc test in xspec.bats which is as follows:

@test "executing the Saxon XProc harness generates a report with UTF-8 encoding" {
    query="declare default element namespace 'http://www.w3.org/1999/xhtml'; /html/head/meta[@http-equiv eq 'Content-Type']/@content = 'text/html; charset=UTF-8'";
    if which saxon && which calabash; then
        run calabash -isource=xspec-72.xspec -oresult=xspec/xspec-72-result.html ../src/harnesses/saxon/saxon-xslt-harness.xproc 
        run saxon --xquery -s:xspec/xspec-72-result.html -qs:"$query" !method=text
    elif [ -z ${XMLCALABASH_CP} ]; then
        skip "test for XProc skipped as XMLCalabash uses a higher version of Saxon";
    else
        run java -Xmx1024m -cp ${XMLCALABASH_CP} com.xmlcalabash.drivers.Main -isource=xspec-72.xspec -p xspec-home=file://${TRAVIS_BUILD_DIR}/ -oresult=${TRAVIS_BUILD_DIR}/test/xspec/xspec-72-result.html ${TRAVIS_BUILD_DIR}/src/harnesses/saxon/saxon-xslt-harness.xproc
        run java -cp ${SAXON_CP} net.sf.saxon.Query -s:${TRAVIS_BUILD_DIR}/test/xspec/xspec-72-result.html -qs:"$query" !method=text
    fi
    echo $output
    [[ "${lines[0]}" = "true"
    || "${lines[2]}" = "true" ]]
}  

When this test is run on Travis, the first if condition which saxon && which calabash is never executed as both Saxon and XMLCalabash are installed and referenced using the environment variables SAXON_CP and XMLCALABASH_CP. This Travis and AppVeyor configuration based on environment variables allows us to run tests with different versions of these tools and makes it very easy to upgrade to a new version of Saxon and XMLCalabash. I assume you used the configuration based on the which command to run the test suite on your machine. The same occurs in "${lines[2]}" = "true": this line is not necessary as when the test is run on Travis the result is always outputted on line 0. I'll write some documentation on how to run the test suite locally so that anyone can install it and run it before sending a pull request.

I modified the test in branch review/103 so that it keeps the changes you made to run the XProc harness and removes the unnecessary conditions which are never executed on Travis.

I created branch review/103 which contains your code for the Saxon XProc harness (including harness-lib.xpl), xspec-bat.cmd and the modified version of xspec.bats. This branch is based on the latest master version that had all the tests passing (f4811a5). I would like to push this branch to master so that we can start using your version of the XProc harness for Saxon. I'm not going to be able to review everything in one go and I prefer this approach to get parts of your pull request immediately available.

Would that be ok for you?

[fgeorges]

• yes, xspec-home is a parameter, is has to be passed as -p, unlike options

• yes, the different line numbers are for when using saxon/calabash scripts to test the packaged XAR file

• I let you organize the way you prefer reviewing it

[cirulls]

@fgeorges: thanks for your feedback. I merged this part of the review in #108 and updated the documentation on the wiki.

I'll continue by reviewing the other XProc harnesses but it's probably going to be next week as I haven't got enough time during the rest of this week. I'll keep you posted in here.

[fgeorges]

@cirulls Any update on this? There start to be conflicts, as it gets desynchronized!

[cirulls]

I started reviewing the XProc harness for BaseX but I'm still trying to run it as I'm not familiar with BaseX. For me it would easier to have one pull request for each component that has changed (e.g. one pull request for each XProc harness + one pull request for each attribute) as this will allow to review and merge the pull requests one by one. If other people could help reviewing, it would be great.

[fgeorges]

@cirulls Any update on this one? Conflicts are accumulating...

[cirulls]

Sorry if it is taking longer than expected, I struggle to find enough time to work on several issues and pull requests at the same time. I start reviewing the BaseX part of the pull request and I'm planning to keep working on it during this weekend.

Regarding conflicts, it is possible to apply the latest commits merged into branch master using git rebase, this allows to keep the pull request up-to-date with the latest changes in master.

[cirulls]

Sorry for the delay on this. I reviewed the XProc harness for BaseX standalone and it looks good to me. I extrapolated that part of your pull request and added documentation and tests, please see #136, I added you as reviewer. If this is fine with you, I'll merge the BaseX standalone harness into master. If you propose changes, please comment directly in #106.

I tried to execute the XProc harness for BaseX server but I didn't manage to make it run, I am not sure if I'm providing the correct options. This is what I tried:

1. I download the BaseX zip file

2. I started the BaseX HTTP server following the instructions on the BaseX wiki. I could access the BaseX HTTP server at localhost:8984.

3. I navigated to my XSpec folder and execute the following command:

   java -cp /path/to/xmlcalabash/xmlcalabash-1.1.15-97.jar com.xmlcalabash.drivers.Main \
    	-i source=tutorial/xquery-tutorial.xspec \
    	-p query-at=tutorial/xquery-tutorial.xq \
    	-p xspec-home=file:///path/to/xspec/ \
    	-p endpoint=http://localhost:8984/ \
    	-p username=admin \
    	-p password=admin \
    	-p auth-method=Basic \
    	-o result=tutorial/xquery-tutorial-result.html \
    	src/harnesses/basex/basex-server-xquery-harness.xproc

   I got the following error message:

   ERROR: src/harnesses/basex/basex-server-xquery-harness.xproc:110:24:Undeclared option specified: xspec-home
   ERROR: src/harnesses/basex/basex-server-xquery-harness.xproc:110:24:err:XS0010:Undeclared option specified: xspec-home
   ERROR: It is a static error if a pipeline contains a step whose specified inputs, outputs, and options do not match the signature for steps of that type.
   

   However, xspec-home is provided as option and it is passed in the same way as in the BaseX standalone implementation.

When I looked at line 110 of basex-server-xquery-harness.xproc, I noticed that t:format-report is called and the variable $xspec-home is passed whereas in the BaseX standalone harness the variable $xspec-home is not passed to t:format-report. I tried to remove the part with <p:with-option name="xspec-home" select="$xspec-home"/> but the t:report is not generated and the command fails with the error ERROR: src/harnesses/harness-lib.xpl:0:t:ERR001: Not a t:report document.

Am I not providing the correct parameters? Could you let me know how you run the BaseX server harness?