Recognize stubbing of described_class in RSpec/SubjectStub

[lovro-bikic]

This PR makes stubbing of described_class register an offense for RSpec/SubjectStub cop. I think this fits under this cop since its purpose is to prevent stubbing the system under test, and described_class is just that.

To give a production code example offense: I recently noticed a spec for a module that had a allow(described_class).to receive(:some_module_method).

---

Before submitting the PR make sure the following are checked:

• Feature branch is up-to-date with master (if not - rebase it).
• Squashed related commits together.
• Added tests.
• Updated documentation.
• Added an entry to the CHANGELOG.md if the new code introduces user-observable changes.
• The build (bundle exec rake) passes (be sure to run this locally, since it may produce updated documentation that you will need to commit).

If you have created a new cop:

• Added the new cop to config/default.yml.
• The cop is configured as Enabled: pending in config/default.yml.
• The cop is configured as Enabled: true in .rubocop.yml.
• The cop documents examples of good and bad code.
• The tests assert both that bad code is reported and that good code is not reported.
• Set VersionAdded: "<<next>>" in default/config.yml.

If you have modified an existing cop's configuration options:

• Set VersionChanged: "<<next>>" in config/default.yml.

[pirj]

In general, this is useful. Why is this bad?

how is this different from allow(Article)?

I see that in a spec dedicated to Article it may be strange to mock its methods, specifically the initializer, or other static ones.
However, how do you otherwise test static methods that call other static methods in isolation? (Considering those static methods arr the subject under test, noth the whole class).

Can you probably give a more realistic example of bad code that made you extend this cop in the forst place?

[lovro-bikic]

how is this different from allow(Article)?

It's the same as allow(Article), but note that there's another cop RSpec/DescribedClass which would autocorrect Article to described_class.

In fact, I think allow(Article) should be registered as an offense as well since it's also the subject under test (because not everyone might have RSpec/DescribedClass enabled). I can work on this as a separate PR.

In general, this is useful. Why is this bad?

Let's say your subject is subject(:article) { Article.new }. This cop will currently register this:

allow(article).to receive(:foo)

as an offense.

However, it won't register:

allow_any_instance_of(described_class).to receive(:foo)

as an offense, which is quite similar to the above (except the former will stub only one instance, and the latter will stub any instance).

In both scenarios, we are stubbing a method of the object under test.

[lovro-bikic]

Can you probably give a more realistic example of bad code that made you extend this cop in the forst place?

Let's say we have a simple module:

module TokenGenerator
  extend self

  def generate
    { token: random_token }
  end

  private

  def random_token
    SecureRandom.uuid
  end
end

and a spec like:

RSpec.describe TokenGenerator do
  before do
    allow(described_class).to receive(:random_token).and_return('123')
  end

  it 'creates a token' do
    expect(TokenGenerator.generate).to eq({ token: '123' })
  end
end

In my view, this is not a good spec because we're not letting the module execute the code we're supposedly testing. A better spec would be:

RSpec.describe TokenGenerator do
  it 'creates a token' do
    expect(TokenGenerator.generate).to include({ token: be_a(String) })
  end
end

which goes through the whole code path of the module.

[pirj]

It’s an easy one.

1. Smells because of stubbing of private methods
2. be_a(String) won’t cut it - if someone mistakenly replaces the call to uuid with a static "not really random", the test won’t catch it. You have to stub, but not the private method, but the external uuid.

Stubbing the class under test’s public methods though may be an indication of a smell or may not. I don’t feel we should always flag corresponding tests as offenses.

Do you have other examples, preferably real life ones?

[lovro-bikic]

This is a real-life example 😄

If TokenGenerator were a class instead, this cop would register an offense:

RSpec.describe TokenGenerator do
  subject(:generator) { TokenGenerator.new }

  before do
    allow(generator).to receive(:random_token).and_return('123')
#   ^^^^^^^^^^^^^^^^ RSpec/SubjectStub: Do not stub methods of the object under test.
  end
end

Can you explain to me why should this scenario be an offense under this cop, but the module TokenGenerator example shouldn't?

[pirj]

Fair enough!

But is it still the same "subject under test" if we make assertions on an instsnce, but stub class methods? 🤔

[lovro-bikic]

That's a good question. With this PR and also #2094 I've expanded the cop to register offenses for not just the subject's methods (which are typically an instance's methods), but also any method of the class/module for any instance. I guess this gives more responsibility to this cop, which might or might not be desirable.

The style guide shows this:

describe 'Article' do
  subject(:article) { Article.new(author: nil) }

  it 'indicates that the author is unknown' do
    expect(article.description).to include('by an unknown author')
  end
end

as a good example, but the test could still stub the #author method to return nil in any of the following ways:

before do
  allow_any_instance_of(Article).to receive(:author).and_return(nil)
  allow_any_instance_of(described_class).to receive(:author).and_return(nil)
end

and these would not be offenses under any RSpec cop, even though they are in practice similar to:

subject(:article) { Article.new }

before do
  allow(article).to receive(:author).and_return(nil)
end

which is an offense.

Personally, I think (allow|expect)_any_instance_of and described_class should register offenses somewhere, much like RSpec/SubjectStub does, but perhaps this should be a different cop (e.g. RSpec/DescribedClassStub)?

[pirj]

Your argument related to the usage of *_any_instance_of is convincing.

It’s tougher with described_class. I especially like this comment.

[lovro-bikic]

It’s tougher with described_class. I especially like rspec/rspec#31 (comment).

True (I also got bit by this when I once used describe :symbol foolishly), but then again, there's the RSpec/DescribedClass cop which enforces usage of described_class, including for stubs. Couldn't offenses for stubbing described_class follow the same logic as that cop?

[pirj]

Please don't get me wrong.
Most of the cops in this repo are born out of extreme frustration with some specs coding patterns.

It happened before that some completely incorrect things were present in one and only repo, so we don't always request to "add a corresponding style guideline" or "harvest real-world-rspec repos for occurrences". I can swear that I saw myself something like before { it { is_expected.to be true } }, and wanted to add a cop to disallow this.

Please help me see the problem, and let's figure out how to deal with it.