Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Override or extend entire factories #229

Closed
roooodcastro opened this issue Feb 6, 2017 · 9 comments
Closed

Override or extend entire factories #229

roooodcastro opened this issue Feb 6, 2017 · 9 comments

Comments

@roooodcastro
Copy link

Hello, where I work we have multiple apps that share the same database, thus sharing most of the models. Because of this, we created a "core" gem that defines these models with common validations and methods, so we don't need to duplicate it.

However, we now want to do the same with factories, as they also are basically the same in our case. We managed to bundle the factories with the gem and "merge" them with each app's specific factories using something like this:

def load_factories
  return unless defined? FactoryGirl
  gem_path = OurCoreGem::Engine.root.join('lib', 'factories')
  FactoryGirl.definition_file_paths << gem_path
  FactoryGirl.reload
end

The problem is, if we need to change an already registered factory to fit the needs of a single app's tests, we can't, because it would throw the error FactoryGirl::DuplicateDefinitionError. The only other way would be to either create a new factory with another name, which defeats the whole purpose of this, or to change the factory directly in our gem, which is not ideal and can cause conflicts and break other app's tests.

Is there a way to override an already registered factory, or maybe a way to "unregister" a single factory so I can register it again?

Btw, if this turns to be a feature request issue, I'll be glad to help.

@duffyjp
Copy link

duffyjp commented Jun 2, 2017

@roooodcastro, did you ever find a workaround for this? I'd like to do the same.

I have an engine we reuse for all of our apps with three models. I append those factories to the host application, but in one case we have a breaking customization. I'd hate to have to load the gem factories by hand in five applications to fix the one that can't use them.

I'm using this technique BTW to load them:

engine.rb

...

# https://stackoverflow.com/questions/20261585/how-to-use-factorygirl-factories-from-an-engine
initializer "model_core.factories", :after => "factory_girl.set_factory_paths" do
  FactoryGirl.definition_file_paths << File.expand_path('../../../spec/factories', __FILE__) if defined?(FactoryGirl)
end

...

@roooodcastro
Copy link
Author

I couldn't find an ideal solution, I thought of a hackish solution, but I still don't think it would work:

  1. In your engine factory definition, check to see if the factory already exists with FactoryGirl.factories.registered? :factory_name. Only define the factory if it's not already registered.
  2. The problem with this is that as far as I know you cannot guarantee the order in which factories are registered, so your engine factories could be registered before your app factories, and the factory duplication error would still occur.
  3. Because of this, in your app factory files, you should always try to unregister the factories you want to overwrite, in case they've been previously registered in the engine. But I don't think this is possible at the moment, at least not using FactoryGirl's public API.

Even if this works, it's still a very hackish solution, and there should be a method to specify that the factory is to be overwritten, and that the duplication is not only a mere error.

@duffyjp
Copy link

duffyjp commented Jun 2, 2017

I think I figured it out. In addition to FactoryGirl.define, there is FactoryGirl.modify

If I change my engine to insert it's definitions at the front of the list rather than the end, then use modify in my host app I'm able to make changes.

engine.rb

    initializer "model_core.factories", after: "factory_girl.set_factory_paths" do
      FactoryGirl.definition_file_paths.unshift File.expand_path('../../../spec/factories', __FILE__) if defined?(FactoryGirl)
    end

Redefined factory:

FactoryGirl.modify do
  factory :foo do
    # MAIN APP CHANGES HERE
  end
end

I haven't actually gottten tests passing yet, but I'll update this comment with fixes if any are needed. Thanks @roooodcastro

@roooodcastro
Copy link
Author

Didn't know about FactoryGirl.modify, will try this later, thank you

@duffyjp
Copy link

duffyjp commented Jun 2, 2017

Followup: It works!

Finished in 50.1 seconds (files took 4.2 seconds to load)
153 examples, 0 failures

There is one trick, you can't add aliases in your modify call. It's easy enough to work around by duplicating the parent factory:

FactoryGirl.modify do
  factory :gem_engine_name_long_model_name do
    # MAIN APP CHANGES HERE
  end
end

FactoryGirl.define do
  factory :model_name, parent: :gem_engine_name_long_model_name
end

@Epigene
Copy link

Epigene commented Oct 2, 2017

I can confirm that this strategy works when you need to add or change attributes defined in the factory from engine, but I can't seem to figure out how to remove attributes in the overriding factory.

Example:

# in engine
FactoryGirl.define do
  factory :user do
    first_name "John"
    last_name "Doe"
  end
end

# in main app
FactoryGirl.modify do
  factory :user do
    name "John Doe"    
  end
end

Results in

NoMethodError:
  undefined method `first_name=' for #<User:0x007f8db9451138>

A dirty workaround would be to define virtual attributes, but, yuck.

@duffyjp
Copy link

duffyjp commented Oct 2, 2017

I wonder if you could misuse a transient block to accomplish that.

# in engine
FactoryGirl.define do
  factory :user do
    first_name "John"
    last_name "Doe"
  end
end

# in main app
FactoryGirl.modify do
  factory :user do
    transient do
      first_name nil
      last_name nil
    end

    name "John Doe"    
  end
end

I've never tried this but it's worth a shot.

@Epigene
Copy link

Epigene commented Oct 3, 2017

It worked! Thanks, @duffyjp

@composerinteralia
Copy link
Collaborator

It looks like between FactoryBot.modify and some cleverness with transient attributes y'all managed to sort this out, so I'm going to close the issue.

Another option in you wanted to remove specific attributes would be to just remove the entire factory and redefine it. We do have a PR for removing factories: thoughtbot/factory_bot#910

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants