Add RSpec/FactoryBot/AssociationStyle cop

[r7kamura]

In FactoryBot, there are 2 ways to define associations.

# implicit style
factory :article do
  user
end

# explicit style
factory :article do
  association :user
end

These styles are documented at the following section:

• https://github.com/thoughtbot/factory_bot/blob/ec71611954d07419c9b668be03641332443dcd78/GETTING_STARTED.md#associations

(Stricyly speaking, there is also inline style, but it behaves differently from the above 2 styles.)

It would be nice to have a cop for consistent use of only one of the styles. This Pull Request has been created for this purpose.

---

Before submitting the PR make sure the following are checked:

• Feature branch is up-to-date with master (if not - rebase it).
• Squashed related commits together.
• Added tests.
• Updated documentation.
• Added an entry to the CHANGELOG.md if the new code introduces user-observable changes.
• The build (bundle exec rake) passes (be sure to run this locally, since it may produce updated documentation that you will need to commit).

If you have created a new cop:

• Added the new cop to config/default.yml.
• The cop is configured as Enabled: pending in config/default.yml.
• The cop is configured as Enabled: true in .rubocop.yml.
• The cop documents examples of good and bad code.
• The tests assert both that bad code is reported and that good code is not reported.
• Set VersionAdded in default/config.yml to the next minor version.

If you have modified an existing cop's configuration options:

• Set VersionChanged in config/default.yml to the next major version.

[pirj]

It seems that over time, all the three progressed. I was still believing that the implicit way didn't allow specifying factory: as it was in 5.0. Good to know FB improved so much on this front.

Personally, I strongly prefer the inline definition, for three reasons:

• usage of instance (could be done before 6.0 with some hack, too) to refer to the model from association definitions
• refer to (transient) attributes
• with one-to-many associations an approach with no need for after(:create) that implies create strategy, or multiple per-strategy hooks that tend to get out of sync

The downside is that it's the most verbose out of the three in most of the cases.

it behaves differently from the above 2 styles

Is there anything else that I'm missing?

[pirj]

Looks good as is. Up to you if you tend to keep it like that, and leave improvements for follow-up PRs/tickets.
The only thing left to do is to run it against real-world-rspec and see which style is most frequently used in the wild to minimize the impact and follow what people really use. I can run this for you if you like.

[pirj]

Could be written as implicit author factory: :user, so I'd count this as an offence.
It's the inline style that we don't detect, but not really "inline" 😄

[pirj]

The implicit style run with real-world-rspec yields some false positives:

/Users/pirj/source/real-world-rspec/gitlabhq/spec/factories/issues.rb:49:5: C: [Correctable] RSpec/FactoryBot/AssociationStyle: Use explicit style to define associations.
    factory :closed_issue, traits: [:closed]
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
/Users/pirj/source/real-world-rspec/gitlabhq/spec/factories/issues.rb:50:5: C: [Correctable] RSpec/FactoryBot/AssociationStyle: Use explicit style to define associations.
    factory :reopened_issue, traits: [:opened]
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

The explicit style run results in some errors:

An error occurred while RSpec/FactoryBot/AssociationStyle cop was inspecting /Users/pirj/source/real-world-rspec/cartodb/spec/factories/likes.rb:2:2.
undefined method `begin_type?' for nil:NilClass
/Users/pirj/source/rubocop-rspec/lib/rubocop/cop/rspec/factory_bot/association_style.rb:154:in `children_of_factory_block'
/Users/pirj/source/rubocop-rspec/lib/rubocop/cop/rspec/factory_bot/association_style.rb:142:in `bad_associations_in'
/Users/pirj/source/rubocop-rspec/lib/rubocop/cop/rspec/factory_bot/association_style.rb:45:in `on_send'

for e.g.:

FactoryBot.define do
  factory :like, class: Carto::Like do
  end
end

    factory 'valid site' do
      # after(:build) { |valid_site| valid_site.stubs(:valid?).returns(true) }
    end

[pirj]

Implicit:

57497 files inspected, 191 offenses detected, 191 offenses autocorrectable

Explicit:

57497 files inspected, 911 offenses detected, 911 offenses autocorrectable

Going with the implicit as the default enforced style seems reasonable 👍

[pirj]

For the reference, this (apart from probably pending -> true) was needed to run the cop against real-world-rspec:

[r7kamura]

it behaves differently from the above 2 styles

Is there anything else that I'm missing?

As noted in the documentation, attributes defined by inline-style always appears with nil when called with attributes_for strategy. This may be a fairly trivial difference, but I think it will have an impact on the existing code.

# implicit
FactoryBot.define do
  factory :article do
    user
  end
end

FactoryBot.attributes_for(:article) #=> {}

# explicit
FactoryBot.define do
  factory :article do
    association :user
  end
end

FactoryBot.attributes_for(:article) #=> {}

# inline
FactoryBot.define do
  factory :article do
    user { association :user }
  end
end

FactoryBot.attributes_for(:article) #=> {:user => nil}

Could be written as implicit author factory: :user, so I'd count this as an offence.
It's the inline style that we don't detect, but not really "inline" 😄

For that reason, it would be difficult to safely say that this is an offense 🤔

