factory_girl does not reload model files #269

Closed
pcasaretto opened this Issue Jan 16, 2012 · 14 comments

Comments

Projects
None yet
2 participants

Hello,

I have a conditional method on a model file, that depends on a Rails config (after_update :do_stuff if something).
In order to test that, I change the config value, and I reload the model file.
factory_girl fails to see that change forcing me to fallback to regular active_record.
I've created a simple example to demonstrate this issue, its on this repo.

https://github.com/pcasaretto/Factory-Issue

Owner

joshuaclayton commented Jan 17, 2012

You're probably better off using dependency injection, which by default passes Rails.config, than removing the constant and reloading the file. Factory Girl caches classes pretty heavily so the reference it stores is different than the class you end up reloading (add user.class.should == User to the failing spec and you'll see it fail, saying that User does not equal User).

Dependency injection? :)

I was hoping for something a little more on the pratical side hehe.
I read some info on the subject and it seems its an anti-pattern on ruby. (Needle at least).
I have no idea how to implement this.

Owner

joshuaclayton commented Jan 17, 2012

Instead of relying on an external object ($config global or Rails.config singleton), pass in that object (either through the method you're calling or make it available as a class or instance accessor that you can override in your tests) and give it a sensible default. It's definitely not an anti-pattern in Ruby; it's incredibly useful and makes testing things a lot easier, especially in examples like yours.

Here's a really simple example (this code may not be syntactically correct, but the idea is there).

class Something < ActiveRecord::Base
  class_accessor :config
  self.config = Rails.config

  after_create :do_something_crazy, :if => :rails_says_so

  private

  def rails_says_so
    self.class.config.allow_email_delivery? # assuming allow_email_delivery is the method you're looking to on Rails.config
  end
end

# in your test:

describe "normal config" do
  subject { Something.new }

  it "does do something crazy" do
    # assert that the results of do_something_crazy did occur
  end

end

describe "overriding config" do
  subject { Something.new }

  let(:different_config) { stub("non-Rails config", :allow_email_delivery? => false) }

  around do |example|
    # you'll want to override the normal config, set it to your new object, and then set it back.
    # this will ensure that you don't break other tests that may depend on the original configuration.
    original_config = Something.config
    Something.config = different_config
    example.run
    Something.config = original_config
  end

  it "does not do something crazy" do
    # assert that the results of do_something_crazy didn't occur
  end
end

In this example, I use dependency injection to override a configuration which implements the same interface that I'm interacting with in the model itself. This allows for passing in a stub which allows me to change the configuration without removing the constant and reloading the file.

Wow! That exceeded my expectations.
Thank you very much, will try that ASAP.

Owner

joshuaclayton commented Jan 17, 2012

Cool! Not sure if you'd be able to make it up to Boston, but I'm running a workshop called Test-Driven Rails where I'll cover ways to test different parts of Rails applications; dependency injection and stubbing are on my list of topics so it'd be awesome if you could make it.

That would be awesome, unfortunately I live in Brazil.
But thanks for the invite!

Hey @joshuaclayton , in your example, given that after_create is a method, I don't think it will matter if we change the config. Unless the reload the file I believe the location of config, Rails or class, does not matter.
Am I wrong?

Owner

joshuaclayton commented Jan 17, 2012

@pcasaretto if after_create passes a symbol to conditionally run something (e.g. after_create :send_mail, :if => :email_present?), #email_present? will be evaluated at runtime. So, the config will change appropriately.

You'll never want to have Ruby's classes change at 'compile time', e.g. when the file is loaded, because of the issue you're seeing right now.

Here's a more straightforward, real-world example of swapping out configurations at runtime. We can rely on the interface of that configuration (it responds to allow_things?) so that we can change the configuration on the fly instead of reloading the file. Here's a full, working example, following a similar structure to the code, spec, and the around filter in rspec to swap out configurations on-the-fly, ensuring that it's reset appropriately when the code is done running.

require "active_support"
require "active_support/core_ext"

class StandardConfig
  def allow_things?
    true
  end
end

class DisallowedConfig
  def allow_things?
    false
  end
end

class Something
  class_attribute :config
  self.config = StandardConfig.new

  def are_things_allowed?
    puts "Are things allowed? #{self.class.config.allow_things?}"
  end
end

def with_overridden_config(new_config)
  old_config = Something.config
  begin
    Something.config = new_config
    yield
  ensure
    Something.config = old_config
  end
end

Then, try running these:

Something.new.are_things_allowed?

with_overridden_config DisallowedConfig.new do
  Something.new.are_things_allowed?
end

Something.new.are_things_allowed?

The first call will report that things are allowed, the second will report that things aren't allowed, and the third will report that they are again. You could pass in any object that responds to allow_things? and it'll print out the result.

class ExcitedConfig
  def allow_things?
    "Most definitely!"
  end
end

with_overridden_config ExcitedConfig.new do
  Something.new.are_things_allowed?
end

Very interesting! Especially the overridden config block construct.
But it seems to work against my goal of not checking a boolean every time I update the model.
I know performance-wise it's no big deal to check a boolean, it just "feels" nice.
Is there some way to achieve that using this paradigm?

Owner

joshuaclayton commented Jan 19, 2012

No. You'd have to reload the file in order for it to recognize the change, which is a bad idea (it's potentially buggy and really hard to test, and introduces weird cases as we're seeing here).

Well, I think that settles it then. :)
Thank you very much for the lesson!

BTW, you can reply to my question on StackOverflow with the outcome of this discussion (if you want to).
http://stackoverflow.com/questions/8803843/make-activerecord-callback-conditional-on-configuration-parameter-and-test-it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment