FactoryBot/FactoryAssociationWithStrategy false positive

[ydakuka]

Actual behavior

ydakuka@yauhenid:~/Work/project$ bin/rails_docker rubocop spec/factories/
Inspecting 88 files
...............C.................C.................C..................................C.

Offenses:

spec/factories/adverts.rb:17:12: C: FactoryBot/FactoryAssociationWithStrategy: Use an implicit, explicit or inline definition instead of hard coding a strategy for setting association within factory.
    site { build(:site, domain_name: UrlNormalizer.new(link).domain) }
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
spec/factories/coupons.rb:22:18: C: FactoryBot/FactoryAssociationWithStrategy: Use an implicit, explicit or inline definition instead of hard coding a strategy for setting association within factory.
      discount { create(:discount, :expired) }
                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^
spec/factories/moderation_comments.rb:6:22: C: FactoryBot/FactoryAssociationWithStrategy: Use an implicit, explicit or inline definition instead of hard coding a strategy for setting association within factory.
      resourceable { create(:site) }
                     ^^^^^^^^^^^^^

RuboCop version

ydakuka@yauhenid:~/Work/project$ bin/rails_docker rubocop -V
1.56.3 (using Parser 3.2.2.3, rubocop-ast 1.29.0, running on ruby 2.7.8) [x86_64-linux]
  - rubocop-capybara 2.18.0
  - rubocop-factory_bot 2.24.0
  - rubocop-performance 1.19.1
  - rubocop-rails 2.21.1
  - rubocop-rake 0.6.0
  - rubocop-rspec 2.24.0
  - rubocop-thread_safety 0.5.1

[pirj]

Why false? Seems legit unless I’m missing something

[ydakuka]

The rule means that there is no need to create a record inside a block in an association, because this will be done by default.

# bad - only works for one strategy
factory :foo do
  profile { create(:profile) }
end

but in my associations from the snippet, the blocks contain code different from the default one.

Therefore, it's a false positive.

[pirj]

Examples are simplistic. There are many more cases, see the spec

rubocop-factory_bot/spec/rubocop/cop/factory_bot/association_style_spec.rb

Line 103 in 5589c19

association :user, :admin

I encourage you to try to rewrite those offences. Please report back if you encounter some unexpected behaviour.

[ydakuka]

Ok, I've fixed the two lines as follows:

site factory: :site, strategy: :build do
  domain_name { UrlNormalizer.new(link).domain }
end

discount factory: %i[discount expired]

But there is the problem with:

spec/factories/moderation_comments.rb:6:22: C: FactoryBot/FactoryAssociationWithStrategy: Use an implicit, explicit or inline definition instead of hard coding a strategy for setting association within factory.
      resourceable { create(:site) }
                     ^^^^^^^^^^^^^

I'll explain it below.

[ydakuka]

I have the following table with polymorphic association and the factory:

create_table "moderation_comments", options: "ENGINE=InnoDB DEFAULT CHARSET=utf8mb4", force: :cascade do |t|
  t.string "resourceable_type", null: false
  t.bigint "resourceable_id", null: false
end

FactoryBot.define do
  factory :moderation_comment do
    transient do
      resourceable { create(:site) }
    end

    resourceable_type { resourceable.class.name }
    resourceable_id { resourceable.id }
  end
end

I run rubocop and get offense:

ydakuka@yauhenid:~/Work/project$ bin/rails_docker rubocop spec/factories/moderation_comments.rb
Inspecting 1 file
C

Offenses:

spec/factories/moderation_comments.rb:6:22: C: FactoryBot/FactoryAssociationWithStrategy: Use an implicit, explicit or inline definition instead of hard coding a strategy for setting association within factory.
      resourceable { create(:site) }
                     ^^^^^^^^^^^^^

1 file inspected, 1 offense detected

I don't know how to fix it, because the "resourceable" trait is also registered.

[ydakuka]

Also, I have the second example with another offense (the FactoryBot/AssociationStyle cop).

create_table "account_balances", options: "ENGINE=InnoDB DEFAULT CHARSET=utf8mb4", force: :cascade do |t|
  t.string "accountable_type", null: false
  t.bigint "accountable_id", null: false
end

FactoryBot.define do
  factory :account_balance do
    association :accountable
  end
end

ydakuka@yauhenid:~/Work/project$ bin/rails_docker rubocop spec/factories/account_balances.rb
Inspecting 1 file
C

Offenses:

spec/factories/account_balances.rb:5:5: C: [Correctable] FactoryBot/AssociationStyle: Use implicit style to define associations.
    association :accountable
    ^^^^^^^^^^^^^^^^^^^^^^^^

1 file inspected, 1 offense detected, 1 offense autocorrectable

If I run rubocop with the autocorrect option and run spec, I will get the error:

ydakuka@yauhenid:~/Work/project$ bin/rails_docker rspec ./spec/models/account_balance_spec.rb:11

AccountBalance
  associations
    example at ./spec/models/account_balance_spec.rb:11 (FAILED - 1)

Failures:

  1) AccountBalance associations 
     Failure/Error: subject(:account_balance) { create(:account_balance, accountable: user) }
     
     KeyError:
       Trait not registered: "accountable" referenced within "account_balance" definition
     # ./spec/models/account_balance_spec.rb:6:in `block (2 levels) in <top (required)>'
     # ./spec/models/account_balance_spec.rb:12:in `block (3 levels) in <top (required)>'
     # ./spec/support/database_cleaning.rb:18:in `block (3 levels) in <main>'
     # ./spec/support/database_cleaning.rb:17:in `block (2 levels) in <main>'
     # ------------------
     # --- Caused by: ---
     # KeyError:
     #   Trait not registered: "accountable" referenced within "account_balance" definition
     #   ./spec/models/account_balance_spec.rb:6:in `block (2 levels) in <top (required)>'

Finished in 0.57175 seconds (files took 3.44 seconds to load)
1 example, 1 failure

Failed examples:

rspec ./spec/models/account_balance_spec.rb:11 # AccountBalance associations 

Randomized with seed 57222

[pirj]

We’ve had some issues with transient here #61
I didn’t look deeply, but your case seems unrelated.
Why so you even have to specify the type, id and transient for tgat polymorphic association?
Wouldn’t just

factory :moderation_comment do
  resourceable { association(:site) }
  # or
  assiciation :resourceable, factory: :site
end

work just as well?

[pirj]

I would be more interested how the model is defined for ‘accountable’.
What you would expect to get back from ‘create(:account_balance).accountable’?
You need to specify a … specific factory, not abstract.
It’s a factory, it produces real things.

[ydakuka]

Why so you even have to specify the type, id and transient for tgat polymorphic association?

Code smells :)

What you would expect to get back from ‘create(:account_balance).accountable’?

class AccountBalance < ApplicationRecord
  belongs_to :accountable,
             polymorphic: true,
             optional: false
end

So accountable { association(:user) } is suitable for my case.

[ydakuka]

I don't know if there is any solution here because there are no naming conventions for polymorphic associations.
And I think it wont be possible to add special cops for rubocop-rails.

[pirj]

The whole purpose of this project is to make your factories better. Do you feel that your code became better with those changes?

Is you try to get rid if the transient for resourceable, would it be easier to read?

[ydakuka]

Do you feel that your code became better with those changes?

Yes, it has become more readable.

Is you try to get rid if the transient for resourceable, would it be easier to read?

Yes, I have removed the transient block and now instead of 5 lines I only have one line.

[pirj]

Do you have more examples that would count as false positives, or can we close this?