Merge pull request #226 from rubocop-hq/dont-stub-your-mock

Add RSpec/StubbedMock cop

Author: pirj

CHANGELOG.md

@@ -5,6 +5,7 @@
 * Move our documentation from rubocop-rspec.readthedocs.io to docs.rubocop.org/rubocop-rspec. ([@bquorning][])
 * Add `RSpec/RepeatedIncludeExample` cop. ([@biinari][])
 * Fix false positives in `RSpec/EmptyExampleGroup`. ([@pirj][])
+* Add `RSpec/StubbedMock` cop. ([@bquorning][], [@pirj][])
 
 ## 1.43.2 (2020-08-25)
 

config/default.yml

@@ -543,6 +543,12 @@ RSpec/SingleArgumentMessageChain:
   VersionChanged: '1.10'
   StyleGuide: https://www.rubydoc.info/gems/rubocop-rspec/RuboCop/Cop/RSpec/SingleArgumentMessageChain
 
+RSpec/StubbedMock:
+  Description: Checks that message expectations do not have a configured response.
+  Enabled: pending
+  VersionAdded: '1.44'
+  StyleGuide: https://www.rubydoc.info/gems/rubocop-rspec/RuboCop/Cop/RSpec/StubbedMock
+
 RSpec/SubjectStub:
   Description: Checks for stubbed test subjects.
   Enabled: true

docs/modules/ROOT/pages/cops.adoc

@@ -86,6 +86,7 @@
 * xref:cops_rspec.adoc#rspecsharedcontext[RSpec/SharedContext]
 * xref:cops_rspec.adoc#rspecsharedexamples[RSpec/SharedExamples]
 * xref:cops_rspec.adoc#rspecsingleargumentmessagechain[RSpec/SingleArgumentMessageChain]
+* xref:cops_rspec.adoc#rspecstubbedmock[RSpec/StubbedMock]
 * xref:cops_rspec.adoc#rspecsubjectstub[RSpec/SubjectStub]
 * xref:cops_rspec.adoc#rspecunspecifiedexception[RSpec/UnspecifiedException]
 * xref:cops_rspec.adoc#rspecvariabledefinition[RSpec/VariableDefinition]

docs/modules/ROOT/pages/cops_rspec.adoc

@@ -3814,6 +3814,36 @@ allow(foo).to receive("bar.baz")
 
 * https://www.rubydoc.info/gems/rubocop-rspec/RuboCop/Cop/RSpec/SingleArgumentMessageChain
 
+== RSpec/StubbedMock
+
+|===
+| Enabled by default | Safe | Supports autocorrection | VersionAdded | VersionChanged
+
+| Pending
+| Yes
+| No
+| 1.44
+| -
+|===
+
+Checks that message expectations do not have a configured response.
+
+=== Examples
+
+[source,ruby]
+----
+# bad
+expect(foo).to receive(:bar).with(42).and_return("hello world")
+
+# good (without spies)
+allow(foo).to receive(:bar).with(42).and_return("hello world")
+expect(foo).to receive(:bar).with(42)
+----
+
+=== References
+
+* https://www.rubydoc.info/gems/rubocop-rspec/RuboCop/Cop/RSpec/StubbedMock
+
 == RSpec/SubjectStub
 
 |===

lib/rubocop/cop/rspec/stubbed_mock.rb

