Never Skip a Callback in Your Tests

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!