What characterization tests are and what problem they solve

Let’s say I come across a piece of code that I want to refactor but unfortunately a) I don’t understand what it does and b) it’s not covered by tests. Refactoring untested code I don’t understand that’s not tested is risky business. But how can I write tests for the code if I don’t even know what it’s supposed to do?

Luckily there’s a technique called Characterization Testing, which I first discovered from Michael Feathers’ book, which makes solving this challenge pretty straightforward and easy. I’m going to show you how I do it.

What I’m going to show you

Below is a chunk of code I found in an old project of mine. The two methods below, services_with_info and products_with_info have a couple problems. One is that they’re very duplicative of each other. Another is that the Appointment class, which clocks in at 708 lines of code (most of which I’ve omitted out of mercy for the reader), is doing way two much stuff, and these two methods aren’t helping. These two methods should probably really belong to some other abstraction so Appointment can focus on being an Appointment and not have to worry about serializing its appointment_services and appointment_products.

class Appointment < ActiveRecord::Base
  def services_with_info
    appointment_services.collect { |item|
      item.serializable_hash.merge(
        "price" => number_with_precision(item.price, :precision => 2),
        "label" => item.service.name,
        "item_id" => item.service.id,
        "type" => "service"
      )
    }
  end

  def products_with_info
    appointment_products.collect { |item|
      item.serializable_hash.merge(
        "price" => number_with_precision(item.price, :precision => 2),
        "label" => item.product.name,
        "item_id" => item.product.id,
        "type" => "product"
      )
    }
  end
end

Refactoring the services_with_info method

Of these two methods I’m going to focus first on services_with_info. My goal is to take the body of the method, ask myself “What currently-nonexistent abstraction does this group of lines represent?”, create that abstraction, and move the body of the method there.

I might come up with an abstraction that I’m happy with on the first try, I might not. The first abstraction idea I’ll try is a new abstraction called AppointmentServiceCollection. Below is the class. For the most part all I’ve done is copy and paste the contents of services_with_info into this new class.

class AppointmentServiceCollection
  def initialize(appointment_services)
    @appointment_services = appointment_services
  end

  def to_h
    @appointment_services.to_h { |item|
      item.serializable_hash.merge(
        "price" => number_with_precision(item.price, :precision => 2),
        "label" => item.service.name,
        "item_id" => item.service.id,
        "type" => "service"
      )
    }
  end
end

First goal: simply instantiate the object inside a test

When I start writing my test for this new class I’m going to put in as little effort as possible. My first question to myself will be: can I even instantiate an AppointmentServiceCollection?

require 'spec_helper'

describe AppointmentServiceCollection do
  describe '#to_h' do
    it 'works' do
      appointment_service = create(:appointment_service)
      collection = AppointmentServiceCollection.new([appointment_service])
    end
  end
end

Second goal: see what value the method-under-test returns

When I run the test it passes. So far so good. My next step will be to add a very silly assertion.

require 'spec_helper'

describe AppointmentServiceCollection do
  describe '#to_h' do
    it 'works' do
      appointment_service = create(:appointment_service)
      collection = AppointmentServiceCollection.new([appointment_service])
      expect(collection.to_h).to eq('asdf')
    end
  end
end

I of course know that collection.to_h won’t equal asdf, but I don’t know what it will equal, so any value I check for will be equally wrong. No point in exerting any mental effort guessing what the output will be. The test result will tell me soon enough anyway.

When I run my test now, here’s what I get:

F

Failures:

  1) AppointmentServiceCollection#to_h works
     Failure/Error: expect(collection.to_h).to eq('asdf')
     TypeError:
       wrong element type AppointmentService at 0 (expected array)
     # ./app/models/appointment_service_collection.rb:7:in `to_h'
     # ./app/models/appointment_service_collection.rb:7:in `to_h'
     # ./spec/models/appointment_service_collection_spec.rb:8:in `block (3 levels) in <top (required)>'

Finished in 0.50943 seconds (files took 3.44 seconds to load)
1 example, 1 failure

That’s unexpected. I could scratch my head and puzzle over why this is happening, but instead I’ll just comment out code until the error goes away.

class AppointmentServiceCollection
  def initialize(appointment_services)
    @appointment_services = appointment_services
  end

  def to_h
    @appointment_services.to_h { |item|
      #item.serializable_hash.merge(
        #"price" => number_with_precision(item.price, :precision => 2),
        #"label" => item.service.name,
        #"item_id" => item.service.id,
        #"type" => "service"
      #)
    }
  end
end

When I run the test again I get the same error again. I’ll comment out more code.

