From 05839126bd2d0fd3e7cb847a48334aa97ba2b169 Mon Sep 17 00:00:00 2001
From: Peter Leitzen <pleitzen@gitlab.com>
Date: Fri, 14 Jul 2023 14:15:48 +0200
Subject: [PATCH] Ensure that all factories work with `build_stubbed`

Use `build` explicitly in factories with `skip_create` where inline
associations won't work.
---
 ee/spec/models/factories_spec.rb | 94 +++++++++++++++++++-------------
 spec/factories/gitaly/commit.rb  |  4 +-
 spec/factories/gitaly/tag.rb     |  2 +-
 spec/factories/wiki_pages.rb     |  2 +-
 4 files changed, 59 insertions(+), 43 deletions(-)

diff --git a/ee/spec/models/factories_spec.rb b/ee/spec/models/factories_spec.rb
index 43e56c3d1ce168a6..bc5e023b4e436ebb 100644
--- a/ee/spec/models/factories_spec.rb
+++ b/ee/spec/models/factories_spec.rb
@@ -11,17 +11,27 @@
 RSpec.describe 'factories', :saas, :with_license, feature_category: :tooling do
   include Database::DatabaseHelpers
 
-  # Used in `skipped` and indicates whether to skip any traits including the
-  # plain factory.
-  any = Object.new
-
   # https://gitlab.com/groups/gitlab-org/-/epics/5464 tracks the remaining
-  # skipped factories or traits.
+  # skipped factories, traits, and/or factory strategies (build_stubbed, build,
+  # or create).
   #
   # Consider adding a code comment if a trait cannot produce a valid object.
+  #
+  # Example:
+  #
+  #   # Skip factory for all traits and any strategy.
+  #   [:factory_name]
+  #
+  #   # Skip factory only for defined traits and any strategy.
+  #   [:factory_name, :trait_name]
+  #   [:factory_name, :another_trait]
+  #
+  #   # Skip factory only for this trait and defined strategies
+  #   [:factory_name, :trait_name, :build]
+  #   [:factory_name, :trait_name, :create]
   skipped = [
     [:audit_event, :unauthenticated],
-    [:ci_build_trace_chunk, :fog_with_data],
+    [:ci_build_trace_chunk, :fog_with_data, :create],
     [:ci_job_artifact, :remote_store],
     [:ci_job_artifact, :raw],
     [:ci_job_artifact, :gzip],
@@ -30,21 +40,26 @@
     [:environment, :non_playable],
     [:issue_customer_relations_contact, :for_contact],
     [:issue_customer_relations_contact, :for_issue],
-    [:pages_domain, :without_certificate],
-    [:pages_domain, :without_key],
-    [:pages_domain, :with_missing_chain],
-    [:pages_domain, :with_trusted_chain],
-    [:pages_domain, :with_trusted_expired_chain],
-    [:pages_domain, :explicit_ecdsa],
-    [:project_member, :blocked],
+    [:pages_domain, :without_certificate, :create],
+    [:pages_domain, :without_key, :create],
+    [:pages_domain, :with_missing_chain, :create],
+    [:pages_domain, :with_trusted_chain, :create],
+    [:pages_domain, :with_trusted_expired_chain, :create],
+    [:pages_domain, :explicit_ecdsa, :create],
+    [:project_member, :blocked, :create],
     [:remote_mirror, :ssh],
     [:user_preference, :only_comments],
     [:ci_pipeline_artifact, :remote_store],
+    [:wiki_page_meta, :for_wiki_page, :build],
+    [:wiki_page_meta, :for_wiki_page, :create],
+    [:suggestion, :content_from_repo, :build],
     # EE
     [:dast_profile, :with_dast_site_validation],
     [:dependency_proxy_manifest, :remote_store],
-    [:ee_ci_build, :dependency_scanning_report],
-    [:ee_ci_build, :license_scan_v1],
+    [:ee_ci_build, :dependency_scanning_report, :build],
+    [:ee_ci_build, :dependency_scanning_report, :create],
+    [:ee_ci_build, :license_scan_v1, :build],
+    [:ee_ci_build, :license_scan_v1, :create],
     [:ee_ci_job_artifact, :v1],
     [:ee_ci_job_artifact, :v1_1],
     [:ee_ci_job_artifact, :v2],
@@ -52,23 +67,23 @@
     [:geo_event_log, :geo_event],
     [:lfs_object, :checksum_failure],
     [:lfs_object, :checksummed],
-    [:merge_request, :blocked],
+    [:merge_request, :blocked, :create],
     [:merge_request_diff, :verification_failed],
     [:merge_request_diff, :verification_succeeded],
     [:package_file, :verification_failed],
     [:package_file, :verification_succeeded],
     [:project, :with_vulnerabilities],
     [:scan_execution_policy, :with_schedule_and_agent],
-    [:vulnerability, :with_cluster_image_scanning_finding],
-    [:vulnerability, :with_findings],
+    [:vulnerability, :with_cluster_image_scanning_finding, :create],
+    [:vulnerability, :with_findings, :create],
     [:vulnerability_export, :finished],
     [:container_repository, :remote_store]
   ].freeze
 
   geo_configured = Gitlab.ee? && Gitlab::Geo.geo_database_configured?
 
-  shared_examples 'factory' do |factory|
-    skip_any = skipped.include?([factory.name, any])
+  shared_examples 'factory' do |factory, strategy|
+    skip_any = skipped.include?([factory.name])
 
     before do
       stub_composer_cache_object_storage # [:composer_cache_file, :object_storage]
@@ -78,33 +93,27 @@
       stub_rpm_repository_file_object_storage # [:rpm_repository_file, :object_storage]
     end
 
-    describe "#{factory.name} factory" do
+    describe "#{strategy}(:#{factory.name})" do
       before(:context) do
         skip 'Geo DB is not set up' if factory.name.start_with?('geo') && !geo_configured
       end
 
-      it 'does not raise error when built' do
-        # We use `skip` here because using `build` mostly work even if
-        # factories break when creating them.
-        skip 'Factory skipped linting due to legacy error' if skip_any
-
-        expect { build(factory.name) }.not_to raise_error
-      end
-
-      it 'does not raise error when created' do
+      it 'does not raise error' do
         pending 'Factory skipped linting due to legacy error' if skip_any
 
-        expect { create(factory.name) }.not_to raise_error # rubocop:disable Rails/SaveBang
+        expect { public_send(strategy, factory.name) }.not_to raise_error # rubocop:disable Rails/SaveBang
       end
 
-      factory.definition.defined_traits.map(&:name).each do |trait_name|
-        skip_trait = skip_any || skipped.include?([factory.name, trait_name.to_sym])
+      factory.definition.defined_traits.each do |trait|
+        skip_trait = skip_any ||
+          skipped.include?([factory.name, trait.name.to_sym]) ||
+          skipped.include?([factory.name, trait.name.to_sym, strategy])
 
-        describe "linting :#{trait_name} trait" do
-          it 'does not raise error when created' do
+        context "and :#{trait.name} trait" do
+          it 'does not raise error' do
             pending 'Trait skipped linting due to legacy error' if skip_trait
 
-            expect { create(factory.name, trait_name) }.not_to raise_error
+            expect { public_send(strategy, factory.name, trait.name) }.not_to raise_error
           end
         end
       end
@@ -164,7 +173,9 @@
     postgres_autovacuum_activity
   ].to_set.freeze
 
-  without_fd, with_fd = FactoryBot.factories
+  all_factories = FactoryBot.factories
+
+  without_fd, with_fd = all_factories
     .partition { |factory| skip_factory_defaults.include?(factory.name) }
 
   # Some EE models check licensed features so stub them.
@@ -206,13 +217,18 @@
     end
 
     with_fd.each do |factory|
-      it_behaves_like 'factory', factory
+      it_behaves_like 'factory', factory, :create
     end
   end
 
   context 'without factory defaults' do
     without_fd.each do |factory|
-      it_behaves_like 'factory', factory
+      it_behaves_like 'factory', factory, :create
     end
   end
+
+  all_factories.each do |factory|
+    it_behaves_like 'factory', factory, :build_stubbed
+    it_behaves_like 'factory', factory, :build
+  end
 end
diff --git a/spec/factories/gitaly/commit.rb b/spec/factories/gitaly/commit.rb
index 4e8220e449a58df6..04ed7181ac617deb 100644
--- a/spec/factories/gitaly/commit.rb
+++ b/spec/factories/gitaly/commit.rb
@@ -14,7 +14,7 @@
     subject { "My commit" }
 
     body { subject + "\nMy body" }
-    author { association(:gitaly_commit_author) }
-    committer { association(:gitaly_commit_author) }
+    author { build(:gitaly_commit_author) } # rubocop:disable RSpec/FactoryBot/InlineAssociation
+    committer { build(:gitaly_commit_author) } # rubocop:disable RSpec/FactoryBot/InlineAssociation
   end
 end
diff --git a/spec/factories/gitaly/tag.rb b/spec/factories/gitaly/tag.rb
index 9dd1b8301c1bdca8..10ace4fb41962ea5 100644
--- a/spec/factories/gitaly/tag.rb
+++ b/spec/factories/gitaly/tag.rb
@@ -6,6 +6,6 @@
 
     name { 'v3.1.4' }
     message { 'Pie release' }
-    target_commit factory: :gitaly_commit
+    target_commit { build(:gitaly_commit) } # rubocop:disable RSpec/FactoryBot/InlineAssociation
   end
 end
diff --git a/spec/factories/wiki_pages.rb b/spec/factories/wiki_pages.rb
index 093a2e9148f7680f..d64d24d2b024fc68 100644
--- a/spec/factories/wiki_pages.rb
+++ b/spec/factories/wiki_pages.rb
@@ -10,7 +10,7 @@
       project { association(:project) }
       container { project }
       wiki { association(:wiki, container: container) }
-      page { ActiveSupport::InheritableOptions.new(url_path: title) }
+      page { ActiveSupport::InheritableOptions.new(path: title, url_path: title) }
     end
 
     initialize_with do
-- 
GitLab