Merge pull request #161 from nevir/feature/Stubject

Add SubjectStub cop

CHANGELOG.md

@@ -12,6 +12,7 @@
 * Add `ExpectActual` cop for detecting literal values within `expect(...)`. ([@backus][])
 * Add `MultipleExpectations` cop for detecting multiple `expect(...)` calls within one example. ([@backus][])
 * Add `Max` configuration option for `MultipleExpectations`. ([@backus][])
+* Add `SubjectStub` cop for testing stubbed test subjects. ([@backus][])
 
 ## 1.6.0 (2016-08-03)
 

config/default.yml

@@ -83,3 +83,7 @@ RSpec/ExampleLength:
 RSpec/NamedSubject:
   Description: 'Name your RSpec subject if you reference it explicitly'
   Enabled: true
+
+RSpec/SubjectStub:
+  Description: 'Checks for stubbed test subjects'
+  Enabled: true

lib/rubocop-rspec.rb

@@ -26,4 +26,5 @@
 require 'rubocop/cop/rspec/named_subject'
 require 'rubocop/cop/rspec/nested_groups'
 require 'rubocop/cop/rspec/not_to_not'
+require 'rubocop/cop/rspec/subject_stub'
 require 'rubocop/cop/rspec/verified_doubles'

lib/rubocop/cop/rspec/subject_stub.rb

