Refactoring tests for better application design
Editor’s Note: Today we have a guest post from Marko Anastasov. Marko is a developer and cofounder of Semaphore, a continuous integration and deployment service, and one of Code Climate’s CI partners.
The act of writing a unit test is more an act of design than of verification. - Bob Martin
A still common misconception is that test-driven development (TDD) is about testing; that by adhering to TDD you can minimize the probability of going astray and forgetting to write tests by mandating that is the first thing we need to do. While I’d pick a solution that’s designed for mere mortals over one that assumes we are superhuman any day, the case here is a bit different. TDD is designed to make us think about our code before writing it, using automated tests as a vehicle — which is, by the way, so much better than firing up the debugger to make sure that every code connected to a certain feature is working as expected. The goal of TDD is better software design. Tests are a byproduct.
Through the act of writing a test first, we ponder on the interface of the object under test, as well as of other objects that we need but that do not yet exist. We work in small, controllable increments. We do not stop the first time the test passes. We then go back to the implementation and refactor the code to keep it clean, confident that we can change it any way we like because we have a test suite to tell us if the code is still correct.
Anyone who’s been doing this has found their code design skills challenged and sharpened. Questions like agh maybe that private code shouldn’t be private or is this class now doing too much are constantly flying through your mind.
Test-driven refactoring
The red-green-refactor cycle may come to a halt when you find yourself in a situation where you don’t know how to write a test for some piece of code, or you do, but it feels like a lot of hard work. Pain in testing often reveals a problem in code design, or simply that you’ve come across a piece of code that was not written with the TDD approach. Some smells in test code are frequent enough to be called an anti-pattern and can identify an opportunity to refactor, both test and application code.
Take, for example, a complex test setup in a Rails controller spec.
describe VenuesController do
let(:leaderboard) { mock_model(Leaderboard) }
let(:leaderboard_decorator) { double(LeaderboardDecorator) }
let(:venue) { mock_model(Venue) }
describe "GET show" do
before do
Venue.stub_chain(:enabled, :find) { venue }
venue.stub(:last_leaderboard) { leaderboard }
LeaderboardDecorator.stub(:new) { leaderboard_decorator }
end
it "finds venue by id and assigns it to @venue" do
get :show, :id => 1
assigns[:venue].should eql(venue)
end
it "initializes @leaderboard" do
get :show, :id => 1
assigns[:leaderboard].should == leaderboard_decorator
end
context "user is logged in as patron" do
include_context "patron is logged in"
context "patron is not in top 10" do
before do
leaderboard_decorator.stub(:include?).and_return(false)
end
it "gets patron stats from leaderboard" do
patron_stats = double
leaderboard_decorator.should_receive(:patron_stats).and_return(patron_stats)
get :show, :id => 1
assigns[:patron_stats].should eql(patron_stats)
end
end
end
# one more case omitted for brevity
end
end
The controller action is technically not very long:
class VenuesController < ApplicationController
def show
begin
@venue = Venue.enabled.find(params[:id])
@leaderboard = LeaderboardDecorator.new(@venue.last_leaderboard)
if logged_in? and is_patron? and @leaderboard.present? and not @leaderboard.include?(@current_user)
@patron_stats = @leaderboard.patron_stats(@current_user)
end
end
end
end
Notice how the extensive spec setup code basically led the developers to forget to write expectations that Venue.enabled.find is called, or LeaderboardDecorator.new is given a correct argument, for example. It is not clear if the assigned @leaderboard comes from the assigned venue at all.
Trapped in the MVC paradigm, the developers (myself included) were adding up some deep business logic in the controller, making it hard to write a good spec and thus maintain both of them. The difficulty comes from the fact that even a one-line Rails controller method does many things:
def show
@venue = Venue.find(params[:id])
end
That method is:
- extracting parameters;
- calling an application-specific method;
- assigning a variable to be used in the view template; and
- rendering a response template.
Adding code that reaches deep inside the database and business rules can only turn a controller method into a mess.
The controller above includes one if statement with four conditions. A full spec, then, should include 15 combinations just for this one part of code. Of course they were not written. But things could be different, if this code was outside the controller.
Let’s try to imagine what a better version of the controller spec would look like, and what interfaces it would prefer to work with in order to carry its’ job of processing the incoming request and preparing a response.
describe VenuesController do
let(:venue) { mock_model(Venue) }
describe "GET show" do
before do
Venue.stub(:find_enabled) { venue }
venue.stub(:last_leaderboard)
end
it "finds the enabled venue by given id" do
Venue.should_receive(:find_enabled).with(1)
get :show, :id => 1
end
it "assigns the found @venue" do
get :show, :id => 1
assigns[:venue].should eql(venue)
end
it "decorates the venue's leaderboard" do
leaderboard = double
venue.stub(:last_leaderboard) { leaderboard }
LeaderboardDecorator.should_receive(:new).with(leaderboard)
get :show, :id => 1
end
it "assigns the @leaderboard" do
decorated_leaderboard = double
LeaderboardDecorator.stub(:new) { decorated_leaderboard }
get :show, :id => 1
assigns[:leaderboard].should eql(decorated_leaderboard)
end
end
end
Where did all the other code go? We’re simplifying the find logic by extending the model:
describe Venue do
describe ".find_enabled" do
before do
@enabled_venue = create(:venue, :enabled => true)
create(:venue, :enabled => true)
create(:venue, :enabled => false)
end
it "finds within the enabled scope" do
Venue.find_enabled(@enabled_venue.id).should eql(@enabled_venue)
end
end
end
The various if statements can be simplified as follows:
if logged_in?- variations on this can be decided in the view template;if @leaderboard.present?- obsolete, the view can decide what to do if it is not;- The rest can be moved to the decorator class under a new, more descriptive method.
describe LeaderboardDecorator do
describe "#includes_patron?" do
context "user is not a patron" { }
context "user is a patron" do
context "user is on the list" { }
context "user is NOT on the list" { }
end
end
end
This new method will help the view decide whether or not to render @leaderboard.patron_stats, which we do not need to change:
# app/views/venues/show.html.erb
<%= render "venues/show/leaderboard" if @leaderboard.present? %>
# app/views/venues/show/_leaderboard.html.erb
<% if @leaderboard.includes_patron?(@current_user) -%>
<%= render "venues/show/patron_stats" %>
<% end -%>
The resulting controller method is now fairly simple:
def show
@venue = Venue.find_enabled(params[:id])
@leaderboard = LeaderboardDecorator.new(@venue.last_leaderboard)
end
The next time we work with this code, we might be annoyed that controller needs to know what is the right argument to give to a LeaderboardDecorator. We could introduce a new decorator for venues that will have a method that returns a decorated leaderboard. The implementation of that step is left as an exercise for the reader. ;)
EPILOGUE
For further reading, check out Marko’s series about antipatterns in testing Rails applications on the Semaphore blog.