My personal preference is to use inline-style only when I need it and not otherwise. If I always use inline-style, it makes difficult to infer from the code whether I am doing so because inline-style is necessary in the case or not.

If there seems to be a certain number of people who want to use the inline-style all the time, it might be nice to have a style in this cop that always prefers the inline-style. The downside is that it is quite difficult to autocorrect from inline-style to the other styles for the reasons mentioned above.

[pirj]

A couple of cosmetic suggestions

[pirj]

implicit

57497 files inspected, 206 offenses detected, 206 offenses autocorrectable

Concerns

/Users/pirj/source/real-world-rspec/hound/spec/factories.rb:77:5: C: [Correctable] RSpec/FactoryBot/AssociationStyle: Use implicit style to define associations.
    association :repo, :active, :private
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

/Users/pirj/source/real-world-rspec/gitlabhq/spec/factories/clusters/kubernetes_namespaces.rb:5:5: C: [Correctable] RSpec/FactoryBot/AssociationStyle: Use implicit style to define
associations.
    association :cluster, :project, :provided_by_gcp
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Would the following be a valid FactoryBot syntax?

repo :active, :private
cluster :project, :provided_by_gcp

Using traits with associations only has examples with the explicit syntax.

explicit

57497 files inspected, 1026 offenses detected, 1026 offenses autocorrectable

False positives

Static attributes

Static attributes were deprecated in FactoryBot 4.11 and removed in 5.0.
4.11 is nearly 5 years old, but I don't think it is a reasonable decision to break support for it for several major repos.

/Users/pirj/source/real-world-rspec/sharetribe/spec/factories.rb:535:5: C: [Correctable] RSpec/FactoryBot/AssociationStyle: Use explicit style to define associations.
    version           '1'
    ^^^^^^^^^^^^^^^^^^^^^
/Users/pirj/source/real-world-rspec/sharetribe/spec/factories.rb:536:5: C: [Correctable] RSpec/FactoryBot/AssociationStyle: Use explicit style to define associations.
    content           CustomLandingPage::ExampleData::DATA_STR
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
/Users/pirj/source/real-world-rspec/sharetribe/spec/factories.rb:541:5: C: [Correctable] RSpec/FactoryBot/AssociationStyle: Use explicit style to define associations.
    enabled           true
    ^^^^^^^^^^^^^^^^^^^^^^
/Users/pirj/source/real-world-rspec/sharetribe/spec/factories.rb:545:5: C: [Correctable] RSpec/FactoryBot/AssociationStyle: Use explicit style to define associations.
    listing_id 123
    ^^^^^^^^^^^^^^
/Users/pirj/source/real-world-rspec/sharetribe/spec/factories.rb:546:5: C: [Correctable] RSpec/FactoryBot/AssociationStyle: Use explicit style to define associations.
    blocked_at Date.current
    ^^^^^^^^^^^^^^^^^^^^^^^

What do you think if the cop should only register an offence if the first argument is a hash?
Another option would be to introduce TargetFactoryBotVersion. Wondering if this is something we can do in a minor release, @bquorning @Darhazer ?

Blockpass

/Users/pirj/source/real-world-rspec/diaspora/spec/factories.rb:69:5: C: [Correctable] RSpec/FactoryBot/AssociationStyle: Use explicit style to define associations.
    password_confirmation(&:password)
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

build_association

/Users/pirj/source/real-world-rspec/sharetribe/spec/factories.rb:499:5: C: [Correctable] RSpec/FactoryBot/AssociationStyle: Use explicit style to define associations.
    build_association(:paypal_account)
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

sequence

/Users/pirj/source/real-world-rspec/gitlabhq/spec/factories/terraform/state_version.rb:9:5: C: [Correctable] RSpec/FactoryBot/AssociationStyle: Use explicit style to define associations.
    sequence(:version)
    ^^^^^^^^^^^^^^^^^^

/Users/pirj/source/real-world-rspec/chatwoot/spec/factories/channel/channel_widget.rb:6:5: C: [Correctable] RSpec/FactoryBot/AssociationStyle: Use explicit style to define associat
ions.
    sequence(:widget_color, &:to_s)
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

factory

/Users/pirj/source/real-world-rspec/gitlabhq/spec/factories/keys.rb:22:5: C: [Correctable] RSpec/FactoryBot/AssociationStyle: Use explicit style to define associations.
    factory :deploy_key, class: 'DeployKey'
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
/Users/pirj/source/real-world-rspec/gitlabhq/spec/factories/issues.rb:48:5: C: [Correctable] RSpec/FactoryBot/AssociationStyle: Use explicit style to define associations.
    factory :closed_issue, traits: [:closed]
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

to_create

/Users/pirj/source/real-world-rspec/cartodb/spec/factories/geocodings.rb:4:5: C: [Correctable] RSpec/FactoryBot/AssociationStyle: Use explicit style to define associations.
    to_create(&:save)
    ^^^^^^^^^^^^^^^^^

[pirj]

I can also think of traits_for_enum for completeness. Wondering if there's anything else available to use/override in the context of a factory that we have to consider ignoring.

[r7kamura]

