Skip to content

Commit

Permalink
Merge pull request #770 from pirj/improve-subject-stub-cop
Browse files Browse the repository at this point in the history 
Improve RSpec/SubjectStub cop
  • Loading branch information
@bquorning
bquorning committed Jun 10, 2019
2 parents d9b46ba + 05bf0c7 commit 9e90461
Show file tree
Hide file tree
Showing 3 changed files with 130 additions and 31 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Expand Up @@ -5,6 +5,7 @@
* Remove `AggregateFailuresByDefault` config option of `RSpec/MultipleExpectations`. ([@pirj][])
* Add `RSpec/LeakyConstantDeclaration` cop. ([@jonatas][], [@pirj][])
* Improve `aggregate_failures` metadata detection of `RSpec/MultipleExpectations`. ([@pirj][])
* Improve `RSpec/SubjectStub` detection and message. ([@pirj][])

## 1.33.0 (2019-05-13)

Expand Down
33 changes: 16 additions & 17 deletions lib/rubocop/cop/rspec/subject_stub.rb
View file
Expand Up @@ -6,6 +6,8 @@ module RSpec
# Checks for stubbed test subjects.
#
# @see https://robots.thoughtbot.com/don-t-stub-the-system-under-test
# @see https://samphippen.com/introducing-rspec-smells-and-where-to-find-them#smell-1-stubject
# @see https://github.com/rubocop-hq/rspec-style-guide#dont-stub-subject
#
# @example
# # bad
Expand All @@ -20,7 +22,7 @@ module RSpec
class SubjectStub < Cop
include RuboCop::RSpec::TopLevelDescribe

MSG = 'Do not stub your test subject.'
MSG = 'Do not stub methods of the object under test.'

