Re: [rspec] Why do mocked methods allow return values

[David Chelimsky]

On Mon, Feb 11, 2013 at 2:55 PM, clay s <thebrok...@gmail.com> wrote:

Why does RSpec allow you set a return value on a mocked method? If the method returns a value, then the dependent code will already have testable behavioral differences based on the return value, making the mocking superfluous.

We use mocks to specify that an interaction is supposed to happen: I expect x to receive the message foo. When you express this in rspec, or pretty much any other mocking tool, when x receives foo it will return nil by default. If another part of the code requires a return value from x.foo in order to continue working, you have to tell x to return a reasonable value when it receives foo. Hence:

x.should_receive(:foo).and_return(:a_reasonable_value)

If you don't care about specifying that x receives foo, then sure, you can just use a stub. But it really depends on what you're trying to express.

[clay s]

On Monday, February 11, 2013 1:00:25 PM UTC-8, dchel...@gmail.com wrote:

x.should_receive(:foo).and_return(:a_reasonable_value)

If the code will fail without specifying that return value, then you didn't need to mock in the first place.

[Myron Marston]

This is often true, but not universally.  Overall, I consider the use of `should_receive` with a return value to be a code smell since I generally care about command/query separation. That said, there are still cases where it makes sense to use `should_receive` with a return value.  For example, consider the case where you are adding some caching optimizations to an object that delegates expensive work to a collaborator.  To test drive this, you may want to specify that a particular message is sent to the collaborator only once, and `should_receive(:message).once` is a fine way to do that.  If the object cares about the return value and does not expect it to ever be nil, you'll need to specify a return value as well.

If using `should_receive(:foo).and_return(value)` bothers you that much, then feel free not to use it.  I think you're instincts that it's often not needed (and that confusing stubs with mocks is bad) are correct.  But there are still cases where it's useful, and RSpec provides it.

Myron

[clay s]

On Monday, February 11, 2013 4:05:12 PM UTC-8, Myron Marston wrote:

I consider the use of `should_receive` with a return value to be a code smell since I generally care about command/query separation.