@@ -0,0 +1,172 @@
+# frozen_string_literal: true
+
+module RuboCop
+  module Cop
+    module RSpec
+      # Checks that message expectations do not have a configured response.
+      #
+      # @example
+      #
+      #   # bad
+      #   expect(foo).to receive(:bar).with(42).and_return("hello world")
+      #
+      #   # good (without spies)
+      #   allow(foo).to receive(:bar).with(42).and_return("hello world")
+      #   expect(foo).to receive(:bar).with(42)
+      #
+      class StubbedMock < Base
+        MSG = 'Prefer `%<replacement>s` over `%<method_name>s` when ' \
+              'configuring a response.'
+
+        # @!method message_expectation?(node)
+        #   Match message expectation matcher
+        #
+        #   @example source that matches
+        #     receive(:foo)
+        #
+        #   @example source that matches
+        #     receive_message_chain(:foo, :bar)
+        #
+        #   @example source that matches
+        #     receive(:foo).with('bar')
+        #
+        #   @param node [RuboCop::AST::Node]
+        #   @return [Array<RuboCop::AST::Node>] matching nodes
+        def_node_matcher :message_expectation?, <<-PATTERN
+          {
+            (send nil? { :receive :receive_message_chain } ...)  # receive(:foo)
+            (send (send nil? :receive ...) :with ...)            # receive(:foo).with('bar')
+          }
+        PATTERN
+
+        def_node_matcher :configured_response?, <<~PATTERN
+          { :and_return :and_raise :and_throw :and_yield
+            :and_call_original :and_wrap_original }
+        PATTERN
+
+        # @!method expectation(node)
+        #   Match expectation
+        #
+        #   @example source that matches
+        #     is_expected.to be_in_the_bar
+        #
+        #   @example source that matches
+        #     expect(cocktail).to contain_exactly(:fresh_orange_juice, :campari)
+        #
+        #   @example source that matches
+        #     expect_any_instance_of(Officer).to be_alert
+        #
+        #   @param node [RuboCop::AST::Node]
+        #   @yield [RuboCop::AST::Node] expectation, method name, matcher
+        def_node_matcher :expectation, <<~PATTERN
+          (send
+            $(send nil? $#{Expectations::ALL.node_pattern_union} ...)
+            :to $_)
+        PATTERN
+
+        # @!method matcher_with_configured_response(node)
+        #   Match matcher with a configured response
+        #
+        #   @example source that matches
+        #     receive(:foo).and_return('bar')
+        #
+        #   @example source that matches
+        #     receive(:lower).and_raise(SomeError)
+        #
+        #   @example source that matches
+        #     receive(:redirect).and_call_original
+        #
+        #   @param node [RuboCop::AST::Node]
+        #   @yield [RuboCop::AST::Node] matcher
+        def_node_matcher :matcher_with_configured_response, <<~PATTERN
+          (send #message_expectation? #configured_response? _)
+        PATTERN
+
+        # @!method matcher_with_return_block(node)
+        #   Match matcher with a return block
+        #
+        #   @example source that matches
+        #     receive(:foo) { 'bar' }
+        #
+        #   @param node [RuboCop::AST::Node]
+        #   @yield [RuboCop::AST::Node] matcher
+        def_node_matcher :matcher_with_return_block, <<~PATTERN
+          (block #message_expectation? args _)  # receive(:foo) { 'bar' }
+        PATTERN
+
+        # @!method matcher_with_hash(node)
+        #   Match matcher with a configured response defined as a hash
+        #
+        #   @example source that matches
+        #     receive_messages(foo: 'bar', baz: 'qux')
+        #
+        #   @example source that matches
+        #     receive_message_chain(:foo, bar: 'baz')
+        #
+        #   @param node [RuboCop::AST::Node]
+        #   @yield [RuboCop::AST::Node] matcher
+        def_node_matcher :matcher_with_hash, <<~PATTERN
+          {
+            (send nil? :receive_messages hash)           # receive_messages(foo: 'bar', baz: 'qux')
+            (send nil? :receive_message_chain ... hash)  # receive_message_chain(:foo, bar: 'baz')
+          }
+        PATTERN
+
+        # @!method matcher_with_blockpass(node)
+        #   Match matcher with a configured response in block-pass
+        #
+        #   @example source that matches
+        #     receive(:foo, &canned)
+        #
+        #   @example source that matches
+        #     receive_message_chain(:foo, :bar, &canned)
+        #
+        #   @example source that matches
+        #     receive(:foo).with('bar', &canned)
+        #
+        #   @param node [RuboCop::AST::Node]
+        #   @yield [RuboCop::AST::Node] matcher
+        def_node_matcher :matcher_with_blockpass, <<~PATTERN
+          {
+            (send nil? { :receive :receive_message_chain } ... block_pass)  # receive(:foo, &canned)
+            (send (send nil? :receive ...) :with ... block_pass)            # receive(:foo).with('foo', &canned)
+          }
+        PATTERN
+
+        def on_send(node)
+          expectation(node, &method(:on_expectation))
+        end
+
+        private
+
+        def on_expectation(expectation, method_name, matcher)
+          flag_expectation = lambda do
+            add_offense(expectation, message: msg(method_name))
+          end
+
+          matcher_with_configured_response(matcher, &flag_expectation)
+          matcher_with_return_block(matcher, &flag_expectation)
+          matcher_with_hash(matcher, &flag_expectation)
+          matcher_with_blockpass(matcher, &flag_expectation)
+        end
+
+        def msg(method_name)
+          format(MSG,
+                 method_name: method_name,
+                 replacement: replacement(method_name))
+        end
+
+        def replacement(method_name)
+          case method_name
+          when :expect
+            :allow
+          when :is_expected
+            'allow(subject)'
+          when :expect_any_instance_of
+            :allow_any_instance_of
+          end
+        end
+      end
+    end
+  end
+end

lib/rubocop/cop/rspec_cops.rb

@@ -86,6 +86,7 @@
 require_relative 'rspec/shared_context'
 require_relative 'rspec/shared_examples'
 require_relative 'rspec/single_argument_message_chain'
+require_relative 'rspec/stubbed_mock'
 require_relative 'rspec/subject_stub'
 require_relative 'rspec/unspecified_exception'
 require_relative 'rspec/variable_definition'

spec/project/default_config_spec.rb

@@ -70,7 +70,8 @@ def cop_configuration(config_key)
     expect(cop_configuration('Description')).to all(end_with('.'))
   end
 
-  it 'includes Enabled: true for every cop' do
-    expect(cop_configuration('Enabled')).to all(be(true).or(be(false)))
+  it 'includes a valid Enabled for every cop' do
+    expect(cop_configuration('Enabled'))
+      .to all be(true).or(be(false)).or(eq('pending'))
   end
 end

spec/rubocop/cop/rspec/stubbed_mock_spec.rb

@@ -0,0 +1,133 @@
+# frozen_string_literal: true
+
+RSpec.describe RuboCop::Cop::RSpec::StubbedMock do
+  subject(:cop) { described_class.new }
+
+  it 'flags stubbed message expectation' do
+    expect_offense(<<-RUBY)
+      expect(foo).to receive(:bar).and_return('hello world')
+      ^^^^^^^^^^^ Prefer `allow` over `expect` when configuring a response.
+    RUBY
+  end
+
+  it 'flags stubbed message expectation with a block' do
+    expect_offense(<<-RUBY)
+      expect(foo).to receive(:bar) { 'hello world' }
+      ^^^^^^^^^^^ Prefer `allow` over `expect` when configuring a response.
+    RUBY
+  end
+
+  it 'flags stubbed message expectation with argument matching' do
+    expect_offense(<<-RUBY)
+      expect(foo).to receive(:bar).with(42).and_return('hello world')
+      ^^^^^^^^^^^ Prefer `allow` over `expect` when configuring a response.
+    RUBY
+  end
+
+  it 'flags stubbed message expectation with argument matching and a block' do
+    expect_offense(<<-RUBY)
+      expect(foo).to receive(:bar).with(42) { 'hello world' }
+      ^^^^^^^^^^^ Prefer `allow` over `expect` when configuring a response.
+    RUBY
+  end
+
+  it 'ignores `have_received`' do
+    expect_no_offenses(<<-RUBY)
+      expect(foo).to have_received(:bar)
+    RUBY
+  end
+
+  it 'flags `receive_messages`' do
+    expect_offense(<<-RUBY)
+      expect(foo).to receive_messages(foo: 42, bar: 777)
+      ^^^^^^^^^^^ Prefer `allow` over `expect` when configuring a response.
+    RUBY
+  end
+
+  it 'flags `receive_message_chain`' do
+    expect_offense(<<-RUBY)
+      expect(foo).to receive_message_chain(:foo, bar: 777)
+      ^^^^^^^^^^^ Prefer `allow` over `expect` when configuring a response.
+    RUBY
+  end
+
+  it 'flags `receive_message_chain` with `.and_return`' do
+    expect_offense(<<-RUBY)
+      expect(foo).to receive_message_chain(:foo, :bar).and_return(777)
+      ^^^^^^^^^^^ Prefer `allow` over `expect` when configuring a response.
+    RUBY
+  end
+
+  it 'flags `receive_message_chain` with a block' do
+    expect_offense(<<-RUBY)
+      expect(foo).to receive_message_chain(:foo, :bar) { 777 }
+      ^^^^^^^^^^^ Prefer `allow` over `expect` when configuring a response.
+    RUBY
+  end
+
+  it 'flags with order and count constraints', :pending do
+    expect_offense(<<-RUBY)
+      expect(foo).to receive(:bar) { 'hello world' }.ordered
+      ^^^^^^^^^^^ Prefer `allow` over `expect` when configuring a response.
+      expect(foo).to receive(:bar).ordered { 'hello world' }
+      ^^^^^^^^^^^ Prefer `allow` over `expect` when configuring a response.
+      expect(foo).to receive(:bar).with(42).ordered { 'hello world' }
+      ^^^^^^^^^^^ Prefer `allow` over `expect` when configuring a response.
+      expect(foo).to receive(:bar).once.with(42).ordered { 'hello world' }
+      ^^^^^^^^^^^ Prefer `allow` over `expect` when configuring a response.
+      expect(foo).to receive(:bar) { 'hello world' }.once.with(42).ordered
+      ^^^^^^^^^^^ Prefer `allow` over `expect` when configuring a response.
+      expect(foo).to receive(:bar).once.with(42).and_return('hello world').ordered
+      ^^^^^^^^^^^ Prefer `allow` over `expect` when configuring a response.
+    RUBY
+  end
+
+  it 'flags block-pass' do
+    expect_offense(<<-RUBY)
+      canned = -> { 42 }
+      expect(foo).to receive(:bar, &canned)
+      ^^^^^^^^^^^ Prefer `allow` over `expect` when configuring a response.
+      expect(foo).to receive(:bar).with(42, &canned)
+      ^^^^^^^^^^^ Prefer `allow` over `expect` when configuring a response.
+      expect(foo).to receive_message_chain(:foo, :bar, &canned)
+      ^^^^^^^^^^^ Prefer `allow` over `expect` when configuring a response.
+    RUBY
+  end
+
+  it 'flags `is_expected`' do
+    expect_offense(<<~RUBY)
+      is_expected.to receive(:bar).and_return(:baz)
+      ^^^^^^^^^^^ Prefer `allow(subject)` over `is_expected` when configuring a response.
+    RUBY
+  end
+
+  it 'flags `expect_any_instance_of`' do
+    expect_offense(<<~RUBY)
+      expect_any_instance_of(Foo).to receive(:bar).and_return(:baz)
+      ^^^^^^^^^^^^^^^^^^^^^^^^^^^ Prefer `allow_any_instance_of` over `expect_any_instance_of` when configuring a response.
+    RUBY
+  end
+
+  it 'ignores message allowances' do
+    expect_no_offenses(<<-RUBY)
+      allow(foo).to receive(:bar).and_return('hello world')
+      allow(foo).to receive(:bar) { 'hello world' }
+      allow(foo).to receive(:bar).with(42).and_return('hello world')
+      allow(foo).to receive(:bar).with(42) { 'hello world' }
+      allow(foo).to receive_messages(foo: 42, bar: 777)
+      allow(foo).to receive_message_chain(:foo, bar: 777)
+      allow(foo).to receive_message_chain(:foo, :bar).and_return(777)
+      allow(foo).to receive_message_chain(:foo, :bar) { 777 }
+      allow(foo).to receive(:bar, &canned)
+    RUBY
+  end
+
+  it 'tolerates passed arguments without parentheses' do
+    expect_offense(<<-RUBY)
+      expect(Foo)
+      ^^^^^^^^^^^ Prefer `allow` over `expect` when configuring a response.
+        .to receive(:new)
+        .with(bar).and_return baz
+    RUBY
+  end
+end