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

How Factory Girl create association with existing object? #683

Closed
crazyjin opened this issue Aug 22, 2014 · 27 comments
Closed

How Factory Girl create association with existing object? #683

crazyjin opened this issue Aug 22, 2014 · 27 comments

Comments

@crazyjin
Copy link

I have two associated models.

class Girl < ActiveRecord::Base
   belongs_to :boy
end
class Boy < ActiveRecord::Base
   has_one :girl
end

And I define factory girl for them:

factory :girl do
  boy
  name 'saria'
end

factory :boy do
  name do
    puts create boy'
   'leo'
  end
end

Then i create a girl with an exsisting boy:

boy = create :boy
girl = create :girl, boy: boy

I will get following result:

>>>create boy
>>>create boy

I am wondering why the block of name of boy runs twice.
And, if i put code that can only runs once, troubles come.

Version of my factory_girl:

  • factory_girl (4.4.0)
  • factory_girl_rails (4.4.1)
@Kriechi
Copy link

Kriechi commented Jan 12, 2015

I have the same issue or question.
In my case this leads to a huge amount of time spent invoking the factory twice.
Only half of the objects are actually used, because one of the two created objects are discarded anyway.

Is there any way to prevent this from happening?
It would save a lot of time in my test suite.

@kofronpi
Copy link

+1, same question as this makes difficult for me to test some things and this is not documented

@Manifold0
Copy link

Ran into a similar problem where we had two associations attempting to create unique parent instances when we wanted them to belong to the same parent.

Class Term < ActiveRecord::Base
  has_many :candidates
  has_many :periods
end

Class Period < ActiveRecord::Base
  belongs_to :term
end

Class Candidate < ActiveRecord::Base
  belongs_to :term
end

In order to avoid needing to make multiple factory calls from the spec to reach the same term we ended up defining the factories like this:

factory :period do
  term { Term.first || association(:term) }
end

factory :candidate do
  term { Term.first || association(:term) }
end

This way the pre-existing Term can be referred to instead of creating two instances.

@joshuaclayton
Copy link
Contributor

Calling Thing.first introduces non-determinism in the test suite, since records can be created at various layers without the developer even being aware. I'd recommend creating a record and assigning it directly if the intent is to share the value.

@scaryguy
Copy link

@joshuaclayton is there any chance that you could give some practical example to how to do that?

@joshuaclayton
Copy link
Contributor

@scaryguy sure thing!

term = create(:term)
period = create(:period, term: term)
candidate = create(:candidate, term: term)

@TuckerWatts
Copy link

Thank you @joshuaclayton !

@twocathouse
Copy link

twocathouse commented May 11, 2017

From OP:

class Girl < ActiveRecord::Base
   belongs_to :boy
end
class Boy < ActiveRecord::Base
   has_one :girl
end

For real? Girl belongs to boy didn't feel a little gross to anyone?

Let's write examples without reducing women to objects, please, and call this stuff out when we see it.

@linkyndy
Copy link

@Euraldius, both Boy and Girl are "objects", as in Ruby objects. Let's not make such a big deal out of nothing, really, it's just an example. Nobody questioned or undermined human rights or the equality of genders.

@skatenerd
Copy link

skatenerd commented Jun 29, 2017

Bumping this, OPs question still isn't answered. In the original post, OP is already using the suggestion provided here.

The only solution I can come up with is to say (in the context of OP example)

factory :girl do
  boy { |weird_thing| somehow_inspect_weird_thing_to_check_if_boy_was_already_provided(weird_thing) }
  name 'saria'
end

But this doesnt seem very idiomatic.

Also, at least where I live, there is a a constant, subtle, barrage of messaging coming from the TV and other sources telling us that women are the property of men. This is insidious, and FWIW, I think we should try to be conscious not to take part. cc @linkyndy.

@estiens
Copy link

estiens commented Oct 2, 2017

just use .where(name: 'James').first_or_create!(foo: bar) in the block

so you could have something like

Class Term < ActiveRecord::Base
  has_many :candidates
  has_many :periods
end

Class Period < ActiveRecord::Base
  belongs_to :term
end

Class Candidate < ActiveRecord::Base
  belongs_to :term
end

Then do something like

factory :period do
  term { Term.where(name: 'Term 1').first_or_create!  }
end

@rsbrown
Copy link

rsbrown commented Dec 5, 2017

@linkyndy Do you seriously think @Euraldius did not understand that those labels referred to Ruby objects? Let me slowly explain to you the point that was being made:

When engineers use labels such as "boy"/"girl" in ways that reinforce toxic gender stereotypes, such engineers show their ignorance and (perhaps unconscious, perhaps latent) misogyny. So, stop. Think about how you use your language. And be considerate.

Another related example: the widespread use of repulsive "master"/"slave" labels in software systems.

Just fucking think about it for a second... bro.

@Manifold0
Copy link

#943

#1051

Bad example or not, the issue has been handled on this and many other levels.

We still haven't really found a great solution to the original problem, but if the only submissions here are going to be in regards to the social issues involved, then the should be locked. @Euraldius @joshuaclayton

@drewcimino
Copy link

drewcimino commented Jan 11, 2018

I solved this problem by using :inverse_of on the association in both models.

class Belonging < ActiveRecord::Base
   belongs_to :having, inverse_of: :belonging
end
class Having < ActiveRecord::Base
   has_one :belonging, inverse_of: :having
end

And then @Manifold0 I used an after-create callback on the factory for the model without the foreign key, so they look like this:

factory :belonging do
  having
end
factory :having do
  after(:create) do |having, evaluator|
    having.belonging || create(:belonging, having: having)
  end
end

