diff --git a/.rubocop.yml b/.rubocop.yml index bf5e3c0fb567c4ae36667687a0b967cebede96d0..cedf6d2f56e78b22e4ce221f39ec6e8e7ffa1b37 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -483,6 +483,12 @@ RSpec/FactoryBot/AvoidCreate: - 'ee/spec/presenters/**/*.rb' - 'ee/spec/serializers/**/*.rb' +RSpec/FactoryBot/StrategyInCallback: + Enabled: true + Include: + - 'spec/factories/**/*.rb' + - 'ee/spec/factories/**/*.rb' + Cop/IncludeSidekiqWorker: Enabled: true Exclude: diff --git a/.rubocop_todo/rspec/factory_bot/strategy_in_callback.yml b/.rubocop_todo/rspec/factory_bot/strategy_in_callback.yml new file mode 100644 index 0000000000000000000000000000000000000000..456eca578044014295de0c1860fdd914a5219410 --- /dev/null +++ b/.rubocop_todo/rspec/factory_bot/strategy_in_callback.yml @@ -0,0 +1,54 @@ +--- +RSpec/FactoryBot/StrategyInCallback: + Details: grace period + Exclude: + - 'ee/spec/factories/ci/builds.rb' + - 'ee/spec/factories/ci/pipelines.rb' + - 'ee/spec/factories/dast_scanner_profiles.rb' + - 'ee/spec/factories/description_version.rb' + - 'ee/spec/factories/elastic/reindexing_tasks.rb' + - 'ee/spec/factories/epic_issues.rb' + - 'ee/spec/factories/geo/design_registry.rb' + - 'ee/spec/factories/geo_nodes.rb' + - 'ee/spec/factories/groups.rb' + - 'ee/spec/factories/merge_requests.rb' + - 'ee/spec/factories/namespaces.rb' + - 'ee/spec/factories/projects.rb' + - 'ee/spec/factories/protected_environments.rb' + - 'ee/spec/factories/users.rb' + - 'ee/spec/factories/vulnerabilities.rb' + - 'ee/spec/factories/vulnerabilities/external_issue_links.rb' + - 'ee/spec/factories/vulnerabilities/findings.rb' + - 'ee/spec/factories/vulnerabilities/issue_links.rb' + - 'ee/spec/factories/work_items.rb' + - 'spec/factories/alert_management/alerts.rb' + - 'spec/factories/ci/builds.rb' + - 'spec/factories/ci/pipelines.rb' + - 'spec/factories/ci/processable.rb' + - 'spec/factories/ci/runner_namespaces.rb' + - 'spec/factories/ci/runner_projects.rb' + - 'spec/factories/ci/runners.rb' + - 'spec/factories/clusters/clusters.rb' + - 'spec/factories/commits.rb' + - 'spec/factories/container_expiration_policies.rb' + - 'spec/factories/design_management/designs.rb' + - 'spec/factories/design_management/versions.rb' + - 'spec/factories/environments.rb' + - 'spec/factories/group_members.rb' + - 'spec/factories/groups.rb' + - 'spec/factories/integrations.rb' + - 'spec/factories/issues.rb' + - 'spec/factories/merge_requests.rb' + - 'spec/factories/ml/candidates.rb' + - 'spec/factories/namespaces.rb' + - 'spec/factories/packages/dependency_links.rb' + - 'spec/factories/packages/package_files.rb' + - 'spec/factories/packages/packages.rb' + - 'spec/factories/pool_repositories.rb' + - 'spec/factories/project_members.rb' + - 'spec/factories/projects.rb' + - 'spec/factories/releases.rb' + - 'spec/factories/resource_label_events.rb' + - 'spec/factories/terraform/state.rb' + - 'spec/factories/users.rb' + - 'spec/factories/work_items/parent_links.rb' diff --git a/doc/development/testing_guide/best_practices.md b/doc/development/testing_guide/best_practices.md index 61f60567418d83460433a2f6dfc1b2a978caa94b..58f6b02c1f1c70c2dac78935bda31a3a7d649be7 100644 --- a/doc/development/testing_guide/best_practices.md +++ b/doc/development/testing_guide/best_practices.md @@ -1440,9 +1440,10 @@ GitLab uses [factory_bot](https://github.com/thoughtbot/factory_bot) as a test f resulting record to pass validation. - When instantiating from a factory, don't supply attributes that aren't required by the test. -- Prefer [implicit](https://github.com/thoughtbot/factory_bot/blob/master/GETTING_STARTED.md#implicit-definition) - or [explicit](https://github.com/thoughtbot/factory_bot/blob/master/GETTING_STARTED.md#explicit-definition) - association definitions instead of using `create` / `build` for association setup. +- Prefer [implicit](https://github.com/thoughtbot/factory_bot/blob/master/GETTING_STARTED.md#implicit-definition), + [explicit](https://github.com/thoughtbot/factory_bot/blob/master/GETTING_STARTED.md#explicit-definition), or + [inline](https://github.com/thoughtbot/factory_bot/blob/master/GETTING_STARTED.md#inline-definition) associations + over `create` / `build` for association setup in callbacks. See [issue #262624](https://gitlab.com/gitlab-org/gitlab/-/issues/262624) for further context. - Factories don't have to be limited to `ActiveRecord` objects. [See example](https://gitlab.com/gitlab-org/gitlab-foss/commit/0b8cefd3b2385a21cfed779bd659978c0402766d). diff --git a/rubocop/cop/rspec/factory_bot/strategy_in_callback.rb b/rubocop/cop/rspec/factory_bot/strategy_in_callback.rb new file mode 100644 index 0000000000000000000000000000000000000000..3153d54887ec9abe9ad5477aa35fbef66969c6bf --- /dev/null +++ b/rubocop/cop/rspec/factory_bot/strategy_in_callback.rb @@ -0,0 +1,45 @@ +# frozen_string_literal: true + +require 'rubocop-rspec' + +module RuboCop + module Cop + module RSpec + module FactoryBot + class StrategyInCallback < RuboCop::Cop::Base + include RuboCop::RSpec::FactoryBot::Language + + MSG = 'Prefer inline `association` over `%{type}`. ' \ + 'See https://docs.gitlab.com/ee/development/testing_guide/best_practices.html#factories' + + FORBIDDEN_METHODS = %i[build build_list build_stubbed build_stubbed_list create create_list].freeze + + def_node_matcher :forbidden_factory_usage, <<~PATTERN + (block + (send nil? { :after :before } (sym _strategy)) + _args + ` # in all descandents + (send + { nil? #factory_bot? } + ${ #{FORBIDDEN_METHODS.map(&:inspect).join(' ')} } + (sym _factory_name) + ... + ) + ) + PATTERN + + RESTRICT_ON_SEND = FORBIDDEN_METHODS + + def on_send(node) + parent = node.each_ancestor(:block).first + + strategy = forbidden_factory_usage(parent) + return unless strategy + + add_offense(node, message: format(MSG, type: strategy)) + end + end + end + end + end +end diff --git a/spec/rubocop/cop/rspec/factory_bot/strategy_in_callback_spec.rb b/spec/rubocop/cop/rspec/factory_bot/strategy_in_callback_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..9dcfebc7c1dae78adecd78574e2c4958dd4e88b8 --- /dev/null +++ b/spec/rubocop/cop/rspec/factory_bot/strategy_in_callback_spec.rb @@ -0,0 +1,71 @@ +# frozen_string_literal: true + +require 'rubocop_spec_helper' + +require_relative '../../../../../rubocop/cop/rspec/factory_bot/strategy_in_callback' + +RSpec.describe RuboCop::Cop::RSpec::FactoryBot::StrategyInCallback do + shared_examples 'an offensive factory call' do |namespace| + described_class::FORBIDDEN_METHODS.each do |forbidden_method| + namespaced_forbidden_method = "#{namespace}#{forbidden_method}(:ci_job_artifact, :archive)" + + it "registers an offence for multiple #{namespaced_forbidden_method} calls" do + expect_offense(<<-RUBY) + FactoryBot.define do + factory :ci_build, class: 'Ci::Build', parent: :ci_processable do + trait :artifacts do + before(:create) do + #{namespaced_forbidden_method} + #{'^' * namespaced_forbidden_method.size} Prefer inline `association` over `#{forbidden_method}`. See https://docs.gitlab.com/ee/development/testing_guide/best_practices.html#factories + end + + after(:create) do |build| + #{namespaced_forbidden_method} + #{'^' * namespaced_forbidden_method.size} Prefer inline `association` over `#{forbidden_method}`. See https://docs.gitlab.com/ee/development/testing_guide/best_practices.html#factories + #{namespaced_forbidden_method} + #{'^' * namespaced_forbidden_method.size} Prefer inline `association` over `#{forbidden_method}`. See https://docs.gitlab.com/ee/development/testing_guide/best_practices.html#factories + end + end + end + end + RUBY + end + + it "registers an offense for #{namespaced_forbidden_method} when is a send node" do + expect_offense(<<-RUBY) + FactoryBot.define do + factory :ci_build, class: 'Ci::Build', parent: :ci_processable do + trait :artifacts do + after(:create) do |build| + #{namespaced_forbidden_method} + #{'^' * namespaced_forbidden_method.size} Prefer inline `association` over `#{forbidden_method}`. See https://docs.gitlab.com/ee/development/testing_guide/best_practices.html#factories + end + end + end + end + RUBY + end + + it "registers an offense for #{namespaced_forbidden_method} when is assigned" do + expect_offense(<<-RUBY) + FactoryBot.define do + factory :ci_build, class: 'Ci::Build', parent: :ci_processable do + trait :artifacts do + after(:create) do |build| + ci_build = #{namespaced_forbidden_method} + #{'^' * namespaced_forbidden_method.size} Prefer inline `association` over `#{forbidden_method}`. See https://docs.gitlab.com/ee/development/testing_guide/best_practices.html#factories + + ci_build + end + end + end + end + RUBY + end + end + end + + it_behaves_like 'an offensive factory call', '' + it_behaves_like 'an offensive factory call', 'FactoryBot.' + it_behaves_like 'an offensive factory call', '::FactoryBot.' +end