Never Skip a Callback in Your Tests

The story on how a missed callback in Rails caused me a whole day to track down.

On SlimWiki code, one failing test took me a whole day to track down. The worst thing is that the spec passes when the example is run in isolation, but fails when run in a test suite.
rspec spec/features/some_spec.rb:123   # passes
rspec spec/features/some_spec.rb       # fails, wtf!

Upon inspection, it turned out that our factory girl is producing a non-paid account, when asked for a paid account.
pry> FactoryGirl.create(:paid_account).subscription.paid?
=> false

The :paid_account factory declares a :pro_subscription like this:
factory :paid_account, :parent => :account do
  association :subscription, :factory => :pro_subscription # ...

But that factory works fine:
pry> FactoryGirl.create(:pro_subscription).paid?
=> true

After one day and sprinkling a bit of binding.pry here and there…
factory :paid_account, :parent => :account do
  # ...
  after(:build) do |account|

It turned out that after building the :paid_account (but before saving), the subscription is paid, which is correct.
pry> account.subscription.paid?
=> true

After the account is saved, however, it turned into a non-paid account!
=> true
pry> account.subscription.paid?
=> false # wtf!!

Something must be changing the global state of the system, but what could it be? One potential cause may be from directly modifying classes to temporarily skip callbacks. So I grepped for skip_callback on our specs, and found this seemingly unrelated factory:
factory :account_without_subscription, :parent => :account do
  to_create do |account|
    account.class.skip_callback :create, :after, :create_subscription!
    account.class.set_callback :create, :after, :create_subscription

Sometimes, we want to create an account, but without a subscription. To do that, we temporarily skip the callback before creating the account. After the account has been created, add the callback back to the class again.

Seems legit, right? Now look at our Account.rb:
after_create :create_subscription, :if => 'subscription.blank?'

Notice the :if clause here, and the lack of :if clause in the factory above. This causes the subscription to be unconditionally replaced with a non-paid subscription. Its impact echoes throughout the age of the rspec run, causing any future tests that depend on a paid subscription to fail.

Today, I learned that skipping callbacks — or in general, modifying the behavior of some class at runtime — can be dangerous and error-prone.

An alternative way is to define an accessor:
# Attributes (for tests)
attr_accessor :_skip_creating_subscription

# ...
after_create :create_subscription, :if => 'subscription.blank?',
                               :unless => :_skip_creating_subscription

The underscore in front of the attribute name means that it’s intended for tests, and not for general use. Our factory became much simpler too:
factory :account_without_subscription, :parent => :account do
  _skip_creating_subscription { true }

And now we are back to our fabulous run!
Like 248 likes
Thai Pangsakulyanont

Join the conversation

This will be shown public
All comments are moderated

Get our stories delivered

From us to your inbox weekly.