From 1c09819f626dd08e484884abf4d592e159b7511b Mon Sep 17 00:00:00 2001 From: Phil Pirozhkov Date: Tue, 18 Aug 2020 20:30:02 +0300 Subject: [PATCH 1/2] Adjust spec to allow `pending` Enabled state --- spec/project/default_config_spec.rb | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/spec/project/default_config_spec.rb b/spec/project/default_config_spec.rb index 702ea1173a..b431543c45 100644 --- a/spec/project/default_config_spec.rb +++ b/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 From ee4f9fdcff10964f39d4629bb88be2c203c6e6d3 Mon Sep 17 00:00:00 2001 From: Benjamin Quorning Date: Tue, 18 Oct 2016 22:56:10 +0200 Subject: [PATCH 2/2] Add RSpec/StubbedMock cop Co-authored-by: Phil Pirozhkov --- CHANGELOG.md | 1 + config/default.yml | 6 + docs/modules/ROOT/pages/cops.adoc | 1 + docs/modules/ROOT/pages/cops_rspec.adoc | 30 ++++ lib/rubocop/cop/rspec/stubbed_mock.rb | 172 ++++++++++++++++++++ lib/rubocop/cop/rspec_cops.rb | 1 + spec/rubocop/cop/rspec/stubbed_mock_spec.rb | 133 +++++++++++++++ 7 files changed, 344 insertions(+) create mode 100644 lib/rubocop/cop/rspec/stubbed_mock.rb create mode 100644 spec/rubocop/cop/rspec/stubbed_mock_spec.rb diff --git a/CHANGELOG.md b/CHANGELOG.md index ba91b12b76..b655c8fe63 100644 --- a/CHANGELOG.md +++ b/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) diff --git a/config/default.yml b/config/default.yml index 9ffa470ba6..f32511682a 100644 --- a/config/default.yml +++ b/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 diff --git a/docs/modules/ROOT/pages/cops.adoc b/docs/modules/ROOT/pages/cops.adoc index 1f3d3ed996..24d8e9ae90 100644 --- a/docs/modules/ROOT/pages/cops.adoc +++ b/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] diff --git a/docs/modules/ROOT/pages/cops_rspec.adoc b/docs/modules/ROOT/pages/cops_rspec.adoc index 71d8ead5b9..ecdb752aa1 100644 --- a/docs/modules/ROOT/pages/cops_rspec.adoc +++ b/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 |=== diff --git a/lib/rubocop/cop/rspec/stubbed_mock.rb b/lib/rubocop/cop/rspec/stubbed_mock.rb new file mode 100644 index 0000000000..bd508ceafc --- /dev/null +++ b/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 `%s` over `%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] 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 diff --git a/lib/rubocop/cop/rspec_cops.rb b/lib/rubocop/cop/rspec_cops.rb index 84281c86c5..22599a7606 100644 --- a/lib/rubocop/cop/rspec_cops.rb +++ b/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' diff --git a/spec/rubocop/cop/rspec/stubbed_mock_spec.rb b/spec/rubocop/cop/rspec/stubbed_mock_spec.rb new file mode 100644 index 0000000000..ab28f052da --- /dev/null +++ b/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