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

Fix CI and add set -e #4605

Merged
merged 2 commits into from Mar 17, 2019

Conversation

Projects
None yet
3 participants
@jrbourbeau
Copy link
Member

commented Mar 17, 2019

This PR removes export NUMPY_EXPERIMENTAL_ARRAY_FUNCTION=1 in continuous_integration/travis/run_tests.sh which was causing Travis CI builds to pass regardless if there were failed tests. For reference, see #4525 (comment) and the comment that follows.

Also added set -e to fail immediately.

cc @mrocklin @pentschev

@jrbourbeau jrbourbeau referenced this pull request Mar 17, 2019

Merged

Add zarr to CI environment #4604

0 of 2 tasks complete
@pentschev

This comment has been minimized.

Copy link
Member

commented Mar 17, 2019

Just my two cents: in a perfect world, we would check all return values and fail at the end of run_tests.sh only. This would ensure later stages of the script get executed and provide a full summary of errors, instead of forcing the author of the PR to fix errors one-by-one.

@jrbourbeau

This comment has been minimized.

Copy link
Member Author

commented Mar 17, 2019

ensure later stages of the script get executed and provide a full summary of errors

That does sound nice. Is there a way to do this?

As it stands now, run_tests.sh exports a couple of environment variables and runs pytest. If run_tests.sh was changed in the future to do many things, then I agree it'd be nice to see a summary of all error messages. But for our current setup I suspect that adding set -e provides more benefit to us as a safeguard than it does inconvenience to future PRs. pytest will still run all our tests and print out any failing test tracebacks before exiting.

@mrocklin

This comment has been minimized.

Copy link
Member

commented Mar 17, 2019

+1 from me. Please feel free merge at your discretion @jrbourbeau

@jrbourbeau jrbourbeau merged commit b711904 into dask:master Mar 17, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jrbourbeau jrbourbeau deleted the jrbourbeau:fix-ci branch Mar 17, 2019

@jrbourbeau

This comment has been minimized.

Copy link
Member Author

commented Mar 17, 2019

Thanks for the review @mrocklin and @pentschev

@pentschev

This comment has been minimized.

Copy link
Member

commented Mar 17, 2019

@jrbourbeau I think the only way is then to check the return value of all commands and return that at the end. But you're right, right now it doesn't make any difference for run_tests.sh, I thought there was more than one line of actual test going on.

jorge-pessoa pushed a commit to jorge-pessoa/dask that referenced this pull request May 14, 2019

Fix CI and add set -e (dask#4605)
* Remove export

* Disable immediate fail at end of script
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.