Instance Doubles: Rspec Anti-Pattern?

Published in Programming on

Over twelve years in software, and I still find it incredibly difficult to write good tests for my code. Almost as difficult as getting a group of programmers to agree on what's a good test.

More than half of my career has now been working in Rails applications, and I’ve often seen specs using instance doubles as a tool to avoid the database. While I admire avoiding the database when possible in tests, the drive seems to lead to situations where the specs

  1. Don’t actually test anything other than if you stubbed out methods properly

  2. Are so brittle that any change to the structure of the code under test breaks them

  3. Are so brittle that they are broken by changes to the structure of code that shouldn’t even be related 😱

It’s come to the point where if I run into a file that heavily relies on instance doubles, it’s almost a guarantee that I'll have to rewrite the specs from scratch.

I don’t have enough experience to say definitively, but I have a strong suspicion that the real issue in these cases is code that is too tightly coupled and/or some leaky abstractions are in use. In a newer codebase, you might be able to untangle these issues on the code side. Yet, in a long-lived project, you’re likely stuck making the compromises in your specs unless you want a 10,000 line PR untangling code that spans 5 years and 10 different pairs of hands, all with different ideas on How It Should Be™.

Definitely feel like I’m channeling Charlie when reading through code sometimes

When I say compromises, I specifically mean test setup code. You might have to build up tons of entities/records to test the logic of a block of code successfully, but this is by far the lesser evil. The best way to explain is with a code example.

To start, here are a few Rails models, along with the specs for an order method. One spec in the style I suspect to be an anti-pattern, and another using my preferred way.

class PaymentMethod < ApplicationRecord
  belongs_to :account
end

class Account < ApplicationRecord
  has_many :payment_methods
  has_one :primary_payment_method, -> { where(is_primary: true) }, class_name: 'PaymentMethod'
end

class Product < ApplicationRecord
end

class OrderProduct < ApplicationRecord
  belongs_to :product
  belongs_to :order
end

class Order < ApplicationRecord
	belongs_to :account
  has_many :order_products
  has_many :products, through: :order_products

  def chargeable?
    return false unless products.size.positive?
    return false unless account.primary_payment_method.present?

    true
  end
end

RSpec.describe Order, type: :model do
  describe 'bad? #chargeable?' do
    subject { order.chargeable? }

    let(:order) { Order.new }
    let(:account) { instance_double(Account, primary_payment_method: double) }
    let(:product) { instance_double(Product) }

    before do
      allow(order).to receive(:account) { account }
      allow(order).to receive(:products) { [product] }
    end

    it { is_expected.to eq(true) }
  end

  describe 'good? #chargeable?' do
    subject { order.chargeable? }

    let(:order) { build_stubbed(:order, user: account) }
    let(:account) { build_stubbed(:account) }
    let(:product) { create(:product) }

    let!(:order_product) { create(:order_product, order: order, product: product) }
    let!(:payment_method) { create(:payment_method, account: account, is_primary: true) }

    it { is_expected.to eq(true) }
  end
end

If there is a #chargeable?, there is likely going to be a #charge! too. For the sake of not confusing the issue, I'll only show a simple version that won't actually hit a payment provider.

class Order < ApplicationRecord
  def charge!
    return false unless chargeable?

    # Call payment SDK
    true
  end
end

RSpec.describe Order, type: :model do
  describe 'bad? #charge!' do
    subject { order.charge! }

    let(:order) { Order.new }

    let(:account) { instance_double(Account, primary_payment_method: double) }
    let(:product) { instance_double(Product) }

    before do
      allow(order).to receive(:account) { account }
      allow(order).to receive(:products) { [product] }
    end

    it { is_expected.to eq(true) }
  end

  describe 'good? #charge!' do
    subject { order.charge! }

    let(:order) { create(:order, account: account) }
    let(:account) { create(:account) }
    let(:product) { create(:product) }

    let!(:order_product) { create(:order_product, order: order, product: product) }
    let!(:payment_method) { create(:payment_method, account: account, is_primary: true) }

    it { is_expected.to eq(true) }
  end
end

All of the specs for this hypothetical charging workflow pass! 🎉For now. 😈

Here is where things get tricky. What happens if a check needs to be added for if the primary_payment_method is expired? I think ideally, the only spec update required would be for #chargeable? with #charge! being left alone. If the default for #expired? is false, bad? #chargeable? is the only spec that has to change. But both should change to cover the new logic branch.

class PaymentMethod < ApplicationRecord
  def expired?
    false
  end
end

class Order < ApplicationRecord
  def chargeable?
    return false unless products.size.positive?
    return false unless account.primary_payment_method.present?
    return false if account.primary_payment_method.expired?

    true
  end
end

RSpec.describe Order, type: :model do
  describe 'bad? #chargeable?' do
    subject { order.chargeable? }

    let(:order) { Order.new }

    let(:account) { instance_double(Account, primary_payment_method: payment_method) }
    let(:product) { instance_double(Product) }
    let(:payment_method) { instance_double(PaymentMethod, expired?: false) }

    before do
      allow(order).to receive(:account) { account }
      allow(order).to receive(:products) { [product] }
    end

    it { is_expected.to eq(true) }
  end
end

Unfortunately, re-running the specs results in an error on bad? #charge!. 

$ bundle exec rspec spec/models/order_spec.rb
..F.

Failures:

  1) Order bad? #charge!
     Failure/Error: return false if account.primary_payment_method.expired?
       # received unexpected message :expired? with (no args)
     # ./app/models/order.rb:10:in `chargeable?'
     # ./app/models/order.rb:16:in `charge!'
     # ./spec/models/order_spec.rb:35:in `block (3 levels) in '
     # ./spec/models/order_spec.rb:47:in `block (3 levels) in '

Finished in 0.08436 seconds (files took 0.99 seconds to load)
4 examples, 1 failure

Failed examples:

rspec ./spec/models/order_spec.rb:47 # Order bad? #charge!

The #expired? method has not been defined on the returned double, so the spec "erroneously" fails even though the code that's supposed to be under test hasn't changed. On the other hand, the spec for good? #charge! ends up passing since the default for the expired state is false.

This is a brief, semi-synthetic example of the issues I end up running into with the instance_double testing style all the time. This specific example could be easily adjusted to be less brittle, but as more time, hands, and complexity are added, things quickly spin out of control. There are few things worse than when I'm trying to make a small, targeted change and end up having to chase down why dozens of mostly unrelated specs are failing.

So that's my case against using instance doubles! Feel free to @ me on Twitter with feedback, especially if you've found a better way to manage complex specs 🙏