-
-
Notifications
You must be signed in to change notification settings - Fork 763
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
Unhandled SystemExit causes susbequent tests to silently not run. #2246
Comments
One is to explicit expect that expect { Kernel.exit 1 }.to raise_error(SystemExit) Another is to handle this globally with an RSpec.configure do |rspec|
rspec.around(:example) do |ex|
begin
ex.run
rescue SystemExit => e
puts "Got SystemExit: #{e.inspect}. Ignoring"
end
end
end You could of course choose to convert it to a failure (instead of ignoring it) by raising a different error in the Given how easy it is to to handle this however you want, I don't think it makes sense for us to provide an option for this. After all, such an option would be less flexible than what you can already accomplish with an |
I mostly agree here, if we were to do anything to improve this behaviour it would be to shutdown rspec properly, and then continue to exit. |
This is true, and is the usual method. But in the case where someone adds a test that doesn't handle this properly, we can no longer rely on the validity of our tests - something that should always be trustworthy.
Thanks for that info - I didn't know about this, and will be doing this as a sanity check in my current scenario. However, it has a similar limitation. If you have a bunch of well behaving tests that don't generate cause for adding this in the first place and someone inadvertently adds a new test that does raise a SystemError, you can't rely on your tests running all the way through. However, carrying the line of thought further: I assume any handling for this in rspec-core would need to be opt-in to avoid breaking the forked test scenarios. And if it has to be enabled anyway, then there's no way around the need for awareness (whether setting an option, or implementing your suggestion above locally). While I still some level of default handling of SystemExit in order to minimally guarantee the validity of the test results is important, I can see how it'd introduce other difficulties around the previously fixed issue. What about ensuring it outputs something
That also seems reasonable as it would (I think) give a way of knowing that something didn't do what it was supposed to. |
I'd really prefer not to add an option to RSpec to handle
I think improving how RSpec shuts down in this situation would be a good idea. Want to take a stab at it, @marcparadise? |
@myronmarston sounds good to me, I'll take a look into it over the next couple of evenings. |
Closing as stale, please feel free to reopen with a PR! (fwiw I'm pretty negative on us doing anything here. |
See rspec/rspec-core#2246 for more info
* Catch unhandled system exits See rspec/rspec-core#2246 for more info * Fix typo * Fix argument passing to rake task * Write specs for version manager tasks
This appears related to the change for #2063.
When testing components that might be expected to call
Kernel.exit
in the normal course of operations, aSystemExit
is raised. The policy of not rescuing SystemExit in this case means any tests that were supposed to run after that point will not run -- and will not report a failure. The only indication is that the number of tests executed is lower than it should be.In automated test runs, this is almost impossible to catch - and even in local runs when the difference is a matter of just a handful of tests not being run, it's easy to miss.
Simple repro:
Result:
The easy answer is to also handle SystemExit, but that would re-introduce #2063. That led me to consider submitting a patch where a SystemExit handling option could be specified via configuration., and handled similarly to:
Another option would be a final check of number of executed tests against the number we expected to execute, failing on a mismatch. This may not catch scenarios related to nested runs though.
Before submitting a patch, I wanted to open this issue for feedback and discussion of best approach.
The text was updated successfully, but these errors were encountered: