`receive` expectation: (optionally) enforce to specify whether a method should be stubbed

[vemv]

The problem

Given the following example:

expect(my_object).to receive(:foo)

As of today, this implicitly tells rspec-mocks to stub the foo method. But that's not what the Ruby code says!

The Ruby code says "this object should receive this method". Nothing else. Nothing stub-related.

So, 90% of the times what I end up writing is:

expect(my_object).to receive(:foo).and_call_original

That's kinda OK, but it requires me to carefully remember adding the and_call_original method. If you forget - boom! Your method suddenly returns nil. Once a year or so I will lose a couple hours debugging why a method suddenly starts returning nil.

Proposed solution

Make expect(my_object).to receive(:foo) optionally illegal. If I opt in (via spec_helper.rb), then I must code one of the following:

• expect(my_object).to receive(:foo).and_return(some_value)
• expect(my_object).to receive(:foo).and_call_original

What do you think?

Cheers - Victor

[JonRowe]

Personally I'm leaning against this, RSpec Mocks is a mocking and stubbing library, thus the inferred default for a partial double like this is to stub. It's well documented that this leads to a nil response by default. It's also considered kind of a test smell to use and_call_original as it's generally better to isolate collaborators in unit tests not just assert they're called. It's worth noting you're the first person to ask for this.

[vemv]

Personally I'm leaning against this

Worth noting that there a different styles of testing. One size rarely fits all. An opt-in functionality surely doesn't hurt?

It's worth noting you're the first person to ask for this

Well, for many years I've (occasionally) suffered this issue without realising that a better API could have prevented all the lost time / frustration. Similarly, it's possible that many people haven't realised this possible improvement, therefore they haven't asked for this?

[myronmarston]

It's true that @vemv is the first to request this feature, but I've heard from multiple users over the years that were surprised by the fact that expect(my_object).to receive(:foo) prevents the original my_object.foo logic from executing when the message is received, so a change to make it less confusing is not out of the question.

I'd note a few things, though:

• and_<anything> off of expect(...).to receive(...) is a code smell in my book -- it means you're method isn't clearly a command or a query but is instead a hybrid. Given it's a smell to use and_<anything>, providing a feature that requires users to do that isn't necessarily a good idea.
• Bearing in mind that rspec-mocks is primarily centered around test doubles (and not partial doubles/mocks), it's worth mentioning that this feature request has some oddities with how it behaves with pure test doubles. For example, and_call_original is nonsensical and unsupported on a pure test double. While it might make sense to have this feature require the user to be explicit about what the object should do when it receives the message for a partial test double....for a pure one, I don't think it makes much sense. But that in turn leads to potential confusion: if you see expect(thing).to receive(:foo), do you need to specify and_<blah> or not? You can't tell at a glance. You have to know what kind of object it is, but if it is just received as an argument to a helper method you might not know.

[vemv]

I think I understand your point: requiring users to expect specific values is not the average intended use of rspec-mocks.

Perhaps my original proposition can be tweaked so it makes sense for everyone?

Let's say now that under the opt-in setting, any of these two would be acceptable/recommended:

• expect(my_object).to receive(:foo).and_return(anything)
• expect(my_object).to receive(:foo).and_call_original

The difference is in the anything. It can be read (in English) as "I don't care what this method returns". As opposed to "I expect this method to return this specific value" (such as 42).

Thoughts?

[gingray]

+1 to force user explicitly return a value from a stubbed method

[YSavir]

This has been a point of frustration for me as well.

I don't like the idea of explicit return values, but what about a config option to run the original by default (when one is available)?

[JonRowe]

Running the original defeats the point of using a stub in the first place, also note that the primary use case, doubles, doesn't even have an original implementation.

[YSavir]

I don't think you can say "Running the original defeats the point of using a stub in the first place" without acknowledging that that's only one approach. As mentioned earlier in the thread, different people test differently. This might be due to personal philosophy, tech needs, optimizations, or other conditions, but whatever the cause, it seems inappropriate to wash away the request with a "that's not how I/we do it" blanket statement.

Regardless of what RSpec is and is not intended to be, the fact remains that people look to use it in this way, and with the inclusion of .and_call_original, RSpec officially allows itself to used in this way. That is inarguable. And if the functionality already exists, is supported, is documented, is actively used, and is actively being asked for extension by the community, I don't see any reason why further opt-in, non-default functionality is seen in a negative light.

[JonRowe]

I don't think you can say "Running the original defeats the point of using a stub in the first place" without acknowledging that that's only one approach.

I'm attempting to explain that its not what a stub is for, sure, it's entirely acceptable to test in a different fashion, checking that messages are sent regardless of their implementation, but this is a mocking and stubbing library, and thus it's point is to stub, (or fake out) a method definitions, or to replace with a mock (or double).

I don't see any reason why further opt-in, non-default functionality is seen in a negative light.

Feature bloat is seen in a negative light, and it's expanding functionality that exists but is not recommended, in the same way we don't expand any_instance functionality as it too is not recommend.

I would happily accept an API that allows a default response to be configured, but it needs to be something generic, not tied to this functionality, as for example, returning self is just as valid a default implementation. Something like:

RSpec::Mocks.configure do |config|
  config.partial_double_default_response { |stub| stub.and_return { self } }
end

Would be an acceptable expansion, and if you'd like to work on it feel free.

[vemv]

this is a mocking and stubbing library

I'd just like to point out that as an user, this fact is fairly irrelevant. I add rspec to my Gemfile, not rspec-mocks, which existence one could only guess by peeking at the Gemfile.lock.

My point is that I use rspec as a testing framework, and if some of its sub-gems declares itself as a "mocking and stubbing library" that shouldn't prevent me to use the rspec testing framework however I consider most convenient.

Isn't it easy to imagine that many developers would think similarly? How many are aware of the specific roles/goals of each rspec subgem?

I think one could look at the bigger picture: rspec, as a framework, currently is not capable to prevent certain type of programmer mistakes for one feature it officially offers.

[JonRowe]

If you'd like to work on it in a fashion similar to what I described above, I'd be happy to help.

[YSavir]

@JonRowe I would be happy to submit a PR in that style.

[myronmarston]

We could support a config option like @JonRowe suggested, although that definitely adds to the future maintenance burden for maintainers of this library. If possible, I'd prefer to see this feature added via an extension gem. If you want partial doubles to default to calling the original implementation, add the gem to your project; no config option is needed.

I think you could implement this in your extension gem doing something like:

module ReceiveOverride
  def receive(message, &block)
    matcher = super(message, &block)
    return matcher if block
    matcher.and_call_original
  end
end

RSpec.configure do |c|
  c.include ReceiveOverride
end

That won't quite work right (you'd have to check what kind of double is being dealt with to ensure and_call_original isn't applied to normal doubles) but it should get you started.

[JonRowe]

Closing due to inactivity during the monorepo migration, but if someone wanted to revisit this they'd be welcome.