# @!method subject(node)
# Find a named or unnamed subject definition
Expand Down Expand Up @@ -56,25 +58,22 @@ class SubjectStub < Cop
# expect(foo).to receive(:bar).with(1)
# expect(foo).to receive(:bar).with(1).and_return(2)
#
# @example source that not matches
# expect(foo).to all(receive(:bar))
#
def_node_matcher :message_expectation?, <<-PATTERN
{
(send nil? :allow (send nil? %))
(send (send nil? :expect (send nil? %)) :to #expectation?)
}
(send
{
(send nil? { :expect :allow } (send nil? {% :subject}))
(send nil? :is_expected)
}
#{Runners::ALL.node_pattern_union}
#message_expectation_matcher?
)
PATTERN

def_node_matcher :all_matcher?, '(send nil? :all ...)'

def_node_search :receive_message?, '(send nil? :receive ...)'

def expectation?(node)
return if all_matcher?(node)

receive_message?(node)
end
def_node_search :message_expectation_matcher?, <<-PATTERN
(send nil? {
:receive :receive_messages :receive_message_chain :have_received
} ...)
PATTERN

def on_block(node)
return unless example_group?(node)
Expand Down
127 changes: 113 additions & 14 deletions spec/rubocop/cop/rspec/subject_stub_spec.rb
View file
Expand Up @@ -3,14 +3,14 @@
RSpec.describe RuboCop::Cop::RSpec::SubjectStub do
subject(:cop) { described_class.new }

it 'complains when subject is stubbed' do
it 'flags when subject is stubbed' do
expect_offense(<<-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.
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Do not stub methods of the object under test.
end
it 'uses expect twice' do
Expand All @@ -20,20 +20,20 @@
RUBY
end

it 'complains when subject is mocked' do
it 'flags when subject is mocked' do
expect_offense(<<-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.
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Do not stub methods of the object under test.
expect(foo).to receive(:bar)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Do not stub your test subject.
^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Do not stub methods of the object under test.
expect(foo).to receive(:bar).with(1)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Do not stub your test subject.
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Do not stub methods of the object under test.
expect(foo).to receive(:bar).with(1).and_return(2)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Do not stub your test subject.
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Do not stub methods of the object under test.
end
it 'uses expect twice' do
Expand All @@ -43,6 +43,45 @@
RUBY
end

it 'flags when an unnamed subject is mocked' do
expect_offense(<<-RUBY)
describe Foo do
subject { described_class.new }
it 'uses unnamed subject' do
expect(subject).to receive(:bar)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Do not stub methods of the object under test.
end
end
RUBY
end

it 'flags an expectation made on an unnamed subject' do
expect_offense(<<-RUBY)
describe Foo do
subject(:foo) { described_class.new }
it 'uses unnamed subject' do
expect(subject).to receive(:bar)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Do not stub methods of the object under test.
end
end
RUBY
end

it 'flags one-line expectcation syntax' do
expect_offense(<<-RUBY)
describe Foo do
subject(:foo) { described_class.new }
it 'uses one-line expectation syntax' do
is_expected.to receive(:bar)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Do not stub methods of the object under test.
end
end
RUBY
end

it 'ignores stub within context where subject name changed' do
expect_no_offenses(<<-RUBY)
describe Foo do
Expand All @@ -59,12 +98,13 @@
RUBY
end

it 'ignores stub when inside all matcher' do
expect_no_offenses(<<-RUBY)
it 'flags stub inside all matcher' do
expect_offense(<<-RUBY)
describe Foo do
subject(:foo) { [Object.new] }
it 'tries to trick rubocop-rspec' do
expect(foo).to all(receive(:baz))
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Do not stub methods of the object under test.
end
end
RUBY
Expand All @@ -80,7 +120,7 @@
before do
allow(foo).to receive(:wow)
^^^^^^^^^^ Do not stub your test subject.
^^^^^^^^^^^^^^^^^^^^^^^^^^^ Do not stub methods of the object under test.
end
it 'tries to trick rubocop-rspec' do
Expand Down Expand Up @@ -119,7 +159,7 @@
context 'when I shake things up' do
before do
allow(foo).to receive(:wow)
^^^^^^^^^^ Do not stub your test subject.
^^^^^^^^^^^^^^^^^^^^^^^^^^^ Do not stub methods of the object under test.
end
it 'tries to trick rubocop-rspec' do
Expand All @@ -141,7 +181,7 @@
before do
allow(foo).to receive(:wow)
allow(bar).to receive(:wow)
^^^^^^^^^^ Do not stub your test subject.
^^^^^^^^^^^^^^^^^^^^^^^^^^^ Do not stub methods of the object under test.
end
it 'tries to trick rubocop-rspec' do
Expand All @@ -163,7 +203,7 @@
it 'still flags this test' do
allow(foo).to receive(:blah)
^^^^^^^^^^ Do not stub your test subject.
^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Do not stub methods of the object under test.
end
end
RUBY
Expand All @@ -184,11 +224,70 @@
allow(foo).to receive(:wow)
allow(bar).to receive(:wow)
allow(baz).to receive(:wow)
^^^^^^^^^^ Do not stub your test subject.
^^^^^^^^^^^^^^^^^^^^^^^^^^^ Do not stub methods of the object under test.
end
end
end
end
RUBY
end

it 'flags negated runners' do
expect_offense(<<-RUBY)
describe Foo do
subject(:foo) { described_class.new }
specify do
expect(foo).not_to receive(:bar)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Do not stub methods of the object under test.
expect(foo).to_not receive(:bar)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Do not stub methods of the object under test.
expect(foo.bar).to eq(baz)
end
end
RUBY
end

it 'flags multiple-method stubs' do
expect_offense(<<-RUBY)
describe Foo do
subject(:foo) { described_class.new }
specify do
expect(foo).to receive_messages(bar: :baz, baz: :baz)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Do not stub methods of the object under test.
expect(foo.bar).to eq(baz)
end
end
RUBY
end

it 'flags chain stubs' do
expect_offense(<<-RUBY)
describe Foo do
subject(:foo) { described_class.new }
specify do
expect(foo).to receive_message_chain(:bar, baz: :baz)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Do not stub methods of the object under test.
expect(foo.bar.baz).to eq(baz)
end
end
RUBY
end

it 'flags spy subject stubs' do
expect_offense(<<-RUBY)
describe Foo do
subject(:foo) { described_class.new }
specify do
allow(foo).to some_matcher_that_allows_a_bar_message
expect(foo.bar).to eq(baz)
expect(foo).to have_received(:bar)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Do not stub methods of the object under test.
end
end
RUBY
end
end

0 comments on commit 9e90461

Please sign in to comment.