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.