Combining should_receive with a return value is not in any way related to command/query separation. You could replace the mock with a stub or (if the return value isn't being used by the code under test) remove the return value, and the test would be equivalent, and would still violate (or not violate) command/query separation just as much as before.

In fact, in the most recent relevant interaction I had with a fellow developer about this, he had mocked SomeModule.display_name (a method with no side-effects), and provided a return value. He never tested that the return value was used correctly, because he felt it would be too cumbersome to check that a CSV file was created with the right data. Hence the mock was some kind of (basically meaningless) reassurance to him.

> If using `should_receive(:foo).and_return(value)` bothers you that much, then feel free not to use it

It's not about some subjective distaste for it. It's a dishonest statement that the method has some side-effect that must be triggered. Indeed, if my friend had found himself unable to return a value from a mock, it would have encouraged him to make a realization about the nature of his test.

you may want to specify that a particular message is sent to the collaborator only once, and `should_receive(:message).once` is a fine way to do that.

This is semantically different than a should_receive; specifically, it's a "should not receive more than once". You could argue that the semantic clarity isn't valuable enough to warrant something like double.should_cache(:foo), but I'd certainly welcome it as part of a change to make it impossible to return values from mocks.

 

I think you're instincts that it's often not needed (and that confusing stubs with mocks is bad) are correct.  But there are still cases where it's useful, and RSpec provides it.

I speculate that the only case in which it's useful is when a value should be cached, and therefore need not be called more than once. In which case something like the should_cache method would solve the problem.

Thanks.

[Myron Marston]

On Feb 11, 9:20 pm, clay s <thebrokenlad...@gmail.com> wrote:
> On Monday, February 11, 2013 4:05:12 PM UTC-8, Myron Marston wrote:
> > I consider the use of `should_receive` with a return value to be a code
> > smell since I generally care about command/query separation.
>
> Combining should_receive with a return value is not in any way related to
> command/query separation. You could replace the mock with a stub or (if the
> return value isn't being used by the code under test) remove the return
> value, and the test would be equivalent, and would still violate (or not
> violate) command/query separation just as much as before.

Sure, you can violate command/query separation without using
`should_receive` with a return value. My point was that:

* As a rule of thumb, I only use `should_receive` for messages that
produce side effects (e.g. commands). It's generally better to use a
stub for a side-effect free message (e.g. a query).

* If I also have to use `and_return` because the code-under-test cares
about the return value, it's design feedback suggesting I'm combining
a command and a query into one method call, when usually it's better
to separate them.

Myron

[clay s]

Yan,

The "better whine" is actually a very compelling argument, in principle. But I don't see how it can actually work. E.g.

  it 'works' do

    f = double

    f.should_receive(:bar) { 42 }

    result = 1

    # Some dumb engineer commented out the line where we compute the value!

    # result += f.bar

    expect(result).to eq 43

  end

This fails because someone commented out the part where we add f.bar to result. But the failure doesn't tell us that the code failed to call f.bar. It just says:

expected: 43

     got: 1

(compared using ==)

So the API I'd like to see is:

- Mocks cannot return values.

- If you want to assert that something is cached (i.e. it shouldn't be called more than once) then then do something like double.stub(:foo).with(42).cache

Not particularly optimistic that this change will be implemented in my lifetime. :)

[Myron Marston]

On Feb 12, 2:17 pm, clay s <thebrokenlad...@gmail.com> wrote:
> Yan,
>

> The "better whine" is actually a very compelling argument, *in principle*.

> But I don't see how it can actually work. E.g.
>

> *  it 'works' do*
> *    f = double*
> *    f.should_receive(:bar) { 42 }*
> *    result = 1*
> *    # Some dumb engineer commented out the line where we compute the value!
> *
> *    # result += f.bar*
> *    expect(result).to eq 43*
> *  end*

>
> This fails because someone commented out the part where we add f.bar to
> result. But the failure doesn't tell us that the code failed to call f.bar.
> It just says:
>

> *expected: 43*
> *     got: 1*
> *
> *
> *(compared using ==)*

You've succeeded in showing us a contrived example where a stub would
make more sense, but that doesn't mean that
`should_receive.and_return` can never work. In your example, it
doesn't ever fail with a mock expectation error because you have a non-
mock expectation that fails first. But consider a spec where the
assertion you want to make is that a particular message is received by
a particular collaborator object (Yan's example above is an example of
this). If the assertion you really want to make is that a particular
message is received (and not about the value returned of the method
called by your spec), then using a mock expectation is the right
tool. If the code-under-test depends upon the mocked method returning
a non-nil value then you will have to use `and_return` to get things
to pass. I think there's a useful bit of design feedback here (e.g.
the use of `should_receive.and_return` suggests you've combined a
command and query into one method, and may want to split it), but the
fact remains that, contrary to your assertion, using
`should_receive.and_return` not only works, but is useful in this case
(just as it was in the caching example I gave before).

Here's another example...consider a case like:

collaborator = double
double.stub(:fuzzy?).and_return(true)
subject = Algorithm.new(collaborator)
expect(subject.result).to eq(47)

In your example, the relationship between the stubbed response (42)
and the final result (43) was obvious. Here it's very non-obvious:
the collaborator has a boolean predicate that plays into the final
result somehow, but it's not clear from reading the test how it plays
in. One can imagine a future refactoring to `Algorithm` such that it
no longer uses `collaborator.fuzzy?`, but, for whatever reason,
continues to pass. The use of the stub rather than `should_receive`
here causes the test to create confusion, as it suggests that `fuzzy?`
is used when, in fact, it may not be. As a pragmatic decision, I
think it would be OK to use `should_receive` here instead, as that
would cause the example to fail as soon as `fuzzy?` stopped being
called.

> So the API I'd like to see is:
>
> - Mocks cannot return values.
> - If you want to assert that something is cached (i.e. it shouldn't be
> called more than once) then then do something like

> double.stub(:foo).with(42)*.cache*

I'm not convinced your idea of the `cache` API adds any benefit or
clarity over what we already have.

> Not particularly optimistic that this change will be implemented in my
> lifetime. :)

Generally speaking, I'm not in the habit of expanding effort to add
code and complexity to make RSpec's APIs less flexible. It would be
more code, not less, to disable `and_return` from being available for
`should_receive`. I've got more important things to work on.

I imagine you could easily create an external gem that provides this
functionality. That would be pretty trivial, and honestly, it
probably would have taken less time then this whole discussion.