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

Set ANT_VERSION in xspec-bat.cmd #244

Merged
merged 17 commits into from
Aug 26, 2018
Merged

Conversation

AirQuick
Copy link
Member

@AirQuick AirQuick commented Aug 3, 2018

To prevent spurious test failures, xspec-bat.cmd (a test script for Windows) skips Ant tests when the ANT_VERSION environment variable is not defined.

You have to set the variable manually if you want to run Ant test on your local machine.

This pull request sets a pseudo ANT_VERSION in xspec-bat.cmd, if ant is available in PATH. Now you don't have to set the variable manually.

No code change in XSpec core.


This pull request derives from #243. So needs to be handled after that.

@AirQuick AirQuick added the test label Aug 3, 2018
@AirQuick AirQuick added this to the v1.1.0 milestone Aug 3, 2018
@AirQuick AirQuick requested a review from cirulls August 3, 2018 00:45
@AirQuick
Copy link
Member Author

AirQuick commented Aug 3, 2018

  • This pull request just improves the way to run tests on a local machine. It doesn't touch the XSpec code. None of the existing tests are modified.
  • The automated tests are not affected. (demonstrated on AppVeyor)
  • This pull request makes it easy to handle some pending pull requests (towards Is it possible to apply Saxon configuration files?  #235 in particular).

@cirulls
So I think you can merge this (after #243) safely without going into details, or I'd like to merge this by myself in a few days.

@cirulls
Copy link
Member

cirulls commented Aug 26, 2018

I executed xspec-bat.cmd on branch master and on my local Windows environment (which doesn't have ANT_VERSION set up) and I can confirm that the ant tests are all skipped. I then executed xspec-bat.cm on the branch of this pull request and in the same local environment and I can confirm that the ant tests are executed. So, as far as the changes in this pull request are concerned, this is all good.

However, I'd like to highlight an inconsistency between xspec.bats and xspec-bat.cmd. The ant tests in xspec.bat assume that ant is installed: there is no check for the existance of ant (see this test as an example). As a result, if ant is not installed, the test is probably going to exit and fail (I haven't tested it though).

Conversely, the ant tests in xspec-bat.cmd (both in this pull request and in master) first check that ant is installed and then execute the tests if that's the case. If not, the tests are skipped.

These scenarios occur when the test suite is executed locally. In fact, on Travis and AppVeyor ant is always installed first and, if the installation is not successful, there is an error code as we saw recently.

I'm not too fuzzy about which way we lean on (i.e. adding the check in xspec.bats or removing the check in xspec-bat.cmd) but I would prefer to avoid this inconsistency and align these two scripts as close as possible in terms of behaviour. I guess it also depends on how strongly we feel that ant tests should be executed locally before submitting a pull request.

@AirQuick, what's your opinion on this?

@AirQuick
Copy link
Member Author

Right. They're inconsistent.
The inconsistencies (not only Ant but also some others) are resolved in #289. Ant is a must thereafter both on *nix and Windows. See the step 3 of How to test in #289.

So this pull request (#244) should be considered as a transitory measure to ease testing locally until #289.

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.

Thanks for clarifying and for fixing the inconsistency in another pull request. I'm merging this into master.

@cirulls cirulls merged commit b28c01e into xspec:master Aug 26, 2018
@AirQuick AirQuick deleted the set-ANT_VERSION branch August 26, 2018 17:12
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.

2 participants