Would the following be a valid FactoryBot syntax?

repo :active, :private
cluster :project, :provided_by_gcp

It seems that the implicit style does not allow such trait usage, so I need to fix #autocorrect_to_implicit_style for this.

# frozen_string_literal: true

require 'bundler/inline'

gemfile do
  source 'https://rubygems.org'

  gem 'factory_bot'
end

FactoryBot.define do
  factory :article do
    trait :active do
      active { true }
    end
  end

  factory :comment do
    article :active
  end
end

$ ruby wrong_association.rb 
/home/r7kamura/.rbenv/versions/3.1.2/lib/ruby/gems/3.1.0/gems/factory_bot-6.2.1/lib/factory_bot/definition_proxy.rb:99:in `method_missing': undefined method 'article' in 'comment' factory (NoMethodError)
Did you mean? 'article { :active }'


        raise NoMethodError.new(<<~MSG)
        ^^^^^
        from wrong_association.rb:19:in `block (2 levels) in <main>'
        from /home/r7kamura/.rbenv/versions/3.1.2/lib/ruby/gems/3.1.0/gems/factory_bot-6.2.1/lib/factory_bot/syntax/default.rb:18:in `instance_eval'
        from /home/r7kamura/.rbenv/versions/3.1.2/lib/ruby/gems/3.1.0/gems/factory_bot-6.2.1/lib/factory_bot/syntax/default.rb:18:in `factory'
        from wrong_association.rb:18:in `block in <main>'
        from /home/r7kamura/.rbenv/versions/3.1.2/lib/ruby/gems/3.1.0/gems/factory_bot-6.2.1/lib/factory_bot/syntax/default.rb:37:in `instance_eval'
        from /home/r7kamura/.rbenv/versions/3.1.2/lib/ruby/gems/3.1.0/gems/factory_bot-6.2.1/lib/factory_bot/syntax/default.rb:37:in `run'
        from /home/r7kamura/.rbenv/versions/3.1.2/lib/ruby/gems/3.1.0/gems/factory_bot-6.2.1/lib/factory_bot/syntax/default.rb:7:in `define'
        from wrong_association.rb:11:in `<main>'

Instead, it can be written by using factory option:

# frozen_string_literal: true

require 'bundler/inline'

gemfile do
  source 'https://rubygems.org'

  gem 'activerecord', '~> 7.0.0'
  gem 'factory_bot'
  gem 'sqlite3'
end

require 'active_record'
require 'logger'
require 'minitest/autorun'

ActiveRecord::Base.establish_connection(
  adapter: 'sqlite3',
  database: ':memory:'
)

ActiveRecord::Schema.define do
  create_table :articles, force: true do |t|
    t.boolean :active, default: false
  end

  create_table :comments, force: true do |t|
    t.references :article
  end
end

class Article < ActiveRecord::Base
end

class Comment < ActiveRecord::Base
  belongs_to :article
end

FactoryBot.define do
  factory :article do
    trait :active do
      active { true }
    end
  end

  factory :comment do
    article factory: [:article, :active]
  end
end

class FactoryTest < Minitest::Test
  def test_association
    assert ::FactoryBot.create(:comment).article.active
  end
end

$ ruby example.rb 
-- create_table(:articles, {:force=>true})
   -> 0.0024s
-- create_table(:comments, {:force=>true})
   -> 0.0003s
Run options: --seed 55593

# Running:

.

Finished in 0.010602s, 94.3227 runs/s, 94.3227 assertions/s.

1 runs, 1 assertions, 0 failures, 0 errors, 0 skips

[pirj]

I might be talking nonsense, but as an alternative to

article factory: [:article, :active]

would it be possible to write it as

article traits: [:active]

Which one would you prefer?

[r7kamura]

I feel you. I actually tried that example before posting the previous comment too, but I couldn't use :traits option on implicit style. In that example, I don't want to write "article" twice, and I too think that ideally :traits should be available here.

Perhaps only :factory option is allowed here:
https://github.com/thoughtbot/factory_bot/blob/ec71611954d07419c9b668be03641332443dcd78/lib/factory_bot/definition_proxy.rb#L252-L254

---

I created the following Pull Request on thoughtbot/factory_bot:

• Support :traits option on implicit association thoughtbot/factory_bot#1552

[pirj]

Does this have to be user configurable?

[r7kamura]

Certainly there is no need to make these default values removable. The main reason why I have this config value is for Global sequences.

Factory.define do
  sequence(:email) { |n| "person#{n}@example.com" }

  factory :user do
    email
  end
end

[pirj]

What do you think of using something that looks like an global sequence attribute call here, e.g. email?

Suggested change

	# # good (defined in NonImplicitAssociationMethodNames)
	# skip_create
	# # good (defined in NonImplicitAssociationMethodNames)
	# email

[r7kamura]

Looks good 👍
Besides, now that skip_create is set by constant instead of config, it is strange that this value is used as an example here.

[pirj]

@r7kamura Would you re-target this to rubocop-factory_bot, please?

[r7kamura]

Recreated on rubocop-factory_bot repo:

• Add FactoryBot/AssociationStyle cop rubocop-factory_bot#33