class AppointmentServiceCollection
  def initialize(appointment_services)
    @appointment_services = appointment_services
  end

  def to_h
    #@appointment_services.to_h { |item|
      #item.serializable_hash.merge(
        #"price" => number_with_precision(item.price, :precision => 2),
        #"label" => item.service.name,
        #"item_id" => item.service.id,
        #"type" => "service"
      #)
    #}
  end
end

Now, not too surprisingly, I get a different error instead:

F

Failures:

  1) AppointmentServiceCollection#to_h works
     Failure/Error: expect(collection.to_h).to eq('asdf')
       
       expected: "asdf"
            got: nil
       
       (compared using ==)
     # ./spec/models/appointment_service_collection_spec.rb:8:in `block (3 levels) in <top (required)>'

Finished in 0.49608 seconds (files took 3.41 seconds to load)
1 example, 1 failure

This makes a lightbulb go on for me. Clearly the problem lies in the #@appointment_services.to_h { |item| line. The problem must be the to_h. What if I try map instead?

class AppointmentServiceCollection
  def initialize(appointment_services)
    @appointment_services = appointment_services
  end

  def to_h
    @appointment_services.map { |item|
      #item.serializable_hash.merge(
        #"price" => number_with_precision(item.price, :precision => 2),
        #"label" => item.service.name,
        #"item_id" => item.service.id,
        #"type" => "service"
      #)
    }
  end
end

That works. Instead of getting the “wrong element type” error I was getting before when the @appointment_services I’m now getting a different error that’s more in line with my expectations:

F

Failures:

  1) AppointmentServiceCollection#to_h works
     Failure/Error: expect(collection.to_h).to eq('asdf')
       
       expected: "asdf"
            got: [nil]
       
       (compared using ==)
       
       Diff:
       @@ -1,2 +1,2 @@
       -"asdf"
       +[nil]
       
     # ./spec/models/appointment_service_collection_spec.rb:8:in `block (3 levels) in <top (required)>'

Finished in 0.48778 seconds (files took 3.5 seconds to load)
1 example, 1 failure

Now let me uncomment the full body of the to_h method and see what happens.

class AppointmentServiceCollection
  def initialize(appointment_services)
    @appointment_services = appointment_services
  end

  def to_h
    @appointment_services.map { |item|
      item.serializable_hash.merge(
        "price" => number_with_precision(item.price, :precision => 2),
        "label" => item.service.name,
        "item_id" => item.service.id,
        "type" => "service"
      )
    }
  end
end

Now I get a different error. It doesn’t like my use of number_with_precision.

F

Failures:

  1) AppointmentServiceCollection#to_h works
     Failure/Error: expect(collection.to_h).to eq('asdf')
     NoMethodError:
       undefined method `number_with_precision' for #<AppointmentServiceCollection:0x007ff60274e640>
     # ./app/models/appointment_service_collection.rb:9:in `block in to_h'
     # ./app/models/appointment_service_collection.rb:7:in `map'
     # ./app/models/appointment_service_collection.rb:7:in `to_h'
     # ./spec/models/appointment_service_collection_spec.rb:8:in `block (3 levels) in <top (required)>'

Finished in 0.52793 seconds (files took 3.32 seconds to load)
1 example, 1 failure

Luckily I happen to know exactly how to fix this. In order to use the number_with_precision helper in a model (as opposed to in a view), you need to include the ActionView::Helpers::NumberHelper module.

class AppointmentServiceCollection
  include ActionView::Helpers::NumberHelper

  def initialize(appointment_services)
    @appointment_services = appointment_services
  end

  def to_h
    @appointment_services.map { |item|
      item.serializable_hash.merge(
        "price" => number_with_precision(item.price, :precision => 2),
        "label" => item.service.name,
        "item_id" => item.service.id,
        "type" => "service"
      )
    }
  end
end

The return values, revealed

With this error fixed, I now get my most interesting test result yet:

F                                              

Failures:                                      

  1) AppointmentServiceCollection#to_h works   
     Failure/Error: expect(collection.to_h).to eq('asdf')                                     
                                               
       expected: "asdf"                        
            got: [{"id"=>1, "appointment_id"=>1, "service_id"=>1, "created_at"=>Sun, 24 Feb 2019 16:45:16 EST -05:00, "updated_at"=>Sun, 24 Feb 2019 16:45:16 EST -05:00, "length"=>nil, "stylist_id"=>2, "price"=>"0.00", "label"=>"ve0xttqqfl", "item_id"=>1, "type"=>"service"}]        
                                               
       (compared using ==)                     
                                               
       Diff:                                   
       @@ -1,2 +1,12 @@                        
       -"asdf"                                 
       +[{"id"=>1,                             
       +  "appointment_id"=>1,                 
       +  "service_id"=>1,                     
       +  "created_at"=>Sun, 24 Feb 2019 16:45:16 EST -05:00,                                 
       +  "updated_at"=>Sun, 24 Feb 2019 16:45:16 EST -05:00,                                 
       +  "length"=>nil,                       
       +  "stylist_id"=>2,                     
       +  "price"=>"0.00",                     
       +  "label"=>"ve0xttqqfl",               
       +  "item_id"=>1,                        
       +  "type"=>"service"}]                  
                                               
     # ./spec/models/appointment_service_collection_spec.rb:8:in `block (3 levels) in <top (required)>'                                                                                      

