Merge pull request #2316 from rspec/myron/handle-errors-in-suite-hooks

Handle errors in :suite hooks.

Author: JonRowe

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,22 @@ 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)
+
+            # Do not run subsequent `before` hooks if one fails.
+            # But for `after` hooks, we run them all so that all
+            # cleanup bits get a chance to complete, minimizing the
+            # chance that resources get left behind.
+            break if hooks.equal?(@before_suite_hooks)
+          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/formatters/exception_presenter.rb

@@ -122,7 +122,8 @@ def encoded_string(string)
         end
 
         def indent_lines(lines, failure_number)
-          alignment_basis = "#{' ' * @indentation}#{failure_number}) "
+          alignment_basis = ' ' * @indentation
+          alignment_basis <<  "#{failure_number}) " if failure_number
           indentation = ' ' * alignment_basis.length
 
           lines.each_with_index.map do |line, index|

lib/rspec/core/metadata.rb

@@ -178,6 +178,7 @@ def description_separator(parent_part, child_part)
 
         def build_description_from(parent_description=nil, my_description=nil)
           return parent_description.to_s unless my_description
+          return my_description.to_s if parent_description.to_s == ''
           separator = description_separator(parent_description, my_description)
           (parent_description.to_s + separator) << my_description.to_s
         end

lib/rspec/core/notifications.rb

@@ -51,8 +51,7 @@ def self.for(example)
                   FailedExampleNotification
                 end
 
-        exception_presenter = Formatters::ExceptionPresenter::Factory.new(example).build
-        klass.new(example, exception_presenter)
+        klass.new(example)
       end
 
       private_class_method :new
@@ -202,7 +201,7 @@ def fully_formatted(failure_number, colorizer=::RSpec::Core::Formatters::Console
 
     private
 
-      def initialize(example, exception_presenter)
+      def initialize(example, exception_presenter=Formatters::ExceptionPresenter::Factory.new(example).build)
         @exception_presenter = exception_presenter
         super(example)
       end

lib/rspec/core/reporter.rb

@@ -151,6 +151,18 @@ def deprecation(hash)
       notify :deprecation, Notifications::DeprecationNotification.from_hash(hash)
     end
 
+    # @private
+    # Provides a way to notify of an exception that is not tied to any
+    # particular example (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)
+    end
+
     # @private
     def finish
       close_after do

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,132 @@
+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 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/formatters/exception_presenter_spec.rb

@@ -51,6 +51,18 @@ module RSpec::Core
         EOS
       end
 
+      it "prints no identifier when no number argument is given" do
+        expect(presenter.fully_formatted(nil)).to eq(<<-EOS.gsub(/^ +\|/, ''))
+          |
+          |  Example
+          |  Failure/Error: # The failure happened here!#{ encoding_check }
+          |
+          |    Boom
+          |    Bam
+          |  # ./spec/rspec/core/formatters/exception_presenter_spec.rb:#{line_num}
+        EOS
+      end
+
       it "allows the caller to specify additional indentation" do
         the_presenter = Formatters::ExceptionPresenter.new(exception, example, :indentation => 4)
 

spec/rspec/core/hooks_spec.rb

@@ -18,6 +18,64 @@ def hook_collection_for(position, scope)
       end
     end
 
+    [:example, :context, :suite].each do |scope|
+      describe "#before(#{scope})" do
+        it "stops running subsequent hooks of the same type when an error is encountered" do
+          sequence = []
+
+          RSpec.configure do |c|
+            c.output_stream = StringIO.new
+
+            c.before(scope) do
+              sequence << :hook_1
+              raise "boom"
+            end
+
+            c.before(scope) do
+              sequence << :hook_2
+              raise "boom"
+            end
+          end
+
+          RSpec.configuration.with_suite_hooks do
+            RSpec.describe do
+              example { sequence << :example }
+            end.run
+          end
+
+          expect(sequence).to eq [:hook_1]
+        end
+      end
+
+      describe "#after(#{scope})" do
+        it "runs subsequent hooks of the same type when an error is encountered so all cleanup can complete" do
+          sequence = []
+
+          RSpec.configure do |c|
+            c.output_stream = StringIO.new
+
+            c.after(scope) do
+              sequence << :hook_2
+              raise "boom"
+            end
+
+            c.after(scope) do
+              sequence << :hook_1
+              raise "boom"
+            end
+          end
+
+          RSpec.configuration.with_suite_hooks do
+            RSpec.describe do
+              example { sequence << :example }
+            end.run
+          end
+
+          expect(sequence).to eq [:example, :hook_1, :hook_2]
+        end
+      end
+    end
+
     [:before, :after, :around].each do |type|
       [:example, :context].each do |scope|
         next if type == :around && scope == :context