From a49fd3c18c46b190fb91f331c4f4bd85c2f6556b Mon Sep 17 00:00:00 2001
From: Peter Leitzen <pleitzen@gitlab.com>
Date: Thu, 4 Apr 2024 11:24:47 +0000
Subject: [PATCH] Revert "Merge branch 'pl-ci-tooling-test-level' into
'master'"
This reverts merge request !148160
---
.gitlab/ci/rails.gitlab-ci.yml | 15 ----
.../rails/rspec-foss-impact.gitlab-ci.yml.erb | 8 --
.../rails/rspec-predictive.gitlab-ci.yml.erb | 18 ----
.gitlab/ci/rules.gitlab-ci.yml | 22 +++--
.rubocop_todo/layout/line_length.yml | 1 +
.../lint/redundant_cop_disable_directive.yml | 1 +
.../style/inline_disable_annotation.yml | 1 +
scripts/generate_rspec_pipeline.rb | 2 +-
spec/tooling/quality/test_level_spec.rb | 90 ++++++++-----------
tooling/quality/test_level.rb | 18 +---
10 files changed, 53 insertions(+), 123 deletions(-)
diff --git a/.gitlab/ci/rails.gitlab-ci.yml b/.gitlab/ci/rails.gitlab-ci.yml
index a2fb4231abed7..1e92858c2b650 100644
--- a/.gitlab/ci/rails.gitlab-ci.yml
+++ b/.gitlab/ci/rails.gitlab-ci.yml
@@ -322,11 +322,6 @@ rspec fast_spec_helper:
# Load fast_spec_helper as well just in case there are no specs available.
- rspec_section bin/rspec --dry-run spec/fast_spec_helper.rb ${fast_spec_helper_specs}
-rspec tooling:
- extends:
- - .rspec-base-pg14
- - .rails:rules:ee-and-foss-tooling
-
rspec unit clickhouse:
extends:
- .rspec-base-pg14-clickhouse23
@@ -411,14 +406,6 @@ rspec:artifact-collector remainder:
- job: rspec background_migration pg14 # 5 jobs
optional: true
-rspec:artifact-collector tooling:
- extends:
- - .artifact-collector
- - .rails:rules:artifact-collector-tooling
- needs:
- - job: rspec tooling # 1 job
- optional: true
-
rspec:artifact-collector unit single-redis:
extends:
- .artifact-collector
@@ -506,8 +493,6 @@ rspec:coverage:
optional: true
- job: rspec:artifact-collector remainder single-redis
optional: true
- - job: rspec:artifact-collector tooling
- optional: true
# as-if-foss jobs
- job: rspec:artifact-collector as-if-foss
optional: true
diff --git a/.gitlab/ci/rails/rspec-foss-impact.gitlab-ci.yml.erb b/.gitlab/ci/rails/rspec-foss-impact.gitlab-ci.yml.erb
index 1fc7dc283833e..e9c66a49f7298 100644
--- a/.gitlab/ci/rails/rspec-foss-impact.gitlab-ci.yml.erb
+++ b/.gitlab/ci/rails/rspec-foss-impact.gitlab-ci.yml.erb
@@ -87,11 +87,3 @@ rspec system foss-impact:
parallel: <%= rspec_files_per_test_level[:system][:parallelization] %>
<% end %>
<% end %>
-
-<% if rspec_files_per_test_level[:tooling][:files].size > 0 %>
-rspec tooling foss-impact:
- extends: .base-rspec-foss-impact
-<% if rspec_files_per_test_level[:tooling][:parallelization] > 1 %>
- parallel: <%= rspec_files_per_test_level[:tooling][:parallelization] %>
-<% end %>
-<% end %>
diff --git a/.gitlab/ci/rails/rspec-predictive.gitlab-ci.yml.erb b/.gitlab/ci/rails/rspec-predictive.gitlab-ci.yml.erb
index b47e7f59163a0..90f1b6fc5a8e1 100644
--- a/.gitlab/ci/rails/rspec-predictive.gitlab-ci.yml.erb
+++ b/.gitlab/ci/rails/rspec-predictive.gitlab-ci.yml.erb
@@ -97,15 +97,6 @@ rspec system predictive:
<% end %>
<% end %>
-<% if rspec_files_per_test_level.dig(:tooling, :files).size > 0 %>
-rspec tooling predictive:
- extends:
- - .base-rspec-predictive
-<% if rspec_files_per_test_level.dig(:tooling, :parallelization) > 1 %>
- parallel: <%= rspec_files_per_test_level.dig(:tooling, :parallelization) %>
-<% end %>
-<% end %>
-
<% end %>
<% if test_suite_prefix == 'ee/' %>
@@ -163,13 +154,4 @@ rspec-ee system predictive:
<% end %>
<% end %>
-<% if rspec_files_per_test_level.dig(:tooling, :files).size > 0 %>
-rspec-ee tooling predictive:
- extends:
- - .base-rspec-ee-predictive
-<% if rspec_files_per_test_level.dig(:tooling, :parallelization) > 1 %>
- parallel: <%= rspec_files_per_test_level.dig(:tooling, :parallelization) %>
-<% end %>
-<% end %>
-
<% end %>
diff --git a/.gitlab/ci/rules.gitlab-ci.yml b/.gitlab/ci/rules.gitlab-ci.yml
index 7e48a1a6395ad..2944dd8afc853 100644
--- a/.gitlab/ci/rules.gitlab-ci.yml
+++ b/.gitlab/ci/rules.gitlab-ci.yml
@@ -603,6 +603,11 @@
- ".gitlab/ci/static-analysis.gitlab-ci.yml"
- "config/feature_categories.yml" # Used by RSpec/FeatureCategory
+.danger-patterns: &danger-patterns
+ - "Dangerfile"
+ - "danger/**/*"
+ - "tooling/danger/**/*"
+
.core-backend-patterns: &core-backend-patterns
- "{,jh/}Gemfile{,.lock}"
- "{,ee/,jh/}config/**/*.rb"
@@ -2053,13 +2058,6 @@
- <<: *if-default-refs
changes: *core-backend-patterns
-.rails:rules:ee-and-foss-tooling:
- rules:
- - <<: *if-fork-merge-request
- when: never
- - <<: *if-merge-request
- - <<: *if-default-refs
-
.rails:rules:code-backstage-qa:
rules:
- <<: *if-default-refs
@@ -2201,11 +2199,6 @@
- !reference ['.rails:rules:ee-and-foss-migration', rules]
- !reference ['.rails:rules:ee-and-foss-background-migration', rules]
-.rails:rules:artifact-collector-tooling:
- rules:
- - if: '$START_AS_IF_FOSS == "true"'
- - !reference ['.rails:rules:ee-and-foss-tooling', rules]
-
.rails:rules:detect-tests:
rules:
- <<: *if-merge-request-labels-run-all-rspec
@@ -2729,6 +2722,11 @@
rules:
- <<: *if-merge-request
+.review:rules:danger-local:
+ rules:
+ - <<: *if-merge-request
+ changes: *danger-patterns
+
###############
# Setup rules #
###############
diff --git a/.rubocop_todo/layout/line_length.yml b/.rubocop_todo/layout/line_length.yml
index eda7a655fc2f9..a028ecf14e11d 100644
--- a/.rubocop_todo/layout/line_length.yml
+++ b/.rubocop_todo/layout/line_length.yml
@@ -4806,3 +4806,4 @@ Layout/LineLength:
- 'tooling/lib/tooling/helm3_client.rb'
- 'tooling/lib/tooling/kubernetes_client.rb'
- 'tooling/merge_request_rspec_failure_rake_task.rb'
+ - 'tooling/quality/test_level.rb'
diff --git a/.rubocop_todo/lint/redundant_cop_disable_directive.yml b/.rubocop_todo/lint/redundant_cop_disable_directive.yml
index 88299a342bd20..020690ef61d4f 100644
--- a/.rubocop_todo/lint/redundant_cop_disable_directive.yml
+++ b/.rubocop_todo/lint/redundant_cop_disable_directive.yml
@@ -287,3 +287,4 @@ Lint/RedundantCopDisableDirective:
- 'tooling/danger/suggestor.rb'
- 'tooling/lib/tooling/helm3_client.rb'
- 'tooling/lib/tooling/kubernetes_client.rb'
+ - 'tooling/quality/test_level.rb'
diff --git a/.rubocop_todo/style/inline_disable_annotation.yml b/.rubocop_todo/style/inline_disable_annotation.yml
index c64e2aa84d549..8fc93cd84b096 100644
--- a/.rubocop_todo/style/inline_disable_annotation.yml
+++ b/.rubocop_todo/style/inline_disable_annotation.yml
@@ -3150,3 +3150,4 @@ Style/InlineDisableAnnotation:
- 'tooling/lib/tooling/helm3_client.rb'
- 'tooling/lib/tooling/job_metrics.rb'
- 'tooling/lib/tooling/kubernetes_client.rb'
+ - 'tooling/quality/test_level.rb'
diff --git a/scripts/generate_rspec_pipeline.rb b/scripts/generate_rspec_pipeline.rb
index d4f6253f15a71..70a0c87be35bd 100755
--- a/scripts/generate_rspec_pipeline.rb
+++ b/scripts/generate_rspec_pipeline.rb
@@ -11,7 +11,7 @@
# Class to generate RSpec test child pipeline with dynamically parallelized jobs.
class GenerateRspecPipeline
SKIP_PIPELINE_YML_FILE = ".gitlab/ci/_skip.yml"
- TEST_LEVELS = %i[migration background_migration unit integration system tooling].freeze
+ TEST_LEVELS = %i[migration background_migration unit integration system].freeze
MAX_NODES_COUNT = 50 # Maximum parallelization allowed by GitLab
OPTIMAL_TEST_JOB_DURATION_IN_SECONDS = 600 # 10 MINUTES
diff --git a/spec/tooling/quality/test_level_spec.rb b/spec/tooling/quality/test_level_spec.rb
index ede375f729876..dfbe5ce283ba5 100644
--- a/spec/tooling/quality/test_level_spec.rb
+++ b/spec/tooling/quality/test_level_spec.rb
@@ -5,8 +5,6 @@
require_relative '../../../tooling/quality/test_level'
RSpec.describe Quality::TestLevel, feature_category: :tooling do
- subject(:test_level) { described_class.new }
-
describe 'TEST_LEVEL_FOLDERS constant' do
it 'ensures all directories it refers to exists', :aggregate_failures do
ee_only_directories = %w[
@@ -33,56 +31,49 @@
describe '#pattern' do
context 'when level is all' do
it 'returns a pattern' do
- expect(test_level.pattern(:all))
+ expect(subject.pattern(:all))
.to eq("spec/**{,/**/}*_spec.rb")
end
end
context 'when level is frontend_fixture' do
it 'returns a pattern' do
- expect(test_level.pattern(:frontend_fixture))
+ expect(subject.pattern(:frontend_fixture))
.to eq("spec/{frontend/fixtures}{,/**/}*.rb")
end
end
context 'when level is unit' do
it 'returns a pattern' do
- expect(test_level.pattern(:unit))
- .to eq("spec/{bin,channels,components,config,contracts,db,dependencies,elastic,elastic_integration,experiments,factories,finders,frontend,graphql,haml_lint,helpers,initializers,keeps,lib,metrics_server,models,policies,presenters,rack_servers,replicators,routing,rubocop,scripts,serializers,services,sidekiq,sidekiq_cluster,spam,support_specs,tasks,uploaders,validators,views,workers}{,/**/}*_spec.rb")
- end
- end
-
- context 'when level is tooling' do
- it 'returns a pattern' do
- expect(test_level.pattern(:tooling))
- .to eq("spec/{dot_gitlab_ci,tooling}{,/**/}*_spec.rb")
+ expect(subject.pattern(:unit))
+ .to eq("spec/{bin,channels,components,config,contracts,db,dependencies,elastic,elastic_integration,experiments,factories,finders,frontend,graphql,haml_lint,helpers,initializers,keeps,lib,metrics_server,models,policies,presenters,rack_servers,replicators,routing,rubocop,scripts,serializers,services,sidekiq,sidekiq_cluster,spam,support_specs,tasks,uploaders,validators,views,workers,tooling,dot_gitlab_ci}{,/**/}*_spec.rb")
end
end
context 'when level is migration' do
it 'returns a pattern' do
- expect(test_level.pattern(:migration))
+ expect(subject.pattern(:migration))
.to eq("spec/{migrations}{,/**/}*_spec.rb")
end
end
context 'when level is background_migration' do
it 'returns a pattern' do
- expect(test_level.pattern(:background_migration))
+ expect(subject.pattern(:background_migration))
.to eq("spec/{lib/gitlab/background_migration,lib/ee/gitlab/background_migration}{,/**/}*_spec.rb")
end
end
context 'when level is integration' do
it 'returns a pattern' do
- expect(test_level.pattern(:integration))
+ expect(subject.pattern(:integration))
.to eq("spec/{commands,controllers,mailers,requests}{,/**/}*_spec.rb")
end
end
context 'when level is system' do
it 'returns a pattern' do
- expect(test_level.pattern(:system))
+ expect(subject.pattern(:system))
.to eq("spec/{features}{,/**/}*_spec.rb")
end
end
@@ -103,11 +94,11 @@
describe 'performance' do
it 'memoizes the pattern for a given level' do
- expect(test_level.pattern(:system).object_id).to eq(test_level.pattern(:system).object_id)
+ expect(subject.pattern(:system).object_id).to eq(subject.pattern(:system).object_id)
end
it 'freezes the pattern for a given level' do
- expect(test_level.pattern(:system)).to be_frozen
+ expect(subject.pattern(:system)).to be_frozen
end
end
end
@@ -115,56 +106,49 @@
describe '#regexp' do
context 'when level is all' do
it 'returns a regexp' do
- expect(test_level.regexp(:all))
+ expect(subject.regexp(:all))
.to eq(%r{spec/})
end
end
context 'when level is frontend_fixture' do
it 'returns a regexp' do
- expect(test_level.regexp(:frontend_fixture))
+ expect(subject.regexp(:frontend_fixture))
.to eq(%r{spec/(frontend/fixtures)/})
end
end
context 'when level is unit' do
it 'returns a regexp' do
- expect(test_level.regexp(:unit))
- .to eq(%r{spec/(bin|channels|components|config|contracts|db|dependencies|elastic|elastic_integration|experiments|factories|finders|frontend|graphql|haml_lint|helpers|initializers|keeps|lib|metrics_server|models|policies|presenters|rack_servers|replicators|routing|rubocop|scripts|serializers|services|sidekiq|sidekiq_cluster|spam|support_specs|tasks|uploaders|validators|views|workers)/})
- end
- end
-
- context 'when level is tooling' do
- it 'returns a regexp' do
- expect(test_level.regexp(:tooling))
- .to eq(%r{spec/(dot_gitlab_ci|tooling)/})
+ expect(subject.regexp(:unit))
+ .to eq(%r{spec/(bin|channels|components|config|contracts|db|dependencies|elastic|elastic_integration|experiments|factories|finders|frontend|graphql|haml_lint|helpers|initializers|keeps|lib|metrics_server|models|policies|presenters|rack_servers|replicators|routing|rubocop|scripts|serializers|services|sidekiq|sidekiq_cluster|spam|support_specs|tasks|uploaders|validators|views|workers|tooling|dot_gitlab_ci)/})
end
end
context 'when level is migration' do
it 'returns a regexp' do
- expect(test_level.regexp(:migration))
+ expect(subject.regexp(:migration))
.to eq(%r{spec/(migrations)/})
end
end
context 'when level is background_migration' do
it 'returns a regexp' do
- expect(test_level.regexp(:background_migration))
+ expect(subject.regexp(:background_migration))
.to eq(%r{spec/(lib/gitlab/background_migration|lib/ee/gitlab/background_migration)/})
end
end
context 'when level is integration' do
it 'returns a regexp' do
- expect(test_level.regexp(:integration))
+ expect(subject.regexp(:integration))
.to eq(%r{spec/(commands|controllers|mailers|requests)/})
end
end
context 'when level is system' do
it 'returns a regexp' do
- expect(test_level.regexp(:system))
+ expect(subject.regexp(:system))
.to eq(%r{spec/(features)/})
end
end
@@ -192,39 +176,38 @@
describe 'performance' do
it 'memoizes the regexp for a given level' do
- expect(test_level.regexp(:system).object_id).to eq(test_level.regexp(:system).object_id)
+ expect(subject.regexp(:system).object_id).to eq(subject.regexp(:system).object_id)
end
it 'freezes the regexp for a given level' do
- expect(test_level.regexp(:system)).to be_frozen
+ expect(subject.regexp(:system)).to be_frozen
end
end
end
describe '#level_for' do
it 'returns the correct level for a unit test' do
- expect(test_level.level_for('spec/models/abuse_report_spec.rb')).to eq(:unit)
+ expect(subject.level_for('spec/models/abuse_report_spec.rb')).to eq(:unit)
end
it 'returns the correct level for a frontend fixture test' do
- expect(test_level.level_for('spec/frontend/fixtures/pipelines.rb')).to eq(:frontend_fixture)
+ expect(subject.level_for('spec/frontend/fixtures/pipelines.rb')).to eq(:frontend_fixture)
end
- it 'returns the correct level for a tooling test', :aggregate_failures do
- expect(test_level.level_for('spec/dot_gitlab_ci/rules_spec.rb')).to eq(:tooling)
- expect(test_level.level_for('spec/tooling/lib/tooling/test_file_finder_spec.rb')).to eq(:tooling)
+ it 'returns the correct level for a tooling test' do
+ expect(subject.level_for('spec/tooling/lib/tooling/test_file_finder_spec.rb')).to eq(:unit)
end
it 'returns the correct level for a migration test' do
- expect(test_level.level_for('spec/migrations/add_default_and_free_plans_spec.rb')).to eq(:migration)
+ expect(subject.level_for('spec/migrations/add_default_and_free_plans_spec.rb')).to eq(:migration)
end
it 'returns the correct level for a background migration test' do
- expect(test_level.level_for('spec/lib/gitlab/background_migration/archive_legacy_traces_spec.rb')).to eq(:background_migration)
+ expect(subject.level_for('spec/lib/gitlab/background_migration/archive_legacy_traces_spec.rb')).to eq(:background_migration)
end
it 'returns the correct level for an EE file without passing a prefix' do
- expect(test_level.level_for('ee/spec/migrations/geo/migrate_ci_job_artifacts_to_separate_registry_spec.rb')).to eq(:migration)
+ expect(subject.level_for('ee/spec/migrations/geo/migrate_ci_job_artifacts_to_separate_registry_spec.rb')).to eq(:migration)
end
it 'returns the correct level for a geo migration test' do
@@ -236,35 +219,32 @@
end
it 'returns the correct level for an integration test' do
- expect(test_level.level_for('spec/mailers/abuse_report_mailer_spec.rb')).to eq(:integration)
+ expect(subject.level_for('spec/mailers/abuse_report_mailer_spec.rb')).to eq(:integration)
end
it 'returns the correct level for an integration test in a subfolder' do
- expect(test_level.level_for('spec/commands/sidekiq_cluster/cli.rb')).to eq(:integration)
+ expect(subject.level_for('spec/commands/sidekiq_cluster/cli.rb')).to eq(:integration)
end
it 'returns the correct level for a system test' do
- expect(test_level.level_for('spec/features/abuse_report_spec.rb')).to eq(:system)
+ expect(subject.level_for('spec/features/abuse_report_spec.rb')).to eq(:system)
end
it 'returns the correct level for a keep test' do
- expect(test_level.level_for('spec/keeps/helpers/postgres_ai_spec.rb')).to eq(:unit)
+ expect(subject.level_for('spec/keeps/helpers/postgres_ai_spec.rb')).to eq(:unit)
end
it 'raises an error for an unknown level' do
- expect { test_level.level_for('spec/unknown/foo_spec.rb') }
- .to raise_error(described_class::UnknownTestLevelError, <<~MESSAGE)
- Test level for spec/unknown/foo_spec.rb couldn't be set.
-
- Please rename the file properly or change the test level detection regexes in tooling/quality/test_level.rb.
- MESSAGE
+ expect { subject.level_for('spec/unknown/foo_spec.rb') }
+ .to raise_error(described_class::UnknownTestLevelError,
+ %r{Test level for spec/unknown/foo_spec.rb couldn't be set. Please rename the file properly or change the test level detection regexes in .+/tooling/quality/test_level.rb.})
end
it 'ensures all spec/ folders are covered by a test level' do
Dir['{,ee/}spec/**/*/'].each do |path|
next if %r{\A(ee/)?spec/(benchmarks|docs_screenshots|fixtures|frontend_integration|support)/}.match?(path)
- expect { test_level.level_for(path) }.not_to raise_error
+ expect { subject.level_for(path) }.not_to raise_error
end
end
end
diff --git a/tooling/quality/test_level.rb b/tooling/quality/test_level.rb
index 62f3dc124c485..f1a76692e3f93 100644
--- a/tooling/quality/test_level.rb
+++ b/tooling/quality/test_level.rb
@@ -55,11 +55,9 @@ class TestLevel
validators
views
workers
- ],
- tooling: %w[
- dot_gitlab_ci
tooling
- ],
+ dot_gitlab_ci
+ ], # ^ tooling and dot_gitlab_ci might be worth to move to another level
integration: %w[
commands
controllers
@@ -78,7 +76,7 @@ def initialize(prefixes = nil)
end
def pattern(level)
- @patterns[level] ||= "#{prefixes_for_pattern}spec/#{folders_pattern(level)}{,/**/}*#{suffix(level)}".freeze
+ @patterns[level] ||= "#{prefixes_for_pattern}spec/#{folders_pattern(level)}{,/**/}*#{suffix(level)}".freeze # rubocop:disable Style/RedundantFreeze
end
def regexp(level, start_with = false)
@@ -99,20 +97,12 @@ def level_for(file_path)
:frontend_fixture
when regexp(:unit)
:unit
- when regexp(:tooling)
- :tooling
when regexp(:integration)
:integration
when regexp(:system)
:system
else
- file = Pathname.new(__FILE__).relative_path_from(File.expand_path("../..", __dir__)).to_s
-
- raise UnknownTestLevelError, <<~MESSAGE
- Test level for #{file_path} couldn't be set.
-
- Please rename the file properly or change the test level detection regexes in #{file}.
- MESSAGE
+ raise UnknownTestLevelError, "Test level for #{file_path} couldn't be set. Please rename the file properly or change the test level detection regexes in #{__FILE__}."
end
end
--
GitLab