Handle errors in :suite hooks. #2316
Conversation
This is necessary for cases where the error is not part of a numbered sequence (such as a :suite hook error).
Previously, a second `exception_presenter` argument was required.
myronmarston
force-pushed
the
myron/handle-errors-in-suite-hooks
branch
4 times, most recently
from
73af50a to
6c3b4c6
Compare
Previously, we just allowed the error to propagate to the user, which was a subpar experience for a few reasons: - The error would cause RSpec to crash, leading to a long stack trace containing lots of extraneous info the user did not see. - The error was not formatted nicely like other errors that happen while RSpec runs. - If the error happened in an `after(:suite)` hook, the test suite had finished running all specs but the summary of the results would not get printed since RSpec had crashed. Now, we handle errors in :suite hooks and format the output the same way failures and errors are normally printed.
myronmarston
force-pushed
the
myron/handle-errors-in-suite-hooks
branch
from
6c3b4c6 to
77f8b01
Compare
| @@ -152,6 +152,18 @@ def deprecation(hash) | |||
| end | |||
|
|
|||
| # @private | |||
| # Provides a way to notify of an exception that is not tied to any | |||
| # particular exception (such as an exception encountered in a :suite hook). | |||
There was a problem hiding this comment.
First exception should read example I think?
|
LGTM, only comments were nits |
- s/exception/example/ in a comment - Do not run later `before(:suite)` hooks when an earlier one has failed. This aligns with `before(:example)` and `before(:context)` hooks. However, `after(:suite)` hooks all get run even if one fails, which again aligns with `after(:example)` and `after(:suite)` hooks.
Thanks for the review. Do you have thoughts with regard to the comments/questions I posed above about SemVer categorization? |
|
bugfix makes sense. Theory aside, I can't think of practical situations where anyone would be relying on this as a feature. |
|
I can't remember the specific issue, but I feel like we considered a similar situation in the past ("this is a bugfix that might break things relying on the bug") and ended up deciding to stick with bugfix. |
|
Cool. Good to merge? |
|
The first bullet point in this commit message is a bit ambiguous to me.
I think what you meant to say is:
Otherwise LGTM 👍. |
|
Awesome work! :) |
|
Cherry picked to 3-5-maintenance, FWIW. |
…-hooks Handle errors in :suite hooks.
Previously, we just allowed the error to propagate to the user, which was a subpar experience for a few reasons:
after(:suite)hook, the test suite had finished running all specs but the summary of the results would not get printed since RSpec had crashed.Now, we handle errors in :suite hooks and format the output the same way failures and errors are normally printed.
Notes/open questions:
after(:context)hooks. Right now the output for these is not nice. If we have errors there notify via the new reporter method I've added in this PR, it'll solve Error in after(:all) hook doesn't fail rspec #2084 w/o the introduction of a config option as discussed there. It could be considered a breaking change, though, although it would be reasonable to consider it a bug that RSpec exits with 0 when all specs pass and anafter(:context)hook fails.after(:context)and load-time errors) are a bit ambiguous in terms of SemVer. I could see an argument for treating them as bug fixes (e.g. RSpec didn't handle errors in these situations properly before, allowing them to crash RSpec) or as enhancements (we're improving error reporting in these situations) or as a breaking change (in the case ofafter(:context)-- it's a situation where the suite for some users currently passes but would be reported as failing if we make that change). For now I put this as a bug fix in the changelog.after(:contextand load-time errors). It'd be nice to get this in 3.5.x so I can update the book to take advantage right away, instead of waiting for 3.6./cc @rspec/rspec