Add Syntax for Spies #220

Closed
jfelchner opened this Issue Feb 4, 2013 · 10 comments

Comments

Projects
None yet
5 participants

Now that we have a handle on the new syntax for mocks and stubs (#153 - the length of the issue is not for the faint of heart), I think that the oft-requested feature of adding spies to RSpec may more easily "fall out" of that implementation.

The consistency of the expect syntax and the new allow syntax reads great and I'd like to discuss taking that syntax further into the notion of spies.

For the uninitiated and to make sure there's no ambiguity regarding terms here are some simplified definitions:

  • A stub is just a dumb object that knows how to receive some set of method calls and returns predetermined values for them.
  • A spy is just like a stub except that it remembers all the messages sent to it.
  • A mock is just like a spy except that you can tell it what messages you're expecting it to receive and it will blow up if it doesn't receive those.

PS: I'd prefer this issue didn't turn into a pedantic holy war debate about what the definition of a spy "really" is.

First, I'm going to summarize the use case for spies in RSpec and how it will improve readability. I'm using the new syntax discussed in #153. And I'm offering two options for a spy syntax; variations of which were also discussed in #153.

Current

Our current syntax (without spies) would produce something like this:

# http://www.imdb.com/title/tt0102926/
context 'when Buffalo Bill gives his captive the hose' do
  it 'puts the lotion in the basket' do
    expect(basket).to receive(:<<).with(lotion).once

    BuffaloBill.spray_water_on_captive
  end
end

Spy Syntax Option 1

context 'when Buffalo Bill gives his captive the hose' do
  it 'puts the lotion in the basket' do
    expect { BuffaloBill.spray_water_on_captive }.to send(:<<).to(basket).with(lotion).once
  end
end

It appears that the only stumbling block is around whether to override send in the context of the matcher. Readability-wise, I think this is great, although it is quite a bit to chew off one one line.

Spy Syntax Option 2

Another option utilizes a new have_received matcher which also has its advantages.

context 'when Buffalo Bill gives his captive the hose' do
  it 'puts the lotion in the basket' do
    allow(basket).to receive(:<<)

    BuffaloBill.spray_water_on_captive

    expect(basket).to have_received(:<<).with(lotion).once
  end
end

Implementation

I'm not sure about everyone else, but I like to approach a new feature from the user's perspective first and then deal with implementation.

In Option 2, it would appear that this would require that all stubs actually be spies under the hood. In other words, all stubbed objects are recording their messages. This is easy if the object is stubbed as it is in Option 2 but we would need to have some mechanism for turning an object into a spy without additionally stubbing any methods. Maybe:

allow(basket).to remember_messages

My Opinion

Honestly I kind of like Option 2 slightly more because building up the stub may need to be done in a separate context which would make Option 1 basically unusable.

Thoughts everyone?

Owner

myronmarston commented Feb 5, 2013

Thanks for bringing this up. I'd like to get spies in rspec-mocks at some point, but it might be awhile before I get around to it. But I'd be happy to merge a PR if someone else wants to take a stab at it, of course :).

