Merge pull request rspec#2284 from rspec/myron/pr-2280-followups

PR 2280 followups

Author: JonRowe

Changelog.md

@@ -1,3 +1,11 @@
+### Development
+[Full Changelog](http://github.com/rspec/rspec-core/compare/v3.5.0...master)
+
+Bug Fixes:
+
+* Ensure that config hooks that are added to existing example groups are
+  added only once. (Eugene Kenny, #2280)
+
 ### 3.5.0 / 2016-07-01
 [Full Changelog](http://github.com/rspec/rspec-core/compare/v3.5.0.beta4...v3.5.0)
 

lib/rspec/core/configuration.rb

@@ -1749,8 +1749,7 @@ def before(scope=nil, *meta, &block)
         handle_suite_hook(scope, meta) do
           @before_suite_hooks << Hooks::BeforeHook.new(block, {})
         end || begin
-          metadata = Metadata.build_hash_from(meta.dup)
-          on_existing_matching_groups(metadata, scope) { |g| g.before(scope, *meta, &block) }
+          add_hook_to_existing_matching_groups(meta, scope) { |g| g.before(scope, *meta, &block) }
           super(scope, *meta, &block)
         end
       end
@@ -1773,8 +1772,7 @@ def prepend_before(scope=nil, *meta, &block)
         handle_suite_hook(scope, meta) do
           @before_suite_hooks.unshift Hooks::BeforeHook.new(block, {})
         end || begin
-          metadata = Metadata.build_hash_from(meta.dup)
-          on_existing_matching_groups(metadata, scope) { |g| g.prepend_before(scope, *meta, &block) }
+          add_hook_to_existing_matching_groups(meta, scope) { |g| g.prepend_before(scope, *meta, &block) }
           super(scope, *meta, &block)
         end
       end
@@ -1792,8 +1790,7 @@ def after(scope=nil, *meta, &block)
         handle_suite_hook(scope, meta) do
           @after_suite_hooks.unshift Hooks::AfterHook.new(block, {})
         end || begin
-          metadata = Metadata.build_hash_from(meta.dup)
-          on_existing_matching_groups(metadata, scope) { |g| g.after(scope, *meta, &block) }
+          add_hook_to_existing_matching_groups(meta, scope) { |g| g.after(scope, *meta, &block) }
           super(scope, *meta, &block)
         end
       end
@@ -1816,8 +1813,7 @@ def append_after(scope=nil, *meta, &block)
         handle_suite_hook(scope, meta) do
           @after_suite_hooks << Hooks::AfterHook.new(block, {})
         end || begin
-          metadata = Metadata.build_hash_from(meta.dup)
-          on_existing_matching_groups(metadata, scope) { |g| g.append_after(scope, *meta, &block) }
+          add_hook_to_existing_matching_groups(meta, scope) { |g| g.append_after(scope, *meta, &block) }
           super(scope, *meta, &block)
         end
       end
@@ -1826,8 +1822,7 @@ def append_after(scope=nil, *meta, &block)
       #
       # See {Hooks#around} for full `around` hook docs.
       def around(scope=nil, *meta, &block)
-        metadata = Metadata.build_hash_from(meta.dup)
-        on_existing_matching_groups(metadata, scope) { |g| g.around(scope, *meta, &block) }
+        add_hook_to_existing_matching_groups(meta, scope) { |g| g.around(scope, *meta, &block) }
         super(scope, *meta, &block)
       end
 
@@ -2031,21 +2026,32 @@ def configure_group_with(group, module_list, application_method)
         end
       end
 
-      def on_existing_matching_groups(meta, scope=:ignore)
-        groups = world.example_groups.dup
-
-        until groups.empty?
-          group = groups.shift
+      def add_hook_to_existing_matching_groups(meta, scope, &block)
+        # For example hooks, we have to apply it to each of the top level
+        # groups, even if the groups do not match. When we apply it, we
+        # apply it with the metadata, so it will only apply to examples
+        # in the group that match the metadata.
+        # #2280 for background and discussion.
+        if scope == :example || scope == :each || scope.nil?
+          world.example_groups.each(&block)
+        else
+          meta = Metadata.build_hash_from(meta.dup)
+          on_existing_matching_groups(meta, &block)
+        end
+      end
 
-          if scope == :example || scope == :each || scope.nil? ||
-              meta.empty? || MetadataFilter.apply?(:any?, meta, group.metadata)
-            yield group
-          else
-            groups.concat(group.children)
+      def on_existing_matching_groups(meta)
+        world.traverse_example_group_trees_until do |group|
+          metadata_applies_to_group?(meta, group).tap do |applies|
+            yield group if applies
           end
         end
       end
 
+      def metadata_applies_to_group?(meta, group)
+        meta.empty? || MetadataFilter.apply?(:any?, meta, group.metadata)
+      end
+
       if RSpec::Support::RubyFeatures.module_prepends_supported?
         def safe_prepend(mod, host)
           host.__send__(:prepend, mod) unless host < mod

lib/rspec/core/example_group.rb

@@ -450,6 +450,20 @@ def self.children
         @children ||= []
       end
 
+      # @private
+      # Traverses the tree of groups, starting with `self`, then the children, recursively.
+      # Halts the traversal of a branch of the tree as soon as the passed block returns true.
+      # Note that siblings groups and their sub-trees will continue to be explored.
+      # This is intended to make it easy to find the top-most group that satisfies some
+      # condition.
+      def self.traverse_tree_until(&block)
+        return if yield self
+
+        children.each do |child|
+          child.traverse_tree_until(&block)
+        end
+      end
+
       # @private
       def self.next_runnable_index_for(file)
         if self == ExampleGroup

lib/rspec/core/world.rb

@@ -92,6 +92,19 @@ def all_examples
         FlatMap.flat_map(all_example_groups) { |g| g.examples }
       end
 
+      # @private
+      # Traverses the tree of each top level group.
+      # For each it yields the group, then the children, recursively.
+      # Halts the traversal of a branch of the tree as soon as the passed block returns true.
+      # Note that siblings groups and their sub-trees will continue to be explored.
+      # This is intended to make it easy to find the top-most group that satisfies some
+      # condition.
+      def traverse_example_group_trees_until(&block)
+        example_groups.each do |group|
+          group.traverse_tree_until(&block)
+        end
+      end
+
       # @api private
       #
       # Find line number of previous declaration.
@@ -215,6 +228,9 @@ def self.registered_example_group_files
           []
         end
 
+        def self.traverse_example_group_trees_until
+        end
+
         # :nocov:
         def self.example_groups
           []

spec/rspec/core/hooks_filtering_spec.rb

@@ -83,33 +83,36 @@ module RSpec::Core
         ]
       end
 
-      it "applies only to examples with matching metadata" do
-        sequence = []
-
-        group = RSpec.describe do
-          example("") { sequence << :ex_1 }
-          example("", :run_hooks) { sequence << :ex_2 }
-        end
+      { ":example" => [:example], ":each" => [:each] }.each do |label, args|
+        args << :run_hooks
+        it "applies only to examples with matching metadata (for hooks declared with #{label})" do
+          sequence = []
+
+          group = RSpec.describe do
+            example("") { sequence << :ex_1 }
+            example("", :run_hooks) { sequence << :ex_2 }
+          end
 
-        RSpec.configure do |c|
-          c.before(:example, :run_hooks) { sequence << :before_ex_2 }
-          c.prepend_before(:example, :run_hooks) { sequence << :before_ex_1 }
+          RSpec.configure do |c|
+            c.before(*args) { sequence << :before_ex_2 }
+            c.prepend_before(*args) { sequence << :before_ex_1 }
 
-          c.after(:example, :run_hooks)  { sequence << :after_ex_1 }
-          c.append_after(:example, :run_hooks) { sequence << :after_ex_2 }
+            c.after(*args)  { sequence << :after_ex_1 }
+            c.append_after(*args) { sequence << :after_ex_2 }
 
-          c.around(:example, :run_hooks) do |ex|
-            sequence << :around_before_ex
-            ex.run
-            sequence << :around_after_ex
+            c.around(*args) do |ex|
+              sequence << :around_before_ex
+              ex.run
+              sequence << :around_after_ex
+            end
           end
-        end
 
-        group.run
-        expect(sequence).to eq [
-          :ex_1,
-          :around_before_ex, :before_ex_1, :before_ex_2, :ex_2, :after_ex_1, :after_ex_2, :around_after_ex,
-        ]
+          group.run
+          expect(sequence).to eq [
+            :ex_1,
+            :around_before_ex, :before_ex_1, :before_ex_2, :ex_2, :after_ex_1, :after_ex_2, :around_after_ex,
+          ]
+        end
       end
 
       it "does not apply `suite` hooks to groups (or print warnings about suite hooks applied to example groups)" do