Stop rescuing all exceptions. #2063
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
| @@ -178,4 +178,12 @@ def self.const_missing(name) | |||
| require MODULES_TO_AUTOLOAD.fetch(name) { return super } | |||
| ::RSpec.const_get(name) | |||
| end | |||
|
|
|||
| module NonPassthroughExceptions | |||
|
no idea ... if only we had a bisect feature... :P |
Tried it. Here's the output so far: |
|
OK, I figured out the source of the problem.
I'm not sure on what the right fix for this is. We could potentially treat Any thoughts, @rspec/rspec? @SephVelut, as the one who opened #2058, what do you think? |
|
Good dig into the problem @myronmarston! Testing a process that happens to fork and exit on the same tty - with a stack that contains callbacks to rspec - is definitely a gotcha. You'll wonder why you're getting multiple runs of the tests. But I think its probably an unavoidable gotcha when testing forks. Tracking the pid on the reporter is an interesting idea. But I can also se why
I'm inclined to agree. I suppose even a feature for 'turning off' reporting in subprocess that exits won't be a better solution than simply using Here is my current suggestion. Lets add a note to gotchas and suggest manually rescuing |
|
I'm not sure if we need it for when users are running specs via the I tried removing the Removing the
Mine, too. (Particularly since I've not heard of anyone else running into this gotcha).
What gotchas list do you want a note added to? You linked to capybara. Anyhow, what you suggested made me realize this would work: RSpec.configure do |c|
c.around(:example) do |ex|
begin
ex.run
rescue SystemExit
exit!
end
end
endYou could use a hook like this to transform an As for this PR...regardless of if we do anything in particular for the |
|
I looked a bit more into the
|
|
fwiw I always wanted
Agree
Legit. |
It's not required to do it twice (or at least it shouldn't be). When you hit In my experience, the 1st ctrl-c does the cleanup exit pretty quickly as long as it's not a large project with lots of cleanup to perform. I just tried it while running Your projects are perhaps just quite large? Or maybe you've hit a genuine bug no one's reported where RSpec isn't able to exit on the first ctrl-c for some reason. |
|
it's the large projects thing. Actually when I think about it the current behaviour is a good default for most people. |
Yeah, my bad. In any case this thread's existence is probably good enough as I'd like to think most people would eventually conduct a search on the issue page. |
myronmarston
force-pushed
the
stop-rescuing-all-exceptions
branch
from
8cada17
to
9672c83
Compare
This was referenced Aug 29, 2015
myronmarston
force-pushed
the
stop-rescuing-all-exceptions
branch
from
9672c83
to
3315bf1
Compare
| @@ -227,14 +227,14 @@ def run(example_group_instance, reporter) | |||
| rescue Pending::SkipDeclaredInExample | |||
| # no-op, required metadata has already been set by the `skip` | |||
| # method. | |||
| rescue Exception => e | |||
| rescue Support::AllExceptionsExceptOnesWeMustNotRescue => e | |||
There was a problem hiding this comment.
I don't understand why, but somehow this change causes a failure for this cucumber scenario on 1.8.7:
https://travis-ci.org/rspec/rspec-core/jobs/77979590
If I change this back to Exception that passes. Makes absolutely no sense to me :(.
Anyone got any ideas why?
myronmarston
force-pushed
the
stop-rescuing-all-exceptions
branch
3 times, most recently
from
8cfd75e
to
bfea12d
Compare
We do want to generally rescue `Exception` and any subclasses but there are a few exceptions we should not rescue. For example, if we rescue `SystemExit`, it prevents `exit` from exiting. Part of #2058.
myronmarston
force-pushed
the
stop-rescuing-all-exceptions
branch
from
bfea12d
to
f2d40c1
Compare
|
This is finally green! Anyone from @rspec/rspec want to do a final review before I merge? |
| set_exception(e) | ||
| ensure | ||
| run_after_example | ||
| end | ||
| end | ||
| end | ||
| rescue Exception => e | ||
| rescue Support::AllExceptionsExceptOnesWeMustNotRescue => e |
There was a problem hiding this comment.
Namespaced here but not above?
There was a problem hiding this comment.
The one above is RSpec::Core::AllExceptionsExcludingDangerousOnesOnRubiesThatAllowIt, not a constant from the RSpec::Support namespace. See the definition and comment below. Yes, it's weird.
|
LGTM |
myronmarston
added a commit
that referenced
this pull request
Oct 7, 2015
Stop rescuing all exceptions.
MatheusRich
pushed a commit
to MatheusRich/rspec-core
that referenced
this pull request
Oct 30, 2020
Stop rescuing all exceptions.
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.
We do want to generally rescue
Exceptionandany subclasses but there are a few exceptions
we should not rescue. For example, if we rescue
SystemExit, it preventsexitfrom exiting.Attempted fix for #2058.
Note that the spec I added failed without these changes and passes with it...but there are some other weird effects that I do not understand:
I'm a bit at a loss for what's causing this. Anyone from @rspec/rspec got any ideas?