In Progress
Unit 1, Lesson 1
In Progress

Mocking Smells 4

Video transcript & code

So today we're working on writing a brand new text editor. What sets this editor apart is that it automatically commits revisions of the file to a Git repository each time a file is saved. We're calling it "RevPad".

We've been using TDD with RSpec to build a minimum viable product. Along the way, we've been trying to use stub and mock objects as best we understand them.

Here are some of our current tests. first, there's a test that the contents of the working buffer is written to a file whenever the user requests a save. In order to do that, we instantiate a RevPad object. Then we put some text in its buffer. Then we stub out File.open so that it will return a fake file object.

We tell the object under test to save itself. Then we check that the fake file object received both the #write message, with the appropriate data, and a #close message.

The next test is similar. This time, we want to show that every time the user saves their file, a snapshot is stored into a local git repository. So this time, we have a bunch of expectations that the appropriate git commands are executed using the system() method.

require "rspec/autorun"

class RevPad
  attr_accessor :buffer

  def save
    f = File.open("histpad.txt", "w")
    f.write(buffer)
    f.close
    system("git init 2>&1 >/dev/null")
    system("git add histpad.txt 2>&1 >/dev/null")
    system("git commit -m'auto revision' 2>&1 >/dev/null")
  end
end

require "fileutils"
RSpec.describe RevPad do
  before do
    FileUtils.rm_f("histfile.txt")
    FileUtils.rmtree(".git")
  end

  it "can save buffer contents to a file" do
    pad = RevPad.new
    pad.buffer = "It was a dark and stormy night"
    allow(File).to receive(:open).and_return(file = spy("file"))
    pad.save
    expect(file).to have_received(:write).with("It was a dark and stormy night")
    expect(file).to have_received(:close)
  end

  it "saves older versions of a file" do
    pad = RevPad.new
    pad.buffer = "It was a dark and stormy night"
    expect(pad).to receive(:system).with("git init 2>&1 >/dev/null")
    expect(pad).to receive(:system).with("git add histpad.txt 2>&1 >/dev/null")
    expect(pad).to receive(:system).with("git commit -m'auto revision' 2>&1 >/dev/null")
    pad.save
  end
end

# >> ..
# >>
# >> Finished in 0.02877 seconds (files took 0.10566 seconds to load)
# >> 2 examples, 0 failures
# >>

Depending on how much RSpec experience you have, and how many RubyTapas testing episodes you've watched, you may already be cringing a little bit. This is not what I'd call ideal use of mocks. But it is highly representative of an enormous amount of mocking test code I've seen out in the wild. And very often, that code exists because the people who wrote it honestly had no idea how to do better.

Before we talk about alternatives, let's look at why these tests are problematic.

Let's say we've been learning more and more about idiomatic Ruby on the job as we build this editor. After watching some short videos by some long-haired nerd on the internet, we get the idea that it's more common to use a bare call to Kernel#open instead of File.open.

class RevPad
  # ...

  def save
    open("histpad.txt", "w") do |f|
      f.write(buffer)
    end
    system("git init 2>&1 >/dev/null")
    system("git add histpad.txt 2>&1 >/dev/null")
    system("git commit -m'auto revision' 2>&1 >/dev/null")
  end
end

Of course, this breaks our tests. Let's just assume for the sake of saving time that we go ahead and update the tests. But then we learn a little more about Ruby, and decide to use the block form of open instead, so that our file will be closed automatically when we are done with it.

class RevPad
  # ...

  def save
    open("histpad.txt", "w") do |f|
      f.write(buffer)
    end
    system("git init 2>&1 >/dev/null")
    system("git add histpad.txt 2>&1 >/dev/null")
    system("git commit -m'auto revision' 2>&1 >/dev/null")
  end
end

This breaks our tests again. We dutifully fix them. Then we one day we learn about the awesome Pathname library, and we start replacing all of our uses of bare filenames with Pathname objects instead.

class RevPad
  # ...

  def save
    Pathname("histpad.txt").open("w") do |f|
      f.write(buffer)
    end
    system("git init 2>&1 >/dev/null")
    system("git add histpad.txt 2>&1 >/dev/null")
    system("git commit -m'auto revision' 2>&1 >/dev/null")
  end
end

