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:

- 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.

Author: myronmarston

Changelog.md

@@ -19,6 +19,8 @@ Bug Fixes:
   conflicting keys if the value in the host group was inherited from
   a parent group instead of being specified at that level.
   (Myron Marston, #2307)
+* Handle errors in `:suite` hooks and provided the same nicely formatted
+  output as errors that happen in examples. (Myron Marston, #2316)
 
 ### 3.5.2 / 2016-07-28
 [Full Changelog](http://github.com/rspec/rspec-core/compare/v3.5.1...v3.5.2)

lib/rspec/core/configuration.rb

@@ -1835,12 +1835,11 @@ def around(scope=nil, *meta, &block)
       def with_suite_hooks
         return yield if dry_run?
 
-        hook_context = SuiteHookContext.new
         begin
-          run_hooks_with(@before_suite_hooks, hook_context)
+          run_suite_hooks("a `before(:suite)` hook", @before_suite_hooks)
           yield
         ensure
-          run_hooks_with(@after_suite_hooks, hook_context)
+          run_suite_hooks("an `after(:suite)` hook", @after_suite_hooks)
         end
       end
 
@@ -1880,8 +1879,16 @@ def handle_suite_hook(scope, meta)
         yield
       end
 
-      def run_hooks_with(hooks, hook_context)
-        hooks.each { |h| h.run(hook_context) }
+      def run_suite_hooks(hook_description, hooks)
+        context = SuiteHookContext.new(hook_description, reporter)
+
+        hooks.each do |hook|
+          begin
+            hook.run(context)
+          rescue Support::AllExceptionsExceptOnesWeMustNotRescue => ex
+            context.set_exception(ex)
+          end
+        end
       end
 
       def get_files_to_run(paths)

lib/rspec/core/example.rb

@@ -632,16 +632,16 @@ def issue_deprecation(_method_name, *_args)
     # @private
     # Provides an execution context for before/after :suite hooks.
     class SuiteHookContext < Example
-      def initialize
-        super(AnonymousExampleGroup, "", {})
+      def initialize(hook_description, reporter)
+        super(AnonymousExampleGroup, hook_description, {})
         @example_group_instance = AnonymousExampleGroup.new
+        @reporter = reporter
       end
 
       # rubocop:disable Style/AccessorMethodName
-
-      # To ensure we don't silence errors.
       def set_exception(exception)
-        raise exception
+        reporter.notify_non_example_exception(exception, "An error occurred in #{description}.")
+        RSpec.world.wants_to_quit = true
       end
       # rubocop:enable Style/AccessorMethodName
     end

lib/rspec/core/reporter.rb

@@ -156,6 +156,8 @@ def deprecation(hash)
     # particular exception (such as an exception encountered in a :suite hook).
     # Exceptions will be formatted the same way they normally are.
     def notify_non_example_exception(exception, context_description)
+      @configuration.world.non_example_failure = true
+
       example = Example.new(AnonymousExampleGroup, context_description, {})
       presenter = Formatters::ExceptionPresenter.new(exception, example, :indentation => 0)
       message presenter.fully_formatted(nil)

lib/rspec/core/runner.rb

@@ -109,14 +109,17 @@ def setup(err, out)
       #   failed.
       def run_specs(example_groups)
         examples_count = @world.example_count(example_groups)
-        @configuration.reporter.report(examples_count) do |reporter|
+        success = @configuration.reporter.report(examples_count) do |reporter|
           @configuration.with_suite_hooks do
             if examples_count == 0 && @configuration.fail_if_no_examples
               return @configuration.failure_exit_code
             end
-            example_groups.map { |g| g.run(reporter) }.all? ? 0 : @configuration.failure_exit_code
+
+            example_groups.map { |g| g.run(reporter) }.all?
           end
-        end
+        end && !@world.non_example_failure
+
+        success ? 0 : @configuration.failure_exit_code
       end
 
     private

lib/rspec/core/world.rb

@@ -10,6 +10,12 @@ class World
       # Used internally to determine what to do when a SIGINT is received.
       attr_accessor :wants_to_quit
 
+      # Used internally to signal that a failure outside of an example
+      # has occurred, and that therefore the exit status should indicate
+      # the run failed.
+      # @private
+      attr_accessor :non_example_failure
+
       def initialize(configuration=RSpec.configuration)
         @configuration = configuration
         configuration.world = self
@@ -224,6 +230,9 @@ def fail_if_config_and_cli_options_invalid
       # @private
       # Provides a null implementation for initial use by configuration.
       module Null
+        def self.non_example_failure; end
+        def self.non_example_failure=(_); end
+
         def self.registered_example_group_files
           []
         end

spec/integration/suite_hooks_errors_spec.rb

@@ -0,0 +1,139 @@
+require 'support/aruba_support'
+require 'support/formatter_support'
+
+RSpec.describe 'Suite hook errors' do
+  include_context "aruba support"
+  include FormatterSupport
+
+  let(:failure_exit_code) { rand(97) + 2 } # 2..99
+
+  if RSpec::Support::Ruby.jruby_9000?
+    let(:spec_line_suffix) { ":in `block in (root)'" }
+  elsif RSpec::Support::Ruby.jruby?
+    let(:spec_line_suffix) { ":in `(root)'" }
+  elsif RUBY_VERSION == "1.8.7"
+    let(:spec_line_suffix) { "" }
+  else
+    let(:spec_line_suffix) { ":in `block (2 levels) in <top (required)>'" }
+  end
+
+  before do
+    # get out of `aruba` sub-dir so that `filter_gems_from_backtrace 'aruba'`
+    # below does not filter out our spec file.
+    expect(dirs.pop).to eq "aruba"
+
+    clean_current_dir
+
+    RSpec.configure do |c|
+      c.filter_gems_from_backtrace "aruba"
+      c.backtrace_exclusion_patterns << %r{/rspec-core/spec/} << %r{rspec_with_simplecov}
+      c.failure_exit_code = failure_exit_code
+    end
+  end
+
+  def run_spec_expecting_non_zero(before_or_after)
+    write_file "the_spec.rb", "
+      RSpec.configure do |c|
+        c.#{before_or_after}(:suite) do
+          raise 'boom'
+        end
+      end
+
+      RSpec.describe do
+        it { }
+      end
+    "
+
+    run_command "the_spec.rb"
+    expect(last_cmd_exit_status).to eq(failure_exit_code)
+    normalize_durations(last_cmd_stdout)
+  end
+
+  it 'nicely formats errors in `before(:suite)` hooks and exits with non-zero' do
+    output = run_spec_expecting_non_zero(:before)
+    expect(output).to eq unindent(<<-EOS)
+
+      An error occurred in a `before(:suite)` hook.
+      Failure/Error: raise 'boom'
+
+      RuntimeError:
+        boom
+      # ./the_spec.rb:4#{spec_line_suffix}
+
+
+      Finished in n.nnnn seconds (files took n.nnnn seconds to load)
+      0 examples, 0 failures
+    EOS
+  end
+
+  it 'nicely formats errors in `after(:suite)` hooks and exits with non-zero' do
+    output = run_spec_expecting_non_zero(:after)
+    expect(output).to eq unindent(<<-EOS)
+      .
+      An error occurred in an `after(:suite)` hook.
+      Failure/Error: raise 'boom'
+
+      RuntimeError:
+        boom
+      # ./the_spec.rb:4#{spec_line_suffix}
+
+
+      Finished in n.nnnn seconds (files took n.nnnn seconds to load)
+      1 example, 0 failures
+    EOS
+  end
+
+  it 'nicely formats errors from multiple :suite hooks of both types and exits with non-zero' do
+    write_file "the_spec.rb", "
+      RSpec.configure do |c|
+        c.before(:suite) { raise 'before 1' }
+        c.before(:suite) { raise 'before 2' }
+        c.after(:suite) { raise 'after 1' }
+        c.after(:suite) { raise 'after 2' }
+      end
+
+      RSpec.describe do
+        it { }
+      end
+    "
+
+    run_command "the_spec.rb"
+    expect(last_cmd_exit_status).to eq(failure_exit_code)
+    output = normalize_durations(last_cmd_stdout)
+
+    expect(output).to eq unindent(<<-EOS)
+
+      An error occurred in a `before(:suite)` hook.
+      Failure/Error: c.before(:suite) { raise 'before 1' }
+
+      RuntimeError:
+        before 1
+      # ./the_spec.rb:3#{spec_line_suffix}
+
+      An error occurred in a `before(:suite)` hook.
+      Failure/Error: c.before(:suite) { raise 'before 2' }
+
+      RuntimeError:
+        before 2
+      # ./the_spec.rb:4#{spec_line_suffix}
+
+      An error occurred in an `after(:suite)` hook.
+      Failure/Error: c.after(:suite) { raise 'after 2' }
+
+      RuntimeError:
+        after 2
+      # ./the_spec.rb:6#{spec_line_suffix}
+
+      An error occurred in an `after(:suite)` hook.
+      Failure/Error: c.after(:suite) { raise 'after 1' }
+
+      RuntimeError:
+        after 1
+      # ./the_spec.rb:5#{spec_line_suffix}
+
+
+      Finished in n.nnnn seconds (files took n.nnnn seconds to load)
+      0 examples, 0 failures
+    EOS
+  end
+end

spec/rspec/core/reporter_spec.rb

@@ -3,6 +3,7 @@ module RSpec::Core
     include FormatterSupport
 
     let(:config)   { Configuration.new }
+    let(:world)    { World.new(config) }
     let(:reporter) { Reporter.new config }
     let(:start_time) { Time.now }
     let(:example) { super() }
@@ -283,6 +284,10 @@ module RSpec::Core
 
     describe "#notify_non_example_exception" do
       it "sends a `message` notification that contains the formatted exception details" do
+        if RSpec::Support::Ruby.jruby_9000?
+          pending "RSpec gets `Unable to find matching line from backtrace` on JRuby 9000"
+        end
+
         formatter_out = StringIO.new
         formatter = Formatters::ProgressFormatter.new(formatter_out)
         reporter.register_listener formatter, :message
@@ -301,6 +306,12 @@ module RSpec::Core
           |# #{Metadata.relative_path(__FILE__)}:#{line}
         EOS
       end
+
+      it "records the fact that a non example failure has occurred" do
+        expect {
+          reporter.notify_non_example_exception(Exception.new, "NonExample Context")
+        }.to change(world, :non_example_failure).from(a_falsey_value).to(true)
+      end
     end
   end
 end

spec/rspec/core/suite_hooks_spec.rb

@@ -15,12 +15,23 @@ module RSpec::Core
           }.not_to yield_control
         end
 
-        it 'allows errors in the hook to propagate to the user' do
+        it 'notifies about errors in the hook' do
           RSpec.configuration.__send__(registration_method, :suite) { 1 / 0 }
 
+          expect(RSpec.configuration.reporter).to receive(:notify_non_example_exception).with(
+            ZeroDivisionError, /suite\)` hook/
+          )
+
+          RSpec.configuration.with_suite_hooks { }
+        end
+
+        it 'sets `wants_to_quit` when an error occurs so that the suite does not get run' do
+          RSpec.configuration.__send__(registration_method, :suite) { 1 / 0 }
+          allow(RSpec.configuration.reporter).to receive(:notify_non_example_exception)
+
           expect {
             RSpec.configuration.with_suite_hooks { }
-          }.to raise_error(ZeroDivisionError)
+          }.to change(RSpec.world, :wants_to_quit).from(a_falsey_value).to(true)
         end
 
         it 'runs in the context of an example group' do