In the callback, inverse_of allows for the associated Belonging to be returned from memory even if it isn't saved yet, as is the case when you create :belonging. You can do some dynamic checking, while not relying on something like Belonging.first.

@Kriechi this will help your test performance by not creating extra throwaway records, and also affect your actual production performance by making fewer trips to the db. Which is the actual purpose of inverse_of.

@adler99
Copy link

adler99 commented Feb 22, 2018

What do you think about:

factory :singleton do
  initialize_with do
      Singleton.where(
        name: 'unique_name'
      ).first_or_initialize
  end
end

this moves the logic to the definition of your singleton factory.

@drewcimino
Copy link

drewcimino commented Mar 6, 2018

If you're really working with a Singleton, I'm not sure you need FactoryBot at all. If it's not really a singleton, than first_or_initialize has the non-determinism problem mentioned above. Am I missing something about what you're trying to do?

@mib32
Copy link

mib32 commented Mar 13, 2018

I feel hurt by the word Singleton because it reminds me of who I think I am.

This message is not trollish intent to hurt anyone, rather a part of a philosophical debate. (cc @skatenerd)

I wholeheartedly agree that we should fight any discrimination.

@skatenerd
Copy link

@mib32 sry i got a bit defensive, seeing how the original namechange discussion ... turned out

@adler99
Copy link

adler99 commented Mar 13, 2018

@drewcimino The requirement in my case was that there is only one entry with a specific name in the db table. Every reference to a :singleton with the same name should be the same instance.
first_or_initialize is not atomic and could lead to race conditions. My tests run single threaded so there shouldn't be any race condition. Anyway the unique condition would be enforced on database level with a unique index, but a race condition would break the test ...

@seanmcmills
Copy link

i can see how the girl-belongs-to-boy rubs the wrong way, linkyndy. imagine a similar example, only swap the Girl model to Black and Boy model to White.

@drewcimino
Copy link

@adler99 So it seems your Singleton class isn't really a singleton, but does have relatively few saved records, like some sort of reference data. If the objects of this class are that standardized - that is, a record with a given name always has the same associated data - I would create them at the beginning of the test suite with a seeds-file-like script, and building some class methods for accessing them.

Something like:

Singleton.drewcimino
=> #<Singleton id: 1, name: "drewcimino">

as opposed to:

Singleton.where(name: "drewcimino").first_or_initialize
=> #<Singleton id: 1, name: "drewcimino"> OR #<Singleton id: nil, name: "drewcimino">

@CyberMew
Copy link

CyberMew commented Aug 15, 2018

Using the term, period and candidate example, if I add another layer, say:

Class Bear < ActiveRecord::Base
  belongs_to :candidate
  belongs_to :period
end

Is it possible to create the term inside of the bear FactoryBot beforehand and then get candidate and period to use the bear to create internally?

Probably a wrong example here but is it possible to do it inside (sometimes an object belongs to an object that belongs to an object, and if we were to manually create each object from the top and reference the next one outside of the factorybot file then it would be a mess everywhere), something like this:

# this is wrong but you get the idea
FactoryBot.define do
  factory :bear do
    # Run this first so everytime a bear gets created, a term for this bear gets created first to be used later
    before(:create) { create(:term }

    # Using the term created before this
    association :period, factory: :period, term: term
    association :candidate, factory: :candidate, term: term
  end
end

@drewcimino
Copy link

drewcimino commented Aug 19, 2018

@CyberMew If you want to complete the period and candidate with Bear#term after Bear/Term are created, I think an after block is the prescription.

FactoryBot.define do
  factory :bear do
    association :term
    association :period
    association :candidate

    after(:build) do |bear|
      bear.period.term ||= bear.term
      bear.candidate.term ||= bear.term
    end
  end
end

I use after(:build) instead of after(:create) and AR methods instead of passing ids around, so this setup will work for both build(:bear) and create(:bear). The associations follow the build strategy (build/create) of the parent, so if you create they'll assign first and then automatically populate all the ids on save.

You can force build strategies if you want to, but I don't recommend it as you can easily end up with some funky saved/not-saved "associated" records. But there's a bit on that here: https://github.com/thoughtbot/factory_bot/blob/master/GETTING_STARTED.md#associations

@fakefarm
Copy link

fakefarm commented Apr 4, 2019

I came here for code and got distracted by social issues. I don't have a problem with discussing our society. In fact, I'd like to know, who decides which social issues are worth discussing while working? Is it as simple as me labeling my issue an injustice - then I proceed to make my case? Currently, I have access to this comment box, so I suppose I have an equal right to say - but can someone point me to the rules?

I opted for before(:create)

FactoryBot.define do
  factory :album do
    name { Faker::Music::album }
    year { rand(1887..Time.new.year).to_i }

    before(:create) do |album|
      album.artist = create(:artist)
      album.genre = create(:genre)
    end
  end
end

@azelenets
Copy link

I believe we need many-to-many association between Boy and Girl classes.

@aesyondu
Copy link

aesyondu commented Apr 9, 2020

Came here because of a frustrating engineering problem, disappointed to see people are discussing irrelevant social issues.

  • Yes the social issues are important
  • No github issues is probably not the right place to discuss/debate them
  • Call the classes Yob and Lrig, I don't care, I need a solution to the inter-dependent factory objects

@composerinteralia
Copy link
Collaborator

As a reminder, anybody participating here agrees to follow our Code of Conduct. A code example where a girl belongs to a boy does not follow our Code of Conduct and I appreciate the folks who called that out.

I'm going to lock this issue. If somebody would be willing to open a new issue with a better example and a summary of the problem and potential solutions offered so far I would be most grateful.

@thoughtbot thoughtbot locked as off-topic and limited conversation to collaborators Apr 12, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests