Merge branch 'retry-failing-tests-2' into 'master'

ci: Retry failed specs in a new process after the initial run

See merge request gitlab-org/gitlab!76839

.gitignore

@@ -79,6 +79,7 @@ eslint-report.html
 /deprecations/
 /knapsack/
 /rspec_flaky/
+/rspec/
 /locale/**/LC_MESSAGES
 /locale/**/*.time_stamp
 /.rspec

.gitlab-ci.yml

@@ -69,10 +69,14 @@ variables:
   GET_SOURCES_ATTEMPTS: "3"
 
   KNAPSACK_RSPEC_SUITE_REPORT_PATH: knapsack/report-master.json
-  FLAKY_RSPEC_SUITE_REPORT_PATH: rspec_flaky/report-suite.json
+  FLAKY_RSPEC_SUITE_REPORT_PATH: rspec/flaky/report-suite.json
   RSPEC_TESTS_MAPPING_PATH: crystalball/mapping.json
   RSPEC_PACKED_TESTS_MAPPING_PATH: crystalball/packed-mapping.json
+  RSPEC_PROFILING_FOLDER_PATH: rspec/profiling
   FRONTEND_FIXTURES_MAPPING_PATH: crystalball/frontend_fixtures_mapping.json
+  RSPEC_LAST_RUN_RESULTS_FILE: rspec/rspec_last_run_results.txt
+  JUNIT_RESULT_FILE: rspec/junit_rspec.xml
+  JUNIT_RETRY_FILE: rspec/junit_rspec-retry.xml
 
   ES_JAVA_OPTS: "-Xms256m -Xmx256m"
   ELASTIC_URL: "http://elastic:changeme@elasticsearch:9200"

.gitlab/ci/rails.gitlab-ci.yml

@@ -29,7 +29,9 @@
     GITLAB_USE_MODEL_LOAD_BALANCING: "true"
 
 .rspec-base:
-  extends: .rails-job-base
+  extends:
+    - .rails-job-base
+    - .base-artifacts
   stage: test
   variables:
     RUBY_GC_MALLOC_LIMIT: 67108864
@@ -40,6 +42,8 @@
   script:
     - !reference [.base-script, script]
     - rspec_paralellized_job "--tag ~quarantine --tag ~geo --tag ~level:migration"
+
+.base-artifacts:
   artifacts:
     expire_in: 31d
     when: always
@@ -48,16 +52,17 @@
       - crystalball/
       - deprecations/
       - knapsack/
