News

Welcome to End Point’s blog

Ongoing observations by End Point people

Tests are contracts, not blank checks

Recently, I wrote up a new class and some tests to go along with it, and I was lazy and sloppy. My class had a fairly simple implementation (mostly a set of accessors, plus a to_s method). It looked something like this:

class Soldier
  attr_accessor :name, :rank, :serial_number
  def initialize(name,rank,serial_number)
    @name = name
    @rank = rank
    @serial_number = serial_number
  end

  def to_s
    "#{name}, #{rank}, #{serial_number}"
  end
end

I had been trying to determine the essential attributes of the class (e.g., what are the minimal elements of this class? should I have a base class, then sub-class it for the various differences, or should I have only a single class that contains everything I need?)

As a result of the speculative nature of the development, my tests only included a few of the attributes.

What's wrong with that?

On the surface, there is nothing technically wrong with skipping accessor tests: after all, testing each accessor individually is really testing Ruby, not the code I wrote. Another excuse I made is that testing each individually is very non-DRY - the testing code itself has lots of duplication.

The problem is that the set of tests should be considered a contract between the class writer and the outside world. By not including the correct and complete list of accessors, I left out important information; it's a check, already signed by the class developer, but with the amount left blank.

I've seen some code solve the non-DRY-ness problem like the following:

class Soldier
  Attributes = [:name, :rank, :serial_number]
  Attributes.each {|attr| attr_accessor attr}
  ...

then testing code of:

  Attributes.each do |attr|
    it "should have an accessor for #{attr}" do
      ...

That let's the testing code be nice and compact; simply load in the class, then iterate over the Attributes to verify that the accessors are present.

From a tests are contracts standpoint, this approach is terrible, though, perhaps even worse than the original, incomplete set of tests I had written. All the reader of the tests learns is that there is an array of attributes; the reader has to go look at the implementation itself to see what those attributes are.

Better is to use an anonymous array in the test, duplicating the attribute list; i.e.,

  [:name,:rank,:serial_number].each do |attr|
    it "should have an accessor for #{attr}" do
      ...
    end
  end

That seems to be a good balance between keeping tests as contacts yet keeping them DRY.

2 comments:

Ethan Rowe said...

"From a tests are contracts standpoint, this approach is terrible, though, perhaps even worse than the original, incomplete set of tests I had written. All the reader of the tests learns is that there is an array of attributes; the reader has to go look at the implementation itself to see what those attributes are."

It's worse than all that. The test needs to explicitly state the expectations for the interface. Not for some stylistic meatspace concern of how readable the test is, but for the test to be fundamentally sound. If you use the structure of the thing you're testing to dynamically determine the actual tests you run, then you're not testing the right stuff. Somebody could break the thing by removing an item from the attribute array, and the test doesn't catch that it's broken.

Steven Jenkins said...

Right. That's the point of the 'blank check' analogy -- by leaving out the actual attributes, the implementor could add some new ones, remove some existing ones, yet the tests would still pass.

Changing an interface should always require tests to change. Anything else is a sign of a problem.