New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Spy verifications shouldn't require stubbing. #636

Closed
searls opened this Issue Mar 23, 2014 · 16 comments

Comments

Projects
None yet
6 participants
@searls

searls commented Mar 23, 2014

In my opinion, it's an anti-pattern to stub a method you hope to verify. Without violating command-query separation, it's not possible for a test to require that a method both be stubbed (to exercise the desired behavior in the subject) as well as that its invocation be verified (to measure a signal/side-effect). If the stubbing was necessary to exercise the behavior, then asserting the invocation is redundant. If the only way to know whether the subject behaves correctly is by measuring a particular invocation, then its stubbing shouldn't matter (because nothing of interest could happen after that point without violating CQS).

This is a subtle point that's lost on lots of TDD practitioners when first presented with it. (I fondly remember holding court with @jimweirich after a talk I gave about test doubles for over an hour, articulating and rearticulating it for other attendees wanting to argue the point.)

And yet, RSpec's test spy implementation requires every single spy verification to first be stubbed. Why is this? I was surprised to see that this test didn't work until I added , :munch => nil to the double invocation:

  let(:bamboo) { double("bamboo") }
  subject { Panda.new(bamboo) }

  before { subject.eat! }

  it "munches bamboo" do
    expect(bamboo).to have_received(:munch)
  end

The same example in gimme does work, in contrast:

  let(:bamboo) { gimme("bamboo") }
  subject { Panda.new(bamboo) }

  before { subject.eat! }

  it "munches bamboo" do
    verify(bamboo).munch
  end
@myronmarston

This comment has been minimized.

Member

myronmarston commented Mar 23, 2014

RSpec's doubles raise errors by default unless you mock or stub a particular method. It's always worked this way and would have been a massive breaking change to stop doing that when we introduced spies.

In addition, RSpec's API and semantics are designed for consistency for working with either doubles or partial double, and on a partial double there's no performant way for RSpec to observe every method invocation.

If you are using a test double as a spy and do not want to stub beforehand, there's a simple solution: tack on 'as_null_object'. This had the effect of allowing any message and you can assert later what messages were received without having to specify them beforehand.

Sent from my iPhone

On Mar 23, 2014, at 1:54 PM, Justin Searls notifications@github.com wrote:

In my opinion, it's an anti-pattern to stub a method you hope to verify. Without violating command-query separation, it's not possible that for a test to require a method both be stubbed (to exercise the desired behavior in the subject) as well as that its invocation be verified (to measure a signal/side-effect). If the stubbing was necessary to exercise the behavior, then asserting the invocation is redundant. If the only way to know whether the subject behaves correctly is by measuring a particular invocation, then its stubbing shouldn't matter (because nothing of interest could happen after that point without violating CQS).

This is a subtle point that's lost on lots of TDD practitioners (I fondly remember holding court with @jimweirich after a talk I gave about test doubles for over an hour, articulating and rearticulating it)

And yet, RSpec's test spy implementation requires every single spy verification to first be stubbed. Why is this? I was surprised to see that this test didn't work until I added , :munch => nil to the double invocation:

let(:bamboo) { double("bamboo") }
subject { Panda.new(bamboo) }

before { subject.eat! }

it "munches bamboo" do
expect(bamboo).to have_received(:munch)
end
The same example in gimme:

let(:bamboo) { gimme("bamboo") }
subject { Panda.new(bamboo) }

before { subject.eat! }

it "munches bamboo" do
verify(bamboo).munch
end

Reply to this email directly or view it on GitHub.

@searls

This comment has been minimized.

searls commented Mar 23, 2014

Understood that there is API baggage, here. That's unfortunate. In practical terms, I'd probably just create a spy method that delegates to double...as_null_object.

In impractical terms, your response makes me feel that RSpec's test spies are a misnomer, because a spy is intended to act as an observation point that silently records invocations. That the assertion is made in proper given-when-then order is only really valuable when the "given" phase isn't redundantly cluttered with spy setup.

@paytonrules

This comment has been minimized.

paytonrules commented Mar 23, 2014

How would you handle a query that returns different values for different parameters? User.for(email) to pick an arbitrary example pulled out of my butt - I'm not violating command-query but I probably want to verify the email being passed to the method and likely need to stub the result.

@searls

This comment has been minimized.

searls commented Mar 23, 2014

