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.

Challenge Leaders

At one of last years Rails Edge conferences I was in a game of Werewolf with Chad Fowler and Dave Thomas. Given the current information I decided that Chad and Dave were in fact the werewolves. Of course, I was not even close to certain, but of the options, it was the one that made the most sense. I started explaining my thought process and suddenly there was a vote to see if I should be killed. This did not surprise me. In life you will generally be better off taking the side of Chad Fowler and Dave Thomas if you find yourself in a situation where they both disagree with me. But, this time, I was right, they killed me and the game ended all villagers dead.

Interestingly, this exact pattern often plays out in software development also. People are afraid to challenge leaders in general. Unfortunately, some of the best solutions are found because leaders are challenged. There's no question that I owe quite a bit of credit to the various team members I have had that wouldn't take my first solution as 'good enough'. I'm a better developer because of them, and the resulting code (some of which is available as Open Source) is higher quality.

Some leaders do not take criticism well. Please do not let that stop you from challenging them. If they are truly good leaders they will recognize that by challenging them, you are doing them a favor. If they do not recognize that, it's important for everyone else to be exposed to your ideas, so that a crowd can overturn an unfit leader.

So don't be afraid to challenge leaders, everyone will benefit from it.

Write Only Ruby

For some reason, DRY and "type less" seem to have become equivalent to many in the the Ruby community. I believe that the essence of DRY is when an application change is localized to one area of the codebase. While DRY is important, it's no more important than maintainability and readability. Therefore, if logic is stored in one and only one location, but that location cannot be found or understood, you've traded maintainability and as a result have gained nothing.

But, I've grown tired of this debate. No one ever wins, instead people just get tired of listening so they stop interacting. If you want to define methods of a module in a loop that later gets included in 3 classes that have nothing else in common, you have the right to do that. In fact, I'm excited about helping you.

Here's how DRY your code could be.

# what's the sense in writing the class name? 
# you already wrote it once when you named the file
C do
  # attr_accessor clearly has 12 too many characters
  a :first_name, :last_name, :favorite_color
  # include is short, but i is good enough
  i Enumerable
  # ctor defines a constructor
  ctor { |*args| s.first_name, s.last_name = *args }
  # d allows you to define any method by calling it from d. 
  # you can also chain the calls together to if you have several methods that are similar.
  d.complete_info? { first_name && last_name && true }
  d.white?.red?.blue?.black? { |color| self.favorite_color.to_s == color.to_s.chomp("?") }
end

That's some DRY code, perhaps even Extremely DRY (EDRY). Code isn't any good unless it runs.

Foo < Enumerable                   # => true
f = Foo.new("Mike", "Ward")        # => #<Foo:0x1e6f4 @first_name="Mike", @last_name="Ward">
f.first_name                       # => "Mike"
f.last_name                        # => "Ward"
f.complete_info?                   # => true
f.red?                             # => false
f.favorite_color = :red            # => :red
f.red?                             # => true

It runs without problem. Dying for the implementation? No worries, here it is.

class Object
  def C(base_class=Object, &block)
    name = File.basename(eval("__FILE__", block.binding),".rb")
    klass = eval "class #{name.capitalize}; end; #{name.capitalize}", binding, __FILE__, __LINE__
    klass.class_eval(&block)
  end
  
  def s
    self
  end
end

class Class
  def ctor(&block)
    define_method :initialize, &block
  end

  def i(mod)
    include mod
  end
  
  def d
    DefineHelper.new(self)
  end
  
  def a(*args)
    attr_accessor(*args)
  end
end

class DefineHelper
  def initialize(klass)
    @klass = klass 
  end
  
  def method_stack
    @method_stack ||= []
  end
  
  def method_missing(sym, *args, &block)
    method_stack << sym
    if block_given?
      method_stack.each do |meth|
        @klass.class_eval do
          define_method meth do
            instance_exec meth, &block
          end
        end
      end
    end
    self
  end
end

# http://eigenclass.org/hiki.rb?instance_exec

module Kernel
  
  def instance_exec(*args, &block)
    mname = "__instance_exec_#{Thread.current.object_id.abs}_#{object_id.abs}"
    Object.class_eval{ define_method(mname, &block) }
    begin
      ret = send(mname, *args)
    ensure
      Object.class_eval{ undef_method(mname) } rescue nil
    end
    ret
  end

end

Of course, any EDRY code will be write only. It would be painful to try to actually read code written in this way, but that's already true of the 150 modules that each contain only one method definition. The difference is generally it's not immediately obvious when you first look at a codebase that has been DRYed up for the sake of keystroke savings. If the implementer instead chose to utilize EDRY, I'd know what I was getting into from the first file opened.