Your tests want you to change your design

I came across an interesting post by Brandon Hilkert looking at the differences between constructor and setter dependency injection. It’s a great introduction to the differences between the two, and his example illustrates them well. Please go and read it first.

Brandon’s example gets started when he realises that the tests he wants to write are difficult:

[A] thing that bothered me was the difficulty simulating different pricing tiers and customer usage…What if I wanted to change the ceiling of that tier next month? I’d have to come in here and adjust the stats being created until it totalled something above the adjustment. It just felt weird..

He uses this as an example of how we can use dependency injection to use different collaborators in tests so we get more control over the context our objects are running in.

The solution he settles on is to use setter injection:

module StripeEvent
  class InvoiceCreated
    attr_writer :usage_service
    attr_reader :payload

    def initialize(payload)
      @payload = payload
    end

    def perform
      if user.created_at < 14.days.ago
        Stripe::InvoiceItem.create(
          customer: user.stripe_id,
          amount: additional_charges_in_cents,
          currency: "usd",
          description: "Usage charges"
        )
      end
    end

    private
    def additional_charges_in_cents
      Billing::Tier.new(usage).additional_charges_in_cents
    end

    def usage
      usage_service.last_30_days
    end

    def usage_service
      @usage_service ||= Billing::Usage.new(user)
    end

    def user
      @user ||= User.find_by(stripe_id: payload["data"]["object"]["customer"])
    end
  end

end

He’s now able to take an initialized object and using the attr_writer :usage_service he can swap in a stub implementation of the usage service and tightly control what is returned. It allows him to ignor the incidental detail of creating a Stat.

When tests are difficult to write they are a great indicator that there’s something about our design that we should take another look at. I’m not sure that adding the setter method really changes the design, it just hides a little complexity when writing our tests.

Here’s the sequence diagram for the original code:

original sequence

Using the setter to inject a usage service shortcuts creating the usage service, but is only relevant in the tests — in normal usage the design is exactly the same.

setter sequence

The tests were hinting that we had too many responsibilities, but this sequence diagram really highlights this:

  • Retrieve a user
  • Get the users usage for the last 30 days
  • Get the billing tier for that usage
  • Create an invoice item for the billing tier amount

The last one is the most important. We want to create an additional invoice item for the user, based on their usage in the last 30 days., and sequence diagram looked something like:

extracted dependencies sequence

Based on this idea I changed the tests to focus the object on the responsibility we care about:

describe "creating an invoice" do
  before do
    @payload = {
      "data" => {
        "object" => {
          "customer" => "stripe_brandon"
        }
      }
    }
  end
  let(:billing_tier_service) { double(:billing_tier_service) }
  let(:level1) { double(:tier, :additional_charges_in_cents => 1900) }
  let(:level2) { double(:tier, :additional_charges_in_cents => 4900) }

  it 'adds invoice item based on usage' do
    allow(billing_tier_service).to receive(:last_30_days_for_stripe_id).
      with("stripe_brandon").and_return(level1)

    expect( Stripe::InvoiceItem ).to receive(:create).with(
      customer: "stripe_brandon",
      amount: 1900,
      currency: "usd",
      description: "Usage charges"
    )
    StripeEvent::InvoiceCreated.new(@payload, billing_tier_service).perform
  end

  it 'adds next level charge for usage' do
    allow(billing_tier_service).to receive(:last_30_days_for_stripe_id)
      .with("stripe_brandon").and_return(level2)

    expect( Stripe::InvoiceItem ).to receive(:create).with(
      customer: "stripe_brandon",
      amount: 4900,
      currency: "usd",
      description: "Usage charges"
    )
    StripeEvent::InvoiceCreated.new(@payload, billing_tier_service).perform
  end
end

We are injecting a new dependency, a billing_tier_service. We’re imagining that this dependency will take on the responsibility of giving us back the correct billing tier for the customer that stripe has asked us for. Our tests will just check we creating the InvoiceItem correctly, and the resulting code is little simpler.

module StripeEvent
  class InvoiceCreated
    attr_reader :payload
    attr_reader :billing_tier_service

    def initialize(payload, billing_tier_service = :some_sensible_default)
      @payload = payload
      @billing_tier_service = billing_tier_service
    end

    def perform
      Stripe::InvoiceItem.create(
        customer: stripe_id,
        amount: additional_charges_in_cents,
        currency: "usd",
        description: "Usage charges"
      )
    end

    private

    def additional_charges_in_cents
      tier = billing_tier_service.last_30_days_for_stripe_id(stripe_id)
      tier.additional_charges_in_cents
    end

    def stripe_id
      payload["data"]["object"]["customer"]
    end
  end
end

Obviously this leaves us with more work to do. We will still need to build the billing_tier_service, to return the current tier for the customer. But this should be simpler, and focus purely on which level is returned based on usage.

The trade-off is that we now have to deal with more objects in our system, but each responsibility is in a single place, and our tests are isolated from incidental changes in other parts of the system, e.g. changing the tier pricing.

Test driven development is about listening to your tests. When something is hard to test it’s usually a good indicator that you should change something in your design. Use it as an opportunity to reevaluate design — and don’t be afraid of breaking out some boxes and arrows on the whiteboard.