Towards More Readable Tests

Making code readable is probably my number two priority after making it function correctly. I want every developer on the team to be able to look at code written by anyone else on the team and understand its intent. Of course that goes for the tests too.

We've had a few new developers join the team recently, and I've heard more than one of them say "Huh, I've never seen that before" about a pattern that shows up in some of our unit tests. It's something I've done and liked, but on further reflection, I'm starting to lean away from it.

Here's a test that could be clearer. (Let's take the interaction being tested as a given and focus on how the tests read. (Let's also not assume I have any interest in fishing. I don't know how I landed on this example.) The mocking library is Mocha.)

test "catches, kills, and bags tasty fish" do
  river = mock
  fish = mock
  river.expects(:give_it_up!).returns(fish)
  fish.expects(:tasty?).returns(true)
  fish.expects(:die!)
  bag = mock
  bag.expects(:put).with(fish)
  
  fisher = Fisher.new(river, bag)
  fisher.fish
end

Here's what makes it hard to read:

  river = mock                              # setup
  fish = mock                               # setup
  river.expects(:give_it_up!).returns(fish) # expectations
  fish.expects(:tasty?).returns(true)       # expectations
  fish.expects(:die!)                       # expectations
  bag = mock                                # setup
  bag.expects(:put).with(fish)              # expectations

As you read that, you have to change gears back and forth as you read lines that create testing fixtures interspersed with lines that specify the interaction we're working on. No good.

After reading a few tests like that (and tests with the same problem magnified), lines like these

  river = mock
  fish = mock
  # ... snip
  bag = mock

start to feel like pure noise. I suppose that was my mindset when I started changing those sorts of tests to look like the following.

test "catches, kills, and bags tasty fish" do
  (river = mock).expects(:give_it_up!).returns(fish = mock)
  fish.expects(:tasty?).returns(true)
  fish.expects(:die!)
  (bag = mock).expects(:put).with(fish)
  
  fisher = Fisher.new(river, bag)
  fisher.fish
end

Here, the creation of mocks is all inlined so that you can read straight through the interaction. The assignment of local variables embedded at the first occurence of each item in the interaction (thanks to Ruby's lack of variable declaration) isn't too distracting once you're accustomed to seeing it. Each test takes up less screen real estate than before, and the world's all around better than it was.

Or at least that's how I felt based on the before and after states of some pretty long tests I had to work with.

But here's something that's no better than before (probably worse): look at the test above and tell me which collaborators come into play when a Fisher catches a tasty fish, as opposed to the following, in which the fish gets thrown back.

test "catches and releases non-tasty fish" do
  (river = mock).expects(:give_it_up!).returns(fish = mock)
  fish.expects(:tasty?).returns(false)
  river.expects(:put).with(fish)
  
  fisher = Fisher.new(river, :ignored_bag)
  fisher.fish
end

It requires some pretty annoying code scanning when mock creation is embedded at various funny spots along the way. Those lines that felt like noise when they interrupted the specification of the interaction suddenly seem a lot more useful.

Having reconsidered, I now think the clearest presentation is closer to the first version, but different in a key way.

test "catches and releases non-tasty fish" do
  river = mock
  fish = mock
  
  river.expects(:give_it_up!).returns(fish)
  fish.expects(:tasty?).returns(false)
  river.expects(:put).with(fish)
  
  fisher = Fisher.new(river, :ignored_bag)
  fisher.fish
end

test "catches, kills, and bags tasty fish" do
  river = mock
  fish = mock
  bag = mock
  
  river.expects(:give_it_up!).returns(fish)
  fish.expects(:tasty?).returns(true)
  fish.expects(:die!)
  bag.expects(:put).with(fish)
  
  fisher = Fisher.new(river, bag)
  fisher.fish
end

Like the list of characters at the beginning of a script, these tests tell the reader who's involved in the interaction before jumping into the story, which makes the story much easier to follow. The tests take up more lines, but are easier for the reader to comprehend, particularly one who isn't interested in all the details. (Imagine looking through your codebase before a big refactoring wondering "how often do we use that FishingBag I want to change?" If collaborators are called out, you can quickly focus on the relevant interactions and read just those in detail.)

In the case of the longer tests that pushed me towards the inline style, the list of players would have been quite a bit uglier than the one above. That shouldn't have discouraged me from keeping it separated from the rest of the test: it should have told me that the object I was trying to test had too much on its plate. When something like that happens, hopefully I can figure out a design that won't require more players than I can easily keep track of all at once. If not, at least the next developer who comes along will have a clean tally of all the collaborators and a fighting chance of improving the design herself.

Good luck!