Disable RSpec/MessageSpies cop by default

[p0deje]

According to @Darhazer the cop should have been disabled by default. I do understand there are reasons (#268) for it to exist and encourage AAA in tests, but there are cases/codebases where it's not appropriate to enforce it.

[pirj]

Do you mean that the cop should be kept enabled, but the default style should be EnforcedStyle: receive?

[p0deje]

Maxim mentioned that it should not be enabled by default:

It is not widely supported though, and it used to be disabled by default Hm, seems it was never really disabled by default, but I believe receive is the more common style

I am actually not 100% sure what should be done. @Darhazer Could you please weigh in?

[Darhazer]

I was under the impression that it is a disabled by default cop, perhaps due to this comment by backus

I'm fine with changing the default though. receive is the more common style. @bquorning WDYT?

[bquorning]

To quote myself from issue https://github.com/rubocop-hq/rubocop-rspec/issues/268#issuecomment-269707746:

The cop is trying to enforce the Four-Phase Test pattern, where you have the expectations at the bottom of the test (right before teardown).

That pattern is, as @p0deje mentions, also known as AAA (Arrange, Act, Assert).

The MessageSpies cop can be configured to use either spies (default behaviour) or “regular” mock objects (with the EnforcedStyle: receive configuration).

If we disable the cop, we are saying there is no need for consistency – you can sometimes use a spy, and you can sometimes use a regular mock (where you have the expectations before the “exercise” step). I personally prefer the enforced consistency, and would like to keep the cop enabled by default.

I'm fine with changing the default though. receive is the more common style.

The current have_received works fine for me. Setting receive as the default kinda goes against the Four-Phase Test pattern that the cop was written to enforce.

[pirj]

I tried to find proof that MessageSpies are disabled more often than not in real-world-ruby-apps, but couldn't.

@p0deje are you up to confirm my results and try to find proof in real-world-rails?

That would be an argument to change the enforced style.

If you are not up for such an effort, I'm more inclined to close this, even though the current default goes against my personal preference of using receive.

[bquorning]

Just for the record: I’d like better to change the default configuration than to disable the cop. But I think it would be confusing for our users.

[p0deje]

There are just 12 examples where the cop is disabled in real-world-rails:

$ rg -C 2 --type yaml  RSpec/MessageSpies | grep -v "_todo" | grep "Enabled: false"
engines/thredded/.rubocop.yml-  Enabled: false
engines/thredded/.rubocop.yml-  Enabled: false
apps/crimethinc-website/.rubocop.yml-  Enabled: false
apps/canvas-lms/.rubocop.yml-  Enabled: false
apps/rubytoolbox/.rubocop.yml-  Enabled: false
apps/lobsters/.rubocop.yml-  Enabled: false
apps/lobsters/.rubocop.yml-  Enabled: false
apps/spree/.rubocop-disabled.yml-  Enabled: false
apps/spree/.rubocop-disabled.yml-  Enabled: false
apps/bridge_troll/.rubocop.yml-  Enabled: false
apps/wiki_edu_dashboard/.rubocop.yml-  Enabled: false
apps/wiki_edu_dashboard/.rubocop.yml-  Enabled: false

There are only 2 examples where receive is a preferred style:

$ rg -C 2 --type yaml  RSpec/MessageSpies | grep -v "_todo" | grep "EnforcedStyle: receive"
apps/atet/.rubocop.yml-  EnforcedStyle: receive
apps/bridge_troll/.rubocop.yml-  EnforcedStyle: receive

[pirj]

It seems to me that in the "real world of Ruby and Rails" people tend not to disable this cop nor to change the enforced style to receive. Even though it feels kind of weird to me personally, I'm more inclined to close this. Please feel free to reopen if you're sure that receive is more commonly used.

[HashNotAdam]

@pirj I've found that if a method is called multiple times in a test run, I cannot use the have_received style. To expand on the discussion in #268, it seems that Rspec doesn't check that all the allows were met, only that you don't pass arguments that weren't allowed.

For example:

class ClassToBeTested
  def self.call
    sleep 1
    sleep 2
    sleep 1 # This should be 3
  end
end

RSpec.describe ClassToBeTested do
  describe "using have_received without expecting arguments" do
    it "passes" do
      allow(described_class).to receive(:sleep).with(1)
      allow(described_class).to receive(:sleep).with(2)
      allow(described_class).to receive(:sleep).with(3)

      described_class.call

      expect(described_class).to have_received(:sleep).exactly(3).times
    end
  end

  describe "using have_received and also expecting arguments" do
    it "fails" do
      allow(described_class).to receive(:sleep).with(1)
      allow(described_class).to receive(:sleep).with(2)
      allow(described_class).to receive(:sleep).with(3)

      described_class.call

      expect(described_class).to have_received(:sleep).
        with(1).
        with(2).
        with(3)
    end
  end

  describe "using receive" do
    it "fails" do
      expect(described_class).to receive(:sleep).with(1)
      expect(described_class).to receive(:sleep).with(2)
      expect(described_class).to receive(:sleep).with(3)

      described_class.call
    end
  end
end

[pirj]

doesn't check that all the allows were met

That's a difference between expect and allow. Both allow to configure responses, but only expect sets constraints on received messages.

[HashNotAdam]

Personally, I think pragmatism should win over programming purity. Why create ugly tests just to appease a general testing ideal?

[pirj]

I don’t completely understand your point. Do you mind providing a less contrived example that should work from your perspective, but doesn’t?

[HashNotAdam]

So, I've just swapped the enforced style because I've continually seen a few problems over the years.

The first and most important is that most people don't seem to understand the fine details of how the expect works, and I regularly need to correct tests that are incorrectly passing. This relates to the example I gave above. Engineers encounter advice like was provided on #268, and they believe that RSpec is testing the arguments in the expect statement without realising it works in that case because the message is only sent once. In situations where the message is sent multiple times, they can think they have a valid test because it is passing when really it isn't testing what they think it is. This leads me to believe that, even though it's unusual to start with an expectation (and I found it jarring initially), using the have_received format is unsafe. I am preferencing higher quality tests over the "correct" way.

The second problem is that it makes for verbose tests with a more considerable cognitive overhead when you must say the same thing multiple times. If we consider the example provided in #268, if that were a situation where multiple messages were sent to the client, the test would need to read something like:

allow(Pusher::Client).to receive(:new).with(
  app_id:    'fake_1',
  key:       'fake_1',
  secret:    'fake_1',
  cluster:   'ap_1',
  encrypted: true
).and_return(pusher_client)
allow(Pusher::Client).to receive(:new).with(
  app_id:    'fake_2',
  key:       'fake_2',
  secret:    'fake_2',
  cluster:   'ap_2',
  encrypted: true
).and_return(pusher_client)

call_the_subject

expect(Pusher::Client).to have_received(:new).with(
  app_id:    'fake_1',
  key:       'fake_1',
  secret:    'fake_1',
  cluster:   'ap_1',
  encrypted: true
).with(
  app_id:    'fake_2',
  key:       'fake_2',
  secret:    'fake_2',
  cluster:   'ap_2',
  encrypted: true
)

vs

expect(Pusher::Client).to receive(:new).with(
  app_id:    'fake_1',
  key:       'fake_1',
  secret:    'fake_1',
  cluster:   'ap_1',
  encrypted: true
).and_return(pusher_client)
expect(Pusher::Client).to receive(:new).with(
  app_id:    'fake_2',
  key:       'fake_2',
  secret:    'fake_2',
  cluster:   'ap_2',
  encrypted: true
).and_return(pusher_client)

call_the_subject

[pirj]

RSpec doesn't check that all the allows were met, only that you don't pass arguments that weren't allowed

Given this, what is your suggestion for rubocop-rspec to make it easier to detect specs susceptible to false positives, @HashNotAdam ?

if a method is called multiple times in a test run, I cannot use the have_received style

Is it so? How about this:

allow(foo).to receive(:bar)
foo.bar(1)
foo.bar(2)
expect(foo).to have_received(:bar).with(1).ordered
expect(foo).to have_received(:bar).with(2).ordered

[HashNotAdam]

Given this, what is your suggestion for rubocop-rspec to make it easier to detect specs susceptible to false positives, @HashNotAdam ?

I'm not sure if you can. As I mentioned, I've stopped the enforced style to avoid problems, and I'm suggesting that Rubocop RSpec do the same because of the likelihood of poorly written tests

Is it so? How about this:

I'll give you that I can't see any issues with the correctness of that test but there are still two problems:

1. I've never seen ordered before so I know most people aren't using it
2. it doesn't address how easy it is to write an incorrectly passing test

[pirj]

I find it increasingly hard to understand your point, @HashNotAdam.

Do you suggest a certain enforced style or a style for writing specs? Which one? Why is your preferred style less prone to writing specs that are deceptively passing?

I don't insist on using ordered, exactly counts, it's just to demonstrate that have_received is not inferior compared to expect(...).to receive.

It strongly depends on what you test and the style if you set constraints on allow or have_received. E.g. you can allow a method to accept (allow) an integer in a range, and expect it to be called twice, but with no further constraints on the order or the range. That factors out the duplicated range constraint to a single allow statement.

You mentioned an incorrectly written test. May I kindly ask you to give one self-contained example of one that one of the enforced styles would tolerate while the other one won't? E.g.:

expect(foo).to receive(:bar).with(1)
expect(foo).to receive(:bar).with(2)
foo.bar(2)
foo.bar(1) # should be called with 1 first, and with 2 after, not the other way around - BOOM
# passes

As you point out, the vast majority are not using or are probably unaware of ordered, and would most likely expect expect to match calls in the defined order, while it's not the case.