RSpec/StubbedMock should offer some rationale

[odinhb]

Related issue: #1061

I've experienced the problem described in #1061 (where the implementation changes and there is a dangling stub) but I fail to see the point of the default (currently only) configuration of this cop.

It seems quite opinionated and should provide some example of a problem which it prevents.

Kind of like these cops do:

• https://docs.rubocop.org/rubocop/1.27/cops_lint.html#lintconstantdefinitioninblock
• https://docs.rubocop.org/rubocop/1.27/cops_lint.html#linteachwithobjectargument

Some cops also link to the ruby style guide, which in turn provides the rationale, but I don't see how this cop corresponds to anything in the style guide.
Many cops are self-explanatory and there may not be any rationale for them other than consistency, but I don't think that applies to this one.

[pirj]

Original ticket. PR that introduced RSpec/StubbedMock.
A notable comment from lead RSpec maintainer on this topic:

Just a personal opinion (which may or may not be shared with my other rspec team members) but this cop should be disabled by default it's very much a personal opinion that creates extra code for people, and rubocop tends to give the impression that something is "recommended best practise" which this isn't.

There is nothing in the RSpec Style guide regarding this, but it's not uncommon:

I could only find 23 cops that enforce style guide guidelines. (out of ~100)

It is common to introduce new cops and enable them in major versions, but with an exception:

Occasionally, some new cops will be introduced as disabled by default. Usually, this means that we believe that the cop is useful, but not for everyone. Typical cases might be the enforcement of programming styles that are not very common in the wild, or cops that yield too many false positives (so you’d run them manually from time to time, instead of running them all the time).

programming styles that are not very common

I can't comment on that as I don't have this information on hand. We can harvest https://github.com/pirj/real-world-rspec to find out.

Off the top of my head, I can think of:

expect(User).to receive(:destroy).and_return(true)
expect(UserCreatorService.create_user).to eq(true)

Changing this to allow in some cases would hinder to detect the mistake (destroy vs create).

Are you up for research to gather legitimate use cases for expect(...).to receive(...).and_..., or otherwise put such spec examples to shame and add a rationale to the cop and a guideline to the style guide?

[pirj]

@odinhb Ping

[odinhb]

Sorry, we disabled this cop really quickly, so it's no longer bothering me. 😅

We discussed it a bit internally, and my coworker mentioned a rule (although I don't know this rule's rationale) which states there should only be a single expectation per test. Apparently this cop kinda aligns with this rule, as expecting a message to be received in addition to stubbing it to return something means there are multiple expectations. This is recommended by some other test framework as well and perhaps most interestingly, the rspec style guide also mentions it:

https://rspec.rubystyle.guide/#expectation-per-example

However, this style guide also says you can have multiple expectations per test if you use aggregate failures.
Personally, this is how I write most of my specs, instead of getting caught up in refactoring the spec setup.

I feel the style guide entry conflicts with this cop being enabled by default. This cop seems to be about keeping a single expectation per spec, because using expect().to receive would mean your test expects multiple things (since you will probably have a regular expectation at the bottom of the example).

I'm not sure what more research there is to do. It looks to me like the pull request you linked contained enough dissenting voices that it should've been disabled by default.

I might check out the real world rspec repo, but I'm not sure what I'll find there or if it'll be of any use, and I think I'll struggle to provide rationale for a cop I disagree with.

I wrote this issue with the hope that someone else could provide some rationale, so I could maybe learn something. As JonRowe said, RuboCop cops leave the impression that something is best practice, but this doesn't seem to be that.

[pirj]

there should only be a single expectation per test

This rule is outdated. For the context:

• an updated guideline
• PR relaxing the guideline
• a comment from Myron, long-time RSpec maintainer

the style guide entry conflicts with this cop being enabled by default

Not really. The cop is more about allowing vs expecting messages with a configured response.
Even if you stick with "one expectation per example" rule, the following code won't trigger RSpec/MultipleExpectations offence:

allow(foo).to receive(:bar).with(42).and_return("hello world") # setup - doesn't count
expect(foo).to receive(:bar).with(42) # make an expectation
subject.exercise # excercise

Though it will if you both make an expectation on both the received message (:bar with 42), and the result returned by e.g. subject.excercise, RSpec/MultipleExpectations will trigger.

enough dissenting voices that it should've been disabled by default

As per RuboCop's practice:

Occasionally, some new cops will be introduced as disabled by default. Usually, this means that we believe that the cop is useful, but not for everyone. Typical cases might be the enforcement of programming styles that are not very common in the wild, or cops that yield too many false positives

