Discussion: RSpec/SubjectStub

[FLeinzi]

Today I want to discuss the RSpec/SubjectStub cop.

# bad
describe Foo do
  subject(:bar) { baz }

  before do
    allow(bar).to receive(:qux?).and_return(true)
  end
end

see also: https://docs.rubocop.org/rubocop-rspec/cops_rspec.html#rspecsubjectstub

I don't know, if we really want this cop. Although I understand its purpose, I think it's sometimes necessary to stub the object under test, e.g. if we mock requests to an external API. I don't want to write let instead of subject only to silence it.

What do you think?

[dastra-mak]

I think it's valid to stub some methods of the subject. Here is an example when this comes in handy:

describe Employee do

  describe '#office' do

    context 'is a manager' do
      before { allow(subject).to receive(:manager?).and_return(true) }
      it { is_expected.to validate_presence_of(:office) }
    end

    context 'is not a manager' do
      before { allow(subject).to receive(:manager?).and_return(false) }
      it { is_expected.not_to validate_presence_of(:office) }
    end

  end
end

[codener]

This cop is based on this article by Thoughtbot:

Why not stub the System Under Test

The goal of the guideline “Don’t Stub the System Under Test” is to help us use tests as a guide for when to split up a class. If a behavior is so complicated that we felt compelled to stub it out in a test, that behavior is its own concern that should be encapsulated in a class.

How to avoid stubbing the System Under Test

Each time I’m tempted to stub the SUT, I think about why I didn’t want to set up the required state.

• If extracting a helper or factory to set up the state wouldn’t be ugly or cause other issues, I’ll do that and remove the stub.
• If the method I’m stubbing has complicated behavior that’s aggravating to retest, I use that as a cue to extract a new class, and then I stub the new dependency.

I have never looked at it like this, but it looks like a simple strategy for better code quality. When writing tests first, this should not even slow down development.

[triskweline]

Vote to disable.

[FLeinzi]

This is an interesting point, @codener.

@dastra-mak But that's also a good case for using traits. You could use a :manager trait in your employee_factory.