-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
RFC: fail fast on Travis for old queued builds in same PR or branch #12387
Conversation
34c728c
to
44ae279
Compare
Alright, pretty sure this is working correctly for PR builds now. It doesn't help for "push" builds though, where Travis builds the branch for committers filing PR's from branches within this repo. We could extend the setup to check for branch builds too, just whitelist master and release branches since it's good to build every commit for those by default. Thoughts? |
Go for it if it's easy to implement? FWIW, personal travis is very easy to set up and use simultaneously with the JuliaLang one if more tests are needed. I see that the url is hard coded in the script. Would be nice to make sure that it doesn't give an error on personal travis. |
I'll see if there's a Travis env var or two for the JuliaLang/julia part. Branch builds is more code that I haven't written yet, but shouldn't be too bad. |
Hm, I don't actually see branch name anywhere in the Travis API: http://docs.travis-ci.com/api/#builds Seems like a pretty big omission, maybe I can ask for them to add it. |
|
No, I can use |
44ae279
to
97a269e
Compare
fi | ||
|
||
# And for non-latest push builds in branches other than master or release* | ||
case $TRAVIS_BRANCH in |
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.
What is TRAVIS_BRANCH
set to for PR? Especially the ones that comes from a different repo.?
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.
It's the branch targeted by the PR http://docs.travis-ci.com/user/environment-variables/#Default-Environment-Variables
So I may want to put this case block inside an else
so it doesn't apply for any PR's.
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.
or just return 0 early for pr
Adjust installation of jq on osx --only-dependencies is not what I want there Rearrange travis steps before_install steps are fatal by default, script steps are not (travis runs all of them and marks the build as failed if any returns nonzero exit code) Fail faster on osx May as well do check-whitespace before installing deps Add fast fail for branch builds too Make sure branch section does not apply to PRs
605237d
to
5f56b9e
Compare
Looks good. Let's do it when the CI turns green. |
No harm in trying it for a while and seeing if we like it, whether it helps, or whether my scripting is horribly buggy. Revert if it causes any issues, accidentally cancels any false-positives, etc. |
Should we merge this? |
Seems like a reasonable change. In general, we could optimize time-to-failure by prioritizing tests better. |
Not sure what you mean by prioritizing tests. Going through the timings and rearranging the order? |
RFC: fail fast on Travis for old queued builds in same PR or branch
This is great, very glad to have it. Thanks, @tkelman! |
By prioritizing, I meant front-loading the ones that are most likely to fail. Of course, that's hard to know. |
Possibly clever idea: run test files corresponding to files that changed in a commit first. Requires knowing what has changed in a PR and having a way of mapping files in |
I like the idea of running the tests in "danger" order. We should rename the files in |
This only works on the Mac builder? For example, here the build stopped due to "There are newer queued builds for this pull request, failing early." but only for the Mac builder: |
The check is at the very beginning of the build so if you push a commit after that, the fail fast will not trigger. The mac builder is generally the slowest one and that's probably why it is the only one that triggers in this case. |
As suggested by @yuyichao in #12292 (comment)
We're doing this on AppVeyor already, and lately the queue has been just about as bad on Travis thanks to now having 2 appveyor workers that aren't freezing and timing out any more.
I'll push a few dummy commits to this branch to test whether or not this works properly.