Monday, January 28, 2008

Testing: One expectation per test

In a previous entry I discussed why I prefer One Assertion Per Test. In that entry I give a few state based examples, but didn't address the topic of mocks. This entry is going to focus on how you can use mocks while maintaining focus on maintainability.

Let's start with an example.

require 'test/unit'
require 'rubygems'
require 'mocha'

module Expectations
class SuiteRunner
attr_accessor :suite

def initialize
self.suite = Expectations::Suite.new
end

def self.suite_eval(&block)
self.new.suite.instance_eval &block
end
end
end

class ObjectTests < Test::Unit::TestCase
def test_suite_eval_evals_the_block_in_the_context_of_the_suite
suite = mock
suite.expects(:instance_eval)
runner = mock
runner.expects(:suite).returns(suite)
Expectations::SuiteRunner.expects(:new).returns runner
Expectations::SuiteRunner.suite_eval {}
end
end

The SuiteRunner class is fairly straightforward, it's simply delegating the block on to the instance_eval method on the suite attribute, which is an instance of the Suite class. Even though the class isn't doing anything very interesting the test isn't completely simple. Due to the chain of method calls you need to set up 2 mocks and a total of 3 expectations. While the test isn't unmaintainable, if the description (the method name) isn't kept up to date you could easily lose the intent of the test.

A step in the right direction toward making this test more readable is to introduce a few stubs. However, before we decide what to stub we need to decide what this test is trying to verify. Given the description it seems that eval'ing in the context of the suite is what is being tested, thus the suite mock should probably remain. Since the suite mock is the focus of the test, the rest of the mocks are probably better expressed as stubs. The test can be written more concisely now that we've chosen to use stubs.

class ObjectTests < Test::Unit::TestCase
def test_suite_eval_evals_the_block_in_the_context_of_the_suite
suite = mock
suite.expects(:instance_eval)
Expectations::SuiteRunner.stubs(:new).returns stub(:suite => suite)
Expectations::SuiteRunner.suite_eval {}
end
end

With one mock and one expectation the test expresses it's purpose. Code should not only express how it works, but also why it's been written in a particular way. The new test not only executes, but it conveys the intent of the test (even if the description becomes stale). Converting that test was easy, but tests that have behavior expectations and state based assertions can be a bit more complicated to clean up.

require 'test/unit'
require 'rubygems'
require 'mocha'

class ReservationService
# implementation
end
class MaidService
# implementation
end
class VipService
# implementation
end

class HotelRoom
attr_accessor :booked
def book_for(customer)
reservation = ReservationService.reserve_for(customer)
self.booked = true
MaidService.notify(reservation)
VipService.notify(reservation) if reservation.for_vip?
reservation.confirmation_number
end
end

class HotelRoomTests < Test::Unit::TestCase
def test_book_for_returns_confirmation_number
customer = mock
room = HotelRoom.new
reservation = mock
reservation.expects(:for_vip?).returns true
reservation.expects(:confirmation_number).returns 1979
ReservationService.expects(:reserve_for).with(customer).returns(reservation)
MaidService.expects(:notify).with(reservation)
VipService.expects(:notify).with(reservation)
assert_equal 1979, room.book_for(customer)
assert_equal true, room.booked
end
end
# >> Loaded suite -
# >> Started
# >> .
# >> Finished in 0.001121 seconds.
# >>
# >> 1 tests, 7 assertions, 0 failures, 0 errors

There's a lot going on in this test. Test::Unit even reports that 7 assertions have been met. Unfortunately, with that much going on in a test, it's hard to tell what the original intent of the test was. Even worse, it would be hard to change anything in the book_for method without breaking this test. The test is completely tied to the implementation and if that implementation changes you'll break one or more of the assertions within the test. What really bothers me about this test is the ability to break "one or more" assertions. If you change the reserve_for method to also take the hotel as an argument the test will immediately stop executing with the following error.

# >> Loaded suite -
# >> Started
# >> F
# >> Finished in 0.001728 seconds.
# >>
# >> 1) Failure:
# >> test_book_for_returns_confirmation_number(HotelRoomTests)
# >> [(eval):1:in `reserve_for'
# >> -:18:in `book_for'
# >> -:36:in `test_book_for_returns_confirmation_number']:
# >> #<Mock:0x50f2a8>.reserve_for(#<Mock:0x50fa50>, #<HotelRoom:0x50fadc>) - expected calls: 0, actual calls: 1
# >> Similar expectations:
# >> #<Mock:0x50f2a8>.reserve_for(#<Mock:0x50fa50>)
# >>
# >> 1 tests, 0 assertions, 1 failures, 0 errors