Of course, this means we have to update our tests again. Next, after reading through some Ruby library documentation, we realize we could reduce the file write to a one-liner using the File.write method.

class RevPad
  # ...

  def save
    IO.write("histpad.txt", buffer)
    system("git init 2>&1 >/dev/null")
    system("git add histpad.txt 2>&1 >/dev/null")
    system("git commit -m'auto revision' 2>&1 >/dev/null")
  end
end

Yay, broken tests again.

It wouldn't be so bad if we were just fixing one or two tests every time. The trouble is, as we've been going through this evolution, we've also been building out the responsibilities of the RevPad class. Every time one of its behaviors involves saving a file, we duplicate the code for stubbing out the file writing process. At this point, we're rewriting a couple dozen tests every time we come up with a better way to write files to disk.

And it's not just the file writing that we find ourselves changing. One day we do a code review with a more experience Ruby hacker, and they tell say "why aren't you using the git Rubygem here?" We dutifully look it up, and rewrite our code to use this gem. Then we resign ourselves to a couple hours of rewriting tests to support this new way of managing Git revisions.

class RevPad
  # ...

  def save
    IO.write("histpad.txt", buffer)
    git = Git.init
    git.add "histpad.txt"
    git.commit("auto revision")
  end
end

At this point, mockist TDD is starting to look like more trouble than it is worth.

The root cause of all of our problems is that we have once again violated the principal guideline for mock objects: "only mock what you own".

But in past examples, where we've been interacting with things like external web services, it's been very easy to tell that we were dealing with a service that we didn't own. What's not always obvious is that even basic language facilities like file I/O and shell command execution fall into the "stuff we don't own" category.

One way we may justify mocking or stubbing core language I/O classes is that unlike 3rd-party libraries, core libraries have stable APIs that are unlikely to change anytime soon. But as we've just seen, it's not just a question of how stable an API is. If there are a half a dozen different ways to do something, then by mocking what we don't own, we've arbitrarily locked our tests down to only accepting that one way.

So what can we do? If you've been watching for a while, this maneuver is going to seem pretty familiar by now. The key is to push out responsibilities for system interaction into their own classes. In this case, we have two separate responsibilities: persisting data to disk, which we might delegate to a "persistor" object, and preserving snapshots to git, which we could push out into a "historian" role.

Our unit tests might end up looking something like this. Each injects a persistor and a historian spy object into the object under test. Then, they verify that the appropriate messages were sent to the right recipients.

require "rspec/autorun"

class RevPad
  attr_accessor :buffer

  def initialize(persistor:, historian:)
    @persistor = persistor
    @historian = historian
  end

  def save
    @persistor.write(buffer)
    @historian.snapshot
  end
end

RSpec.describe RevPad do
  it "can save buffer contents to a file" do
    pad = RevPad.new(persistor: persistor = spy, historian: spy)
    pad.buffer = "It was a dark and stormy night"
    pad.save
    expect(persistor).to have_received(:write).with("It was a dark and stormy night")
  end

  it "saves older versions of a file" do
    pad = RevPad.new(persistor: persistor = spy, historian: historian = spy)
    pad.buffer = "It was a dark and stormy night"
    pad.save
    expect(persistor).to have_received(:write).ordered
    expect(historian).to have_received(:snapshot).ordered
  end
end

# >> ..
# >>
# >> Finished in 0.00174 seconds (files took 0.10828 seconds to load)
# >> 2 examples, 0 failures
# >>

It might feel like we've lost a lot of verification here. But let's look back at our original tests. What are these tests really verifying? We assert that a series of messages are sent to core file I/O classes, but at no point do we actually verify that these message will have the intended effect of causing a file to be persisted to disk. Likewise, we set expectations that certain git commands will be executed, but nowhere do we prove that these are, in fact, the right git commands.

The truth is, what we had before were low-value tests, and constantly updating them to cope with new implementations merely simply slowed us down. In getting rid of these tests, we haven't lost anything of value.

How can we build high-value tests for these newly-extracted responsibilities of persisting data and snapshotting versions? That's a great question, and we'll tackle it in a future episode. For now, I want to leave you with this observation: mocking system I/O libraries, no matter how simple it might seem at first, is the first step down an endless rabbit hole. Tests quickly diminish in value as the more they mock objects we don't own, and that is just as true of core classes as it is of third-party APIs.

Happy hacking!

Responses