Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RSpec/SubjectStub matches all matcher #488

Closed
walf443 opened this issue Nov 3, 2017 · 12 comments

Comments

@walf443
Copy link
Contributor

commented Nov 3, 2017

Here is a patch to reproduce problem.

diff --git spec/rubocop/cop/rspec/subject_stub_spec.rb spec/rubocop/cop/rspec/subject_stub_spec.rb
index fc661f2..4b0aa88 100644
--- spec/rubocop/cop/rspec/subject_stub_spec.rb
+++ spec/rubocop/cop/rspec/subject_stub_spec.rb
@@ -43,6 +43,18 @@ RSpec.describe RuboCop::Cop::RSpec::SubjectStub do
     RUBY
   end

+  it 'ignores stub when inside all matcher' do
+    expect_no_offenses(<<-RUBY)
+      describe Foo do
+        subject(:foo) { [Object.new] }
+
+        it 'tries to trick rubocop-rspec' do
+            expect(foo).to all(receive(:baz))
+        end
+      end
+    RUBY
+  end
+
   it 'ignores stub within context where subject name changed' do
     expect_no_offenses(<<-RUBY)
       describe Foo do

In above case, subject is array, and it just stub item of subject. So it should not add offence, I think.

@Darhazer

This comment has been minimized.

Copy link
Member

commented Nov 6, 2017

I don't get your point, You are still stubbing the part of the subject. Maybe you can help with a more real-world example of this?

@bquorning

This comment has been minimized.

Copy link
Member

commented Nov 6, 2017

@Darhazer

This comment has been minimized.

Copy link
Member

commented Nov 6, 2017

10x. Now it makes perfect sense

@pirj

This comment has been minimized.

Copy link
Member

commented May 15, 2019

So there is an object that acts as a container for and a delegator of method calls to containing items by inheriting from an Array.

The original code that suffered from a false detection was:

expect(formatter_set).to all(receive(:started).with(files))
formatter_set.started(files)

However, it has been reworked to:

formatter_set.each do |formatter|
  expect(formatter).to receive(:started).with(files)
end
formatter_set.started(files)

which in case of this specific object design is more indicative of what happens.

@bquorning @Darhazer WDYT of dropping the special case for all from SubjectStub cop?

@pirj pirj referenced this issue May 15, 2019
5 of 5 tasks complete
@bquorning

This comment has been minimized.

Copy link
Member

commented May 15, 2019

@pirj

This comment has been minimized.

Copy link
Member

commented May 15, 2019

You're right, I followed the wrong link, sorry for the confusion. Anyway, the current way to test it is a bit odd:

before do
  formatter_set.add_formatter('simple')
  formatter_set.add_formatter('emacs')

  formatter_set.each do |formatter|
    allow(formatter).to receive(:started)
  end
end

it 'invokes same method of all containing formatters' do
  formatter_set.started(files)

  expect(formatter_set).to all(have_received(:started).with(files))
end

How is it better than the following?

before do
  formatter_set.add_formatter('simple')
  formatter_set.add_formatter('emacs')
end

it 'invokes the same method of all containing formatters' do
  formatter_set.each do |formatter|
    expect(formatter).to receive(:started).with(files)
  end
  formatter_set.started(files)
end
@Darhazer

This comment has been minimized.

Copy link
Member

commented May 17, 2019

formatter_set.each do |formatter|
    expect(formatter).to receive(:started).with(files)
  end

triggers RSpec/IteratedExpectation ;)

@pirj

This comment has been minimized.

Copy link
Member

commented May 17, 2019

Since there are only two items on that list, it could be

expect(formatter_set[0]).to receive(:started).with(files)
expect(formatter_set[1]).to receive(:started).with(files)
formatter_set.started(files)

As far as I can see, all appeared as a measure against RSpec/IteratedExpectation.

@bquorning

This comment has been minimized.

Copy link
Member

commented Jun 9, 2019

The only thing keeping from approving #770 is this issue.

So, we have an RSpec/IteratedExpectation cop that prevents me from writing expect inside a loop (e.g. each). RSpec provides the all matcher which provides basically the same functionality. This confuses me. Is using the all matcher not as bad as having explicit expectation in a loop?

@pirj, in your last comment you say we can just write out each expectation,

since there are only two items on that list …

What if there were ten or twenty items on the list? I wouldn’t write 20 lines with identical expectations. But would using the all matcher instead just hide the fact that it’s a bad spec?

@pirj

This comment has been minimized.

Copy link
Member

commented Jun 9, 2019

@bquorning good question.

There are two of them in the spec to, I assume, make sure that more than one formatter can be added to the set. Covering the case with more formatters is a slippery path since there's no limitation on the number of formatters.

It's possible to interpret "Needed data" recommendation in a way that one shouldn't test all possible cases, just enough of them.
So, it doesn't make much sense to add more than two or three expectations like that.

Let's look at this spec from a different angle. There are formatters, and there is a container of formatters. We chose the container to be a subject of this spec.

In this example, we send some message to the container and expect formatters to receive the same message.

A typical spec would look like this:

subject(:formatter_set) { FormatterSet.new(formatters) }
let(:formatters) { 2.times { instance_double(Formatter) } } # or spy(Formatter)

it 'invokes the same method of all containing formatters' do
  files = double
  expect(formatters).to all receive(:started).with(files)
  formatter_set.started(files)
end

which will be ignored by both SubjectStub and IteratedExpectation cops. Even with 20 formatters passed in.

Unfortunately, due to the current FormatterSet design, there is no way to pass formatters to it, and we are forced to make an expectation using the subject itself.

Well, I would agree if you said that usage of

expect(formatter_set[0]).to receive(...)

is not any better than

expect(formatter_set).to all receive(...)

and I would partially agree with that.
With an exception that all is a matcher that operates directly on a subject/formatter_set provided to it, while in the case with expect(formatter_set[0]) it's not the subject directly on which the expectation is being made.

Both cases are kind of odd. But this doesn't mean the cop in question should tolerate the case with all.

@Darhazer

This comment has been minimized.

Copy link
Member

commented Jun 10, 2019

all is an interesting edge case. It doesn't really stub the subject (if stub is used at all in the matcher) - it stubs whatever each returns, which we can't know what it is. Still, I think it makes specs harder to understand and the way @pirj suggest is a way better.

So I tend to agree with @pirj line of reasoning and if you really need to test the delegation in the collection while not having control of what collection actually contains, you can always disable the cop for this spec, or use expect(formatter_set.each).to all(receive(:started)), which won't trigger either of the cops

@bquorning

This comment has been minimized.

Copy link
Member

commented Jun 10, 2019

all is an interesting edge case.

Agreed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.