• ❔ the cop is useful, but not for everyone
• ❌ enforcement of programming styles that are not very common - can't comment on that. I believe we did some analysis of real-world-rspec, but I can't quickly find it
• ❌ yield too many false positives

RuboCop and RuboCop RSpec provide cops with configurable styles, and every so often those styles are opposite. They are enabled by default. It doesn't mean the default enforced style is the one everyone should use. E.g. RSpec/ExpectChange:

      # @example `EnforcedStyle: block`
      #   # bad
      #   expect { run }.to change(Foo, :bar)
      #
      #   # good
      #   expect { run }.to change { Foo.bar }
      #
      # @example `EnforcedStyle: method_call`
      #   # bad
      #   expect { run }.to change { Foo.bar }
      #
      #   # good
      #   expect { run }.to change(Foo, :bar)

We usually enable cops to make them visible.

RuboCop is extremely flexible and most aspects of its behavior can be tweaked via various configuration options.

It's up to those who use RuboCop and its extensions to configure it to meet their style.
RuboCop is a tool, not an opinionated linter dictating its rigid defaults, even though some cops are tightly linked to style guide guidelines.

Occasionally, we change cops' default enforced styles, but this is a rare case. See one such discussion as an example.

someone else could provide some rationale

We encourage contributions, but typically people scratch their itch, and expecting someone else to fill in the gaps doesn't work.

It would certainly be fantastic if every cop is linked to a style guide guideline, and those guidelines and "independent" cops provided rationales and reasoning. But the amount of work to be done is beyond the current maintainers' capacity to be done in a reasonable time.

If you feel that you are personally interested in contributing a rationale for this cop - you are very welcome. Otherwise, I'm inclined to close the ticket, as this is a more generic problem than just one cop, and it's unlikely that someone would take this over with nearly 100 other open tickets.

[odinhb]

We encourage contributions, but typically people scratch their itch, and expecting someone else to fill in the gaps doesn't work.

If you feel that you are personally interested in contributing a rationale for this cop - you are very welcome. Otherwise, I'm inclined to close the ticket, as this is a more generic problem than just one cop, and it's unlikely that someone would take this over with nearly 100 other open tickets.

I don't know what the rationale is. That's why I opened this issue: someone should know why this cop exists. Otherwise it implies that the cop is arbitrary.
I'm not expecting this issue to be solved quickly, and I don't understand the rush.

As per RuboCop's practice: <...snip>

What do the question marks and X'ses mean? I don't entirely follow this section.

It's up to those who use RuboCop and its extensions to configure it to meet their style.

RuboCop is a tool, not an opinionated linter dictating its rigid defaults

It doesn't mean the default enforced style is the one everyone should use. E.g. RSpec/ExpectChange:

I understand wanting to ease the maintenance burden, but good defaults are important. The configurability of RuboCop is great, but the defaults it ships with still imply something. That's the reason I created this issue in the first place: I'd been searching for rationale because this cop was enabled by default, yet it seemed offensively arbitrary to me and I could not find anything other than prescriptive statements.

You quoted someone earlier who was pointing out the exact same thing:

A #226 (comment) from lead RSpec maintainer on this topic:

Just a personal opinion (which may or may not be shared with my other rspec team members) but this cop should be disabled by default it's very much a personal opinion that creates extra code for people, and rubocop tends to give the impression that something is "recommended best practise" which this isn't.

A quick and cheap solution is to disable this specific cop.
However, fixing the broader issue of every cop not having rationale starts by providing rationale for one cop at a time, so my preference would be to leave this issue here as a discussion, an admission of sorts that this cop has no rationale. If no one knows anything then that implies that the cop is entirely arbitrary.

[pirj]

I don't know what the rationale is

If this cop doesn't fit your project's style - turn it off in your .rubocop.yml.

A quick and cheap solution is to disable this specific cop.

Same as above.

fixing the broader issue of every cop not having rationale starts by providing rationale for one cop at a time, so my preference would be to leave this issue here as a discussion, an admission of sorts that this cop has no rationale

You are welcome to open a broader issue, add a checklist and send PRs. Adding clarity to cops' descriptions and the style guide is very much appreciated. I must confess, I gave up on the idea of restoring references to old discussions for the style guide.

someone should know why this cop exists. Otherwise it implies that the cop is arbitrary

It is arbitrary and disabled if it suggests uncommon practices, or is yields too many false positives.
Do you have evidence of those two?
Please feel free to reopen this ticket if it turns out that there's a noticeable number of expect(...).to receive(...).and_* cases if popular public codebases, or the cop yields many false positives.

I clearly see your point, but it is based on an incorrect assumption that the default configuration is ideal and fits everyone.