Finished in 0.72959 seconds (files took 3.24 seconds to load)                                 
1 example, 1 failure

The reason I say this result is interesting is because instead of an error I get the actual value that the method is returning. The secrets of the universe are being revealed to me.

More realistic assertions

At this point I’m able to come up with some more realistic values to put in my test. I know that my test will fail because my values are just arbitrary and made up, but I feel confident that my values are pretty close.

require 'spec_helper'

describe AppointmentServiceCollection do
  describe '#to_h' do
    it 'works' do
      appointment_service = create(:appointment_service)
      collection = AppointmentServiceCollection.new([appointment_service])

      item = collection.to_h.first
      expect(item['price']).to eq('30.00')
      expect(item['label']).to eq('Mens Haircut')
      expect(item['item_id']).to eq(appointment_service.item.id)
      expect(item['type']).to eq('service')
    end
  end
end

I run my test just for fun and it of course fails. My next step is to make the test setup match my assertion values.

require 'spec_helper'

describe AppointmentServiceCollection do
  describe '#to_h' do
    it 'works' do
      appointment_service = create(
        :appointment_service,
        price: 30,
        service: create(:service, name: 'Mens Haircut')
      )

      collection = AppointmentServiceCollection.new([appointment_service])

      item = collection.to_h.first
      expect(item['price']).to eq('30.00')
      expect(item['label']).to eq('Mens Haircut')
      expect(item['item_id']).to eq(appointment_service.service.id)
      expect(item['type']).to eq('service')
    end
  end
end

What I do after I get the test passing

The test now passes. So far I’ve touched my application code as little as possible because I wanted to maximize the chances of preserving the original functionality. Now that I have a test in place, my test can do the job of preserving the original functionality, and I’m free to clean up and refactor the code.

class AppointmentServiceCollection
  include ActionView::Helpers::NumberHelper
  ITEM_TYPE = 'service'

  def initialize(appointment_services)
    @appointment_services = appointment_services
  end

  def to_h
    @appointment_services.map do |appointment_service|
      appointment_service.serializable_hash.merge(extra_attributes(appointment_service))
    end
  end

  private

  def extra_attributes(appointment_service)
    {
      'price'   => number_with_precision(appointment_service.price, precision: 2),
      'label'   => appointment_service.service.name,
      'item_id' => appointment_service.service.id,
      'type'    => ITEM_TYPE
    }
  end
end

I’ll also want to clean up my test code.

require 'spec_helper'

describe AppointmentServiceCollection do
  describe '#to_h' do
    let(:appointment_service) do
      create(
        :appointment_service,
        price: 30,
        service: create(:service, name: 'Mens Haircut')
      )
    end

    let(:item) { AppointmentServiceCollection.new([appointment_service]).to_h.first }

    it 'adds the right attributes' do
      expect(item['price']).to eq('30.00')
      expect(item['label']).to eq('Mens Haircut')
      expect(item['item_id']).to eq(appointment_service.service.id)
      expect(item['type']).to eq('service')
    end
  end
end

Lastly, I’ll replace the original services_with_info method body with my new abstraction. Here’s the original code.

def services_with_info
  appointment_services.collect { |item|
    item.serializable_hash.merge(
      "price" => number_with_precision(item.price, :precision => 2),
      "label" => item.service.name,
      "item_id" => item.service.id,
      "type" => "service"
    )
  }
end

Here’s the new version which uses AppointmentServiceCollection.

def services_with_info
  AppointmentServiceCollection.new(appointment_services).to_h
end

What we’ve accomplished

This is only a drop in the bucket but it helps. “A journey of a thousand miles begins with a single step,” as they say. This area of the code is now slightly more understandable. Next I could give the products_with_info the same treatment. Perhaps I could extract the duplication between the two methods into a superclass. If I apply this tactic to the Appointment class over and over, I could probably whittle it down from its current 708 lines to the more reasonable size of perhaps a few tens of lines.