Merge pull request rubocop#254 from backus/feature/SeparatedHooks

Add ScatteredSetup cop

CHANGELOG.md

@@ -7,6 +7,7 @@
 * Add autocorrect support for `SingleArgumentMessageChain` cop. ([@bquorning][])
 * Rename `NestedGroups`' configuration key from `MaxNesting` to `Max` in order to be consistent with other cop configuration. ([@backus][])
 * Add `RepeatedExample` cop for detecting repeated examples within example groups. ([@backus][])
+* Add `ScatteredSetup` cop for enforcing that only one `before`, `around`, and `after` hook are used per example group scope. ([@backus][])
 
 ## 1.9.1 (2017-01-02)
 

config/default.yml

@@ -149,6 +149,10 @@ RSpec/SingleArgumentMessageChain:
   Description: Checks that chains of messages contain more than one element.
   Enabled: true
 
+RSpec/ScatteredSetup:
+  Description: Checks for setup scattered across multiple hooks in an example group.
+  Enabled: true
+
 RSpec/SubjectStub:
   Description: Checks for stubbed test subjects.
   Enabled: true

lib/rubocop-rspec.rb

@@ -11,8 +11,10 @@
 require 'rubocop/rspec/util'
 require 'rubocop/rspec/language'
 require 'rubocop/rspec/language/node_pattern'
+require 'rubocop/rspec/interface'
 require 'rubocop/rspec/example_group'
 require 'rubocop/rspec/example'
+require 'rubocop/rspec/hook'
 require 'rubocop/cop/rspec/cop'
 
 RuboCop::RSpec::Inject.defaults!
@@ -44,6 +46,7 @@
 require 'rubocop/cop/rspec/not_to_not'
 require 'rubocop/cop/rspec/repeated_description'
 require 'rubocop/cop/rspec/repeated_example'
+require 'rubocop/cop/rspec/scattered_setup'
 require 'rubocop/cop/rspec/single_argument_message_chain'
 require 'rubocop/cop/rspec/subject_stub'
 require 'rubocop/cop/rspec/verified_doubles'

lib/rubocop/cop/rspec/scattered_setup.rb

@@ -0,0 +1,49 @@
+# frozen_string_literal: true
+
+module RuboCop
+  module Cop
+    module RSpec
+      # Checks for setup scattered across multiple hooks in an example group.
+      #
+      # Unify `before`, `after`, and `around` hooks when possible.
+      #
+      # @example
+      #   # bad
+      #   describe Foo do
+      #     before { setup1 }
+      #     before { setup2 }
+      #   end
+      #
+      #   # good
+      #   describe Foo do
+      #     before do
+      #       setup1
+      #       setup2
+      #     end
+      #   end
+      #
+      class ScatteredSetup < Cop
+        MSG = 'Do not define multiple hooks in the same example group.'.freeze
+
+        def on_block(node)
+          return unless example_group?(node)
+
+          analyzable_hooks(node).each do |repeated_hook|
+            add_offense(repeated_hook, :expression)
+          end
+        end
+
+        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] }
+            .values
+            .reject(&:one?)
+            .flatten
+            .map(&:to_node)
+        end
+      end
+    end
+  end
+end

lib/rubocop/rspec/example.rb

@@ -3,17 +3,11 @@
 module RuboCop
   module RSpec
     # Wrapper for RSpec examples
-    class Example
-      extend RuboCop::NodePattern::Macros
-
+    class Example < Concept
       def_node_matcher :extract_doc_string,     '(send _ _ $str ...)'
       def_node_matcher :extract_metadata,       '(send _ _ _ $...)'
       def_node_matcher :extract_implementation, '(block send args $_)'
 
-      def initialize(node)
-        @node = node
-      end
-
       def doc_string
         extract_doc_string(definition)
       end
@@ -26,31 +20,13 @@ def implementation
         extract_implementation(node)
       end
 
-      def eql?(other)
-        node.eql?(other.node)
-      end
-
-      alias == eql?
-
-      def hash
-        [self.class, node].hash
-      end
-
-      def to_node
-        node
-      end
-
       def definition
         if node.send_type?
           node
         else
           node.children.first
         end
       end
-
-      protected
-
-      attr_reader :node
     end
   end
 end

lib/rubocop/rspec/example_group.rb

@@ -3,10 +3,7 @@
 module RuboCop
   module RSpec
     # Wrapper for RSpec example groups
-    class ExampleGroup
-      include Language
-      extend NodePattern::Macros
-
+    class ExampleGroup < Concept
       def_node_matcher :example?, Examples::ALL.block_pattern
 
       # @!method scope_change?(node)
@@ -18,21 +15,42 @@ class ExampleGroup
       def_node_matcher :scope_change?,
                        (ExampleGroups::ALL + SharedGroups::ALL).block_pattern
 
