Jakub Arnold's Blog


Duplication in Tests Is Sometimes Good

Having powerful tools like RSpec gives us so much power, that what was once a nice suite of readable specs becomes a huge bunch of unreadable mess, just because someone tried to DRY it up.

When writing your production code, there’s a good reason to keep the code DRY. Most of the times having duplication in your code can be a smell. But just because something sometimes smells, it doesn’t mean you should try to fix it all the time. This becomes even more important when writing tests.

Let’s compare these two examples

specify :draft? do
  build(:post, status: Post::DRAFT).should be_draft
  build(:post, status: Post::PUBLISHED).should_not be_draft
end

specify :published? do
  build(:post, status: Post::DRAFT).should_not be_published
  build(:post, status: Post::PUBLISHED).should be_published
end

This looks ok, but could we maybe refactor it a little bit to avoid the duplication there?

let(:draft_post) { build(:post, status: Post::DRAFT) }
let(:published_post) { build(:post, status: Post::PUBLISHED) }

specify :draft? do
  draft_post.should be_draft
  published_post.should_not be_draft
end

specify :published? do
  draft_post.should_not be_published
  published_post.should be_published
end

Now that we don’t have that ugly duplication anymore, let’s ask ourselves if the refactored test is really better? There is less duplication, but we’ve split each test in two parts. The problem comes when one of these tests fails, and suddenly you need to look around in the whole file to see where the setup is being performed.

This becomes even worse if you use more than one let to setup your specs, such as

let(:user1) { create(:user) }
let(:user2) { create(:user) }
let(:post) { create(:post, user: user1) }
let(:admin) { create(:user, :admin) }

it "doesn't allow any other user to delete a post" do
  user2.can_delete?(post).should be_false
end

it "allows admins to delete any post" do
  admin.can_delete?(post).should be_true
end

Imagine you have 20 tests like this for each context, and then define some other variables in the context above. A single failure will force you to scroll up and down and look around in 500 lines of test code, instead of just seeing everything in one place, such as.

it "doesn't allow any other user to delete a post" do
  user1 = create(:user)
  user2 = create(:user)
  post = create(:post, user: user1)

  user2.can_delete?(post).should be_false
end

it "allows admin to delete any post" do
  post = create(:post)
  admin = create(:admin)

  admin.can_delete?(post).should be_true
end

Is there more duplication? Yes. But if test #2 fails tomorrow, you’ll see what exactly is being tested, instead of having to spend 5 minutes goofing around in the spec file to see what is actually going on.

Related
Ruby