Rubocop-rspec: RSpec/SubjectStub matches all matcher

[walf443]

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]

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]

The real-world example is in https://github.com/bbatsov/rubocop/pull/4989/files#diff-be297a2e1f10d4ed00f8faaf78f0f06c

[Darhazer]

10x. Now it makes perfect sense

[pirj]

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?

[bquorning]

You’re linking to the wrong repository. This is RuboCop master: https://github.com/rubocop-hq/rubocop/blob/70ae6ca7978770ea699a2809f57ca2a6bdeec6be/spec/rubocop/formatter/formatter_set_spec.rb#L27

[pirj]

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]

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

triggers RSpec/IteratedExpectation ;)

[pirj]

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]

The only thing keeping from approving https://github.com/rubocop-hq/rubocop-rspec/pull/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]

@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]

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]

all is an interesting edge case.

Agreed.