Anyhow, some random thoughts:

  • I confess I've never spent any significant time with a test double library that provided spies. My opinions below are not informed by learning from what others have done. At some point we should see how other libraries do this and compare.
  • I believe option 1 can be implemented in terms of option 2 but not vice versa. This suggests that option 2 is a better starting place, with option 1 potentially as extra syntactic sugar on top (or potentially as a custom matcher users could define in their project).
  • I think using send as a matcher (option 1) would cause problems. send is part of the core metaprogramming API in ruby, and we don't want to prevent it from working normally. We could use send_message, though.
  • I think option 2 works better when you have multiple messages you want to spy on, because you can setup multiple allowances, and then assert on the sent messages at the end. With option 1, you'd have to have multiple nested expect blocks, which would quickly get unwieldy.
  • Option 2 provides better support for allowing users to configure how the object should respond to the message (since that's the basic thing allow will do to begin with). It's hard to imagine how to get that flexibility with option 1.

This is easy if the object is stubbed as it is in Option 2 but we would need to have some mechanism for turning an object into a spy without additionally stubbing any methods. Maybe:

We already have and_call_original...so you could do this:

allow(object).to receive(:foo).and_call_original

Note that that only works on a partial mock object; on a pure test double there is not original implementation to delegate to, so it responds with nil by default and you can use the fluent interface or a block implementation to do more with it. allow(basket).to remember_messages reads nicely but I'm concerned that it would be a very leaky abstraction due to how it would have to be implemented -- specifically, to intercept all messages, it would have to undefine all methods, so that it could use method_missing to handle them all. That's going to cause crazy side effects.

MDaubs commented Mar 13, 2013

The spy synax in option 2 is already (partially) implemented by the rspec-spies gem. I'm a fan of including this syntax (option 2) in RSpec proper so that we can write four phase tests out-of-the-box and be able to choose an assertion style that best fits a given situation.

Contributor

jferris commented Mar 14, 2013

I'm the author of the Bourne gem (https://github.com/thoughtbot/bourne), which extends mocha to add test spies.

Bourne requires that you stub out any methods you plan on spying on later. It fails with a different message if you attempt to spy on an unstubbed method.

The syntax Bourne uses for RSpec integration is:

invitation = stub('invitation', deliver: true)
# exercise
invitation.should have_received(:deliver)

This is basically Option 1 from the original message. I can take a stab at implementing have_received in rspec-mocks if that syntax makes sense.

Owner

myronmarston commented Mar 14, 2013

That's useful to see the approach bourne uses, although isn't that option 2 from the original message above? Anyhow, it's nice to see some confirmation that the approach we were planning on is in line with what other spy libraries use.

I can take a stab at implementing have_received in rspec-mocks if that syntax makes sense.

Please do! One slightly challenging aspect is the interplay between rspec-mocks and rspec-expectations: we want both to be usable on their own (outside of rspec-core even), while still combining nicely when used together. have_received really only makes sense as a matcher for use with rspec-expectations. There should be an underlying API that exposes the state of whether or not the message was received, so that if rspec-expectations is not being used, the spy can be verified with a simple assert.

Actually, looking at the bourne readme, it looks like you've already handled this there...

Contributor

jferris commented Mar 14, 2013

Yes, you're right - it's option two.

Bourne builds the matcher and calls `matches? on it when using Test::Unit: https://github.com/thoughtbot/bourne/blob/master/lib/bourne/api.rb#L13

I can easily build a boolean API that you could use with just assert, but true/false is pretty terrible output for expectation matching. It seems like what I should be doing is:

  • An API to get the result of a spy (did it match, if not, why) in rspec-mocks
  • A matcher that uses that API in rspec-expectations

Sound good?

Owner

myronmarston commented Mar 14, 2013

Bourne builds the matcher and calls `matches? on it when using Test::Unit:
https://github.com/thoughtbot/bourne/blob/master/lib/bourne/api.rb#L13

I like that approach a lot; much better than my original assert suggestion. And you're right about the failure output.

How about this?

  • Add the underlying spy tracking to rspec-mocks
  • Add the definition of the matcher class to rspec-mocks as well
  • Add the have_received method to rspec-expectations, which will create an instance of the matcher class.

Given that matchers are based on a protocol and do not have to subclass a common matcher base class, there's no reason the matcher class has to be defined in rspec-expectations. Given that rspec-mocks and expectations are separate repos, it'll make it easier to complete the changes in rspec-mocks, and test them, if the matcher remains in rspec-mocks. We can easily define have_received directly in the example that's testing this spy stuff.

How does that sound?

Contributor

jferris commented Mar 14, 2013

Sounds good. I'll try to take a look this week.

@jferris jferris referenced this issue Mar 15, 2013

Merged

Add test spies #241

Contributor

jferris commented Mar 15, 2013

Submitted as #241.

harlow commented Mar 15, 2013

I like this a lot. 👍

Owner

myronmarston commented Mar 18, 2013

Closing so we can move discussion to #241.

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