Noel Rappin Writes Here

This post is about a very small Rails design decision. A Rails design decision that I’ve made over and over without thinking about it. You probably have, too. And then a weird test failure made me think about it. And then it made me overthink it.

Let me explain.

I like the idea of writing Rails code such that there is minimal logic in ActiveRecord classes, and then writing a lot of tests that don’t need to touch the database, and therefore run fast.

I frequently find that even if I start writing these workflow tests using test doubles, I often wind up rewriting the tests to use ActiveRecord objects, because they break. And the tension is:

• ActiveRecord associations are really useful.
• ActiveRecord associations are really hard to replace with test doubles because Rails magic.

And I like fast tests and Rails magic, what can I do?

A design problem

The design decision came up while writing examples for the upcoming Money book. The code examples for the book are — spoiler alert — based on a theater selling tickets. The object model has a Purchase and PurchaseLineItem class, which is a common structure. When I started, the only thing you could buy were tickets, so PurchaseLineItem initially had a one-to-many relationship with Ticket.

I wrote a workflow object to purchase tickets from the shopping cart. And I drove the code using a bunch of tests with doubles.

I wound up with workflow code like this (edited for simplicity and context):

def create_payment
  payment = Payment.create(attribute_hash)
  tickets.each do |ticket|
    payment.payment_line_items.create(
      ticket: ticket, price_cents: ticket.price_cents)
  end
end

In the fullness of time, I needed to write a new chapter that let a user buy subscriptions. Now, the application has a second thing that can be purchased. And the easiest way to handle that in Rails is to make the relationship between my PurchaseLineItem and the things being purchased polymorphic.

Making the relationship polymorphic is relatively straightforward. There’s a database migration to change the name of the column from ticket_id to purchasable_id, adding a purchasable_type column, adjusting the declaration of the association in PurchaseLineItem, and changing some references from ticket to purchasable. Done.

The code now looks like:

def create_payment
  payment = Payment.create(attribute_hash)
  tickets.each do |ticket|
    payment.payment_line_items.create(
      purchasable: ticket, price_cents: ticket.price_cents)
  end
end

Very similar

And… all my workflow tests started failing. With really cryptic error messages.

A Test Failure

Turns out that when I call payment_line_items.create, Rails sets the purchasable attribute using the double as the value. When the relationship is not polymorphic, that’s fine. Rails calls for the id of the double — which I specify in the test — and puts the value in the database record.

But when the attribute is polymorphic, Rails also introspects on the class of the object to store in the type field. Which would be fine except that Rails expects that class to be a subclass of ActiveRecord::Base and calls methods based on that assumption. When the object is a test double — boom. The tests fail.

There are at least 3 quick ways to make this test pass again:

• Replace the ticket double with an actual ActiveRecord object
• Replace the ticket double with a FactoryGirl stubbed object, which would be a real ActiveRecord object with it’s database interactions stubbed out.
• Change the create call to set the purchaseable_id and purchasable_type, explicitly setting purchasable_type to Ticket without introspection.

All three of these options feel at least a little icky to me (technical term). And as I’ve written this I’ve talked myself in and out of all three of them.

• Creating a real object slows down the test and encourages more interaction between the service object and the database.
• The FactoryGirl object is faster, but still does not discourage separation between the workflow and the model. Also FactoryGirl stubs raise an error if you try to save them, which can be a pain when trying to test this kind of workflow
• Changing the create call would seem to violate the general principal that you don’t do idiosyncratic things in code just to protect your tests, and it arguably hard codes a dependency. It also lends itself to odd bugs if it’s inadvertently called with something that isn’t a ticket.

The most practical option is probably FactoryGirl, it gives me most of the speed of a test double and some of the separation of a mock.

That said, let’s see if I can talk myself into a different option.

Test Failures May Mean Design Flaws

I have an unexpected test failure caused by an implicit relationship that I didn’t take into account.

In test-driven development, I should look at that kind of failure as potentially a software design flaw, and not a flaw in the test. In particular, we’re looking for the possibility that an object uses information that it should maybe not know.

My eventual conclusion was that the workflow object should not know how the payment object creates line items. How line items are stored is an implementation detail. Like many implementation details, it’s a choice where Rails provides a very useful default that is very easily exposed to the rest of code, but it’s still an implementation detail.

Basically, that’s the whole Rails/OO argument:

Rails: We’ve made this hard thing easy

OO: But now it’s also easy for parts of the code that should be separate to be entangled.

Rails: Weren’t you listening? We made this thing really easy.

Both sides are right: Rails makes things easy, and that can come back to bite you if the app gets complex enough.

In the code, this means the call payment.payment_line_items.create should probably be inside the Payment class. While we’re at it, we can just pass in a list of tickets and do the whole loop inside Payment:

def create_payment
  payment = new_payment(attributes)
  payment.create_ticket_line_items(ticket)
end

And then, in the Payment class

def create_ticket_line_items(tickets)
  tickets.each do |ticket|
    payment_line_items.create(
      purchasable: ticket, price_cents: ticket.price_cents)
  end
end

Moving this code doesn’t solve the test problem by itself, but by moving the object creation out of the workflow, which is the object being tested, we can now use our existing test doubles to stub the object creation, and make our workflow test robust against changes in how the payment saves line items.

Something like this:

payment_double = instance_double(Payment)
allow(workflow).to receive(:new_payment).and_return(payment_double)
expect(payment_double).to
    receive(:create_ticket_line_items).with(\[ticket_1\])

With an optional test in Payment that would use the database to test how create_ticket_line_items works.

My tests can now handle changes in how line items are stored, at the cost of some additional complexity, namely one level of indirection in storing them.

Is the complexity worth it? Is it practical? Or is this just a case of needless complexity being the answer to a problem that only exists because of the needless complexity of using test doubles for unit tests?

For a small application, maybe not. The problem is that all these kinds of encapsulation and indirection arguments seem like overkill on small applications, and pretty much every example you are likely to see in a blog post is a small application. It feels like too much typing, and the gain seems far away.

In this application, though, where there’s every possibility that the business logic around payments is likely to get rather complicated by the end, there is value in separating processing from storage. For me, the hard part is often even realizing that these small breaks in encapsulation even exist — often I can’t tell until they’ve already become a huge problem. Thanks to this weird RSpec and Rails incompatibility, I was able to see a potential problem while it still seemed small.