Hook refactoring plus suite hook fixes

Changelog.md

@@ -22,6 +22,9 @@ Enhancements:
 * Improve the documentation of around hooks. (Jim Kingdon, #1772)
 * Support prepending of modules into example groups from config and allow
   filtering based on metadata. (Arlandis Word, #1806)
+* Emit warnings when `:suite` hooks are registered on an example group
+  (where it has always been ignored) or are registered with metadata
+  (which has always been ignored). (Myron Marston, #1805)
 
 Bug Fixes:
 

benchmarks/allocations/1000_groups_1_example.rb

@@ -15,23 +15,51 @@
 
                class_plus                 count
 ----------------------------------------  -----
-String                                    32000
-Array                                     14000
+String                                    28000
+Array                                     15000
 RubyVM::Env                                9000
-Hash                                       9000
 Proc                                       9000
+Hash                                       9000
 RSpec::Core::Hooks::HookCollection         6000
 Array<String>                              5000
 MatchData                                  3000
-RSpec::Core::Example::ExecutionResult      2000
 Array<String,Fixnum>                       2000
 Array<Module>                              2000
 Module                                     2000
+RSpec::Core::Example::ExecutionResult      2000
+RSpec::Core::Metadata::ExampleGroupHash    1000
 Class                                      1000
 Array<Hash>                                1000
-RSpec::Core::Metadata::ExampleGroupHash    1000
+RSpec::Core::Hooks::AroundHookCollection   1000
+RSpec::Core::Hooks::HookCollections        1000
 RSpec::Core::Metadata::ExampleHash         1000
 RSpec::Core::Example                       1000
 Array<RSpec::Core::Example>                1000
-RSpec::Core::Hooks::AroundHookCollection   1000
+
+
+After removing `:suite` support from `Hooks` module,
+it cut Array and RSpec::Core::Hooks::HookCollection
+allocations by 2000 each:
+
+               class_plus                 count
+----------------------------------------  -----
+String                                    28000
+Array                                     13000
+Proc                                       9000
+RubyVM::Env                                9000
+Hash                                       9000
+Array<String>                              5000
+RSpec::Core::Hooks::HookCollection         4000
+MatchData                                  3000
+Array<Module>                              2000
+RSpec::Core::Example::ExecutionResult      2000
+Module                                     2000
+Array<String,Fixnum>                       2000
 RSpec::Core::Hooks::HookCollections        1000
+RSpec::Core::Example                       1000
+Array<RSpec::Core::Example>                1000
+RSpec::Core::Metadata::ExampleHash         1000
+RSpec::Core::Hooks::AroundHookCollection   1000
+RSpec::Core::Metadata::ExampleGroupHash    1000
+Class                                      1000
+Array<Hash>                                1000

lib/rspec/core/configuration.rb

@@ -1411,8 +1411,111 @@ def apply_derived_metadata_to(metadata)
         end
       end
 
+      # Defines a `before` hook. See {Hooks#before} for full docs.
+      #
+      # This method differs from {Hooks#before} in only one way: it supports
+      # the `:suite` scope. Hooks with the `:suite` scope will be run once before
+      # the first example of the entire suite is executed.
+      #
+      # @see #prepend_before
+      # @see #after
+      # @see #append_after
+      def before(*args, &block)
+        handle_suite_hook(args, before_suite_hooks, :append,
+                          Hooks::BeforeHook, block) || super(*args, &block)
+      end
+      alias_method :append_before, :before
+
+      # Adds `block` to the start of the list of `before` blocks in the same
+      # scope (`:example`, `:context`, or `:suite`), in contrast to {#before},
+      # which adds the hook to the end of the list.
+      #
+      # See {Hooks#before} for full `before` hook docs.
+      #
+      # This method differs from {Hooks#prepend_before} in only one way: it supports
+      # the `:suite` scope. Hooks with the `:suite` scope will be run once before
+      # the first example of the entire suite is executed.
+      #
+      # @see #before
+      # @see #after
+      # @see #append_after
+      def prepend_before(*args, &block)
+        handle_suite_hook(args, before_suite_hooks, :prepend,
+                          Hooks::BeforeHook, block) || super(*args, &block)
+      end
+
+      # Defines a `after` hook. See {Hooks#after} for full docs.
+      #
+      # This method differs from {Hooks#after} in only one way: it supports
+      # the `:suite` scope. Hooks with the `:suite` scope will be run once after
+      # the last example of the entire suite is executed.
+      #
+      # @see #append_after
+      # @see #before
+      # @see #prepend_before
+      def after(*args, &block)
+        handle_suite_hook(args, after_suite_hooks, :prepend,
+                          Hooks::AfterHook, block) || super(*args, &block)
+      end
+      alias_method :prepend_after, :after
+
+      # Adds `block` to the end of the list of `after` blocks in the same
+      # scope (`:example`, `:context`, or `:suite`), in contrast to {#after},
+      # which adds the hook to the start of the list.
+      #
+      # See {Hooks#after} for full `after` hook docs.
+      #
+      # This method differs from {Hooks#append_after} in only one way: it supports
+      # the `:suite` scope. Hooks with the `:suite` scope will be run once after
+      # the last example of the entire suite is executed.
+      #
+      # @see #append_after
+      # @see #before
+      # @see #prepend_before
+      def append_after(*args, &block)
+        handle_suite_hook(args, after_suite_hooks, :append,
+                          Hooks::AfterHook, block) || super(*args, &block)
+      end
+
+      # @private
+      def with_suite_hooks
+        return yield if dry_run?
+
+        hook_context = SuiteHookContext.new
+        begin
+          before_suite_hooks.with(hook_context).run
+          yield
+        ensure
+          after_suite_hooks.with(hook_context).run
+        end
+      end
+
     private
 
+      def handle_suite_hook(args, collection, append_or_prepend, hook_type, block)
+        scope, meta = *args
+        return nil unless scope == :suite
+
+        if meta
+          # TODO: in RSpec 4, consider raising an error here.
+          # We warn only for backwards compatibility.
+          RSpec.warn_with "WARNING: `:suite` hooks do not support metadata since " \
+                          "they apply to the suite as a whole rather than " \
+                          "any individual example or example group that has metadata. " \
+                          "The metadata you have provided (#{meta.inspect}) will be ignored."
+        end
+
+        collection.__send__(append_or_prepend, hook_type.new(block, {}))
+      end
+
+      def before_suite_hooks
+        @before_suite_hooks ||= Hooks::HookCollection.new
+      end
+
+      def after_suite_hooks
+        @after_suite_hooks ||= Hooks::HookCollection.new
+      end
+
       def get_files_to_run(paths)
         FlatMap.flat_map(paths_to_check(paths)) do |path|
           path = path.gsub(File::ALT_SEPARATOR, File::SEPARATOR) if File::ALT_SEPARATOR

lib/rspec/core/example_group.rb

@@ -441,26 +441,24 @@ def self.store_before_context_ivars(example_group_instance)
 
       # @private
       def self.run_before_context_hooks(example_group_instance)
-        return if descendant_filtered_examples.empty?
-        begin
-          set_ivars(example_group_instance, superclass.before_context_ivars)
+        set_ivars(example_group_instance, superclass.before_context_ivars)
 
-          ContextHookMemoizedHash::Before.isolate_for_context_hook(example_group_instance) do
-            hooks.run(:before, :context, example_group_instance)
-          end
-        ensure
-          store_before_context_ivars(example_group_instance)
+        ContextHookMemoizedHash::Before.isolate_for_context_hook(example_group_instance) do
+          hooks.run(:before, :context, example_group_instance)
         end
+      ensure
+        store_before_context_ivars(example_group_instance)
       end
 
       # @private
       def self.run_after_context_hooks(example_group_instance)
-        return if descendant_filtered_examples.empty?
         set_ivars(example_group_instance, before_context_ivars)
 
         ContextHookMemoizedHash::After.isolate_for_context_hook(example_group_instance) do
           hooks.run(:after, :context, example_group_instance)
         end
+      ensure
+        before_context_ivars.clear
       end
 
       # Runs all the examples in this group.
@@ -471,9 +469,9 @@ def self.run(reporter)
         end
         reporter.example_group_started(self)
 
+        should_run_context_hooks = descendant_filtered_examples.any?
         begin
-          instance = new('before(:context) hook')
-          run_before_context_hooks(instance)
+          run_before_context_hooks(new('before(:context) hook')) if should_run_context_hooks
           result_for_this_group = run_examples(reporter)
           results_for_descendants = ordering_strategy.order(children).map { |child| child.run(reporter) }.all?
           result_for_this_group && results_for_descendants
@@ -483,9 +481,7 @@ def self.run(reporter)
           RSpec.world.wants_to_quit = true if fail_fast?
           for_filtered_examples(reporter) { |example| example.fail_with_exception(reporter, ex) }
         ensure
-          instance = new('after(:context) hook')
-          run_after_context_hooks(instance)
-          before_context_ivars.clear
+          run_after_context_hooks(new('after(:context) hook')) if should_run_context_hooks
           reporter.example_group_finished(self)
         end
       end

lib/rspec/core/hooks.rb

@@ -184,6 +184,9 @@ module Hooks
       #
       # @note The `:example` and `:context` scopes are also available as
       #       `:each` and `:all`, respectively. Use whichever you prefer.
+      # @note The `:scope` alias is only supported for hooks registered on
+      #       `RSpec.configuration` since they exist independently of any
+      #       example or example group.
       def before(*args, &block)
         hooks.register :append, :before, *args, &block
       end
@@ -255,6 +258,9 @@ def prepend_before(*args, &block)
       #
       # @note The `:example` and `:context` scopes are also available as
       #       `:each` and `:all`, respectively. Use whichever you prefer.
+      # @note The `:scope` alias is only supported for hooks registered on
+      #       `RSpec.configuration` since they exist independently of any
+      #       example or example group.
       def after(*args, &block)
         hooks.register :prepend, :after, *args, &block
       end
@@ -326,12 +332,8 @@ def hooks
         @hooks ||= HookCollections.new(
           self,
           :around => { :example => AroundHookCollection.new },
-          :before => { :example => HookCollection.new,
-                       :context => HookCollection.new,
-                       :suite => HookCollection.new },
-          :after  => { :example => HookCollection.new,
-                       :context => HookCollection.new,
-                       :suite => HookCollection.new }
+          :before => { :example => HookCollection.new, :context => HookCollection.new },
+          :after  => { :example => HookCollection.new, :context => HookCollection.new }
         )
       end
 
@@ -496,6 +498,17 @@ def around_example_hooks_for(example, initial_procsy=nil)
 
         def register(prepend_or_append, hook, *args, &block)
           scope, options = scope_and_options_from(*args)
+
+          if scope == :suite
+            # TODO: consider making this an error in RSpec 4. For SemVer reasons,
+            # we are only warning in RSpec 3.
+            RSpec.warn_with "WARNING: `#{hook}(:suite)` hooks are only supported on " \
+                            "the RSpec configuration object. This " \
+                            "`#{hook}(:suite)` hook, registered on an example " \
+                            "group, will be ignored."
+            return
+          end
+
           self[hook][scope].__send__(prepend_or_append,
                                      HOOK_TYPES[hook][scope].new(block, options))
         end
@@ -509,7 +522,7 @@ def run(hook, scope, example_or_group, initial_procsy=nil)
           find_hook(hook, scope, example_or_group, initial_procsy).run
         end
 
-        SCOPES = [:example, :context, :suite]
+        SCOPES = [:example, :context]
 
         SCOPE_ALIASES = { :each => :example, :all => :context }
 
@@ -532,6 +545,7 @@ def process(host, globals, position, scope)
         end
 
         def scope_and_options_from(*args)
+          return :suite if args.first == :suite
           scope = extract_scope_from(args)
           meta  = Metadata.build_hash_from(args, :warn_about_example_group_filtering)
           return scope, meta
@@ -573,8 +587,6 @@ def find_hook(hook, scope, example_or_group, initial_procsy)
             before_example_hooks_for(example_or_group)
           when [:after, :example]
             after_example_hooks_for(example_or_group)
-          when [:before, :suite], [:after, :suite]
-            self[hook][:suite].with(example_or_group)
           end
         end
 

lib/rspec/core/runner.rb

@@ -106,12 +106,8 @@ def setup(err, out)
       #   failed.
       def run_specs(example_groups)
         @configuration.reporter.report(@world.example_count(example_groups)) do |reporter|
-          begin
-            hook_context = SuiteHookContext.new
-            @configuration.hooks.run(:before, :suite, hook_context)
+          @configuration.with_suite_hooks do
             example_groups.map { |g| g.run(reporter) }.all? ? 0 : @configuration.failure_exit_code
-          ensure
-            @configuration.hooks.run(:after, :suite, hook_context)
           end
         end
       end

spec/rspec/core/hooks_spec.rb

@@ -48,7 +48,7 @@ def parent_groups
     end
 
     [:before, :after].each do |type|
-      [:example, :context, :suite].each do |scope|
+      [:example, :context].each do |scope|
         describe "##{type}(#{scope.inspect})" do
           let(:instance) { HooksHost.new }
           let!(:hook) do
@@ -76,26 +76,6 @@ def parent_groups
       end
     end
 
-    context "when an error happens in `after(:suite)`" do
-      it 'allows the error to propagate to the user' do
-        RSpec.configuration.after(:suite) { 1 / 0 }
-
-        expect {
-          RSpec.configuration.hooks.run(:after, :suite, SuiteHookContext.new)
-        }.to raise_error(ZeroDivisionError)
-      end
-    end
-
-    context "when an error happens in `before(:suite)`" do
-      it 'allows the error to propagate to the user' do
-        RSpec.configuration.before(:suite) { 1 / 0 }
-
-        expect {
-          RSpec.configuration.hooks.run(:before, :suite, SuiteHookContext.new)
-        }.to raise_error(ZeroDivisionError)
-      end
-    end
-
     describe "#around" do
       context "when it does not run the example" do
         context "for a hook declared in the group" do