Merge branch 'rc/32308-rspec-retry-hack' into 'master'

Detect and keep track of flaky specs

See merge request !13021

.rubocop.yml

@@ -1045,7 +1045,7 @@ RSpec/BeforeAfterAll:
 RSpec/DescribeClass:
   Enabled: false
 
-# Use `described_class` for tested class / module.
+# Checks that the second argument to `describe` specifies a method.
 RSpec/DescribeMethod:
   Enabled: false
 
@@ -1053,8 +1053,7 @@ RSpec/DescribeMethod:
 RSpec/DescribeSymbol:
   Enabled: true
 
-# Checks that the second argument to top level describe is the tested method
-# name.
+# Checks that tests use `described_class`.
 RSpec/DescribedClass:
   Enabled: true
 

doc/development/testing.md

@@ -157,8 +157,9 @@ trade-off:
 
 - Unit tests are usually cheap, and you should consider them like the basement
   of your house: you need them to be confident that your code is behaving
-  correctly. However if you run only unit tests without integration / system tests, you might [miss] the [big] [picture]!
-- Integration tests are a bit more expensive, but don't abuse them. A feature test
+  correctly. However if you run only unit tests without integration / system
+  tests, you might [miss] the [big] [picture]!
+- Integration tests are a bit more expensive, but don't abuse them. A system test
   is often better than an integration test that is stubbing a lot of internals.
 - System tests are expensive (compared to unit tests), even more if they require
   a JavaScript driver. Make sure to follow the guidelines in the [Speed](#test-speed)
@@ -195,11 +196,27 @@ Please consult the [dedicated "Frontend testing" guide](./fe_guide/testing.md).
 - Try to match the ordering of tests to the ordering within the class.
 - Try to follow the [Four-Phase Test][four-phase-test] pattern, using newlines
   to separate phases.
-- Try to use `Gitlab.config.gitlab.host` rather than hard coding `'localhost'`
+- Use `Gitlab.config.gitlab.host` rather than hard coding `'localhost'`
+- Don't assert against the absolute value of a sequence-generated attribute (see
+  [Gotchas](gotchas.md#dont-assert-against-the-absolute-value-of-a-sequence-generated-attribute)).
+- Don't supply the `:each` argument to hooks since it's the default.
 - On `before` and `after` hooks, prefer it scoped to `:context` over `:all`
 
 [four-phase-test]: https://robots.thoughtbot.com/four-phase-test
 
+### Automatic retries and flaky tests detection
+
+On our CI, we use [rspec-retry] to automatically retry a failing example a few
+times (see [`spec/spec_helper.rb`] for the precise retries count).
+
+We also use a home-made `RspecFlaky::Listener` listener which records flaky
+examples in a JSON report file on `master` (`retrieve-tests-metadata` and `update-tests-metadata` jobs), and warns when a new flaky example
+is detected in any other branch (`flaky-examples-check` job). In the future, the
+`flaky-examples-check` job will not be allowed to fail.
+
+[rspec-retry]: https://github.com/NoRedInk/rspec-retry
+[`spec/spec_helper.rb`]: https://gitlab.com/gitlab-org/gitlab-ce/blob/master/spec/spec_helper.rb
+
 ### `let` variables
 
 GitLab's RSpec suite has made extensive use of `let` variables to reduce

lib/rspec_flaky/example.rb

+module RspecFlaky
+  # This is a wrapper class for RSpec::Core::Example
+  class Example
+    delegate :status, :exception, to: :execution_result
+
+    def initialize(rspec_example)
+      @rspec_example = rspec_example.try(:example) || rspec_example
+    end
+
+    def uid
+      @uid ||= Digest::MD5.hexdigest("#{description}-#{file}")
+    end
+
+    def example_id
+      rspec_example.id
+    end
+
+    def file
+      metadata[:file_path]
+    end
+
+    def line
+      metadata[:line_number]
+    end
+
+    def description
+      metadata[:full_description]
+    end
+
+    def attempts
+      rspec_example.try(:attempts) || 1
+    end
+
+    private
+
+    attr_reader :rspec_example
+
+    def metadata
+      rspec_example.metadata
+    end
+
+    def execution_result
+      rspec_example.execution_result
+    end
+  end
+end

lib/rspec_flaky/flaky_example.rb

+module RspecFlaky
+  # This represents a flaky RSpec example and is mainly meant to be saved in a JSON file
+  class FlakyExample < OpenStruct
+    def initialize(example)
+      if example.respond_to?(:example_id)
+        super(
+          example_id: example.example_id,
+          file: example.file,
+          line: example.line,
+          description: example.description,
+          last_attempts_count: example.attempts,
+          flaky_reports: 1)
+      else
+        super
+      end
+    end
+
+    def first_flaky_at
+      self[:first_flaky_at] || Time.now
+    end
+
+    def last_flaky_at
+      Time.now
+    end
+
+    def last_flaky_job
+      return unless ENV['CI_PROJECT_URL'] && ENV['CI_JOB_ID']
+
+      "#{ENV['CI_PROJECT_URL']}/-/jobs/#{ENV['CI_JOB_ID']}"
+    end
+
+    def to_h
+      super.merge(
+        first_flaky_at: first_flaky_at,
+        last_flaky_at: last_flaky_at,
+        last_flaky_job: last_flaky_job)
+    end
+  end
+end

lib/rspec_flaky/listener.rb

+require 'json'
+
+module RspecFlaky
+  class Listener
+    attr_reader :all_flaky_examples, :new_flaky_examples
+
+    def initialize
+      @new_flaky_examples = {}
+      @all_flaky_examples = init_all_flaky_examples
+    end
+
+    def example_passed(notification)
+      current_example = RspecFlaky::Example.new(notification.example)
+
+      return unless current_example.attempts > 1
+
+      flaky_example_hash = all_flaky_examples[current_example.uid]
+
+      all_flaky_examples[current_example.uid] =
+        if flaky_example_hash
+          FlakyExample.new(flaky_example_hash).tap do |ex|
+            ex.last_attempts_count = current_example.attempts
+            ex.flaky_reports += 1
+          end
+        else
+          FlakyExample.new(current_example).tap do |ex|
+            new_flaky_examples[current_example.uid] = ex
+          end
+        end
+    end
+
+    def dump_summary(_)
+      write_report_file(all_flaky_examples, all_flaky_examples_report_path)
+
+      if new_flaky_examples.any?
+        Rails.logger.warn "\nNew flaky examples detected:\n"
+        Rails.logger.warn JSON.pretty_generate(to_report(new_flaky_examples))
+
+        write_report_file(new_flaky_examples, new_flaky_examples_report_path)
+      end
+    end
+
+    def to_report(examples)
+      Hash[examples.map { |k, ex| [k, ex.to_h] }]
+    end
+
+    private
+
+    def init_all_flaky_examples
+      return {} unless File.exist?(all_flaky_examples_report_path)
+
+      all_flaky_examples = JSON.parse(File.read(all_flaky_examples_report_path))
+
+      Hash[(all_flaky_examples || {}).map { |k, ex| [k, FlakyExample.new(ex)] }]
+    end
+
+    def write_report_file(examples, file_path)
+      return unless ENV['FLAKY_RSPEC_GENERATE_REPORT'] == 'true'
+
+      report_path_dir = File.dirname(file_path)
+      FileUtils.mkdir_p(report_path_dir) unless Dir.exist?(report_path_dir)
+      File.write(file_path, JSON.pretty_generate(to_report(examples)))
+    end
+
+    def all_flaky_examples_report_path
+      @all_flaky_examples_report_path ||= ENV['ALL_FLAKY_RSPEC_REPORT_PATH'] ||
+        Rails.root.join("rspec_flaky/all-report.json")
+    end
+
+    def new_flaky_examples_report_path
+      @new_flaky_examples_report_path ||= ENV['NEW_FLAKY_RSPEC_REPORT_PATH'] ||
+        Rails.root.join("rspec_flaky/new-report.json")
+    end
+  end
+end

scripts/detect-new-flaky-examples

+#!/usr/bin/env ruby
+
+require 'json'
+
+report_file = ARGV.shift
+unless report_file
+  puts 'usage: detect-new-flaky-examples <report-file>'
+  exit 1
+end
+
+puts "Loading #{report_file}..."
+report = JSON.parse(File.read(report_file))
+
+if report.any?
+  puts "New flaky examples were detected!\n"
+  puts JSON.pretty_generate(report)
+  exit 1
+else
+  puts "No new flaky examples detected.\n"
+  exit 0
+end

scripts/merge-reports

@@ -4,7 +4,7 @@ require 'json'
 
 main_report_file = ARGV.shift
 unless main_report_file
-  puts 'usage: merge_reports <main-report> [extra reports...]'
+  puts 'usage: merge-reports <main-report> [extra reports...]'
   exit 1
 end
 

spec/lib/gitlab/health_checks/fs_shards_check_spec.rb

@@ -106,12 +106,6 @@ describe Gitlab::HealthChecks::FsShardsCheck do
           }.with_indifferent_access
         end
 
-        # Unsolved intermittent failure in CI https://gitlab.com/gitlab-org/gitlab-ce/issues/31128
-        around do |example| # rubocop:disable RSpec/AroundBlock
-          times_to_try = ENV['CI'] ? 4 : 1
-          example.run_with_retry retry: times_to_try
-        end
-
         it 'provides metrics' do
           metrics = described_class.metrics
 

spec/lib/rspec_flaky/example_spec.rb

+require 'spec_helper'
+
+describe RspecFlaky::Example do
+  let(:example_attrs) do
+    {
+      id: 'spec/foo/bar_spec.rb:2',
+      metadata: {
+        file_path: 'spec/foo/bar_spec.rb',
+        line_number: 2,
+        full_description: 'hello world'
+      },
+      execution_result: double(status: 'passed', exception: 'BOOM!'),
+      attempts: 1
+    }
+  end
+  let(:rspec_example) { double(example_attrs) }
+
+  describe '#initialize' do
+    shared_examples 'a valid Example instance' do
+      it 'returns valid attributes' do
+        example = described_class.new(args)
+
+        expect(example.example_id).to eq(example_attrs[:id])
+      end
+    end
+
+    context 'when given an Rspec::Core::Example that responds to #example' do
+      let(:args) { double(example: rspec_example) }
+
+      it_behaves_like 'a valid Example instance'
+    end
+
+    context 'when given an Rspec::Core::Example that does not respond to #example' do
+      let(:args) { rspec_example }
+
+      it_behaves_like 'a valid Example instance'
+    end
+  end
+
+  subject { described_class.new(rspec_example) }
+
+  describe '#uid' do
+    it 'returns a hash of the full description' do
+      expect(subject.uid).to eq(Digest::MD5.hexdigest("#{subject.description}-#{subject.file}"))
+    end
+  end
+
+  describe '#example_id' do
+    it 'returns the ID of the RSpec::Core::Example' do
+      expect(subject.example_id).to eq(rspec_example.id)
+    end
+  end
+
+  describe '#attempts' do
+    it 'returns the attempts of the RSpec::Core::Example' do
+      expect(subject.attempts).to eq(rspec_example.attempts)
+    end
+  end
+
+  describe '#file' do
+    it 'returns the metadata[:file_path] of the RSpec::Core::Example' do
+      expect(subject.file).to eq(rspec_example.metadata[:file_path])
+    end
+  end
+
+  describe '#line' do
+    it 'returns the metadata[:line_number] of the RSpec::Core::Example' do
+      expect(subject.line).to eq(rspec_example.metadata[:line_number])
+    end
+  end
+
+  describe '#description' do
+    it 'returns the metadata[:full_description] of the RSpec::Core::Example' do
+      expect(subject.description).to eq(rspec_example.metadata[:full_description])
+    end
+  end
+
+  describe '#status' do
+    it 'returns the execution_result.status of the RSpec::Core::Example' do
+      expect(subject.status).to eq(rspec_example.execution_result.status)
+    end
+  end
+
+  describe '#exception' do
+    it 'returns the execution_result.exception of the RSpec::Core::Example' do
+      expect(subject.exception).to eq(rspec_example.execution_result.exception)
+    end
+  end
+end

spec/lib/rspec_flaky/flaky_example_spec.rb

+require 'spec_helper'
+
+describe RspecFlaky::FlakyExample do
+  let(:flaky_example_attrs) do
+    {
+      example_id: 'spec/foo/bar_spec.rb:2',
+      file: 'spec/foo/bar_spec.rb',
+      line: 2,
+      description: 'hello world',
+      first_flaky_at: 1234,
+      last_flaky_at: 2345,
+      last_attempts_count: 2,
+      flaky_reports: 1
+    }
+  end
+  let(:example_attrs) do
+    {
+      uid: 'abc123',
+      example_id: flaky_example_attrs[:example_id],
+      file: flaky_example_attrs[:file],
+      line: flaky_example_attrs[:line],
+      description: flaky_example_attrs[:description],
+      status: 'passed',
+      exception: 'BOOM!',
+      attempts: flaky_example_attrs[:last_attempts_count]
+    }
+  end
+  let(:example) { double(example_attrs) }
+
+  describe '#initialize' do
+    shared_examples 'a valid FlakyExample instance' do
+      it 'returns valid attributes' do
+        flaky_example = described_class.new(args)
+
+        expect(flaky_example.uid).to eq(flaky_example_attrs[:uid])
+        expect(flaky_example.example_id).to eq(flaky_example_attrs[:example_id])
+      end
+    end
+
+    context 'when given an Rspec::Example' do
+      let(:args) { example }
+
+      it_behaves_like 'a valid FlakyExample instance'
+    end
+
+    context 'when given a hash' do
+      let(:args) { flaky_example_attrs }
+
+      it_behaves_like 'a valid FlakyExample instance'
+    end
+  end
+
+  describe '#to_h' do
+    before do
+      # Stub these env variables otherwise specs don't behave the same on the CI
+      stub_env('CI_PROJECT_URL', nil)
+      stub_env('CI_JOB_ID', nil)
+    end
+
+    shared_examples 'a valid FlakyExample hash' do
+      let(:additional_attrs) { {} }
+
+      it 'returns a valid hash' do
+        flaky_example = described_class.new(args)
+        final_hash = flaky_example_attrs
+          .merge(last_flaky_at: instance_of(Time), last_flaky_job: nil)
+          .merge(additional_attrs)
+
+        expect(flaky_example.to_h).to match(hash_including(final_hash))
+      end
+    end
+
+    context 'when given an Rspec::Example' do
+      let(:args) { example }
+
+      context 'when run locally' do
+        it_behaves_like 'a valid FlakyExample hash' do
+          let(:additional_attrs) do
+            { first_flaky_at: instance_of(Time) }
+          end
+        end
+      end
+
+      context 'when run on the CI' do
+        before do
+          stub_env('CI_PROJECT_URL', 'https://gitlab.com/gitlab-org/gitlab-ce')
+          stub_env('CI_JOB_ID', 42)
+        end
+
+        it_behaves_like 'a valid FlakyExample hash' do
+          let(:additional_attrs) do
+            { first_flaky_at: instance_of(Time), last_flaky_job: "https://gitlab.com/gitlab-org/gitlab-ce/-/jobs/42" }
+          end
+        end
+      end
+    end
+
+    context 'when given a hash' do
+      let(:args) { flaky_example_attrs }
+
+      it_behaves_like 'a valid FlakyExample hash'
+    end
+  end
+end

spec/lib/rspec_flaky/listener_spec.rb

+require 'spec_helper'
+
+describe RspecFlaky::Listener do
+  let(:flaky_example_report) do
+    {
+      'abc123' => {
+        example_id: 'spec/foo/bar_spec.rb:2',
+        file: 'spec/foo/bar_spec.rb',
+        line: 2,
+        description: 'hello world',
+        first_flaky_at: 1234,
+        last_flaky_at: instance_of(Time),
+        last_attempts_count: 2,
+        flaky_reports: 1,
+        last_flaky_job: nil
+      }
+    }
+  end
+  let(:example_attrs) do
+    {
+      id: 'spec/foo/baz_spec.rb:3',
+      metadata: {
+        file_path: 'spec/foo/baz_spec.rb',
+        line_number: 3,
+        full_description: 'hello GitLab'
+      },
+      execution_result: double(status: 'passed', exception: nil)
+    }
+  end
+
+  before do
+    # Stub these env variables otherwise specs don't behave the same on the CI
+    stub_env('CI_PROJECT_URL', nil)
+    stub_env('CI_JOB_ID', nil)
+  end
+
+  describe '#initialize' do
+    shared_examples 'a valid Listener instance' do
+      let(:expected_all_flaky_examples) { {} }
+
+      it 'returns a valid Listener instance' do
+        listener = described_class.new
+
+        expect(listener.to_report(listener.all_flaky_examples))
+          .to match(hash_including(expected_all_flaky_examples))
+        expect(listener.new_flaky_examples).to eq({})
+      end
+    end
+
+    context 'when no report file exists' do
+      it_behaves_like 'a valid Listener instance'
+    end
+
+    context 'when a report file exists and set by ALL_FLAKY_RSPEC_REPORT_PATH' do
+      let(:report_file) do
+        Tempfile.new(%w[rspec_flaky_report .json]).tap do |f|
+          f.write(JSON.pretty_generate(flaky_example_report))
+          f.rewind
+        end
+      end
+
+      before do
+        stub_env('ALL_FLAKY_RSPEC_REPORT_PATH', report_file.path)
+      end
+
+      after do
+        report_file.close
+        report_file.unlink
+      end
+
+      it_behaves_like 'a valid Listener instance' do
+        let(:expected_all_flaky_examples) { flaky_example_report }
+      end
+    end
+  end
+
+  describe '#example_passed' do
+    let(:rspec_example) { double(example_attrs) }
+    let(:notification) { double(example: rspec_example) }
+
+    shared_examples 'a non-flaky example' do
+      it 'does not change the flaky examples hash' do
+        expect { subject.example_passed(notification) }
+          .not_to change { subject.all_flaky_examples }
+      end
+    end
+
+    describe 'when the RSpec example does not respond to attempts' do
+      it_behaves_like 'a non-flaky example'
+    end
+
+    describe 'when the RSpec example has 1 attempt' do
+      let(:rspec_example) { double(example_attrs.merge(attempts: 1)) }
+
+      it_behaves_like 'a non-flaky example'
+    end
+
+    describe 'when the RSpec example has 2 attempts' do
+      let(:rspec_example) { double(example_attrs.merge(attempts: 2)) }
+      let(:expected_new_flaky_example) do
+        {
+          example_id: 'spec/foo/baz_spec.rb:3',
+          file: 'spec/foo/baz_spec.rb',
+          line: 3,
+          description: 'hello GitLab',
+          first_flaky_at: instance_of(Time),
+          last_flaky_at: instance_of(Time),
+          last_attempts_count: 2,
+          flaky_reports: 1,
+          last_flaky_job: nil
+        }
+      end
+
+      it 'does not change the flaky examples hash' do
+        expect { subject.example_passed(notification) }
+          .to change { subject.all_flaky_examples }
+
+        new_example = RspecFlaky::Example.new(rspec_example)
+
+        expect(subject.all_flaky_examples[new_example.uid].to_h)
+          .to match(hash_including(expected_new_flaky_example))
+      end
+    end
+  end
+
+  describe '#dump_summary' do
+    let(:rspec_example) { double(example_attrs) }
+    let(:notification) { double(example: rspec_example) }
+
+    context 'when a report file path is set by ALL_FLAKY_RSPEC_REPORT_PATH' do
+      let(:report_file_path) { Rails.root.join('tmp', 'rspec_flaky_report.json') }
+
+      before do
+        stub_env('ALL_FLAKY_RSPEC_REPORT_PATH', report_file_path)
+        FileUtils.rm(report_file_path) if File.exist?(report_file_path)
+      end
+
+      after do
+        FileUtils.rm(report_file_path) if File.exist?(report_file_path)
+      end
+
+      context 'when FLAKY_RSPEC_GENERATE_REPORT == "false"' do
+        before do
+          stub_env('FLAKY_RSPEC_GENERATE_REPORT', 'false')
+        end
+
+        it 'does not write the report file' do
+          subject.example_passed(notification)
+
+          subject.dump_summary(nil)
+
+          expect(File.exist?(report_file_path)).to be(false)
+        end
+      end
+
+      context 'when FLAKY_RSPEC_GENERATE_REPORT == "true"' do
+        before do
+          stub_env('FLAKY_RSPEC_GENERATE_REPORT', 'true')
+        end
+
+        it 'writes the report file' do
+          subject.example_passed(notification)
+
+          subject.dump_summary(nil)
+
+          expect(File.exist?(report_file_path)).to be(true)
+        end
+      end
+    end
+  end
+
+  describe '#to_report' do
+    it 'transforms the internal hash to a JSON-ready hash' do
+      expect(subject.to_report('abc123' => RspecFlaky::FlakyExample.new(flaky_example_report['abc123'])))
+        .to match(hash_including(flaky_example_report))
+    end
+  end
+end

spec/spec_helper.rb

@@ -69,6 +69,12 @@ RSpec.configure do |config|
 
   config.raise_errors_for_deprecations!
 
+  if ENV['CI']
+    # This includes the first try, i.e. tests will be run 4 times before failing.
+    config.default_retry_count = 4
+    config.reporter.register_listener(RspecFlaky::Listener.new, :example_passed, :dump_summary)
+  end
+
   config.before(:suite) do
     TestEnv.init
   end
@@ -97,12 +103,6 @@ RSpec.configure do |config|
     reset_delivered_emails!
   end
 
-  if ENV['CI']
-    config.around(:each) do |ex|
-      ex.run_with_retry retry: 2
-    end
-  end
-
   config.around(:each, :use_clean_rails_memory_store_caching) do |example|
     caching_store = Rails.cache
     Rails.cache = ActiveSupport::Cache::MemoryStore.new