Why Your Tests are Broken and How to Fix Them

Writing and maintaining a useful test suite can be difficult. Today I’ll discuss some common causes of bad tests, and how to fix them.

Messing with global state

Global state is shared by all your tests. Mutating it in one test can lead to spurious successes or failures in other tests. It can lead to different behavior depending on the order in which the tests are run.

The following Clojure example contains some helper functions for

Busted up:

(def default-opts
  {:throw-exceptions false
   :cookie-store (cookies/cookie-store)})

(defn get [url opts]
  (client/get url (merge default-opts opts)))

(defn post [url opts]
  (client/post url (merge default-opts opts)))

Identifying these types of failures can be difficult. As a rule of thumb, if you reliably get one result running a test as part of a suite, and another running a test in isolation, inconsistent global state may be to blame.

If you can, stop messing with global state entirely. It will generally make your tests (and code) easier to reason about since there’s less you’ll have to keep in your head at any given time. If that’s not an option, take care to revert any mutations after the test has finished. In this case, when the namespace is loaded, a single global cookie store is created. Removing the global cookie store and managing a (new) cookie store for each test where it was required resolved the issue.

Fixed up:

(def default-opts
  {:throw-exceptions false})

...

Hitting external services

Relying on external services often causes problems. In the following Ruby and Capybara example, we click a button to add an item to the shopping cart, and then immediately click a link to proceed to checkout. While this may seem innocuous, the problem is that the “Proceed to Checkout” link doesn’t appear until after the “Add to Cart” button is clicked. To make matters worse, a web request to an external service is made after clicking the button, but before rendering the link. The test would occasionally work, but depending on the speed of the web request, would fail intermittently.

Busted up:

scenario 'checks out a product', js: true do
  visit feed_path
  expect(page).to have_selector('.product-preview')
  page.find('.product-preview').click
  click_button('Add to Cart')
  click_link('Proceed to Checkout') # BOOM. This fails if the service is too slow.
  expect(page).to have_selector '.checkout-wizard'
end

The key to fixing this test is understanding what not to test. A browser-based test like the above is not the place to be testing an external service’s uptime and responsiveness.

In an ideal world, instead of hitting any external services, we would create a mocked up version of the service that always returned the same data. This would not require changing the tests or the application code, beyond exposing a way to direct the application at the mocked service. However, depending on the complexity of the service, this might not be possible. Another option would be to stub out the requests in your application when in a test environment. This would be less work to implement, but would also exercise less of your application code, and require more invasive changes. A balance must be struck between usefulness and implementation cost.

In this example, mocking up the external service fixed the test without requiring any changes to the test itself. As it’s a big topic, a more in-depth look at how to mock up an external service will have to wait for another blog post.

Fixed up:

scenario 'checks out a product', js: true do
  visit feed_path
  expect(page).to have_selector('.product-preview')
  page.find('.product-preview').click
  click_button('Add to Cart')
  click_link('Proceed to Checkout') # This works now, even though the test is exactly the same
  expect(page).to have_selector '.checkout-wizard'
end

##“Happy path” only

When testing code, only passing expected “good” data may mask bugs that arise when passing unexpected data. In the following example, what happens if a

Busted up:

describe Payment do
  let(:attributes) do
    {
      amount: 2500,
      credentials: "ACCESS_TOKEN"
    }
  end

  describe "#amount" do
    subject { described_class.new(attributes) }

    it "wraps the supplied amount in cents" do
      expect(subject.amount.to_s).to eq "25.00"
    end
  end
end

Since

Fixed up:

describe Payment do
  let(:attributes) do
    {
      amount: 2500,
      credentials: "ACCESS_TOKEN"
    }
  end

  describe "#new" do
    subject { described_class }

    %i( amount credentials ).each do |attribute|
      it "fails if #{attribute} is not supplied" do
        attributes.delete(attribute)
        expect { subject.new(attributes) }.to raise_error ArgumentError, ":#{attribute} is required"
      end
    end
  end

  describe "#amount" do
    subject { described_class.new(attributes) }

    it "wraps the supplied amount in cents" do
      expect(subject.amount.to_s).to eq "25.00"
    end
  end
end

Commented out tests

Commented out tests are often a sign that something broke, but for some reason the test couldn’t be fixed. Often this is because of time constraints. Or it was unreliable, or slow, or no longer important. You just don’t know, which is exactly the problem. As time goes on, the original context is lost, and the commented out code becomes the new normal.

Busted up:

# scenario 'authenticates via Facebook', js: true do
#   OmniAuth.config.mock_auth[:facebook] = OmniAuth::AuthHash.new(
#     provider: 'facebook',
#     uid: '123545',
#     info: {
#       name: 'John Doe',
#       image: 'http://placehold.it/256x256',
#       email: 'user@user_email.com'
#     }
#   )

#   visit feed_path
#   click_button('Sign Up')
#   click_button('Continue with Facebook')
#   expect(page).to have_selector('.user-avatar', visible: true)
# end

Fix the test or remove it. The code should be in source control, so nothing will be lost by just removing the commented out code. If there are relevant details that should be captured, move them to a ticket. In this case, the test case was outdated, no longer necessary, and thus safe to remove.

Fixed up:

crickets

Testing the framework

Most frameworks worth using have exhaustive test suites, so it shouldn’t be necessary to spend time testing their functions. Doing so would lower the signal to noise ratio of your test suite and waste developer time.

Busted up:

describe Person, type: :model do
  it { should have_many(:friends) }
end

Remove the test, or consider if there’s a way to test actual business logic, instead of just the framework itself. A good rule of thumb: if your code is purely declarative, and uses only framework methods, you can probably skip testing it.

Fixed up:

crickets

Is that all?

This is just a small number of the possible types of test failures, but they occur in the wild often. Once you know what to look for, the become much easier to avoid.