@paytonrules - in that case I would just supply two stubbings. No verification necessary.

@JonRowe

This comment has been minimized.

Member

JonRowe commented Mar 23, 2014

A double isn't a spy, but we're giving you the ability to spy on specific methods on a double, but that doesn't make the double itself a spy.

We could expose functionality that behaves like a spy by aliasing spy to return a double with a null object, and potentially we could introduce spying on specific instances without the need to stub specific methods but that would be separate to our double concept.

If you'd like to work up a PR for either I'd be happy to review.

@myronmarston

This comment has been minimized.

Member

myronmarston commented Mar 23, 2014

@searls What do you think about a config option that makes doubles 'as_null_object' by default? I can imagine that for users who primarily use spies, that could make for a better experience.

@searls

This comment has been minimized.

searls commented Mar 23, 2014

@JonRowe -- thank you, I think either approach would be an improvement.

@myronmarston -- I think it would be an improvement, for sure.

A related question about rspec-mocks & null objects (I honestly don't know this b/c it's been so long since I've used rspec-mocks), but do your doubles include a feature where they can be backed by a real class (e.g. double(Bamboo) and simply ensure that Bamboo instances can actually respond to munch before allowing a stub or verify?

@myronmarston

This comment has been minimized.

Member

myronmarston commented Mar 23, 2014

Yep. See verifying doubles in the relish or API docs. 'instance_double' or 'class_double'.

@JonRowe Given that we support different kinds of verifying doubles, I'd rather not add 'spy', 'instance_spy' and 'class_spy'. The config for null-by-default will be simpler, I think.

@JonRowe

This comment has been minimized.

Member

JonRowe commented Mar 23, 2014

I've used rspec-mocks), but do your doubles include a feature where they can be backed by a real class (e.g. double(Bamboo) and simply ensure that Bamboo instances can actually respond to munch before allowing a stub or verify?

instance_double("Bamboo")

@JonRowe

This comment has been minimized.

Member

JonRowe commented Mar 23, 2014

@myronmarston I feel better about explicit code paths than implicit based on configuration :/

@myronmarston

This comment has been minimized.

Member

myronmarston commented Mar 24, 2014

@myronmarston I feel better about explicit code paths than implicit based on configuration :/

I get that. One concern I have about introducing new APIs like spy, instance_spy and class_spy is that it suggests the returned object is a different kind of a thing than normal doubles. I don't consider it to be different though; it's just the as_null_object "loose by default" behavior with a different name.

Awhile back there was community support for making double loose by default: #56. We ultimately decided against that since we decided to remove mock and stub and just go with double (plus the new verifying versions of that). A config option would be one way to provide that -- particularly since there are situations where you may want a null object double but you do not plan to use it as a spy.

If we go that direction, I'm thinking we'd provide an API like as_null_object for making a double non-null, which could be used when you've set the config to make doubles null-by-default but have a one-off situation where you do not want it.

@myronmarston myronmarston added this to the Post 3.0 milestone Mar 24, 2014

@JonRowe

This comment has been minimized.

Member

JonRowe commented Mar 24, 2014

This issue is more about the concept of spys vs doubles and I kind of agree, so even though the behaviour isn't much for us internally, it's more of a conceptual change for the user, hence why I'm suggesting the aliases...

@soulcutter

This comment has been minimized.

Member

soulcutter commented Mar 24, 2014

@JonRowe Given that we support different kinds of verifying doubles, I'd rather not add 'spy', 'instance_spy' and 'class_spy'. The config for null-by-default will be simpler, I think.

I feel like those three methods would be valuable in larger applications where it's quite probable that you employ a variety of double behavior.

@myronmarston

This comment has been minimized.

Member

myronmarston commented Mar 24, 2014

Works for me. I've tagged this as a Post-3.0 feature.

@samphippen

This comment has been minimized.

Member

samphippen commented May 5, 2014

Would it be best to close this issue and open a new one that says something like "Add spy, instance_spy and class_spy methods that creates doubles as null objects for spying" with a reference to this issue?

@myronmarston

This comment has been minimized.

Member

myronmarston commented May 5, 2014

Would it be best to close this issue and open a new one that says something like "Add spy, instance_spy and class_spy methods that creates doubles as null objects for spying" with a reference to this issue?

Sure, go for it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment