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