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 # ...
end

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|
    binding.pry
  end
end

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!
pry> account.save!
=> 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.save!
    account.class.set_callback :create, :after, :create_subscription
  end
end

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 }
end

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

Join the conversation

This will be shown public
All comments are moderated

Get our stories delivered

From us to your inbox weekly.