From 69574d3ad9f622703bde20b7fc57102f497a4b66 Mon Sep 17 00:00:00 2001 From: Peter Leitzen <pleitzen@gitlab.com> Date: Wed, 17 Apr 2024 18:07:37 +0200 Subject: [PATCH] Lint factories per associated model class This commit removes the huge ee/spec/models/factories_spec.rb in favor of linting factories per associated model. Linting is run in a isolated, outer context starting with `Lint factories for <described_class>`. --- .../Geo Replicate a new blob type.md | 2 +- .../style/inline_disable_annotation.yml | 1 - .../testing_guide/best_practices.md | 2 +- ee/spec/models/factories_spec.rb | 241 ------------- scripts/verify-tff-mapping | 12 - .../helpers/user_with_namespace_shim.yml | 1 - .../lint_factories_shared_examples.rb | 323 ++++++++++++++++++ tests.yml | 4 - 8 files changed, 325 insertions(+), 261 deletions(-) delete mode 100644 ee/spec/models/factories_spec.rb create mode 100644 spec/support/shared_examples/lint_factories_shared_examples.rb diff --git a/.gitlab/issue_templates/Geo Replicate a new blob type.md b/.gitlab/issue_templates/Geo Replicate a new blob type.md index 23275d0d5a3cc..0776b14c632d6 100644 --- a/.gitlab/issue_templates/Geo Replicate a new blob type.md +++ b/.gitlab/issue_templates/Geo Replicate a new blob type.md @@ -555,7 +555,7 @@ That's all of the required database changes. end ``` -- [ ] Add `[:cool_widget, :remote_store]` to `skipped` in `spec/models/factories_spec.rb` +- [ ] Add `[:cool_widget, :remote_store]` to `skipped` in `spec/support/shared_examples/lint_factories_shared_examples.rb` #### Step 2. Implement metrics gathering diff --git a/.rubocop_todo/style/inline_disable_annotation.yml b/.rubocop_todo/style/inline_disable_annotation.yml index c9d95369df31b..4caf3eaf31d96 100644 --- a/.rubocop_todo/style/inline_disable_annotation.yml +++ b/.rubocop_todo/style/inline_disable_annotation.yml @@ -1969,7 +1969,6 @@ Style/InlineDisableAnnotation: - 'ee/spec/models/ee/ci/secure_file_spec.rb' - 'ee/spec/models/ee/groups/feature_setting_spec.rb' - 'ee/spec/models/elasticsearch_indexed_namespace_spec.rb' - - 'ee/spec/models/factories_spec.rb' - 'ee/spec/models/gitlab_subscription_spec.rb' - 'ee/spec/models/license_spec.rb' - 'ee/spec/models/members/member_role_spec.rb' diff --git a/doc/development/testing_guide/best_practices.md b/doc/development/testing_guide/best_practices.md index 0156f123a9072..dfddafed9263e 100644 --- a/doc/development/testing_guide/best_practices.md +++ b/doc/development/testing_guide/best_practices.md @@ -1773,7 +1773,7 @@ GitLab uses [`factory_bot`](https://github.com/thoughtbot/factory_bot) as a test - Factories don't have to be limited to `ActiveRecord` objects. [See example](https://gitlab.com/gitlab-org/gitlab-foss/commit/0b8cefd3b2385a21cfed779bd659978c0402766d). -- Factories and their traits should produce valid objects that are [verified by specs](https://gitlab.com/gitlab-org/gitlab/-/blob/master/spec/models/factories_spec.rb). +- Factories and their traits should produce valid objects that are [verified by shared specs](https://gitlab.com/gitlab-org/gitlab/-/blob/master/spec/support/shared_examples/models/factories_shared_examples.rb) run in every model spec. - Avoid the use of [`skip_callback`](https://api.rubyonrails.org/classes/ActiveSupport/Callbacks/ClassMethods.html#method-i-skip_callback) in factories. See [issue #247865](https://gitlab.com/gitlab-org/gitlab/-/issues/247865) for details. diff --git a/ee/spec/models/factories_spec.rb b/ee/spec/models/factories_spec.rb deleted file mode 100644 index acd477b72274f..0000000000000 --- a/ee/spec/models/factories_spec.rb +++ /dev/null @@ -1,241 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -# We only test factories in EE as `FactoryBot.factories` include both FOSS and EE factories there. -# Otherwise, we'd test EE factories even in `rspec` jobs which are supposed to run only FOSS tests. -# It should be fine since FOSS is a subset of EE anyway, so if this test file passes on EE, it means -# FOSS factories are valid (and FOSS doesn't **add** anything to EE so we're good). -# `: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, :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. - # - # Consider adding a code comment if a trait cannot produce a valid object. - 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], - [:dependency_proxy_blob, :remote_store], - [: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, :with_untrusted_root_ca_in_chain], - [:pages_domain, :explicit_ecdsa], - [:pages_domain, :extra_long_key], # used to test key length validation - [:project_member, :blocked], - [:remote_mirror, :ssh], - [:user_preference, :only_comments], - [:ci_pipeline_artifact, :remote_store], - # EE - [:ci_secure_file, :verification_failed], - [:ci_secure_file, :verification_succeeded], - [:container_repository, :remote_store], - [:container_repository, :verification_failed], - [:container_repository, :verification_succeeded], - [:dast_profile, :with_dast_site_validation], - [:dependency_proxy_manifest, :remote_store], - [:dependency_proxy_manifest, :verification_failed], - [:dependency_proxy_manifest, :verification_succeeded], - [:dependency_proxy_blob, :verification_failed], - [:dependency_proxy_blob, :verification_succeeded], - [: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], - [:ee_ci_job_artifact, :verification_failed], - [:ee_ci_job_artifact, :verification_succeeded], - [:lfs_object, :checksum_failure], - [:lfs_object, :checksummed], - [:lfs_object, :verification_failed], - [:lfs_object, :verification_succeeded], - [:merge_request, :blocked], - [:external_merge_request_diff, :verification_failed], - [:external_merge_request_diff, :verification_succeeded], - [:package_file, :verification_failed], - [:package_file, :verification_succeeded], - [:pages_deployment, :verification_failed], - [:pages_deployment, :verification_succeeded], - [:project, :with_vulnerabilities], - [:project, :fork_repository], - [:scan_execution_policy, :with_schedule_and_agent], - [:vulnerability, :with_cluster_image_scanning_finding], - [:vulnerability, :with_findings], - [:vulnerability_export, :finished], - [:member_role, :instance] # this trait is not available for saas - ].freeze - - geo_configured = Gitlab.ee? && Gitlab::Geo.geo_database_configured? - check_create = !Support::GitlabCi.predictive_job? && !Support::GitlabCi.fail_fast_job? - - shared_examples 'factory' do |factory| - skip_any = skipped.include?([factory.name, any]) - - before do - stub_composer_cache_object_storage # [:composer_cache_file, :object_storage] - stub_package_file_object_storage # [:package_file, :object_storage] - debian_component_file_object_storage # [:debian_project_component_file, :object_storage] - debian_distribution_release_file_object_storage # [:debian_project_distribution, :object_storage] - stub_rpm_repository_file_object_storage # [:rpm_repository_file, :object_storage] - end - - describe "#{factory.name} factory" 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', if: check_create 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', if: check_create do - skip 'Trait skipped linting due to legacy error' if skip_trait - - expect { create(factory.name, trait_name) }.not_to raise_error - end - end - end - end - end - - # FactoryDefault speed up specs by creating associations only once - # and reuse them in other factories. - # - # However, for some factories we cannot use FactoryDefault because the - # associations must be unique and cannot be reused, or the factory default - # is being mutated. - skip_factory_defaults = %i[ - ci_catalog_resource_component - ci_catalog_resource_component_usage - ci_catalog_resource_version - ci_job_token_project_scope_link - ci_subscriptions_project - compliance_standards_adherence - evidence - exported_protected_branch - fork_network_member - group_member - import_state - issue_customer_relations_contact - 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 - approval_project_rules_protected_branch - approval_group_rules_protected_branch - protected_tag - protected_tag_create_access_level - release - release_link - shard - users_star_project - vulnerabilities_finding_identifier - wiki_page - wiki_page_meta - workspace - workspace_variable - member_approval - external_status_checks_protected_branch - ].to_set.freeze - - # Some factories and their corresponding models are based on - # database views. In order to use those, we have to swap the - # view out with a table of the same structure. - factories_based_on_view = %i[ - postgres_index - postgres_index_bloat_estimate - postgres_autovacuum_activity - ].to_set.freeze - - 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) - - if Gitlab.jh? - licensed_features.merge! %i[ - dingtalk_integration - feishu_bot_integration - ].index_with(true) - end - - 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 } - let_it_be(:user) { create_default(:user).freeze } - - before do - factories_based_on_view.each do |factory| - view = build(factory).class.table_name - view_gitlab_schema = Gitlab::Database::GitlabSchema.table_schema(view) - Gitlab::Database::EachDatabase.each_connection(include_shared: false) do |connection| - next unless Gitlab::Database.gitlab_schemas_for_connection(connection).include?(view_gitlab_schema) - - swapout_view_for_table(view, connection: connection) - end - end - end - - with_fd.each do |factory| - it_behaves_like 'factory', factory - end - end - - context 'without factory defaults' do - without_fd.each do |factory| - it_behaves_like 'factory', factory - end - end -end diff --git a/scripts/verify-tff-mapping b/scripts/verify-tff-mapping index 9a8f48aa402d4..aedce04d2c556 100755 --- a/scripts/verify-tff-mapping +++ b/scripts/verify-tff-mapping @@ -143,18 +143,6 @@ tests = [ ] }, - { - explanation: 'FOSS factory should map to factories spec', - changed_file: 'spec/factories/users.rb', - expected: ['ee/spec/models/factories_spec.rb'] - }, - - { - explanation: 'EE factory should map to factories spec', - changed_file: 'ee/spec/factories/users.rb', - expected: ['ee/spec/models/factories_spec.rb'] - }, - { explanation: 'Whats New should map to its respective spec', changed_file: 'data/whats_new/202101140001_13_08.yml', diff --git a/spec/support/helpers/user_with_namespace_shim.yml b/spec/support/helpers/user_with_namespace_shim.yml index 5f7f69f9407b1..ca5f78ea5d1e9 100644 --- a/spec/support/helpers/user_with_namespace_shim.yml +++ b/spec/support/helpers/user_with_namespace_shim.yml @@ -175,7 +175,6 @@ - ee/spec/models/ee/project_wiki_spec.rb - ee/spec/models/elastic/migration_record_spec.rb - ee/spec/models/epic_spec.rb -- ee/spec/models/factories_spec.rb - ee/spec/models/gitlab_subscription_spec.rb - ee/spec/models/iteration_spec.rb - ee/spec/models/member_spec.rb diff --git a/spec/support/shared_examples/lint_factories_shared_examples.rb b/spec/support/shared_examples/lint_factories_shared_examples.rb new file mode 100644 index 0000000000000..8714f43169c4a --- /dev/null +++ b/spec/support/shared_examples/lint_factories_shared_examples.rb @@ -0,0 +1,323 @@ +# frozen_string_literal: true + +module Support + # Lint factories based on example group's `described_class`. + module LintFactories + Any = Object.new + + # To avoid factories from being linted multiple times + # we ignore the following example group paths. + IGNORE_EXAMPLE_GROUP_PATHS = [ + # Don't run factory lints in frontend fixtures + 'spec/frontend/fixtures/', + # Skip EE extensions specs as FOSS+EE factories are tested by corresponding models + '/ee/', + # Skip concerns specs + '/concerns/' + ].freeze + + def self.skip_example_group?(example_group) + # ./ee/foo/ee/bar_spec.rb -> ee/foo/ee/bar_spec.rb + file_path = example_group.file_path.delete_prefix('./') + + example_group.described_class.nil? || + IGNORE_EXAMPLE_GROUP_PATHS.any? { |ignore| file_path.include?(ignore) } + end + + def self.lint_factories_for(example_group) + return if skip_example_group?(example_group) + + described_class = example_group.described_class + + without_factory_defaults, with_factory_defaults = factories_for(described_class) + .partition { |factory| skip_factory_defaults?(factory.name) } + + return if without_factory_defaults.empty? && with_factory_defaults.empty? + + RSpec.describe "Lint factories for #{described_class}" do + include_examples 'Lint factories', with_factory_defaults, without_factory_defaults + end + end + + def self.factories_for(model) + factories_by_model[model] || [] + end + + def self.factories_by_model + @factories_by_model ||= + begin + group = FactoryBot.factories.group_by do |factory| + class_name = factory.send(:class_name) + class_name = class_name.to_s.camelize if class_name.is_a?(Symbol) + class_name.constantize if class_name.is_a?(String) + end + group.delete(nil) + group + end + end + + def self.skip?(factory_name, trait_name = nil) + return true if skipped.include?([factory_name, Any]) + return false unless trait_name + + skipped.include?([factory_name, trait_name.to_sym]) + end + + # https://gitlab.com/groups/gitlab-org/-/epics/5464 tracks the remaining + # skipped factories or traits. + # + # Consider adding a code comment if a trait cannot produce a valid object. + def self.skipped + @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], + [:dependency_proxy_blob, :remote_store], + [: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, :with_untrusted_root_ca_in_chain], + [:pages_domain, :explicit_ecdsa], + [:pages_domain, :extra_long_key], # used to test key length validation + [:project_member, :blocked], + [:remote_mirror, :ssh], + [:user_preference, :only_comments], + [:ci_pipeline_artifact, :remote_store], + # EE + [:ci_secure_file, :verification_failed], + [:ci_secure_file, :verification_succeeded], + [:container_repository, :remote_store], + [:container_repository, :verification_failed], + [:container_repository, :verification_succeeded], + [:dast_profile, :with_dast_site_validation], + [:dependency_proxy_manifest, :remote_store], + [:dependency_proxy_manifest, :verification_failed], + [:dependency_proxy_manifest, :verification_succeeded], + [:dependency_proxy_blob, :verification_failed], + [:dependency_proxy_blob, :verification_succeeded], + [: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], + [:ee_ci_job_artifact, :verification_failed], + [:ee_ci_job_artifact, :verification_succeeded], + [:lfs_object, :checksum_failure], + [:lfs_object, :checksummed], + [:lfs_object, :verification_failed], + [:lfs_object, :verification_succeeded], + [:merge_request, :blocked], + [:external_merge_request_diff, :verification_failed], + [:external_merge_request_diff, :verification_succeeded], + [:package_file, :verification_failed], + [:package_file, :verification_succeeded], + [:pages_deployment, :verification_failed], + [:pages_deployment, :verification_succeeded], + [:project, :with_vulnerabilities], + [:project, :fork_repository], + [:scan_execution_policy, :with_schedule_and_agent], + [:vulnerability, :with_cluster_image_scanning_finding], + [:vulnerability, :with_findings], + [:vulnerability_export, :finished], + [:member_role, :instance] # this trait is not available for saas + ].freeze + end + + def self.skip_factory_defaults?(factory_name) + skip_factory_defaults.include?(factory_name) + end + + # FactoryDefault speed up specs by creating associations only once + # and reuse them in other factories. + # + # However, for some factories we cannot use FactoryDefault because the + # associations must be unique and cannot be reused, or the factory default + # is being mutated. + def self.skip_factory_defaults + @skip_factory_defaults ||= %i[ + ci_catalog_resource_component + ci_catalog_resource_component_usage + ci_catalog_resource_version + ci_job_token_project_scope_link + ci_subscriptions_project + compliance_standards_adherence + evidence + exported_protected_branch + fork_network_member + group_member + import_state + issue_customer_relations_contact + 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 + approval_project_rules_protected_branch + approval_group_rules_protected_branch + protected_tag + protected_tag_create_access_level + release + release_link + shard + users_star_project + vulnerabilities_finding_identifier + wiki_page + wiki_page_meta + workspace + workspace_variable + member_approval + external_status_checks_protected_branch + ].to_set.freeze + end + + # Some EE models check licensed features so stub them. + def self.licensed_features + @licensed_features ||= + begin + features = %i[ + board_milestone_lists + board_assignee_lists + ] + + if Gitlab.jh? + features += %i[ + dingtalk_integration + feishu_bot_integration + ] + end + + features.index_with(true).freeze + end + end + + # Some factories and their corresponding models are based on + # database views. In order to use those, we have to swap the + # view out with a table of the same structure. + def self.database_views + @database_views ||= %w[ + postgres_indexes + postgres_index_bloat_estimates + postgres_autovacuum_activity + ].freeze + end + end +end + +RSpec.shared_examples 'Lint factories' do |with_factory_defaults, without_factory_defaults| + shared_context 'with licensed features' do + before do + stub_licensed_features(Support::LintFactories.licensed_features) + end + end + + shared_context 'with database views' do + include Database::DatabaseHelpers + + before do + Support::LintFactories.database_views.each do |view| + view_gitlab_schema = Gitlab::Database::GitlabSchema.table_schema(view) + Gitlab::Database::EachDatabase.each_connection(include_shared: false) do |connection| + next unless Gitlab::Database.gitlab_schemas_for_connection(connection).include?(view_gitlab_schema) + + swapout_view_for_table(view, connection: connection) + end + end + end + end + + shared_context 'with factory defaults' do + let_it_be(:namespace) { create_default(:namespace).freeze } + let_it_be(:project) { create_default(:project, :repository).freeze } + let_it_be(:user) { create_default(:user).freeze } + end + + shared_context 'with stubbed storage' do + before do + stub_composer_cache_object_storage # [:composer_cache_file, :object_storage] + stub_package_file_object_storage # [:package_file, :object_storage] + debian_component_file_object_storage # [:debian_project_component_file, :object_storage] + debian_distribution_release_file_object_storage # [:debian_project_distribution, :object_storage] + stub_rpm_repository_file_object_storage # [:rpm_repository_file, :object_storage] + end + end + + shared_examples 'factory' do |factory| + include_context 'with stubbed storage' + include_context 'with licensed features' if Gitlab.ee? + + 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 Support::LintFactories.skip?(factory.name) + + 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 Support::LintFactories.skip?(factory.name) + + expect { create(factory.name) }.not_to raise_error # rubocop:disable Rails/SaveBang -- It's not Rails + end + + factory.definition.defined_traits.map(&:name).each do |trait_name| + describe "linting :#{trait_name} trait" do + it 'does not raise error when created' do + skip 'Trait skipped linting due to legacy error' if Support::LintFactories.skip?(factory.name, trait_name) + + expect { create(factory.name, trait_name) }.not_to raise_error + end + end + end + end + end + + if with_factory_defaults.any? + context 'with saas, license, and factory defaults', :saas, :with_license, factory_default: :keep do + include_context 'with database views' + include_context 'with factory defaults' + + with_factory_defaults.each do |factory| + it_behaves_like 'factory', factory + end + end + end + + if without_factory_defaults.any? + context 'with saas, license, and no factory defaults', :saas, :with_license do + without_factory_defaults.each do |factory| + it_behaves_like 'factory', factory + end + end + end +end + +RSpec.configure do |config| + config.on_example_group_definition do |example_group| + # Hook into every top-level example group definition. + # + # Define a new isolated context `Lint factories for <described_class>` for + # associated factories. + # + # Creating this context triggers this callback again with <described_class> + # being `nil` so recursive definitions are prevented. + Support::LintFactories.lint_factories_for(example_group) + end +end diff --git a/tests.yml b/tests.yml index d65db6c395344..2abed8a5910f8 100644 --- a/tests.yml +++ b/tests.yml @@ -81,10 +81,6 @@ mapping: - source: 'ee/spec/(?<directory>.*/)ee/(?<rest>.+\.rb)' test: 'spec/%{directory}%{rest}' - # EE/FOSS factory should map to factories spec - - source: '(?<prefix>ee/)?spec/factories/.+\.rb' - test: 'ee/spec/models/factories_spec.rb' - # Whats New should map to its respective spec - source: 'data/whats_new/\w*\.yml' test: 'spec/lib/release_highlights/validator_spec.rb' -- GitLab