-      - rspec_flaky/
-      - rspec_profiling/
+      - rspec/
       - tmp/capybara/
       - tmp/memory_test/
       - log/*.log
     reports:
-      junit: junit_rspec.xml
+      junit: ${JUNIT_RESULT_FILE}
 
 .rspec-base-migration:
-  extends: .rails:rules:ee-and-foss-migration
+  extends:
+    - .base-artifacts
+    - .rails:rules:ee-and-foss-migration
   script:
     - !reference [.base-script, script]
     - rspec_paralellized_job "--tag ~quarantine --tag ~geo --tag level:migration"
@@ -617,54 +622,26 @@ rspec:feature-flags:
         run_timed_command "bundle exec scripts/used-feature-flags";
       fi
 
-rspec:skipped-flaky-tests-report:
+rspec:flaky-tests-report:
   extends:
     - .default-retry
-    - .rails:rules:skipped-flaky-tests-report
-  image: ruby:2.7-alpine
+    - .rails:rules:flaky-tests-report
   stage: post-test
   # We cannot use needs since it would mean needing 84 jobs (since most are parallelized)
   # so we use `dependencies` here.
-  dependencies:
-    # FOSS/EE jobs
-    - rspec migration pg12
-    - rspec unit pg12
-    - rspec integration pg12
-    - rspec system pg12
-    # FOSS/EE minimal jobs
-    - rspec migration pg12 minimal
-    - rspec unit pg12 minimal
-    - rspec integration pg12 minimal
-    - rspec system pg12 minimal
-    # EE jobs
-    - rspec-ee migration pg12
-    - rspec-ee unit pg12
-    - rspec-ee integration pg12
-    - rspec-ee system pg12
-    # EE minimal jobs
-    - rspec-ee migration pg12 minimal
-    - rspec-ee unit pg12 minimal
-    - rspec-ee integration pg12 minimal
-    - rspec-ee system pg12 minimal
-    # Geo jobs
-    - rspec-ee unit pg12 geo
-    - rspec-ee integration pg12 geo
-    - rspec-ee system pg12 geo
-    # Geo minimal jobs
-    - rspec-ee unit pg12 geo minimal
-    - rspec-ee integration pg12 geo minimal
-    - rspec-ee system pg12 geo minimal
+  dependencies: !reference ["rspec:coverage", "dependencies"]
   variables:
-    SKIPPED_FLAKY_TESTS_REPORT: skipped_flaky_tests_report.txt
+    SKIPPED_FLAKY_TESTS_REPORT_PATH: rspec/flaky/skipped_flaky_tests_report.txt
+    RETRIED_TESTS_REPORT_PATH: rspec/flaky/retried_tests_report.txt
   before_script:
-    - 'echo "SKIP_FLAKY_TESTS_AUTOMATICALLY: $SKIP_FLAKY_TESTS_AUTOMATICALLY"'
-    - mkdir -p rspec_flaky
+    - source scripts/utils.sh
+    - source scripts/rspec_helpers.sh
   script:
-    - find rspec_flaky/ -type f -name 'skipped_flaky_tests_*_report.txt' -exec cat {} + >> "${SKIPPED_FLAKY_TESTS_REPORT}"
+    - generate_flaky_tests_reports
   artifacts:
     expire_in: 31d
     paths:
-      - ${SKIPPED_FLAKY_TESTS_REPORT}
+      - rspec/
 
 # EE/FOSS: default refs (MRs, default branch, schedules) jobs #
 #######################################################

.gitlab/ci/rules.gitlab-ci.yml

@@ -121,9 +121,6 @@
 .if-security-pipeline-merge-result: &if-security-pipeline-merge-result
   if: '$CI_PIPELINE_SOURCE == "merge_request_event" && $CI_MERGE_REQUEST_TARGET_BRANCH_NAME == $CI_DEFAULT_BRANCH && $CI_PROJECT_NAMESPACE == "gitlab-org/security" && $GITLAB_USER_LOGIN == "gitlab-release-tools-bot"'
 
-.if-skip-flaky-tests-automatically: &if-skip-flaky-tests-automatically
-  if: '$SKIP_FLAKY_TESTS_AUTOMATICALLY == "true"'
-
 ####################
 # Changes patterns #
 ####################
@@ -1436,13 +1433,16 @@
       when: never
     - changes: *code-backstage-patterns
 
-.rails:rules:skipped-flaky-tests-report:
+.rails:rules:flaky-tests-report:
   rules:
     - <<: *if-not-ee
       when: never
-    - <<: *if-skip-flaky-tests-automatically
+    - if: '$SKIP_FLAKY_TESTS_AUTOMATICALLY == "true" || $RETRY_FAILED_TESTS_IN_NEW_PROCESS == "true"'
       changes: *code-backstage-patterns
-    - changes: *ci-patterns
+      when: always
+    - if: '$SKIP_FLAKY_TESTS_AUTOMATICALLY == "true" || $RETRY_FAILED_TESTS_IN_NEW_PROCESS == "true"'
+      changes: *ci-patterns
+      when: always
 
 #########################
 # Static analysis rules #

.gitlab/ci/test-metadata.gitlab-ci.yml

@@ -6,8 +6,7 @@
     expire_in: 31d
     paths:
       - knapsack/
-      - rspec_flaky/
-      - rspec_profiling/
+      - rspec/
       - crystalball/
 
 retrieve-tests-metadata:
@@ -44,6 +43,6 @@ update-tests-metadata:
   script:
     - run_timed_command "retry gem install fog-aws mime-types activesupport rspec_profiling postgres-copy --no-document"
     - source ./scripts/rspec_helpers.sh
-    - test -f rspec_flaky/report-suite.json || echo -e "\e[31m" 'Consider add ~"pipeline:run-all-rspec" to run full rspec jobs' "\e[0m"
+    - test -f "${FLAKY_RSPEC_SUITE_REPORT_PATH}" || echo -e "\e[31m" 'Consider add ~"pipeline:run-all-rspec" to run full rspec jobs' "\e[0m"
     - update_tests_metadata
     - update_tests_mapping

config/initializers/rspec_profiling.rb

@@ -62,7 +62,7 @@ RspecProfiling.configure do |config|
     config.collector = RspecProfilingExt::Collectors::CSVWithTimestamps
     config.csv_path = -> do
       prefix = "#{ENV['CI_JOB_NAME']}-".gsub(%r{[ /]}, '-') if ENV['CI_JOB_NAME']
-      "rspec_profiling/#{prefix}#{Time.now.to_i}-#{SecureRandom.hex(8)}-rspec-data.csv"
+      "#{ENV['RSPEC_PROFILING_FOLDER_PATH']}/#{prefix}#{Time.now.to_i}-#{SecureRandom.hex(8)}-rspec-data.csv"
     end
   end
 end

doc/development/pipelines.md

@@ -170,10 +170,19 @@ After that, the next pipeline uses the up-to-date `knapsack/report-master.json`
 
 ### Flaky tests
 
+#### Automatic skipping of flaky tests
+
 Tests that are [known to be flaky](testing_guide/flaky_tests.md#automatic-retries-and-flaky-tests-detection) are
 skipped unless the `$SKIP_FLAKY_TESTS_AUTOMATICALLY` variable is set to `false` or if the `~"pipeline:run-flaky-tests"`
 label is set on the MR.
 
+#### Automatic retry of failing tests in a separate process
+
+When the `$RETRY_FAILED_TESTS_IN_NEW_PROCESS` variable is set to `true`, RSpec tests that failed are automatically retried once in a separate
+RSpec process. The goal is to get rid of most side-effects from previous tests that may lead to a subsequent test failure.
+
+We keep track of retried tests in the `$RETRIED_TESTS_REPORT_FILE` file saved as artifact by the `rspec:flaky-tests-report` job.
+
 ### Monitoring
 
 The GitLab test suite is [monitored](performance.md#rspec-profiling) for the `main` branch, and any branch

scripts/insert-rspec-profiling-data

@@ -43,4 +43,4 @@ def insert_data(path)
   end
 end
 
-insert_data('rspec_profiling') if ENV['RSPEC_PROFILING_POSTGRES_URL'].present?
+insert_data(ENV['RSPEC_PROFILING_FOLDER_PATH']) if ENV['RSPEC_PROFILING_POSTGRES_URL'].present?

scripts/utils.sh

@@ -64,16 +64,20 @@ function setup_db() {
 }
 
 function install_api_client_dependencies_with_apk() {
-  apk add --update openssl curl jq
+  run_timed_command "apk add --update openssl curl jq"
 }
 
 function install_gitlab_gem() {
-  gem install httparty --no-document --version 0.18.1
-  gem install gitlab --no-document --version 4.17.0
+  run_timed_command "gem install httparty --no-document --version 0.18.1"
+  run_timed_command "gem install gitlab --no-document --version 4.17.0"
 }
 
 function install_tff_gem() {
-  gem install test_file_finder --version 0.1.1
+  run_timed_command "gem install test_file_finder --no-document --version 0.1.1"
+}
+
+function install_junit_merge_gem() {
+  run_timed_command "gem install junit_merge --no-document --version 0.1.2"
 }
 
 function run_timed_command() {

spec/spec_helper.rb

@@ -112,11 +112,11 @@ RSpec.configure do |config|
     # falling back to all tests when there is no `:focus` example.
     config.filter_run focus: true
     config.run_all_when_everything_filtered = true
-
-    # Re-run failures locally with `--only-failures`
-    config.example_status_persistence_file_path = './spec/examples.txt'
   end
 
+  # Re-run failures locally with `--only-failures`
+  config.example_status_persistence_file_path = ENV.fetch('RSPEC_LAST_RUN_RESULTS_FILE', './spec/examples.txt')
+
   config.define_derived_metadata(file_path: %r{(ee)?/spec/.+_spec\.rb\z}) do |metadata|
     location = metadata[:location]
 
@@ -208,7 +208,9 @@ RSpec.configure do |config|
     config.exceptions_to_hard_fail = [DeprecationToolkitEnv::DeprecationBehaviors::SelectiveRaise::RaiseDisallowedDeprecation]
   end
 
-  if ENV['FLAKY_RSPEC_GENERATE_REPORT']
+  require_relative '../tooling/rspec_flaky/config'
+
+  if RspecFlaky::Config.generate_report?
     require_relative '../tooling/rspec_flaky/listener'
 
     config.reporter.register_listener(

spec/support/flaky_tests.rb

@@ -4,14 +4,14 @@ return unless ENV['CI']
 return if ENV['SKIP_FLAKY_TESTS_AUTOMATICALLY'] == "false"
 return if ENV['CI_MERGE_REQUEST_LABELS'].to_s.include?('pipeline:run-flaky-tests')
 
+require_relative '../../tooling/rspec_flaky/config'
 require_relative '../../tooling/rspec_flaky/report'
 
 RSpec.configure do |config|
   $flaky_test_example_ids = begin # rubocop:disable Style/GlobalVars
-    raise "$SUITE_FLAKY_RSPEC_REPORT_PATH is empty." if ENV['SUITE_FLAKY_RSPEC_REPORT_PATH'].to_s.empty?
-    raise "#{ENV['SUITE_FLAKY_RSPEC_REPORT_PATH']} doesn't exist" unless File.exist?(ENV['SUITE_FLAKY_RSPEC_REPORT_PATH'])
+    raise "#{RspecFlaky::Config.suite_flaky_examples_report_path} doesn't exist" unless File.exist?(RspecFlaky::Config.suite_flaky_examples_report_path)
 
-    RspecFlaky::Report.load(ENV['SUITE_FLAKY_RSPEC_REPORT_PATH']).map { |_, flaky_test_data| flaky_test_data.to_h[:example_id] }
+    RspecFlaky::Report.load(RspecFlaky::Config.suite_flaky_examples_report_path).map { |_, flaky_test_data| flaky_test_data.to_h[:example_id] }
   rescue => e # rubocop:disable Style/RescueStandardError
     puts e
     []
@@ -29,8 +29,9 @@ RSpec.configure do |config|
   end
 
   config.after(:suite) do
-    next unless ENV['SKIPPED_FLAKY_TESTS_REPORT_PATH']
+    next unless RspecFlaky::Config.skipped_flaky_tests_report_path
+    next if $skipped_flaky_tests_report.empty? # rubocop:disable Style/GlobalVars
 
-    File.write(ENV['SKIPPED_FLAKY_TESTS_REPORT_PATH'], "#{$skipped_flaky_tests_report.join("\n")}\n") # rubocop:disable Style/GlobalVars
+    File.write(RspecFlaky::Config.skipped_flaky_tests_report_path, "#{ENV['CI_JOB_URL']}\n#{$skipped_flaky_tests_report.join("\n")}\n\n") # rubocop:disable Style/GlobalVars
   end
 end

spec/tooling/lib/tooling/parallel_rspec_runner_spec.rb

@@ -18,7 +18,9 @@ RSpec.describe Tooling::ParallelRSpecRunner do # rubocop:disable RSpec/FilePath
       allow(File).to receive(:exist?).with(filter_tests_file).and_return(true)
       allow(File).to receive(:read).and_call_original
       allow(File).to receive(:read).with(filter_tests_file).and_return(filter_tests)
-      allow(subject).to receive(:exec)
+      allow(Process).to receive(:spawn)
+      allow(Process).to receive(:wait)
+      allow(Process).to receive(:last_status).and_return(double(exitstatus: 0))
     end
 
     subject { described_class.new(allocator: allocator, filter_tests_file: filter_tests_file, rspec_args: rspec_args) }
@@ -86,7 +88,7 @@ RSpec.describe Tooling::ParallelRSpecRunner do # rubocop:disable RSpec/FilePath
     end
 
     def expect_command(cmd)
-      expect(subject).to receive(:exec).with(*cmd)
+      expect(Process).to receive(:spawn).with(*cmd)
     end
   end
 end

spec/tooling/rspec_flaky/config_spec.rb

@@ -11,9 +11,10 @@ RSpec.describe RspecFlaky::Config, :aggregate_failures do
   before do
     # Stub these env variables otherwise specs don't behave the same on the CI
     stub_env('FLAKY_RSPEC_GENERATE_REPORT', nil)
-    stub_env('SUITE_FLAKY_RSPEC_REPORT_PATH', nil)
+    stub_env('FLAKY_RSPEC_SUITE_REPORT_PATH', nil)
     stub_env('FLAKY_RSPEC_REPORT_PATH', nil)
     stub_env('NEW_FLAKY_RSPEC_REPORT_PATH', nil)
+    stub_env('SKIPPED_FLAKY_TESTS_REPORT_PATH', nil)
     # Ensure the behavior is the same locally and on CI (where Rails is defined since we run this test as part of the whole suite), i.e. Rails isn't defined
     allow(described_class).to receive(:rails_path).and_wrap_original do |method, path|
       path
@@ -51,15 +52,15 @@ RSpec.describe RspecFlaky::Config, :aggregate_failures do
   end
 
   describe '.suite_flaky_examples_report_path' do
-    context "when ENV['SUITE_FLAKY_RSPEC_REPORT_PATH'] is not set" do
+    context "when ENV['FLAKY_RSPEC_SUITE_REPORT_PATH'] is not set" do
       it 'returns the default path' do
-        expect(described_class.suite_flaky_examples_report_path).to eq('rspec_flaky/suite-report.json')
+        expect(described_class.suite_flaky_examples_report_path).to eq('rspec/flaky/suite-report.json')
       end
     end
 
-    context "when ENV['SUITE_FLAKY_RSPEC_REPORT_PATH'] is set" do
+    context "when ENV['FLAKY_RSPEC_SUITE_REPORT_PATH'] is set" do
       before do
-        stub_env('SUITE_FLAKY_RSPEC_REPORT_PATH', 'foo/suite-report.json')
+        stub_env('FLAKY_RSPEC_SUITE_REPORT_PATH', 'foo/suite-report.json')
       end
 
       it 'returns the value of the env variable' do
@@ -71,7 +72,7 @@ RSpec.describe RspecFlaky::Config, :aggregate_failures do
   describe '.flaky_examples_report_path' do
     context "when ENV['FLAKY_RSPEC_REPORT_PATH'] is not set" do
       it 'returns the default path' do
-        expect(described_class.flaky_examples_report_path).to eq('rspec_flaky/report.json')
+        expect(described_class.flaky_examples_report_path).to eq('rspec/flaky/report.json')
       end
     end
 
@@ -89,7 +90,7 @@ RSpec.describe RspecFlaky::Config, :aggregate_failures do
   describe '.new_flaky_examples_report_path' do
     context "when ENV['NEW_FLAKY_RSPEC_REPORT_PATH'] is not set" do
       it 'returns the default path' do
-        expect(described_class.new_flaky_examples_report_path).to eq('rspec_flaky/new-report.json')
+        expect(described_class.new_flaky_examples_report_path).to eq('rspec/flaky/new-report.json')
       end
     end
 
@@ -103,4 +104,22 @@ RSpec.describe RspecFlaky::Config, :aggregate_failures do
       end
     end
   end
+
+  describe '.skipped_flaky_tests_report_path' do
+    context "when ENV['SKIPPED_FLAKY_TESTS_REPORT_PATH'] is not set" do
+      it 'returns the default path' do
+        expect(described_class.skipped_flaky_tests_report_path).to eq('rspec/flaky/skipped_flaky_tests_report.txt')
+      end
+    end
+
+    context "when ENV['SKIPPED_FLAKY_TESTS_REPORT_PATH'] is set" do
+      before do
+        stub_env('SKIPPED_FLAKY_TESTS_REPORT_PATH', 'foo/skipped_flaky_tests_report.txt')
+      end
+
+      it 'returns the value of the env variable' do
+        expect(described_class.skipped_flaky_tests_report_path).to eq('foo/skipped_flaky_tests_report.txt')
+      end
+    end
+  end
 end

spec/tooling/rspec_flaky/listener_spec.rb

@@ -54,7 +54,7 @@ RSpec.describe RspecFlaky::Listener, :aggregate_failures do
   before do
     # Stub these env variables otherwise specs don't behave the same on the CI
     stub_env('CI_JOB_URL', nil)
-    stub_env('SUITE_FLAKY_RSPEC_REPORT_PATH', nil)
+    stub_env('FLAKY_RSPEC_SUITE_REPORT_PATH', nil)
   end
 
   describe '#initialize' do
@@ -73,11 +73,11 @@ RSpec.describe RspecFlaky::Listener, :aggregate_failures do
       it_behaves_like 'a valid Listener instance'
     end
 
-    context 'when SUITE_FLAKY_RSPEC_REPORT_PATH is set' do
+    context 'when FLAKY_RSPEC_SUITE_REPORT_PATH is set' do
       let(:report_file_path) { 'foo/report.json' }
 
       before do
-        stub_env('SUITE_FLAKY_RSPEC_REPORT_PATH', report_file_path)
+        stub_env('FLAKY_RSPEC_SUITE_REPORT_PATH', report_file_path)
       end
 
       context 'and report file exists' do

tooling/bin/parallel_rspec

@@ -16,4 +16,4 @@ OptionParser.new do |opts|
   end
 end.parse!
 
-Tooling::ParallelRSpecRunner.run(**options)
+exit Tooling::ParallelRSpecRunner.run(**options)

tooling/lib/tooling/parallel_rspec_runner.rb

@@ -38,12 +38,8 @@ module Tooling
       Knapsack.logger.info tests_to_run
       Knapsack.logger.info
 
-      if tests_to_run.empty?
-        Knapsack.logger.info 'No tests to run on this node, exiting.'
-        return
-      end
-
-      exec(*rspec_command)
+      Process.wait Process.spawn(*rspec_command)
+      Process.last_status.exitstatus
     end
 
     private

tooling/rspec_flaky/config.rb

@@ -7,15 +7,19 @@ module RspecFlaky
     end
 
     def self.suite_flaky_examples_report_path
-      ENV['SUITE_FLAKY_RSPEC_REPORT_PATH'] || rails_path("rspec_flaky/suite-report.json")
+      ENV['FLAKY_RSPEC_SUITE_REPORT_PATH'] || rails_path("rspec/flaky/suite-report.json")
     end
 
     def self.flaky_examples_report_path
-      ENV['FLAKY_RSPEC_REPORT_PATH'] || rails_path("rspec_flaky/report.json")
+      ENV['FLAKY_RSPEC_REPORT_PATH'] || rails_path("rspec/flaky/report.json")
     end
 
     def self.new_flaky_examples_report_path
-      ENV['NEW_FLAKY_RSPEC_REPORT_PATH'] || rails_path("rspec_flaky/new-report.json")
+      ENV['NEW_FLAKY_RSPEC_REPORT_PATH'] || rails_path("rspec/flaky/new-report.json")
+    end
+
+    def self.skipped_flaky_tests_report_path
+      ENV['SKIPPED_FLAKY_TESTS_REPORT_PATH'] || rails_path("rspec/flaky/skipped_flaky_tests_report.txt")
     end
 
     def self.rails_path(path)