TDD Antipatterns: The Free Ride

There is an excellent thread on Stack Overflow TDD Anti-patterns catalogue which contains short descriptions of some common traps we can fall into when writing test-driven code. In the upcoming series of short blog posts, I will illustrate some of them with snippets of Ruby code.

> The Free Ride a.k.a Piggyback: rather than write a new test case method
> to test another/distinct feature/functionality, a new assertion rides along
> in an existing test case.

In the following spec we are verifying that an event contains a feedback form whenever a user posts some content that indicates that she was present at the event. It comes from a real code base, just a little bit adapted for readability.

describe Event do
  describe "feedback form presence" do

    before do
      @user = create(:user)
      @event = create(:event)
    end

    it "is displayed when user generates an activity" do
      comment = @event.create_comment(:user => @user)
      expect(@event).to include_feedback_form(@user)

      comment.destroy
      expect(@event).not_to include_feedback_form(@user)

      photo = @user.post_photo(@event)
      expect(@event).to include_feedback_form(@user)

      photo.destroy
      expect(@event).not_to include_feedback_form(@user)

      like = @user.like(@event)
      expect(@event).to include_feedback_form(@user)

      like.destroy
      expect(@event).not_to include_feedback_form(@user)
    end
  end
end

What’s the problem here? Aside from unnecessary repetition of expectations that an event without any associated data does not include the feedback form, the description of the test does not match the things it tests. It is vague (“is displayed when user generates an activity”), which naturally comes from the fact that the test contains multiple different expectations. Once we let ourselves go down this path though, it is easy to end up with further complications. This example is actually rather simple; sometimes the expectations may not even appear to be connected when you look at them with fresh eyes, because of a lacking description.

A failure in any of these expectations would then not point to the exact cause of the test failure, which breaks down one of the main benefits of tests — a short feedback loop about our programming decisions. In a moment when more than one expectation would be broken, eg during a refactoring session, you would not be able to get that information because the test runner stops at the first failed expectation.

If we consider this OK, then the logical conclusion would be to make the whole suite a single test with numerous expectations. But the opposite is rather easy to achieve:

describe Event do
  describe "feedback form" do

    before do
      @user = create(:user)
      @event = create(:event)
    end

    context "before all activity" do
      it "is not displayed" do
        expect(@event).not_to include_feedback_form(@user)
      end
    end

    context "user posts a comment" do

      before { @event.create_comment(:user => @user) }

      it "is displayed" do
        expect(@event).to include_feedback_form(@user)
      end
    end

    context "user removes a comment" do

      before do
        comment = @event.create_comment(:user => @user)
        comment.destroy
      end

      it "is not displayed" do
        expect(@event).not_to include_feedback_form(@user)
      end
    end

    context "user posts a photo" do

      before { @user.post_photo(@event) }

      it "is displayed" do
        expect(@event).to include_feedback_form(@user)
      end
    end

    context "user likes it" do

      before { @user.like(@event) }

      it "is displayed" do
        expect(@event).to include_feedback_form(@user)
      end
    end
  end
end

Using different test values in a single test case

A related case, perhaps more a matter of readability, is to avoid grouping different test values in a single test case. For example:

describe City do

  it "generates slug based on name" do
    city = create(:city, :name => "San Sebastián")
    expect(city.slug).to eq("sansebastian")

    city = create(:city, :name => "Maceió")
    expect(city.slug).to eq("maceio")
  end
end

Instead we can follow the approach which would produce the most informative form of test output and feedback during development:

describe City do

  describe "slug, generated from name" do

    context "name is 'San Sebastián'" do
      subject { create(:city, :name => "San Sebastián") }
      its(:slug) { is_expected.to eq("sansebastian") }
    end

    context "name is 'Maceió'" do
      subject { create(:city, :name => "Maceió") }
      its(:slug) { is_expected.to eq("maceio") }
    end
  end
end

Update Jun 25: The examples above could be improved by explicitly stating what they are testing:

describe City do

  describe "slug, generated from name" do

    context "name is 'San Domingo'" do
      subject { create(:city, :name => "San Domingo") }

      it "strips the whitespace and converts to lowercase" do
        expect(subject.slug).to eq("sandomingo") }
      end
    end

    context "name is 'Maceió'" do
      subject { create(:city, :name => "Maceió") }

      it "strips replaces accented character" do
        expect(subject.slug).to eq("maceio") }
      end
    end
  end
end