From 5a275b7ec4fb8b63ee7de2f96585d9805d6fd568 Mon Sep 17 00:00:00 2001 From: Peter Leitzen <pleitzen@gitlab.com> Date: Fri, 21 Oct 2022 11:55:23 +0000 Subject: [PATCH] Define FactoryBot file paths only once Instead of overriding `definition_file_paths` in multiple places define file paths via application configuration. This allows factories for EE and JH to be loaded also in Rails console if available. --- config/application.rb | 5 + .../development/20_vulnerabilities.rb | 2 - .../fixtures/development/21_dast_profiles.rb | 2 - .../32_compliance_report_violations.rb | 3 - ee/lib/tasks/gitlab/seed/awesome_co.rake | 7 - .../alert_management/alert_payload_fields.rb | 2 + .../ci/reports/license_scanning/dependency.rb | 7 +- .../ci/reports/license_scanning/license.rb | 8 +- .../ci/reports/license_scanning/report.rb | 4 + .../factories/ci/reports/metrics/report.rb | 2 + .../security/locations/container_scanning.rb | 2 +- .../ci/reports/security/locations/dast.rb | 2 +- .../security/locations/dependency_scanning.rb | 2 +- ee/spec/factories/compliance_violations.rb | 1 + .../dast/profile_schedule_input_type.rb | 25 ++-- ee/spec/factories/dependencies.rb | 2 + ee/spec/factories/group_deletion_schedules.rb | 2 +- ee/spec/factories/groups.rb | 2 +- ee/spec/factories/insights.rb | 6 + .../{lfs_object_spec.rb => lfs_object.rb} | 0 .../factories/project_security_settings.rb | 2 +- .../protected_environments/approval_rules.rb | 2 + ee/spec/factories/spdx_catalogue.rb | 2 + ee/spec/factories/spdx_license.rb | 2 + ee/spec/support/factory_bot.rb | 10 -- spec/models/factories_spec.rb | 127 +++++++++++++----- 26 files changed, 153 insertions(+), 78 deletions(-) rename ee/spec/factories/{lfs_object_spec.rb => lfs_object.rb} (100%) delete mode 100644 ee/spec/support/factory_bot.rb diff --git a/config/application.rb b/config/application.rb index 368036ce0649f..a741ca78ac5f8 100644 --- a/config/application.rb +++ b/config/application.rb @@ -472,6 +472,11 @@ class Application < Rails::Application g.factory_bot false end + if defined?(FactoryBotRails) + config.factory_bot.definition_file_paths << 'ee/spec/factories' if Gitlab.ee? + config.factory_bot.definition_file_paths << 'jh/spec/factories' if Gitlab.jh? + end + # sprocket-rails adds some precompile assets we actually do not need. # # It copies all _non_ js and CSS files from the app/assets/ older. diff --git a/ee/db/fixtures/development/20_vulnerabilities.rb b/ee/db/fixtures/development/20_vulnerabilities.rb index c442de3bda95b..d09ae7e616114 100644 --- a/ee/db/fixtures/development/20_vulnerabilities.rb +++ b/ee/db/fixtures/development/20_vulnerabilities.rb @@ -7,8 +7,6 @@ class Gitlab::Seeder::Vulnerabilities def initialize(project) @project = project - FactoryBot.definition_file_paths << Rails.root.join('ee', 'spec', 'factories') - FactoryBot.reload # rubocop:disable Cop/ActiveRecordAssociationReload end def seed! diff --git a/ee/db/fixtures/development/21_dast_profiles.rb b/ee/db/fixtures/development/21_dast_profiles.rb index 3d17b45237287..f3da4a7e730a3 100644 --- a/ee/db/fixtures/development/21_dast_profiles.rb +++ b/ee/db/fixtures/development/21_dast_profiles.rb @@ -5,8 +5,6 @@ class Gitlab::Seeder::DastProfiles def initialize(project) @project = project - FactoryBot.definition_file_paths << Rails.root.join('ee', 'spec', 'factories') - FactoryBot.reload # rubocop:disable Cop/ActiveRecordAssociationReload end def seed! diff --git a/ee/db/fixtures/development/32_compliance_report_violations.rb b/ee/db/fixtures/development/32_compliance_report_violations.rb index e7ac49c452327..a87e4f8e73da1 100644 --- a/ee/db/fixtures/development/32_compliance_report_violations.rb +++ b/ee/db/fixtures/development/32_compliance_report_violations.rb @@ -55,9 +55,6 @@ def create_merge_request(approvers) end Gitlab::Seeder.quiet do - FactoryBot.definition_file_paths << Rails.root.join('ee', 'spec', 'factories') - FactoryBot.reload # rubocop:disable Cop/ActiveRecordAssociationReload - projects = Project .non_archived .with_merge_requests_enabled diff --git a/ee/lib/tasks/gitlab/seed/awesome_co.rake b/ee/lib/tasks/gitlab/seed/awesome_co.rake index a6cf298d7942c..ca65958946b3e 100644 --- a/ee/lib/tasks/gitlab/seed/awesome_co.rake +++ b/ee/lib/tasks/gitlab/seed/awesome_co.rake @@ -16,13 +16,6 @@ namespace :ee do puts "Seeding AwesomeCo demo data for #{namespace.name}" - if Gitlab.ee? - FactoryBot.definition_file_paths = [ - Rails.root.join('ee/spec/factories') - ] - FactoryBot.find_definitions - end - require seed_file AwesomeCo.seed(User.admins.first) diff --git a/ee/spec/factories/alert_management/alert_payload_fields.rb b/ee/spec/factories/alert_management/alert_payload_fields.rb index d7ad3e152c720..f6fd21eeeacb8 100644 --- a/ee/spec/factories/alert_management/alert_payload_fields.rb +++ b/ee/spec/factories/alert_management/alert_payload_fields.rb @@ -6,5 +6,7 @@ path { ['title'] } label { 'Title' } type { 'string' } + + skip_create end end diff --git a/ee/spec/factories/ci/reports/license_scanning/dependency.rb b/ee/spec/factories/ci/reports/license_scanning/dependency.rb index 380ae79b7a02e..b1331f8bdd919 100644 --- a/ee/spec/factories/ci/reports/license_scanning/dependency.rb +++ b/ee/spec/factories/ci/reports/license_scanning/dependency.rb @@ -2,11 +2,16 @@ FactoryBot.define do factory :license_scanning_dependency, class: '::Gitlab::Ci::Reports::LicenseScanning::Dependency' do - initialize_with { new(name, path: path) } + name { 'name' } + path { '.' } trait :rails do name { 'rails' } path { '.' } end + + initialize_with { new(name: name, path: path) } + + skip_create end end diff --git a/ee/spec/factories/ci/reports/license_scanning/license.rb b/ee/spec/factories/ci/reports/license_scanning/license.rb index 8227a46b0ebae..c865d75848f07 100644 --- a/ee/spec/factories/ci/reports/license_scanning/license.rb +++ b/ee/spec/factories/ci/reports/license_scanning/license.rb @@ -2,7 +2,9 @@ FactoryBot.define do factory :license_scanning_license, class: '::Gitlab::Ci::Reports::LicenseScanning::License' do - initialize_with { new(id: id, name: name, url: url) } + id { 'ID' } + name { 'Some License' } + url { '' } trait :mit do id { 'MIT' } @@ -15,5 +17,9 @@ name { 'Unknown' } url { '' } end + + initialize_with { new(id: id, name: name, url: url) } + + skip_create end end diff --git a/ee/spec/factories/ci/reports/license_scanning/report.rb b/ee/spec/factories/ci/reports/license_scanning/report.rb index b2fcf52f732df..445bc09cd335f 100644 --- a/ee/spec/factories/ci/reports/license_scanning/report.rb +++ b/ee/spec/factories/ci/reports/license_scanning/report.rb @@ -5,9 +5,11 @@ trait :version_1 do version { '1.0' } end + trait :version_2 do version { '2.0' } end + trait :report_1 do after(:build) do |report, evaluator| report.add_license(id: 'MIT', name: 'MIT', url: 'https://opensource.org/licenses/mit').add_dependency(name: 'Library1') @@ -27,5 +29,7 @@ report.add_license(id: 'MIT', name: 'MIT', url: 'https://opensource.org/licenses/mit').add_dependency(name: 'rails') end end + + skip_create end end diff --git a/ee/spec/factories/ci/reports/metrics/report.rb b/ee/spec/factories/ci/reports/metrics/report.rb index de75e6f4fce99..f5596fbb7003c 100644 --- a/ee/spec/factories/ci/reports/metrics/report.rb +++ b/ee/spec/factories/ci/reports/metrics/report.rb @@ -15,5 +15,7 @@ report.add_metric('extra_metric_name', 'metric_value') end end + + skip_create end end diff --git a/ee/spec/factories/ci/reports/security/locations/container_scanning.rb b/ee/spec/factories/ci/reports/security/locations/container_scanning.rb index a46bc0407d610..4d2c8a46548c1 100644 --- a/ee/spec/factories/ci/reports/security/locations/container_scanning.rb +++ b/ee/spec/factories/ci/reports/security/locations/container_scanning.rb @@ -10,7 +10,7 @@ skip_create initialize_with do - ::Gitlab::Ci::Reports::Security::Locations::ContainerScanning.new(attributes) + ::Gitlab::Ci::Reports::Security::Locations::ContainerScanning.new(**attributes) end end end diff --git a/ee/spec/factories/ci/reports/security/locations/dast.rb b/ee/spec/factories/ci/reports/security/locations/dast.rb index b3d4032e48bc2..55db1e214d5c6 100644 --- a/ee/spec/factories/ci/reports/security/locations/dast.rb +++ b/ee/spec/factories/ci/reports/security/locations/dast.rb @@ -10,7 +10,7 @@ skip_create initialize_with do - ::Gitlab::Ci::Reports::Security::Locations::Dast.new(attributes) + ::Gitlab::Ci::Reports::Security::Locations::Dast.new(**attributes) end end end diff --git a/ee/spec/factories/ci/reports/security/locations/dependency_scanning.rb b/ee/spec/factories/ci/reports/security/locations/dependency_scanning.rb index b66315c5ad872..9829f8918c782 100644 --- a/ee/spec/factories/ci/reports/security/locations/dependency_scanning.rb +++ b/ee/spec/factories/ci/reports/security/locations/dependency_scanning.rb @@ -9,7 +9,7 @@ skip_create initialize_with do - ::Gitlab::Ci::Reports::Security::Locations::DependencyScanning.new(attributes) + ::Gitlab::Ci::Reports::Security::Locations::DependencyScanning.new(**attributes) end end end diff --git a/ee/spec/factories/compliance_violations.rb b/ee/spec/factories/compliance_violations.rb index 9a20e64940958..c779d05033032 100644 --- a/ee/spec/factories/compliance_violations.rb +++ b/ee/spec/factories/compliance_violations.rb @@ -3,6 +3,7 @@ FactoryBot.define do factory :compliance_violation, class: 'MergeRequests::ComplianceViolation' do violating_user factory: :user + reason { :approved_by_merge_request_author } merge_request trait :approved_by_merge_request_author do diff --git a/ee/spec/factories/dast/profile_schedule_input_type.rb b/ee/spec/factories/dast/profile_schedule_input_type.rb index a74fbb81b0a7a..867d2a9b30584 100644 --- a/ee/spec/factories/dast/profile_schedule_input_type.rb +++ b/ee/spec/factories/dast/profile_schedule_input_type.rb @@ -2,21 +2,22 @@ FactoryBot.define do factory :dast_profile_schedule_input_type, class: 'Types::Dast::ProfileScheduleInputType' do - context = GraphQL::Query::Context.new( - query: GraphQL::Query.new(GitlabSchema, document: nil, context: {}, variables: {}), - values: {}, - object: nil - ) skip_create - arguments = { - active: true, - timezone: ActiveSupport::TimeZone.all.map { |tz| tz.tzinfo.identifier }.sample, - startsAt: Time.now, - cadence: { unit: %w(day month year week).sample, duration: 1 } - } - initialize_with do + context = GraphQL::Query::Context.new( + query: GraphQL::Query.new(GitlabSchema, document: nil, context: {}, variables: {}), + values: {}, + object: nil + ) + + arguments = { + active: true, + timezone: ActiveSupport::TimeZone.all.map { |tz| tz.tzinfo.identifier }.sample, + startsAt: Time.now, + cadence: { unit: %w(day month year week).sample, duration: 1 } + } + ruby_kwargs = arguments.transform_keys { |key| key.to_s.underscore.to_sym } ::Types::Dast::ProfileScheduleInputType.new(ruby_kwargs: ruby_kwargs, defaults_used: [], context: context) diff --git a/ee/spec/factories/dependencies.rb b/ee/spec/factories/dependencies.rb index 3c1917eebd830..aebd90a26f16b 100644 --- a/ee/spec/factories/dependencies.rb +++ b/ee/spec/factories/dependencies.rb @@ -78,5 +78,7 @@ end initialize_with { attributes } + + skip_create end end diff --git a/ee/spec/factories/group_deletion_schedules.rb b/ee/spec/factories/group_deletion_schedules.rb index 69e31b0a05c26..de82ae0c32678 100644 --- a/ee/spec/factories/group_deletion_schedules.rb +++ b/ee/spec/factories/group_deletion_schedules.rb @@ -4,6 +4,6 @@ factory :group_deletion_schedule do association :group, factory: :group association :deleting_user, factory: :user - marked_for_deletion_on { nil } + marked_for_deletion_on { Date.current } end end diff --git a/ee/spec/factories/groups.rb b/ee/spec/factories/groups.rb index de5a5774db831..179c053d7c790 100644 --- a/ee/spec/factories/groups.rb +++ b/ee/spec/factories/groups.rb @@ -67,7 +67,7 @@ factory :group_with_deletion_schedule, parent: :group do transient do deleting_user { association(:user) } - marked_for_deletion_on { nil } + marked_for_deletion_on { Date.current } end after(:create) do |group, evaluator| diff --git a/ee/spec/factories/insights.rb b/ee/spec/factories/insights.rb index 48a58185c9ab9..773ab0ca0639b 100644 --- a/ee/spec/factories/insights.rb +++ b/ee/spec/factories/insights.rb @@ -15,6 +15,8 @@ undefined: 1 }.with_indifferent_access end + + skip_create end factory :insights_merge_requests_per_month, class: 'Hash' do @@ -25,6 +27,8 @@ 'March 2019' => 3 } end + + skip_create end factory :insights_issues_by_team_per_month, class: 'Hash' do @@ -50,5 +54,7 @@ }.with_indifferent_access } end + + skip_create end end diff --git a/ee/spec/factories/lfs_object_spec.rb b/ee/spec/factories/lfs_object.rb similarity index 100% rename from ee/spec/factories/lfs_object_spec.rb rename to ee/spec/factories/lfs_object.rb diff --git a/ee/spec/factories/project_security_settings.rb b/ee/spec/factories/project_security_settings.rb index 765c60ab469a6..6c9068aaffca9 100644 --- a/ee/spec/factories/project_security_settings.rb +++ b/ee/spec/factories/project_security_settings.rb @@ -2,7 +2,7 @@ FactoryBot.define do factory :project_security_setting do - project + project { association :project, security_setting: instance } auto_fix_container_scanning { true } auto_fix_dast { true } auto_fix_dependency_scanning { true } diff --git a/ee/spec/factories/protected_environments/approval_rules.rb b/ee/spec/factories/protected_environments/approval_rules.rb index c4bd680f85b91..dc563b0b9a0a9 100644 --- a/ee/spec/factories/protected_environments/approval_rules.rb +++ b/ee/spec/factories/protected_environments/approval_rules.rb @@ -4,6 +4,8 @@ factory :protected_environment_approval_rule, class: 'ProtectedEnvironments::ApprovalRule' do protected_environment required_approvals { 1 } + # Define association only if access level and group is not null to mimic DB constraint. + user { |e| association(:user) unless e.access_level || e.group || e.group_id } trait :maintainer_access do access_level { Gitlab::Access::MAINTAINER } diff --git a/ee/spec/factories/spdx_catalogue.rb b/ee/spec/factories/spdx_catalogue.rb index 4e63933777736..dac575f87b991 100644 --- a/ee/spec/factories/spdx_catalogue.rb +++ b/ee/spec/factories/spdx_catalogue.rb @@ -6,5 +6,7 @@ content = IO.read(Rails.root.join('spec', 'fixtures', 'spdx.json')) ::Gitlab::SPDX::Catalogue.new(Gitlab::Json.parse(content, symbolize_names: true)) end + + skip_create end end diff --git a/ee/spec/factories/spdx_license.rb b/ee/spec/factories/spdx_license.rb index f507eb116e723..cc03137bbd622 100644 --- a/ee/spec/factories/spdx_license.rb +++ b/ee/spec/factories/spdx_license.rb @@ -31,5 +31,7 @@ id { 'GPL-1.0-only' } name { 'GNU General Public License v1.0 only' } end + + skip_create end end diff --git a/ee/spec/support/factory_bot.rb b/ee/spec/support/factory_bot.rb deleted file mode 100644 index 8278c979abd99..0000000000000 --- a/ee/spec/support/factory_bot.rb +++ /dev/null @@ -1,10 +0,0 @@ -# frozen_string_literal: true - -RSpec.configure do |config| - config.before(:suite) do - FactoryBot.definition_file_paths = [ - Rails.root.join('ee', 'spec', 'factories') - ] - FactoryBot.find_definitions - end -end diff --git a/spec/models/factories_spec.rb b/spec/models/factories_spec.rb index c931c96bafd12..01331b0552f7b 100644 --- a/spec/models/factories_spec.rb +++ b/spec/models/factories_spec.rb @@ -2,59 +2,98 @@ require 'spec_helper' -RSpec.describe 'factories' do +# `:saas` is used to test `gitlab_subscription` factory. +# It's not available on FOSS but also this very factory is not. +RSpec.describe 'factories', :saas 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 traits. + # skipped factories or traits. # # Consider adding a code comment if a trait cannot produce a valid object. - def skipped_traits - [ - [:audit_event, :unauthenticated], - [:ci_build_trace_chunk, :fog_with_data], - [:ci_job_artifact, :remote_store], - [:ci_job_artifact, :raw], - [:ci_job_artifact, :gzip], - [:ci_job_artifact, :correct_checksum], - [:environment, :non_playable], - [:composer_cache_file, :object_storage], - [:debian_project_component_file, :object_storage], - [:debian_project_distribution, :object_storage], - [:debian_file_metadatum, :unknown], - [:issue_customer_relations_contact, :for_contact], - [:issue_customer_relations_contact, :for_issue], - [:package_file, :object_storage], - [:rpm_repository_file, :object_storage], - [: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], - [:remote_mirror, :ssh], - [:user_preference, :only_comments], - [:ci_pipeline_artifact, :remote_store] - ] - end + skipped = [ + [:audit_event, :unauthenticated], + [:ci_build_trace_chunk, :fog_with_data], + [:ci_job_artifact, :remote_store], + [:ci_job_artifact, :raw], + [:ci_job_artifact, :gzip], + [:ci_job_artifact, :correct_checksum], + [:environment, :non_playable], + [:composer_cache_file, :object_storage], + [:debian_project_component_file, :object_storage], + [:debian_project_distribution, :object_storage], + [:debian_file_metadatum, :unknown], + [:issue_customer_relations_contact, :for_contact], + [:issue_customer_relations_contact, :for_issue], + [:package_file, :object_storage], + [:rpm_repository_file, :object_storage], + [: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], + [:remote_mirror, :ssh], + [:user_preference, :only_comments], + [:ci_pipeline_artifact, :remote_store], + # EE + [:dast_profile, :with_dast_site_validation], + [:ee_ci_build, :dependency_scanning_report], + [:ee_ci_build, :license_scan_v1], + [:ee_ci_job_artifact, :v1], + [:ee_ci_job_artifact, :v1_1], + [:ee_ci_job_artifact, :v2], + [:ee_ci_job_artifact, :v2_1], + [:geo_ci_secure_file_state, any], + [:geo_event_log, :geo_event], + [:geo_job_artifact_state, any], + [:geo_lfs_object_state, any], + [:geo_pages_deployment_state, any], + [:geo_upload_state, any], + [:geo_ci_secure_file_state, any], + [:lfs_object, :checksum_failure], + [:lfs_object, :checksummed], + [:merge_request, :blocked], + [: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_export, :finished] + ].freeze shared_examples 'factory' do |factory| + skip_any = skipped.include?([factory.name, any]) + describe "#{factory.name} factory" do 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 + pending 'Factory skipped linting due to legacy error' if skip_any + expect { create(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]) + describe "linting :#{trait_name} trait" do it 'does not raise error when created' do - if skipped_traits.include?([factory.name, trait_name.to_sym]) - pending("Trait skipped linting due to legacy error") - end + pending 'Trait skipped linting due to legacy error' if skip_trait expect { create(factory.name, trait_name) }.not_to raise_error end @@ -71,6 +110,7 @@ def skipped_traits # is being mutated. skip_factory_defaults = %i[ ci_job_token_project_scope_link + ci_subscriptions_project evidence exported_protected_branch fork_network_member @@ -78,22 +118,27 @@ def skipped_traits import_state issue_customer_relations_contact member_task + merge_request_block milestone_release namespace project_namespace project_repository + project_security_setting prometheus_alert prometheus_alert_event prometheus_metric protected_branch protected_branch_merge_access_level protected_branch_push_access_level + protected_branch_unprotect_access_level protected_tag + protected_tag_create_access_level release release_link self_managed_prometheus_alert_event shard users_star_project + vulnerabilities_finding_identifier wiki_page wiki_page_meta ].to_set.freeze @@ -110,6 +155,20 @@ def skipped_traits without_fd, with_fd = FactoryBot.factories .partition { |factory| skip_factory_defaults.include?(factory.name) } + # Some EE models check licensed features so stub them. + shared_context 'with licensed features' do + licensed_features = %i[ + board_milestone_lists + board_assignee_lists + ].index_with(true) + + before do + stub_licensed_features(licensed_features) + end + end + + include_context 'with licensed features' if Gitlab.ee? + context 'with factory defaults', factory_default: :keep do let_it_be(:namespace) { create_default(:namespace).freeze } let_it_be(:project) { create_default(:project, :repository).freeze } -- GitLab