Knowing that an assertion failed is good, knowing that other assertions might also fail when you fix the first problem is not so good. The problem with this test is that it's verifying 7 different things. This can most likely be resolved by writing several different tests that all focus on one thing. (There are several tests that can be written, here are 5 examples)

require 'test/unit'
require 'rubygems'
require 'mocha'

class ReservationService
# implementation
end
class MaidService
# implementation
end
class VipService
# implementation
end

class HotelRoom
attr_accessor :booked
def book_for(customer)
reservation = ReservationService.reserve_for(customer, self)
self.booked = true
MaidService.notify(reservation)
VipService.notify(reservation) if reservation.for_vip?
reservation.confirmation_number
end
end

class HotelRoomTests < Test::Unit::TestCase
def test_book_for_reserves_via_ReservationService
room = HotelRoom.new
ReservationService.expects(:reserve_for).with(:customer, room).returns(stub_everything)
MaidService.stubs(:notify)
room.book_for(:customer)
end

def test_book_for_notifys_MaidService
reservation = stub_everything
MaidService.expects(:notify).with(reservation)
ReservationService.stubs(:reserve_for).returns(reservation)
HotelRoom.new.book_for(:customer)
end

def test_book_for_notifys_VipService_if_reservation_if_for_vip
reservation = stub_everything(:for_vip? => true)
VipService.expects(:notify).with(reservation)
MaidService.stubs(:notify)
ReservationService.stubs(:reserve_for).returns(reservation)
HotelRoom.new.book_for(:customer)
end

def test_book_for_sets_booked_to_true
room = HotelRoom.new
MaidService.stubs(:notify)
ReservationService.stubs(:reserve_for).returns(stub_everything)
room.book_for(:customer)
assert_equal true, room.booked
end

def test_book_for_returns_confirmation_number
reservation = stub_everything
reservation.stubs(:confirmation_number).returns 1979
ReservationService.stubs(:reserve_for).returns(reservation)
MaidService.stubs(:notify)
assert_equal 1979, HotelRoom.new.book_for(:customer)
end
end
# >> Loaded suite -
# >> Started
# >> .....
# >> Finished in 0.002766 seconds.
# >>
# >> 5 tests, 5 assertions, 0 failures, 0 errors

The above tests take more lines of code than the original, but they are far more maintainable. You can change the implementation of the book_for method in various ways and only break the tests that are relevant to the change. The tests have become more robust. They've also become more readable because they express what their focus is. The tests that have a mock with an expectation are written to test a behavior that is expected. The tests that have an assertion are written to statefully verify that the object has been set as expected. When the implementation does inevitably change the tests can communicate on their own what the original author intended.

Writing tests in this way is actually quite easy if you follow a few simple suggestions.
  • If your test has an assertion, do not add any mock expectations (instead use stubs for any methods that need to be changed for the test).
  • If you add a mock expectation you probably don't need it to return a meaningful value (since you won't be verifying that value).
  • If you already have a mock expectation you should use the stubs method for any other method where you don't want to use the implementation.
  • Do not use .with when using a stub unless you need to. If you are stubbing, you don't need to verify the arguments.
  • When returning stubs, prefer stub_everything so that additional calls to the stub will not cause unnecessary exceptions to be raised.
The above suggestions focus on creating tests that assert (or expect) one thing at a time and specify as little other implementation as possible. Every expectation, .with, and .returns statement is specifying implementation. The less implementation you express in a test, the more that test becomes.

I consider each call to stub (or stub_everything), stubs, mock, expects, with, and returns to be 1 implementation specification point. Any test that has an implementation specification score of 5 or more has the high implementation specification smell. As with all smells, everything might be okay, but I'm going to take the extra time to see if anything can be broken up by moving responsibilities or creating smaller methods.

By limiting your tests to one assertion or one expectation you create tests that express their intent. By creating tests that specify as little implementation as possible you reduce the amount of noise taking away from the intent of the test. An additional consequence of creating tests that focus on intent is that they specify less and are therefore more robust.

