Add error-exit-code to differentiate from failures #2749
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
robotdana
force-pushed
the
error_exit_code
branch
7 times, most recently
from
c6929c6
to
a9c50b9
Compare
|
I did have a green build: https://travis-ci.org/github/rspec/rspec-core/builds/717497518 |
robotdana
force-pushed
the
error_exit_code
branch
from
a9c50b9
to
18f43bc
Compare
JonRowe
requested changes
Aug 13, 2020
There was a problem hiding this comment.
Thanks for taking the time to add this, I've suggested some refactoring to how this should work, specifically I don't think config is the right place for introspecting the world (especially with injection, thats a potential confusion I'd like to avoid) like this.
If you could also drop the build changes, or seperate them to another PR that'd be helpful.
robotdana
force-pushed
the
error_exit_code
branch
from
18f43bc
to
07598ad
Compare
|
the build changes are now in #2750 |
JonRowe
reviewed
Aug 14, 2020
JonRowe
reviewed
Aug 14, 2020
JonRowe
reviewed
Aug 14, 2020
Thanks, you can see this PR actually passed without them |
robotdana
force-pushed
the
error_exit_code
branch
from
07598ad
to
5beca64
Compare
JonRowe
approved these changes
Aug 14, 2020
it can be helpful to know if RSpec fails because of an example or if it errors out because it couldn't load a spec file, or there was an issue in the a before(:suite) hook or etc i've named the setting error-exit-code, and tried to add it to all the relevant places, but i don't know rspec codebase well so there was some guessing. in particular i'm not sure what bisect should do. i have it fall back to the failure-exit-code before defaulting to 1 so there's no changes if people don't opt in to the setting. the specific use of this is our CI automatically retries failures using the persistence file, and assumes if they pass on that retry they were flaky. however, the persistence file isn't written to when there's an error outside of examples, so this could mean falsely passing builds. by checking for a different exit code, and then not running the retry we can avoid this issue.
robotdana
force-pushed
the
error_exit_code
branch
from
5beca64
to
0709dcb
Compare
JonRowe
approved these changes
Aug 14, 2020
|
@robotdana I'm going to merge this if you're done with it? |
|
yeah thank you :) |
|
Thanks, @robotdana ! |
MatheusRich
pushed a commit
to MatheusRich/rspec-core
that referenced
this pull request
Oct 30, 2020
Add error-exit-code to differentiate from failures
MatheusRich
pushed a commit
to MatheusRich/rspec-core
that referenced
this pull request
Oct 30, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
it can be helpful to know if RSpec fails because of an example or if it
errors out because it couldn't load a spec file, or there was an issue
in the a before(:suite) hook or etc
i've named the setting error-exit-code, and tried to add it to all the
relevant places, but i don't know rspec codebase well so there was some
guessing. in particular i'm not sure what bisect should do.
i have it fall back to the failure-exit-code before defaulting to 1 so
there's no changes if people don't opt in to the setting.
the specific usecase of this is our CI automatically retries failures using
the persistence file, and assumes if they pass on that retry they were
flaky. however, the persistence file isn't written to when there's an
error outside of examples, so this could mean falsely passing builds.
by checking for a different exit code, and then not running the retry we
can avoid this issue.
This PR also includes me trying to make travis builds work. there were gem/ruby version issues with simplecov-html, childprocecss, & rake (these issues were also present in other RSpec projects, so when it tries to build e.g. rspec-expectations it still fails. that's when i gave up trying to make travis work)