@@ -0,0 +1,137 @@
+# frozen_string_literal: true
+
+module RuboCop
+  module Cop
+    module RSpec
+      # Checks for stubs on test subjects
+      #
+      # @see https://robots.thoughtbot.com/don-t-stub-the-system-under-test
+      #
+      # @example
+      #   # bad
+      #   describe Foo do
+      #     subject(:bar) { baz }
+      #
+      #     before do
+      #       allow(bar).to receive(:qux?).and_return(true)
+      #     end
+      #   end
+      #
+      class SubjectStub < Cop
+        include RuboCop::RSpec::TopLevelDescribe, RuboCop::RSpec::Language
+
+        MSG = 'Do not stub your test subject.'.freeze
+
+        # @!method subject(node)
+        #   Find a named or unnamed subject definition
+        #
+        #   @example anonymous subject
+        #     subject(parse('subject { foo }').ast) do |name|
+        #       name # => :subject
+        #     end
+        #
+        #   @example named subject
+        #     subject(parse('subject(:thing) { foo }').ast) do |name|
+        #       name # => :thing
+        #     end
+        #
+        #   @param node [RuboCop::Node]
+        #
+        #   @yield [Symbol] subject name
+        def_node_matcher :subject, <<-PATTERN
+        {
+          (block (send nil :subject (sym $_)) args ...)
+          (block (send nil $:subject) args ...)
+        }
+        PATTERN
+
+        # @!method message_expectation?(node, method_name)
+        #   Match `allow` and `expect(...).to receive`
+        #
+        #   @example source that matches
+        #     allow(foo).to  receive(:bar)
+        #     allow(foo).to  receive(:bar).with(1)
+        #     allow(foo).to  receive(:bar).with(1).and_return(2)
+        #     expect(foo).to receive(:bar)
+        #     expect(foo).to receive(:bar).with(1)
+        #     expect(foo).to receive(:bar).with(1).and_return(2)
+        def_node_matcher :message_expectation?, <<-PATTERN
+          {
+            (send nil :allow (send nil %))
+            (send (send nil :expect (send nil %)) :to #receive_message?)
+          }
+        PATTERN
+
+        def_node_search :receive_message?, '(send nil :receive ...)'
+
+        def_node_matcher :example_group?, <<-PATTERN
+          (block (send _ {#{ExampleGroups::ALL.to_node_pattern}} ...) ...)
+        PATTERN
+
+        def on_block(node)
+          return unless example_group?(node)
+
+          find_subject_stub(node) { |stub| add_offense(stub, :expression) }
+        end
+
+        private
+
+        # Find subjects within tree and then find (send) nodes for that subject
+        #
+        # @param node [RuboCop::Node] example group
+        #
+        # @yield [RuboCop::Node] message expectations for subject
+        def find_subject_stub(node, &block)
+          find_subject(node) do |subject_name, context|
+            find_subject_expectation(context, subject_name, &block)
+          end
+        end
+
+        # Find a subject message expectation
+        #
+        # @param context [RuboCop::Node]
+        # @param subject_name [Symbol] name of subject
+        #
+        # @yield [RuboCop::Node] message expectation
+        def find_subject_expectation(node, subject_name, &block)
+          # Do not search node if it is an example group with its own subject.
+          return if example_group?(node) && redefines_subject?(node)
+
+          # Yield the current node if it is a message expectation.
+          yield(node) if message_expectation?(node, subject_name)
+
+          # Recurse through node's children looking for a messag expectation.
+          node.each_child_node do |child|
+            find_subject_expectation(child, subject_name, &block)
+          end
+        end
+
+        # Check if node's children contain a subject definition
+        #
+        # @param node [RuboCop::Node]
+        #
+        # @return [Boolean]
+        def redefines_subject?(node)
+          node.each_child_node.any? do |child|
+            subject(child) || redefines_subject?(child)
+          end
+        end
+
+        # Find a subject definition
+        #
+        # @param node [RuboCop::Node]
+        # @param parent [RuboCop::Node,nil]
+        #
+        # @yieldparam subject_name [Symbol] name of subject being defined
+        # @yieldparam parent [RuboCop::Node] parent of subject definition
+        def find_subject(node, parent: nil, &block)
+          subject(node) { |name| yield(name, parent) }
+
+          node.each_child_node do |child|
+            find_subject(child, parent: node, &block)
+          end
+        end
+      end
+    end
+  end
+end

spec/rubocop/cop/rspec/subject_stub_spec.rb

@@ -0,0 +1,183 @@
+# frozen_string_literal: true
+
+describe RuboCop::Cop::RSpec::SubjectStub do
+  subject(:cop) { described_class.new }
+
+  it 'complains when subject is stubbed' do
+    expect_violation(<<-RUBY)
+      describe Foo do
+        subject(:foo) { described_class.new }
+
+        before do
+          allow(foo).to receive(:bar).and_return(baz)
+          ^^^^^^^^^^ Do not stub your test subject.
+        end
+
+        it 'uses expect twice' do
+          expect(foo.bar).to eq(baz)
+        end
+      end
+    RUBY
+  end
+
+  it 'complains when subject is mocked' do
+    expect_violation(<<-RUBY)
+      describe Foo do
+        subject(:foo) { described_class.new }
+
+        before do
+          expect(foo).to receive(:bar).and_return(baz)
+          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Do not stub your test subject.
+          expect(foo).to receive(:bar)
+          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Do not stub your test subject.
+          expect(foo).to receive(:bar).with(1)
+          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Do not stub your test subject.
+          expect(foo).to receive(:bar).with(1).and_return(2)
+          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Do not stub your test subject.
+        end
+
+        it 'uses expect twice' do
+          expect(foo.bar).to eq(baz)
+        end
+      end
+    RUBY
+  end
+
+  it 'ignores stub within context where subject name changed' do
+    expect_no_violations(<<-RUBY)
+      describe Foo do
+        subject(:foo) { described_class.new }
+
+        context 'when I shake things up' do
+          subject(:bar) { described_class.new }
+
+          it 'tries to trick rubocop-rspec' do
+            allow(foo).to receive(:baz)
+          end
+        end
+      end
+    RUBY
+  end
+
+  it 'flags nested subject stubs when nested subject uses same name' do
+    expect_violation(<<-RUBY)
+      describe Foo do
+        subject(:foo) { described_class.new }
+
+        context 'when I shake things up' do
+          subject(:foo) { described_class.new }
+
+          before do
+            allow(foo).to receive(:wow)
+            ^^^^^^^^^^ Do not stub your test subject.
+          end
+
+          it 'tries to trick rubocop-rspec' do
+            expect(foo).to eql(:neat)
+          end
+        end
+      end
+    RUBY
+  end
+
+  it 'ignores nested stubs when nested subject is anonymous' do
+    expect_no_violations(<<-RUBY)
+      describe Foo do
+        subject(:foo) { described_class.new }
+
+        context 'when I shake things up' do
+          subject { described_class.new }
+
+          before do
+            allow(foo).to receive(:wow)
+          end
+
+          it 'tries to trick rubocop-rspec' do
+            expect(foo).to eql(:neat)
+          end
+        end
+      end
+    RUBY
+  end
+
+  it 'flags nested subject stubs when example group does not define subject' do
+    expect_violation(<<-RUBY)
+      describe Foo do
+        subject(:foo) { described_class.new }
+
+        context 'when I shake things up' do
+          before do
+            allow(foo).to receive(:wow)
+            ^^^^^^^^^^ Do not stub your test subject.
+          end
+
+          it 'tries to trick rubocop-rspec' do
+            expect(foo).to eql(:neat)
+          end
+        end
+      end
+    RUBY
+  end
+
+  it 'flags nested subject stubs' do
+    expect_violation(<<-RUBY)
+      describe Foo do
+        subject(:foo) { described_class.new }
+
+        context 'when I shake things up' do
+          subject(:bar) { described_class.new }
+
+          before do
+            allow(foo).to receive(:wow)
+            allow(bar).to receive(:wow)
+            ^^^^^^^^^^ Do not stub your test subject.
+          end
+
+          it 'tries to trick rubocop-rspec' do
+            expect(bar).to eql(foo)
+          end
+        end
+      end
+    RUBY
+  end
+
+  it 'flags nested subject stubs when adjacent context redefines' do
+    expect_violation(<<-RUBY)
+      describe Foo do
+        subject(:foo) { described_class.new }
+
+        context 'when I do something in a context' do
+          subject { blah }
+        end
+
+        it 'stil flags this test' do
+          allow(foo).to receive(:blah)
+          ^^^^^^^^^^ Do not stub your test subject.
+        end
+      end
+    RUBY
+  end
+
+  it 'flags deeply nested subject stubs' do
+    expect_violation(<<-RUBY)
+      describe Foo do
+        subject(:foo) { described_class.new }
+
+        context 'level 1' do
+          subject(:bar) { described_class.new }
+
+          context 'level 2' do
+            subject(:baz) { described_class.new }
+
+            before do
+              allow(foo).to receive(:wow)
+              allow(bar).to receive(:wow)
+              allow(baz).to receive(:wow)
+              ^^^^^^^^^^ Do not stub your test subject.
+            end
+          end
+        end
+      end
+    RUBY
+  end
+end