Merge pull request #851 from pirj/fix-scattered-setup-cop

Add metadata detection to RSpec/ScatteredSetup cop

Author: bquorning

CHANGELOG.md

@@ -3,6 +3,7 @@
 ## Master (Unreleased)
 
 * Fix `RSpec/InstanceVariable` detection inside custom matchers. ([@pirj][])
+* Fix `RSpec/ScatteredSetup` to distinguish hooks with different metadata. ([@pirj][])
 
 ## 1.37.1 (2019-12-16)
 

lib/rubocop/cop/rspec/scattered_setup.rb

@@ -36,8 +36,8 @@ def on_block(node)
         def analyzable_hooks(node)
           RuboCop::RSpec::ExampleGroup.new(node)
             .hooks
-            .select { |hook| hook.knowable_scope? && hook.valid_scope? }
-            .group_by { |hook| [hook.name, hook.scope] }
+            .select(&:knowable_scope?)
+            .group_by { |hook| [hook.name, hook.scope, hook.metadata] }
             .values
             .reject(&:one?)
             .flatten

lib/rubocop/rspec/hook.rb

@@ -4,39 +4,72 @@ module RuboCop
   module RSpec
     # Wrapper for RSpec hook
     class Hook < Concept
-      STANDARDIZED_SCOPES = %i[each context suite].freeze
-      private_constant(:STANDARDIZED_SCOPES)
+      def_node_matcher :extract_metadata, <<~PATTERN
+        (block
+          {
+            (send _ _ #valid_scope? $...)
+            (send _ _ $...)
+          }
+          ...
+        )
+      PATTERN
 
       def name
         node.method_name
       end
 
       def knowable_scope?
-        return true unless scope_argument
-
-        scope_argument.sym_type?
-      end
-
-      def valid_scope?
-        STANDARDIZED_SCOPES.include?(scope)
+        scope_argument.nil? ||
+          scope_argument.sym_type? ||
+          scope_argument.hash_type?
       end
 
       def example?
         scope.equal?(:each)
       end
 
       def scope
+        return :each if scope_argument&.hash_type?
+
         case scope_name
         when nil, :each, :example then :each
         when :context, :all       then :context
         when :suite               then :suite
-        else
-          scope_name
         end
       end
 
+      def metadata
+        (extract_metadata(node) || [])
+          .map { |meta| transform_metadata(meta) }
+          .flatten
+          .inject(&:merge)
+      end
+
       private
 
+      def valid_scope?(node)
+        node&.sym_type? && Hooks::Scopes::ALL.include?(node.value)
+      end
+
+      def transform_metadata(meta)
+        if meta.sym_type?
+          { meta => true }
+        else
+          # This check is to be able to compare those two hooks:
+          #
+          #   before(:example, :special) { ... }
+          #   before(:example, special: true) { ... }
+          #
+          # In the second case it's a node with a pair that has a value
+          # of a `true_type?`.
+          meta.pairs.map { |pair| { pair.key => transform_true(pair.value) } }
+        end
+      end
+
+      def transform_true(node)
+        node.true_type? ? true : node
+      end
+
       def scope_name
         scope_argument.to_a.first
       end

lib/rubocop/rspec/language.rb

@@ -94,6 +94,18 @@ module Hooks
             append_after
           ]
         )
+
+        module Scopes
+          ALL = SelectorSet.new(
+            %i[
+              each
+              example
+              context
+              all
+              suite
+            ]
+          )
+        end
       end
 
       module Helpers

spec/rubocop/cop/rspec/scattered_setup_spec.rb

@@ -38,7 +38,7 @@
     RUBY
   end
 
-  it 'does not flag different hooks' do
+  it 'ignores different hooks' do
     expect_no_offenses(<<-RUBY)
       describe Foo do
         before { bar }
@@ -48,7 +48,7 @@
     RUBY
   end
 
-  it 'does not flag different hook types' do
+  it 'ignores different hook types' do
     expect_no_offenses(<<-RUBY)
       describe Foo do
         before { bar }
@@ -58,7 +58,7 @@
     RUBY
   end
 
-  it 'does not flag hooks in different example groups' do
+  it 'ignores hooks in different example groups' do
     expect_no_offenses(<<-RUBY)
       describe Foo do
         before { bar }
@@ -70,7 +70,7 @@
     RUBY
   end
 
-  it 'does not flag hooks in different shared contexts' do
+  it 'ignores hooks in different shared contexts' do
     expect_no_offenses(<<-RUBY)
       describe Foo do
         shared_context 'one' do
@@ -84,7 +84,7 @@
     RUBY
   end
 
-  it 'does not flag similar method names inside of examples' do
+  it 'ignores similar method names inside of examples' do
     expect_no_offenses(<<-RUBY)
       describe Foo do
         before { bar }
@@ -95,4 +95,29 @@
       end
     RUBY
   end
