Closed tagliala closed 1 year ago
Why would you want to put an association in a transient block? Wouldn't this code do the same as
FactoryBot.define do
factory :post do
user { create(:user) }
end
end
If yes, then why shouldn't this be treated as a FactoryAssociationWithStrategy
offence?
Hi,
apologies for the unclear scenario, but in the transient block there is not an association
As per https://github.com/thoughtbot/factory_bot/blob/main/GETTING_STARTED.md#transient-attributes
Transient attributes are attributes only available within the factory definition, and not set on the object being built. This allows for more complex logic inside factories.
user
is not an attribute of the post, so user=
is not defined and association will fail.
Here it is maybe a better example
FactoryBot.define do
factory :post do
transient do
external_website { create(:external_website) }
end
title { "A post talking about #{external_website}" }
link { external_website.url }
body { "I was impressed by #{external_website}'s blog post about #{}" }
end
end
Post
does not belong to ExternalWebsite
, but the external website object may be needed for complex logic inside the factory
but the example of FactoryBot's documentation works better, because post
is not associated to a user
, so user
does not respond to post=
# `user` has many posts, but it does not belong to a `post`.
# `post` will be used for complex logic in the factory
factory :user do
transient do
post { build(:post) }
end
end
Hope that this clarifies
Understood.
Do you really need to create
the external_website
? Would attributes_for
work? Do you rely on ExternalWebsite
model callbacks to populate certain attributes that you refer to from the :post
factory?
Hardcoding a strategy, specifically create
is known to be a source of major performance impacts on the growing spec suites, and is hard to refactor with multiple interdependent factories.
FactoryBot's documentation works better
# `user` has many posts, but it does not belong to a `post`.
# `post` will be used for complex logic in the factory
Please accept my apologies, is this note from the factory_bot
documentation, or is it just your comment to it, @tagliala ?
If user
has many post
s and you want to have one:
for a solution that works with build, build_stubbed, and create (although it doesn't work well with attributes_for), you can use inline associations:
factory :user_with_posts do
posts { [association(:post)] }
end
If a user
does not belong to a post
, can you please help me understand, how exactly is a single post
"will be used for complex logic in the factory"? I assume you mean "for complex logic in the user
factory", is this a correct assumption?
Sorry again for not providing enough information, I can clarify further
Do you really need to create the external_website?
Some transient attributes we use are ActiveModel
objects that are convenient to initialize through a factory
Hardcoding a strategy, specifically create is known to be a source of major performance impacts
I'm making this change wherever I can, because this cop is helping identifying unwanted create
s, but the transient
block has a different purpose
Please accept my apologies, is this note from the factory_bot documentation, or is it just your comment to it, @tagliala ?
Just my comment that can be added to FactoryBot documentation to clarify
I assume you mean "for complex logic in the user factory", is this a correct assumption?
Correct. A transient post
attribute in user
factory should be related to complex logic in the user factory
If a user does not belong to a post, can you please help me understand, how exactly is a single post "will be used for complex logic in the factory"?
I think the one in the documentation is just an example, and mine are examples too
If I should do an example from my domain logic, let's say that we have a plan with a fiscal year start date and End date. Fiscal year are not stored in the database, but they have some logic.
class FiscalYear
include ActiveModel::Model
include ActiveModel::Attributes
include Comparable
# ...
end
# frozen_string_literal: true
FactoryBot.define do
factory :fiscal_year do
skip_create
initialize_with { new(start_date, end_date) }
calendar
trait :calendar do
start_date { '2022-01-01' }
end_date { '2022-12-31' }
end
trait :non_calendar do
start_date { '2022-07-01' }
end_date { '2023-06-30' }
end
end
end
FactoryBot.define do
factory :plan do
transient do
fiscal_year { create(:fiscal_year) }
end
fiscal_year_start { fiscal_year.start_date }
fiscal_year_end { fiscal_year.end_date }
end
end
This can of course be achieved by passing dates directly to the plan, but if we want to create a plan from another object's fiscal year, it is convenient to pass it as a transient object
# before
create(:plan, fiscal_year_start: activity.fiscal_year.start_date, fiscal_year_end: activity.fiscal_year.end_date)
# after
create(:plan, fiscal_year: activity.fiscal_year)
Understood. You have factories backed by ActiveModel::Model
and use initialize_with
and skip_create
.
From a context of inspecting the plan
factory (where FiscalYear
is an in-memory model), there is no reliable way to make sure that the association built with a create
is not actually persisted and this is fine.
Would adding a configuration option to the cop, like IgnoreFactories
would work?
FactoryBot/FactoryAssociationWithStrategy:
Enabled: true
IgnoreFactories:
- fiscal_year
My only concern that this one could be abused by e.g. adding post
there to tolerate create(:post)
. Do you have an idea for a better name for this configuration option to minimize the possibility of such abuse?
By the way, thank you so much for being an early adopter and enabling all newly introduced cops. This lets us address issues before those cops are enabled for everyone, as we do in major releases. And thanks for reporting and clarifying the issue.
Welcome!
Would adding a configuration option to the cop, like IgnoreFactories would work?
This can help, but probably it is not even needed
I can see that
FactoryBot.define do
factory :plan do
transient do
fiscal_year { association :fiscal_year }
end
fiscal_year_start { fiscal_year.start_date }
fiscal_year_end { fiscal_year.end_date }
end
end
Works just fine, even if fiscal year
is not an association, and I was able to fix all the offenses and remove a lot of undesired create
s
44 files changed, 111 insertions(+), 100 deletions(-)
If a create is absolutely needed, fiscal_year { association :fiscal_year, strategy: :create }
will not raise offenses
I'm going to close here, hoping that his could help someone
Ah, that makes total sense. Thank you!
Sorry, I may did not understand correctly, but, as per https://github.com/thoughtbot/factory_bot/blob/main/GETTING_STARTED.md#with-associations
So I expect
not to be considered an offense