-      def initialize(node)
-        @node = node
-      end
+      # @!method hook(node)
+      #
+      #   Detect if node is `before`, `after`, `around`
+      def_node_matcher :hook, <<-PATTERN
+      (block {$(send nil #{Hooks::ALL.node_pattern_union} ...)} ...)
+      PATTERN
 
       def examples
         examples_in_scope(node).map(&Example.public_method(:new))
       end
 
+      def hooks
+        hooks_in_scope(node).map(&Hook.public_method(:new))
+      end
+
       private
 
-      attr_reader :node
+      def hooks_in_scope(node)
+        node.each_child_node.flat_map do |child|
+          find_hooks(child)
+        end
+      end
+
+      def find_hooks(node)
+        return [] if scope_change?(node) || example?(node)
+
+        if hook(node)
+          [node]
+        else
+          hooks_in_scope(node)
+        end
+      end
 
-      def examples_in_scope(node)
+      def examples_in_scope(node, &blk)
         node.each_child_node.flat_map do |child|
-          find_examples(child)
+          find_examples(child, &blk)
         end
       end
 

lib/rubocop/rspec/hook.rb

@@ -0,0 +1,45 @@
+# frozen_string_literal: true
+
+module RuboCop
+  module RSpec
+    # Wrapper for RSpec hook
+    class Hook < Concept
+      STANDARDIZED_SCOPES = %i(each context suite).freeze
+      private_constant(:STANDARDIZED_SCOPES)
+
+      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)
+      end
+
+      def scope
+        case scope_name
+        when nil, :each, :example then :each
+        when :context, :all       then :context
+        when :suite               then :suite
+        else
+          scope_name
+        end
+      end
+
+      private
+
+      def scope_name
+        scope_argument.to_a.first
+      end
+
+      def scope_argument
+        node.method_args.first
+      end
+    end
+  end
+end

lib/rubocop/rspec/interface.rb

@@ -0,0 +1,33 @@
+# frozen_string_literal: true
+
+module RuboCop
+  module RSpec
+    # Wrapper for RSpec DSL methods
+    class Concept
+      include Language
+      extend NodePattern::Macros
+
+      def initialize(node)
+        @node = node
+      end
+
+      def eql?(other)
+        node.eql?(other.node)
+      end
+
+      alias == eql?
+
+      def hash
+        [self.class, node].hash
+      end
+
+      def to_node
+        node
+      end
+
+      protected
+
+      attr_reader :node
+    end
+  end
+end

spec/rubocop/cop/rspec/scattered_setup_spec.rb

@@ -0,0 +1,96 @@
+describe RuboCop::Cop::RSpec::ScatteredSetup do
+  subject(:cop) { described_class.new }
+
+  it 'flags multiple hooks in the same example group' do
+    expect_violation(<<-RUBY)
+      describe Foo do
+        before { bar }
+        ^^^^^^^^^^^^^^ Do not define multiple hooks in the same example group.
+        before { baz }
+        ^^^^^^^^^^^^^^ Do not define multiple hooks in the same example group.
+      end
+    RUBY
+  end
+
+  it 'flags multiple hooks of the same scope with different symbols' do
+    expect_violation(<<-RUBY)
+      describe Foo do
+        before { bar }
+        ^^^^^^^^^^^^^^ Do not define multiple hooks in the same example group.
+        before(:each) { baz }
+        ^^^^^^^^^^^^^^^^^^^^^ Do not define multiple hooks in the same example group.
+        before(:example) { baz }
+        ^^^^^^^^^^^^^^^^^^^^^^^^ Do not define multiple hooks in the same example group.
+      end
+    RUBY
+  end
+
+  it 'flags multiple before(:all) hooks in the same example group' do
+    expect_violation(<<-RUBY)
+      describe Foo do
+        before(:all) { bar }
+        ^^^^^^^^^^^^^^^^^^^^ Do not define multiple hooks in the same example group.
+        before(:all) { baz }
+        ^^^^^^^^^^^^^^^^^^^^ Do not define multiple hooks in the same example group.
+      end
+    RUBY
+  end
+
+  it 'does not flag different hooks' do
+    expect_no_violations(<<-RUBY)
+      describe Foo do
+        before { bar }
+        after { baz }
+        around { qux }
+      end
+    RUBY
+  end
+
+  it 'does not flag different hook types' do
+    expect_no_violations(<<-RUBY)
+      describe Foo do
+        before { bar }
+        before(:all) { baz }
+        before(:suite) { baz }
+      end
+    RUBY
+  end
+
+  it 'does not flag hooks in different example groups' do
+    expect_no_violations(<<-RUBY)
+      describe Foo do
+        before { bar }
+
+        describe '.baz' do
+          before { baz }
+        end
+      end
+    RUBY
+  end
+
+  it 'does not flag hooks in different shared contexts' do
+    expect_no_violations(<<-RUBY)
+      describe Foo do
+        shared_context 'one' do
+          before { bar }
+        end
+
+        shared_context 'two' do
+          before { baz }
+        end
+      end
+    RUBY
+  end
+
+  it 'does not flag similar method names inside of examples' do
+    expect_no_violations(<<-RUBY)
+      describe Foo do
+        before { bar }
+
+        it 'uses an instance method called before' do
+          expect(before { tricky }).to_not confuse_rubocop_rspec
+        end
+      end
+    RUBY
+  end
+end