Skip to main content

Mocha Test Spies

My thoughts on Joe Ferris' fork of Mocha.

Introduction

Today I finally got round to looking at Joe Ferris’ Test Spies fork of Mocha. Joe explained how Test Spies work and why he thinks they are useful in Spy vs spy on the ThoughtBot blog. I apologise to Joe for not getting round to this sooner.

I have a number of hesitations about merging these changes into Mocha as they stand. My thinking hasn’t really crystallized, but in these situations I often find it’s useful to force myself to try and explain myself in writing. Hopefully it’ll also generate some feedback.

I’d like to thank Joe for taking the time to fork Mocha and add this functionality. I’m particularly grateful that he’s taken the trouble to write a bunch of high-level acceptance tests.

Concept

In this section I’ve tried to write up some comments on Joe’s blog post. I’ve tried to break it down into roughly the same headings that Joe used :-

Why would I let spies in my code?

Joe correctly points out that setting up expectations before exercising the object under test is a bit of a departure from the traditional Four-Phase Test regime. I can see how some people might see this as counter-intuitive, but it doesn’t seem particularly so to me. Also the idea of Expected Behaviour Specification (i.e. setting up expectations before the Exercise phase) has a couple of advantages :-

  • Using Test Spies, you have to (a) set up a stub so the mock object knows what value to return and (b) assert that the stubbed method was invoked the requisite number of times with the appropriate parameters. Whereas using Expected Behaviour Specification, these are combined into setting up an expectation on the mock object. I think this results in terser code with less duplication.
  • Using Expected Behaviour Specification means that the test can fail fast if an unexpected invocation is made against a mock object. Failing fast is generally a good idea, because you are more likely to be able to identify the root cause of the problem.

Sharing stubs in a context

describe PostsController, "on GET show" do
  before(:each) do
    @post = stub('a post', :to_param => '1')
    Post.stubs(:find => @post)

    get :show, :id => @post.to_param
  end

  it { should render_template(:show) }

  it "should find and assign the given post" do
    Post.should have_received(:find).with(@post.to_param)
    should assign_to(:post).with(@post)
  end
end

I’m going to struggle a bit when commenting on this code example for a couple of reasons :-

  • I dislike the use of setup methods (before in this RSpec example). I prefer to do my setup in-line in the test to make the test as independent and as explicit as possible. I particularly dislike exercising the object under test within the setup method. To me this seems like a more fundamental breakage of the Four-Phase Test concept.
  • In this example, I would not want to assert that Post.find has been called. Nat Pryce (one of the authors of jMock) has a rule of thumb that says: “stub queries and expect commands”. In this example, the call to Post.find is clearly a query, so I would just stub it. I think about it like this :-
  1. stubbing Post.find is setting up the environment and making the instance of Post available
  2. the assertion then checks that the instance of Post is assigned to the @post instance variable
  3. the combination of (1) and (2) is enough to tell me that the Post.find method has been called – where else would the controller have got the instance of Post from?

So I’m not convinced I would need Test Spies for this example.

Sharing stubs between tests

module PostHelpers
  def stub_post(post_attrs = {})
    post_attrs = {
      :to_param => '1',
      :published? => true,
      :title => 'a title'
    }.update(post_attrs)
    stub('a post', post_attrs)
  end
  def stub_post!(post_attrs = {})
    returning stub_post do |post|
      Post.stubs(:find => post)
    end
  end
end
describe PostsController do
  include PostHelpers
  it "should show a published post on GET show" do
    post = stub_post!
    get :show, :id => post.to_param
    Post.should have_received(:find).with(post.to_param)
    post.should have_received(:published?)
    should render_template(:show)
    should assign_to(:post).with(post)
  end
end
describe "/posts/show" do
  include PostHelpers
  it "should display a post" do
    assigns[:post] = stub_post
    render '/posts/show'
    template.should have_tag('h1', assigns[:post].title)
  end
end
  • In this example, as above, I would stub the call to Post.find rather than expect it.
  • I’d also prefer to use a real Post instance rather than a stub object. I’d use some kind of Test Data Builder like factory_girl to remove any duplication rather than methods like those in the PostHelpers module. While these ActiveRecord objects aren’t strictly Value Objects, I can’t help but think that this quote from Nat Pryce is relevant :-

Some languages might allow you to stub a value, but what’s the point? It is easier and less error prone to use the real type. If it’s not easier to use the real type then, I think, you should investigate why. What can you do to make the value type easier to use?

Steve Freeman makes a similar point in Test Smell: Everything is mocked.

Reusable matchers and assertions

# post.class.should have_received(:find).with(post.to_param)
should find(post)
# Post.should have_received(:new).with(post_attrs)
should build(Post, post_attrs)
# Post.should have_received(:new).with(post_attrs)
# post.should have_received(:save)
should build_and_save(Post, post_attrs)

I’m not convinced that extracting the assertion part of an expectation is sensible, because it’ll always need to be used in conjunction with the stub part of the same expectation.

Syntax

I think the suggested syntax extension is inconsistent with the existing Mocha API and a little clunky. See James Rosen’s comment on the mailing list. Looking at the following examples given in the documentation :-

assert_received(mock, :to_s)
assert_received(Radio, :new) {|expect| expect.with(1041) }
assert_received(radio, :volume) {|expect| expect.with(11).twice }

I think I’d prefer to see something like :-

assert_that mock.received(:to_s)
assert_that Radio.received(:new).with(1041)
assert_that radio.received(:volume).with(11).twice

Or something like :-

mock.assert_received(:to_s)
Radio.assert_received(:new).with(1041)
radio.assert_received(:volume).with(11).twice

Note that I haven’t completely thought through the implementation implications of these suggestions i.e. whether they are even possible!

Implementation

The code looks good and, since Joe has written acceptance tests, there shouldn’t be too much difficulty in doing a bit of refactoring. I like the idea of introducing a model for an Invocation , but I think the Invocation instances should be stored by the relevant Mock instance rather than the Mockery. Similarly I’d push more of the logic in the HaveReceived class into relevant objects. And I’d like to avoid having to make Expectation#invocation_count writeable.

One slight concern is what happens if someone mixes up Expected Behaviour Specification expectations with Test Spies stubs and assertions? I’d probably at least want some acceptance tests to convince me that there are no complications on this front.

Conclusion

I’m sure that some people find the changes useful, but I’m not yet convinced I want to merge the Test Spies changes into Mocha. Please feel free to try and convince me, either here in the comments or in the mailing list thread.