Skip to content

Handle errors in :suite hooks.

gitlab-qa-bot requested to merge myron/handle-errors-in-suite-hooks into master

Created by: myronmarston

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.

Notes/open questions:

  • The first 4 commits are refactoring commits that pave the way for this change. If desired, I can put those into a separate PR so it can be reviewed and merged separately, then this PR can be rebased on top. (Whoever reviews this, please let me know if you'd like me to do that).
  • There are a few other things I'm thinking of giving this kind of treatment:
    • Errors raised while loading spec files: instead of crashing RSpec, have it print a nice failure like we do here.
    • Errors raised in 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 #2084 (closed) 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 an after(:context) hook fails.
  • I decided to hold off on these additional changes for now -- they can be in later PRs.
  • The changes here (and in any followup PRs for 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 of after(: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.
  • This change was prompted by running into this for the new RSpec book I'm working on, and I need this in a release before the book is done (but not necessarily the changes for after(:context and 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

Merge request reports