Dave Schweisguth in a Bottle

Agile and lean principles lead to committing frequently, each commit potentially deployable to production. I’ve always been one to save my work often, and I commit often, not just whenever I finish an entire task, but whenever I’ve written enough code to justify running my tests, I do so, and they pass. This always seems like the right thing to do: it’s usually easy to break down big tasks into small, committable, deployable bits, and I can almost always point to the value in each commit, whether an implemented test, a refactoring or just a spelling fix. But I recently found myself in a situation where I realized that committing a (correct) test and its implementation that I’d just written would introduce a bug.

Background break: GWW has People, Photos and Guesses. A Photo has a Person (the person who posted it) and zero or more Guesses (a Guess is a record of who successfully guessed the photo’s location). A Guess also has a Person (the person who guessed). GWW only cares about People who have posted and/or guessed a photo, so if someone’s Guess is removed (which can happen, since guesses are judged by humans who can change their minds) and the Guess’s Person has no other Guesses or any Photos, that Person should be removed too. That wasn’t being done, leading to People with no Guesses or Photos in the database, which led to divisions by zero and broken pages.

So, the task is to, when removing a Guess, also remove its Person if that Person has neither other Guesses nor any Photos. Here’s the new test:

describe Guess do
  describe '#destroy' do
    it 'removes the Guess and its Person' do
      guess = Guess.make!
      guess.destroy
      Guess.count.should == 0
      guess.person.exists?.should == false
    end
  end
end


Actually, I’m lying; the method I was working on did more than this, and I hadn’t yet moved deletion of useless Persons into Guess.destroy. But it was almost as easy to see as it is here that if I just implemented this test and deployed the result I’d be deleting Persons in good standing. To add this feature safely, I needed a total of three tests:

describe Guess do
  describe '#destroy' do
    it 'removes the Guess and its Person' do
      guess = Guess.make!
      guess.destroy
      Guess.count.should == 0
      # TODO Dave
      #guess.person.exists?.should == false
    end

    it 'spares the Person if they have another Guess'

    it 'spares the Person if they have a Photo'

  end
end


(I’ll spare you the full additional specifications, which are what you’d expect.)

To safely add deletion of the Guess’s Person, I needed to write two new tests which passed without writing any new code, then reinstate the first test I’d written and make that pass without breaking the tests I’d just added. I really needed both additional tests, too, because deleting a Person with no other Guess but a Photo, or vice versa, would be just as broken as not checking at all. This goes against the usual TDD cycle of writing a test that fails, implementing that, and then refactoring (red-green-refactor). Without the new requirement of deleting the Person, the tests that test that it’s not deleted didn’t cover any new code or outcomes. They only had value as part of a group of tests.

Note that If I’d already had a test of Guess.destroy that asserted that the Guess’s Person was not destroyed, it would have broken when I implemented my first, overeager version of deletion, and I’d have changed that test to match the new requirements and fixed the new code so that both tests passed. I’d never have thought to write that test or that assertion, though: it too would pass without writing any new code (in fact Guess.destroy at that point would have been provided entirely by ActiveRecord::Base) and why would the person be destroyed anyway?

This situation breaks one of two principles I live by (frequent committing or red-green-refactor), my choice. The best solution I have right now is to think ahead about the sequence of tests I’m going to write, consider the effect on the system of implementing them in various orders, and choose a safe order, possibly writing some tests first that pass without any additional implementation. Besides the weirdness of green-from-birth tests — no, let’s call them Immaculate Tests, since they are written seemingly without cause, and never fail — that’s more thinking ahead than TDD usually requires of me. The alternative, what I actually did in this case, of writing and implementing the test which does delete first, then writing and implementing the tests which fill the holes in the first test, preserves red-green-refactor but prevents me from committing until I’ve finished the whole batch — and means I have to think about much more than whether the tests are green before I commit, which is worse than having to think ahead.

The best way out might be to generalize the situation to make it easier to recognize again. If I want to change a set of tests for which X is always true (whether tested or not) so that X is sometimes true and sometimes false, I’ll need to write an Immaculate Test of X to prevent me from committing green but incorrect code. A little wordy, but let’s see how it goes.