12 comments:

  1. shane4:49 AM

    Hi Jay ... hey my one issue with the one expectation per test approach is that I often end up performing the same setup/context repeatedly. Now in most cases this is ok and not much of an overhead, but in some circumstances it means having a slower test ... where it's faster to do the Arrange and Act (thinking of Bill Wake's 3 A's here ... Arrange, Act, Assert) once and then do the Asserts.

    ReplyDelete
  2. Shane,

    One assertion (or expectation) per test is a good rule for unit testing and a good guideline for functional testing. If your test requires a fair amount of setup that makes it run slowly, I assume you are writing functional tests, in which case you'll need to balance speed with purity.

    I would first take a good long look at the code and make sure there's no other way to do what you want to accomplish. There might be a way to extract the slow code to a method, mock that method and turn this into a unit test.

    If you can't figure out a way to tease things apart, then it's one of those situations where you'll need to go against the suggestion and do multiple asserts. There's no silver bullet. =)

    Cheers, Jay

    ReplyDelete
  3. I went through a very similar exercise yesterday with a coworker. I'm a big fan now of the style you describe as its definitely made the specs cleaner and clearer. I'd really suggest trying this out to anyone who's skeptical.

    @jay: At the moment I feel comfortable with the 'how' but sometimes get stuck at the 'what', especially with controllers. At times I've written a spec for almost every line of a controller and that just feels wrong; at the same time, I don't want to be too broad and not catch situations where people are changing instance variable names and screwing everything up. Do you have any guidelines or 'mantras' if you will about what types of calls you test and what you avoid? Have you had any new ideas or thoughts since http://blog.jayfields.com/2007/09/rails-testing-controllers.html

    ReplyDelete
  4. Jack, I can't stand trying to test controllers. It's incredibly painful. I'll post something new in the near future, but the situation is largely the same as the previous post you already linked to.

    I wish I had better news.

    Cheers, Jay

    ReplyDelete
  5. The other week on the rspec list David Chelimsky posted this http://pastie.caboo.se/142403 which I think is really cool for using mocks and doing the one assertion per test. Not sure if this helps with the controller issues you guys have been having?

    Cheers
    Shane

    ReplyDelete
  6. Thanks for this, it's a great example of how to clarify the tests.

    One objection I've seen from people is that if you were to test-drive it with one expectation per test (rather than starting with the big ol lump and then refactoring into multiple tests), you end up having to edit more than one test for each bit of production code you add.

    For example, I imagine the order would go like

    1. Write ReservationService.expects(:reserve_for) expectation in Test #1
    2. Implement call to ReservationService.reserve_for
    3. Write MaidService.expects(:notify) expectation in Test #2
    4. Implement call to MaidService.notify

    Here's where the problem arises...Test #2 is passing right now, but you have to go back to Test #1 and stub MaidService.notify.

    I don't really have a problem with that. I think it's perfectly simple. However I've encountered some resistance from people who claim that in making this test pass, I've made a previously passing test turn red. Is there merit to that argument, or do you think it might just be an excuse to avoid mock-based testing?

    ReplyDelete
  7. I've been trying this out in rspec today, it's also (sometimes) a nice space saver when you write your specs on a single line:

    it("should be nil"){@x.should be_nil}

    Now if we can just get rid of having to write the "should be nil" bit ourselves when it's redundant...

    ReplyDelete
  8. @Pat, I see breaking a previous test as a good thing. Ideally, changing the implementation would not break a previously written test. When adding new implementation does break a test it signals to me that either the test or the implementation can be written in a better way.

    @Ana, I believe you could write the test as: it {@x.should be_nil}

    Of course, I can't help but mention that I'd also be looking to Inline Setup

    ReplyDelete
  9. @Jay Yes, that works, I should have tried it first. Thanks. :-) It's all there in spec/spec/matchers/description_generation_spec.rb

    ReplyDelete
  10. Jay: So how would you change the test/implementation to avoid breaking tests when you add a new test? The code looks pretty good to me.

    ReplyDelete
  11. Pat,
    In this case, I don't think there is a way I'd improve the test or implementation. Unfortunately, sometimes you cannot avoid having to add things such as MaidService.stubs(:notify) in several tests. It's not ideal, but it sucks the least in my opinion.

    Of course, the domain could be modeled in another way which might help, but that's outside the scope of the example.

    ReplyDelete
  12. michael9:52 PM

    Very nice post, Jay. How would you suggest writing tests for a method that throws an exception? Given certain input conditions, this method is expected to throw an exception, and I want to test for that. But there are other expectations to test for as well (such as confirming that the method closes a connection it has opened), and in order to separate these out, it seems like one would have to catch and discard the exception in all the other tests.

    This seems like it would introduce a lot of clutter and introduce an irrelevant implementation detail to the other tests. (They don't care whether an exception is thrown; they should ideally be able to testing their piece in relative isolation.)

    ReplyDelete

Note: Only a member of this blog may post a comment.