Skip to content

Recognize stubbing of described_class in RSpec/SubjectStub #2088

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Recognize stubbing of described_class in RSpec/SubjectStub #2088

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@

## Master (Unreleased)

- Recognize stubbing of `described_class` in `RSpec/SubjectStub`. ([@lovro-bikic])

## 3.6.0 (2025-04-18)

- Fix false positive in `RSpec/Pending`, where it would mark the default block `it` as an offense. ([@bquorning])
Expand Down
10 changes: 10 additions & 0 deletions docs/modules/ROOT/pages/cops_rspec.adoc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6096,6 +6096,16 @@ describe Article do
end
end

# bad - described_class stubs are recognized as well
describe Article do
it 'indicates that the author is unknown' do
article = double(Article, description: 'by an unknown author')
allow(described_class).to receive(:new).and_return(article)

expect(Article.new.description).to include('by an unknown author')
end
end

# good
describe Article do
subject(:article) { Article.new(author: nil) }
Expand Down
12 changes: 11 additions & 1 deletion lib/rubocop/cop/rspec/subject_stub.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,16 @@ module RSpec
# end
# end
#
# # bad - described_class stubs are recognized as well
# describe Article do
# it 'indicates that the author is unknown' do
# article = double(Article, description: 'by an unknown author')
# allow(described_class).to receive(:new).and_return(article)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@lovro-bikic lovro-bikic Jul 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@lovro-bikic lovro-bikic Jul 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough!

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

Copy link
Contributor Author

@lovro-bikic lovro-bikic Aug 1, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Copy link
Contributor Author

@lovro-bikic lovro-bikic Aug 1, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

#
# expect(Article.new.description).to include('by an unknown author')
# end
# end
#
# # good
# describe Article do
# subject(:article) { Article.new(author: nil) }
Expand Down Expand Up @@ -141,7 +151,7 @@ def find_subject_expectations(node, subject_names = [], &block)
subject_names = [*subject_names, *@explicit_subjects[node]]
subject_names -= @subject_overrides[node] if @subject_overrides[node]

names = Set[*subject_names, :subject]
names = Set[*subject_names, :subject, :described_class]
expectation_detected = message_expectation?(node, names)
return yield(node) if expectation_detected

Expand Down
11 changes: 11 additions & 0 deletions spec/rubocop/cop/rspec/subject_stub_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -364,6 +364,17 @@
RUBY
end

it 'flags when described_class is mocked' do
expect_offense(<<~RUBY)
describe Foo do
it 'uses described_class' do
expect(described_class).to receive(:bar).and_return(baz)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Do not stub methods of the object under test.
end
end
RUBY
end

it 'flags when there are several top level example groups' do
expect_offense(<<~RUBY)
RSpec.describe Foo do
Expand Down