Rubocop-rspec: Avoid `not_to receive`, `not_to have_received` etc.

[bquorning]

# bad
foo.something
expect(foo).not_to have_received(:decrease_counter)

# good
foo.something
expect(foo.counter).to eq(1)
# or
expect { foo.something }.not_to change { foo.counter }

If you refactor and rename :decrease_counter to e.g. :decrease, the first spec would check nothing, but the second specs would still cover the relevant side effect.

[andyw8]

Hmm, there are valid cases where you would want to ensure a particular message wasn't sent to a collaborator.

I would say the 'bad' in the first example example is that an expectation is being set on the SUT (i.e. it's testing an implementing detail).

[backus]

Interesting. I actually have to think about this more. I was initially going to comment that I don't think I would ever write that test but apparently I have at least three times in the last year.

For example, I have some code which parses user input. If we parse it properly and it is invalid then we just return a user error. If we fail to even parse it entirely (can happen on rare occasions) then we notify our bug tracker. As a result, we have specs like this

context 'when invalid input given' do
  let(:input) { 'invalid but parseable' }

  it 'does not notify bug tracker' do
    allow(BugTracker).to receive(:notify)

    expect(handle_input).to be(false)

    expect(BugTracker).not_to have_received(:notify)
  end

  it_behaves_like 'a rejected request'
end

context 'when unparseable input given' do
  let(:input) { 'unparseable!' }

  it 'is invalid and notifies bug tracker' do
    allow(BugTracker).to receive(:notify)

    expect(handle_input).to be(false)

    expect(BugTracker).to have_received(:notify).with(SomeLib::ParseError)
  end

  it_behaves_like 'a rejected request'
end

This seems hard to avoid. We also have similar code for bootstrapping our application. For example, for test mode we expect Sidekiq::Testing to receive inline!. In dev mode and prod we assert it doesn't. I'm not sure how we would test these branches otherwise

[dgollahon]

There is a valid point here because it seems kind of similar to the expect { ... }.to raise_error problem in terms of potentially missing something, but OTOH the real problem is that it's doing an expect on something that it doesn't check for valid methods. You can have the same class of problem by doing expect(...).to have_received(...) as well though if your test calls a method you allow in rspec but doesn't exist.

In either case the problem is easily solved by using an instance_double (if you can insert a double). So, it seems like we might be punishing a few valid uses when the problem seems to me to be more of choosing to do an allow/expect on an unverifiable option.

@bquorning, @andyw8, @backus, any further thoughts? I think it'd be nice if we could agree on whether or not this makes sense and convert it into an (in?)action item.

[pirj]

In case of properly defined balancing cases, when there is another example that is checking that decrease_counter has been received, and with verifying doubles turned on, this doesn't seem to be a problem anymore.

It's not practical in common case to test side effects, as this might require more stubbing, slows specs down due to real method calls.

What's worst is it breaks boundaries of what we actually test, resulting in brittle tests when the implementation of receiver changes, and we're forced to update all consumer tests (expectations and stubs).

[dgollahon]

@pirj I follow some of what you're saying but I'm actually not clear on your conclusion--are you saying we don't need a cop for this and doing not_to have_received is ok or the opposite?

[pirj]

Yes, basically my point is that this cop is unnecessary.

I feel it's easier to explain with an example:

module Backend
  class << self
    attr_reader :state

    def long(state)
      sleep 2
      @state = state
    end
  end
end

class Notifier
  def initialize(use_shortcut: false)
    @use_shortcut = use_shortcut
  end

  def notify(state)
    if @use_shortcut
      "meh"
    else
      Backend.long(state)
    end
  end
end

# Isolated, balanced
RSpec.describe Notifier, 'isolated' do
  context 'without shortcut' do
    it 'receives a precious result' do
      expect(Backend).to receive(:long).with(1)
      Notifier.new.notify(1)
    end
  end

  context 'with shortcut' do
    it 'receives zilch' do
      expect(Backend).not_to receive(:long)
      Notifier.new(use_shortcut: true).notify(2)
    end
  end
end

# Non-isolated, balanced
RSpec.describe Notifier, 'breached' do
  context 'without shortcut' do
    it 'receives a precious result' do
      expect { Notifier.new.notify(3) }
        .to change { Backend.state }.to(3)
    end
  end

  context 'with shortcut' do
    it 'receives zilch' do
      expect { Notifier.new(use_shortcut: true).notify(4) }
        .not_to change { Backend.state }
    end
  end
end

$ rspec --profile=4 spec/a_spec.rb
...

Top 4 slowest examples (2.07 seconds, 99.5% of total time):
  Notifier breached without shortcut receives a precious result
    2.01 seconds ./spec/a_spec.rb:46
  Notifier isolated without shortcut receives a precious result
    0.0516 seconds ./spec/a_spec.rb:29
  Notifier breached with shortcut receives zilch
    0.01 seconds ./spec/a_spec.rb:53
  Notifier isolated with shortcut receives zilch
    0.00071 seconds ./spec/a_spec.rb:36

Non-isolated is slower (in this case insanely slower).

In both cases, we rely on Backend's interface (state or long), as in the original example with foo we rely either on decrease_counter or counter.
However, we have an agreement that Notifier is able to call long, while there's no guarantee that state is there to stay. In case the reader interface changes, the examples using change will have to be updated, indicating brittleness.

My eye is soaped though, pretty sure there are equally valid opposite arguments.

On a side note, it seems that the original bad example is also bad because it's stubbing the subject:

# bad
foo.something
expect(foo).not_to have_received(:decrease_counter)

PS Tried to use doubles in those examples, but encountered that case when a real object keeps the double for itself between the examples:

       #<InstanceDouble(String) (anonymous)> was originally created in one example but has leaked into another example and can no longer be used. rspec-mocks' doubles are designed to only last for one example, and you need to create a new one in each example you wish to use it for.

so resorted to use arbitrary numbers instead. There's a trick to reuse and reset mock proxy, but it's over the top for this spec.

[dgollahon]

Got it.

I'm in favor of just closing this--I don't think it has a clear enough benefit to merit a cop. @bquorning, do you have a dissenting opinion?

[pirj]

Closing as controversial.