rubocop / rspec-style-guide

Best practices for writing your specs!
http://rspec.rubystyle.guide
953 stars 118 forks source link

Suggested addition: Use `receive_messages` where appropriate #96

Closed andyw8 closed 5 years ago

andyw8 commented 5 years ago

When there multiple methods stubbed on the same object, it's often helpful to use the more concise receive_messages feature, available since RSpec 3:

# bad
allow(dbl).to receive(:foo).and_return(2)
allow(dbl).to receive(:bar).and_return(3)

# good
allow(dbl).to receive_messages(:foo => 2, :bar => 3)

https://relishapp.com/rspec/rspec-mocks/v/3-8/docs/basics/allowing-messages

dgollahon commented 5 years ago

I know this is more concise so it might be reasonable for some projects/cases but I personally try to avoid it because I think it's pretty opaque. The first few times I saw it, it was not obvious to me what it was supposed to do and I mistook it for a combination of receive and with not receive and and_return.

I would probably feel differently if it were called receive_and_return or something but as-is i don't know if it's a clear win for most situations. It also composes poorly with with so if it became a cop we should be aware of that.

There's definitely a balance of choosing advanced features that might seem opaque to a new user but are actually helpful and being overly explicit and I might be overvaluing the explicit in this case. It may also be something where it's just a matter of judgment and it's sometimes clearer and sometimes not, but I'm curious how others feel about it.

pirj commented 5 years ago

It took me a while to understand that you only propose to recommend it to be used for allowed messages. Do you think we need to emphasize that it shouldn't be used in message expectations, e.g. expect(dbl).to receive_messages, that would hide multiple expectations in a non-obvious way?

What do you think about defining those messages on the double itself, e.g.:

dbl = double(:foo => 2, :bar => 3)
andyw8 commented 5 years ago

@pirj I haven't yet considered how it would work with expect, but for a plain double, I agree it's better to pass a hash to double() where possible. Maybe that should be a separate guideline, e.g.

# bad
widget = double
allow(widget).to receive(:foo).and_return(:bar)

# good
widget = double(foo: :bar)
dgollahon commented 5 years ago

I'm in agreement with the hash-in-double recommendation. I think this is usually the best option when multiple methods have to be stubbed (and it's very often good for just one).

The only caveat is if I want a with attached, then i have to do things differently but that's a shared drawback with the receive_messages option.

pirj commented 5 years ago

Closing this, since there's no clear way how to proceed. Hash-in-double is a worthwhile approach, we can get back to this if we see some really bad examples that are better expressed using it.