+
+  it 'ignores hooks with different metadata' do
+    expect_no_offenses(<<-RUBY)
+      describe Foo do
+        before(:example) { foo }
+        before(:example, :special_case) { bar }
+      end
+    RUBY
+  end
+
+  it 'flags hooks with similar metadata' do
+    expect_offense(<<-RUBY)
+      describe Foo do
+        before(:each, :special_case) { foo }
+        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Do not define multiple hooks in the same example group.
+        before(:example, :special_case) { bar }
+        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Do not define multiple hooks in the same example group.
+        before(:example, special_case: true) { bar }
+        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Do not define multiple hooks in the same example group.
+        before(special_case: true) { bar }
+        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Do not define multiple hooks in the same example group.
+        before(:example, special_case: false) { bar }
+      end
+    RUBY
+  end
 end

spec/rubocop/rspec/hook_spec.rb

@@ -11,43 +11,86 @@ def hook(source)
     expect(hook('around(:each) { }').name).to be(:around)
   end
 
-  it 'does not break if a hook is not given a symbol literal' do
-    expect(hook('before(scope) { example_setup }').knowable_scope?).to be(false)
-  end
+  describe '#knowable_scope?' do
+    it 'does not break if a hook is not given a symbol literal' do
+      expect(hook('before(scope) { example_setup }').knowable_scope?)
+        .to be(false)
+    end
 
-  it 'knows the scope of a hook with a symbol literal' do
-    expect(hook('before { example_setup }').knowable_scope?).to be(true)
-  end
+    it 'knows the scope of a hook with a symbol literal' do
+      expect(hook('before(:example) { example_setup }').knowable_scope?)
+        .to be(true)
+    end
 
-  it 'ignores other arguments to hooks' do
-    expect(hook('before(:each, :metadata) { example_setup }').scope)
-      .to be(:each)
-  end
+    it 'knows the scope of a hook with no argument' do
+      expect(hook('before { example_setup }').knowable_scope?)
+        .to be(true)
+    end
 
-  it 'classifies nonstandard hook arguments as invalid' do
-    expect(hook('before(:nothing) { example_setup }').valid_scope?).to be(false)
+    it 'knows the scope of a hook with hash metadata' do
+      expect(hook('before(special: true) { example_setup }').knowable_scope?)
+        .to be(true)
+    end
   end
 
-  it 'classifies :each as a valid hook argument' do
-    expect(hook('before(:each) { example_setup }').valid_scope?).to be(true)
-  end
+  describe '#scope' do
+    it 'ignores other arguments to hooks' do
+      expect(hook('before(:each, :metadata) { example_setup }').scope)
+        .to be(:each)
+    end
 
-  it 'classifies :each as an example hook' do
-    expect(hook('before(:each) { }').example?).to be(true)
-  end
+    it 'classifies :each as an example hook' do
+      expect(hook('before(:each) { }').example?).to be(true)
+    end
+
+    it 'defaults to example hook with hash metadata' do
+      expect(hook('before(special: true) { }').example?).to be(true)
+    end
 
-  shared_examples 'standardizes scope' do |source, scope|
-    it "interprets #{source} as having scope #{scope}" do
-      expect(hook(source).scope).to equal(scope)
+    shared_examples 'standardizes scope' do |source, scope|
+      it "interprets #{source} as having scope #{scope}" do
+        expect(hook(source).scope).to equal(scope)
+      end
     end
+
+    include_examples 'standardizes scope', 'before(:each) { }',    :each
+    include_examples 'standardizes scope', 'around(:example) { }', :each
+    include_examples 'standardizes scope', 'after { }',            :each
+
+    include_examples 'standardizes scope', 'before(:all) { }',     :context
+    include_examples 'standardizes scope', 'around(:context) { }', :context
+
+    include_examples 'standardizes scope', 'after(:suite) { }', :suite
   end
 
-  include_examples 'standardizes scope', 'before(:each) { }',    :each
-  include_examples 'standardizes scope', 'around(:example) { }', :each
-  include_examples 'standardizes scope', 'after { }',            :each
+  describe '#metadata' do
+    def metadata(source)
+      hook(source).metadata.to_s
+    end
 
-  include_examples 'standardizes scope', 'before(:all) { }',     :context
-  include_examples 'standardizes scope', 'around(:context) { }', :context
+    it 'extracts symbol metadata' do
+      expect(metadata('before(:example, :special) { foo }'))
+        .to eq('{s(:sym, :special)=>true}')
+    end
+
+    it 'extracts hash metadata' do
+      expect(metadata('before(:example, special: true) { foo }'))
+        .to eq('{s(:sym, :special)=>true}')
+    end
 
-  include_examples 'standardizes scope', 'after(:suite) { }', :suite
+    it 'combines symbol and hash metadata' do
+      expect(metadata('before(:example, :symbol, special: true) { foo }'))
+        .to eq('{s(:sym, :symbol)=>true, s(:sym, :special)=>true}')
+    end
+
+    it 'extracts hash metadata with no scope given' do
+      expect(metadata('before(special: true) { foo }'))
+        .to eq('{s(:sym, :special)=>true}')
+    end
+
+    it 'withstands no arguments' do
+      expect(metadata('before { foo }'))
+        .to be_empty
+    end
+  end
 end