Merge branch '37721-clean-outdated-flaky-specs-from-the-flaky-specs-report' into 'master'

Resolve "Clean outdated flaky specs from the flaky specs report"

Closes #37721

See merge request gitlab-org/gitlab-ce!18196

.gitlab-ci.yml

@@ -368,6 +368,7 @@ update-tests-metadata:
     - scripts/merge-reports ${KNAPSACK_RSPEC_SUITE_REPORT_PATH} knapsack/${CI_PROJECT_NAME}/rspec-pg_node_*.json
     - scripts/merge-reports ${KNAPSACK_SPINACH_SUITE_REPORT_PATH} knapsack/${CI_PROJECT_NAME}/spinach-pg_node_*.json
     - scripts/merge-reports ${FLAKY_RSPEC_SUITE_REPORT_PATH} rspec_flaky/all_*_*.json
+    - scripts/prune-old-flaky-specs ${FLAKY_RSPEC_SUITE_REPORT_PATH}
     - '[[ -z ${TESTS_METADATA_S3_BUCKET} ]] || scripts/sync-reports put $TESTS_METADATA_S3_BUCKET $KNAPSACK_RSPEC_SUITE_REPORT_PATH $KNAPSACK_SPINACH_SUITE_REPORT_PATH'
     - '[[ -z ${TESTS_METADATA_S3_BUCKET} ]] || scripts/sync-reports put $TESTS_METADATA_S3_BUCKET $FLAKY_RSPEC_SUITE_REPORT_PATH'
     - rm -f knapsack/${CI_PROJECT_NAME}/*_node_*.json

lib/rspec_flaky/config.rb

-require 'json'
-
 module RspecFlaky
   class Config
     def self.generate_report?
-      ENV['FLAKY_RSPEC_GENERATE_REPORT'] == 'true'
+      !!(ENV['FLAKY_RSPEC_GENERATE_REPORT'] =~ /1|true/)
     end
     def self.suite_flaky_examples_report_path

lib/rspec_flaky/flaky_examples_collection.rb

-require 'json'
+require 'active_support/hash_with_indifferent_access'
+
+require_relative 'flaky_example'
 module RspecFlaky
   class FlakyExamplesCollection < SimpleDelegator
-    def self.from_json(json)
-      new(JSON.parse(json))
-    end
-
     def initialize(collection = {})
       unless collection.is_a?(Hash)
         raise ArgumentError, "`collection` must be a Hash, #{collection.class} given!"
@@ -22,7 +20,7 @@ module RspecFlaky
       super(Hash[collection_of_flaky_examples])
     end
-    def to_report
+    def to_h
       Hash[map { |uid, example| [uid, example.to_h] }].deep_symbolize_keys
     end

lib/rspec_flaky/listener.rb

 require 'json'
+require_relative 'config'
+require_relative 'example'
+require_relative 'flaky_example'
+require_relative 'flaky_examples_collection'
+require_relative 'report'
+
 module RspecFlaky
   class Listener
     # - suite_flaky_examples: contains all the currently tracked flacky example
@@ -9,7 +15,7 @@ module RspecFlaky
     attr_reader :suite_flaky_examples, :flaky_examples
     def initialize(suite_flaky_examples_json = nil)
-      @flaky_examples = FlakyExamplesCollection.new
+      @flaky_examples = RspecFlaky::FlakyExamplesCollection.new
       @suite_flaky_examples = init_suite_flaky_examples(suite_flaky_examples_json)
     end
@@ -18,47 +24,36 @@ module RspecFlaky
       return unless current_example.attempts > 1
-      flaky_example = suite_flaky_examples.fetch(current_example.uid) { FlakyExample.new(current_example) }
+      flaky_example = suite_flaky_examples.fetch(current_example.uid) { RspecFlaky::FlakyExample.new(current_example) }
       flaky_example.update_flakiness!(last_attempts_count: current_example.attempts)
       flaky_examples[current_example.uid] = flaky_example
     end
     def dump_summary(_)
-      write_report_file(flaky_examples, RspecFlaky::Config.flaky_examples_report_path)
+      RspecFlaky::Report.new(flaky_examples).write(RspecFlaky::Config.flaky_examples_report_path)
+      # write_report_file(flaky_examples, RspecFlaky::Config.flaky_examples_report_path)
       new_flaky_examples = flaky_examples - suite_flaky_examples
       if new_flaky_examples.any?
         Rails.logger.warn "\nNew flaky examples detected:\n"
-        Rails.logger.warn JSON.pretty_generate(new_flaky_examples.to_report)
-        write_report_file(new_flaky_examples, RspecFlaky::Config.new_flaky_examples_report_path)
+        Rails.logger.warn JSON.pretty_generate(new_flaky_examples.to_h)
+        RspecFlaky::Report.new(new_flaky_examples).write(RspecFlaky::Config.new_flaky_examples_report_path)
+        # write_report_file(new_flaky_examples, RspecFlaky::Config.new_flaky_examples_report_path)
       end
     end
-    def to_report(examples)
-      Hash[examples.map { |k, ex| [k, ex.to_h] }]
-    end
-
     private
     def init_suite_flaky_examples(suite_flaky_examples_json = nil)
-      unless suite_flaky_examples_json
+      if suite_flaky_examples_json
+        RspecFlaky::Report.load_json(suite_flaky_examples_json).flaky_examples
+      else
         return {} unless File.exist?(RspecFlaky::Config.suite_flaky_examples_report_path)
-        suite_flaky_examples_json = File.read(RspecFlaky::Config.suite_flaky_examples_report_path)
+        RspecFlaky::Report.load(RspecFlaky::Config.suite_flaky_examples_report_path).flaky_examples
       end
-
-      FlakyExamplesCollection.from_json(suite_flaky_examples_json)
-    end
-
-    def write_report_file(examples_collection, file_path)
-      return unless RspecFlaky::Config.generate_report?
-
-      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(examples_collection.to_report))
     end
   end
 end

lib/rspec_flaky/report.rb

+require 'json'
+require 'time'
+
+require_relative 'config'
+require_relative 'flaky_examples_collection'
+
+module RspecFlaky
+  # This class is responsible for loading/saving JSON reports, and pruning
+  # outdated examples.
+  class Report < SimpleDelegator
+    OUTDATED_DAYS_THRESHOLD = 90
+
+    attr_reader :flaky_examples
+
+    def self.load(file_path)
+      load_json(File.read(file_path))
+    end
+
+    def self.load_json(json)
+      new(RspecFlaky::FlakyExamplesCollection.new(JSON.parse(json)))
+    end
+
+    def initialize(flaky_examples)
+      unless flaky_examples.is_a?(RspecFlaky::FlakyExamplesCollection)
+        raise ArgumentError, "`flaky_examples` must be a RspecFlaky::FlakyExamplesCollection, #{flaky_examples.class} given!"
+      end
+
+      @flaky_examples = flaky_examples
+      super(flaky_examples)
+    end
+
+    def write(file_path)
+      unless RspecFlaky::Config.generate_report?
+        puts "! Generating reports is disabled. To enable it, please set the `FLAKY_RSPEC_GENERATE_REPORT=1` !" # rubocop:disable Rails/Output
+        return
+      end
+
+      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(flaky_examples.to_h))
+    end
+
+    def prune_outdated(days: OUTDATED_DAYS_THRESHOLD)
+      outdated_date_threshold = Time.now - (3600 * 24 * days)
+      updated_hash = flaky_examples.dup
+        .delete_if do |uid, hash|
+          hash[:last_flaky_at] && Time.parse(hash[:last_flaky_at]).to_i < outdated_date_threshold.to_i
+        end
+
+      self.class.new(RspecFlaky::FlakyExamplesCollection.new(updated_hash))
+    end
+  end
+end

scripts/prune-old-flaky-specs

+#!/usr/bin/env ruby
+
+require_relative '../lib/rspec_flaky/report'
+
+report_file = ARGV.shift
+unless report_file
+  puts 'usage: prune-old-flaky-specs <report-file> <new-report-file>'
+  exit 1
+end
+
+new_report_file = ARGV.shift || report_file
+report = RspecFlaky::Report.load(report_file)
+puts "Loading #{report_file}..."
+puts "Current report has #{report.size} entries."
+
+new_report = report.prune_outdated
+
+puts "New report has #{new_report.size} entries: #{report.size - new_report.size} entries older than 90 days were removed."
+puts "Saved #{new_report_file}." if new_report.write(new_report_file)

spec/lib/rspec_flaky/config_spec.rb

@@ -16,23 +16,25 @@ describe RspecFlaky::Config, :aggregate_failures do
       end
     end
-    context "when ENV['FLAKY_RSPEC_GENERATE_REPORT'] is set to 'false'" do
-      before do
-        stub_env('FLAKY_RSPEC_GENERATE_REPORT', 'false')
-      end
-
-      it 'returns false' do
-        expect(described_class).not_to be_generate_report
+    context "when ENV['FLAKY_RSPEC_GENERATE_REPORT'] is set" do
+      using RSpec::Parameterized::TableSyntax
+
+      where(:env_value, :result) do
+        '1'      | true
+        'true'   | true
+        'foo'    | false
+        '0'      | false
+        'false'  | false
       end
-    end
-    context "when ENV['FLAKY_RSPEC_GENERATE_REPORT'] is set to 'true'" do
-      before do
-        stub_env('FLAKY_RSPEC_GENERATE_REPORT', 'true')
-      end
-      it 'returns true' do
-        expect(described_class).to be_generate_report
+      with_them do
+        before do
+          stub_env('FLAKY_RSPEC_GENERATE_REPORT', env_value)
+        end
+        it 'returns false' do
+          expect(described_class.generate_report?).to be(result)
+        end
       end
     end
   end

spec/lib/rspec_flaky/flaky_examples_collection_spec.rb

@@ -24,14 +24,6 @@ describe RspecFlaky::FlakyExamplesCollection, :aggregate_failures do
     }
   end
-  describe '.from_json' do
-    it 'accepts a JSON' do
-      collection = described_class.from_json(JSON.pretty_generate(collection_hash))
-
-      expect(collection.to_report).to eq(described_class.new(collection_hash).to_report)
-    end
-  end
-
   describe '#initialize' do
     it 'accepts no argument' do
       expect { described_class.new }.not_to raise_error
@@ -46,11 +38,11 @@ describe RspecFlaky::FlakyExamplesCollection, :aggregate_failures do
     end
   end
-  describe '#to_report' do
+  describe '#to_h' do
     it 'calls #to_h on the values' do
       collection = described_class.new(collection_hash)
-      expect(collection.to_report).to eq(collection_report)
+      expect(collection.to_h).to eq(collection_report)
     end
   end
@@ -61,7 +53,7 @@ describe RspecFlaky::FlakyExamplesCollection, :aggregate_failures do
         a: { example_id: 'spec/foo/bar_spec.rb:2' },
         c: { example_id: 'spec/bar/baz_spec.rb:4' })
-      expect((collection2 - collection1).to_report).to eq(
+      expect((collection2 - collection1).to_h).to eq(
         c: {
           example_id: 'spec/bar/baz_spec.rb:4',
           first_flaky_at: nil,

spec/lib/rspec_flaky/listener_spec.rb

@@ -4,7 +4,7 @@ describe RspecFlaky::Listener, :aggregate_failures do
   let(:already_flaky_example_uid) { '6e869794f4cfd2badd93eb68719371d1' }
   let(:suite_flaky_example_report) do
     {
-      already_flaky_example_uid => {
+      "#{already_flaky_example_uid}": {
         example_id: 'spec/foo/bar_spec.rb:2',
         file: 'spec/foo/bar_spec.rb',
         line: 2,
@@ -55,8 +55,7 @@ describe RspecFlaky::Listener, :aggregate_failures do
       it 'returns a valid Listener instance' do
         listener = described_class.new
-        expect(listener.to_report(listener.suite_flaky_examples))
-          .to eq(expected_suite_flaky_examples)
+        expect(listener.suite_flaky_examples.to_h).to eq(expected_suite_flaky_examples)
         expect(listener.flaky_examples).to eq({})
       end
     end
@@ -65,25 +64,35 @@ describe RspecFlaky::Listener, :aggregate_failures do
       it_behaves_like 'a valid Listener instance'
     end
-    context 'when a report file exists and set by SUITE_FLAKY_RSPEC_REPORT_PATH' do
-      let(:report_file) do
-        Tempfile.new(%w[rspec_flaky_report .json]).tap do |f|
-          f.write(JSON.pretty_generate(suite_flaky_example_report))
-          f.rewind
-        end
-      end
+    context 'when SUITE_FLAKY_RSPEC_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('SUITE_FLAKY_RSPEC_REPORT_PATH', report_file_path)
       end
-      after do
-        report_file.close
-        report_file.unlink
+      context 'and report file exists' do
+        before do
+          expect(File).to receive(:exist?).with(report_file_path).and_return(true)
+        end
+
+        it 'delegates the load to RspecFlaky::Report' do
+          report = RspecFlaky::Report.new(RspecFlaky::FlakyExamplesCollection.new(suite_flaky_example_report))
+
+          expect(RspecFlaky::Report).to receive(:load).with(report_file_path).and_return(report)
+          expect(described_class.new.suite_flaky_examples.to_h).to eq(report.flaky_examples.to_h)
+        end
       end
-      it_behaves_like 'a valid Listener instance' do
-        let(:expected_suite_flaky_examples) { suite_flaky_example_report }
+      context 'and report file does not exist' do
+        before do
+          expect(File).to receive(:exist?).with(report_file_path).and_return(false)
+        end
+
+        it 'return an empty hash' do
+          expect(RspecFlaky::Report).not_to receive(:load)
+          expect(described_class.new.suite_flaky_examples.to_h).to eq({})
+        end
       end
     end
   end
@@ -186,74 +195,21 @@ describe RspecFlaky::Listener, :aggregate_failures do
     let(:notification_already_flaky_rspec_example) { double(example: already_flaky_rspec_example) }
     context 'when a report file path is set by FLAKY_RSPEC_REPORT_PATH' do
-      let(:report_file_path) { Rails.root.join('tmp', 'rspec_flaky_report.json') }
-      let(:new_report_file_path) { Rails.root.join('tmp', 'rspec_flaky_new_report.json') }
-      before do
-        stub_env('FLAKY_RSPEC_REPORT_PATH', report_file_path)
-        stub_env('NEW_FLAKY_RSPEC_REPORT_PATH', new_report_file_path)
-        FileUtils.rm(report_file_path) if File.exist?(report_file_path)
-        FileUtils.rm(new_report_file_path) if File.exist?(new_report_file_path)
-      end
-      after do
-        FileUtils.rm(report_file_path) if File.exist?(report_file_path)
-        FileUtils.rm(new_report_file_path) if File.exist?(new_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 any report file' do
-          listener.example_passed(notification_new_flaky_rspec_example)
-          listener.dump_summary(nil)
-
-          expect(File.exist?(report_file_path)).to be(false)
-          expect(File.exist?(new_report_file_path)).to be(false)
-        end
+      it 'delegates the writes to RspecFlaky::Report' do
+        listener.example_passed(notification_new_flaky_rspec_example)
+        listener.example_passed(notification_already_flaky_rspec_example)
+        report1 = double
+        report2 = double
+        expect(RspecFlaky::Report).to receive(:new).with(listener.flaky_examples).and_return(report1)
+        expect(report1).to receive(:write).with(RspecFlaky::Config.flaky_examples_report_path)
+        expect(RspecFlaky::Report).to receive(:new).with(listener.flaky_examples - listener.suite_flaky_examples).and_return(report2)
+        expect(report2).to receive(:write).with(RspecFlaky::Config.new_flaky_examples_report_path)
+        listener.dump_summary(nil)
       end
-
-      context 'when FLAKY_RSPEC_GENERATE_REPORT == "true"' do
-        before do
-          stub_env('FLAKY_RSPEC_GENERATE_REPORT', 'true')
-        end
-
-        around do |example|
-          Timecop.freeze { example.run }
-        end
-
-        it 'writes the report files' do
-          listener.example_passed(notification_new_flaky_rspec_example)
-          listener.example_passed(notification_already_flaky_rspec_example)
-
-          listener.dump_summary(nil)
-
-          expect(File.exist?(report_file_path)).to be(true)
-          expect(File.exist?(new_report_file_path)).to be(true)
-
-          expect(File.read(report_file_path))
-            .to eq(JSON.pretty_generate(listener.to_report(listener.flaky_examples)))
-
-          new_example = RspecFlaky::Example.new(notification_new_flaky_rspec_example)
-          new_flaky_example = RspecFlaky::FlakyExample.new(new_example)
-          new_flaky_example.update_flakiness!
-
-          expect(File.read(new_report_file_path))
-            .to eq(JSON.pretty_generate(listener.to_report(new_example.uid => new_flaky_example)))
-        end
-      end
-    end
-  end
-
-  describe '#to_report' do
-    let(:listener) { described_class.new(suite_flaky_example_report.to_json) }
-
-    it 'transforms the internal hash to a JSON-ready hash' do
-      expect(listener.to_report(already_flaky_example_uid => already_flaky_example))
-        .to match(hash_including(suite_flaky_example_report))
     end
   end
 end

spec/lib/rspec_flaky/report_spec.rb

+require 'spec_helper'
+
+describe RspecFlaky::Report, :aggregate_failures do
+  let(:a_hundred_days) { 3600 * 24 * 100 }
+  let(:collection_hash) do
+    {
+      a: { example_id: 'spec/foo/bar_spec.rb:2' },
+      b: { example_id: 'spec/foo/baz_spec.rb:3', first_flaky_at: (Time.now - a_hundred_days).to_s, last_flaky_at: (Time.now - a_hundred_days).to_s }
+    }
+  end
+  let(:suite_flaky_example_report) do
+    {
+      '6e869794f4cfd2badd93eb68719371d1': {
+        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: 4321,
+        last_attempts_count: 3,
+        flaky_reports: 1,
+        last_flaky_job: nil
+      }
+    }
+  end
+  let(:flaky_examples) { RspecFlaky::FlakyExamplesCollection.new(collection_hash) }
+  let(:report) { described_class.new(flaky_examples) }
+
+  describe '.load' do
+    let!(:report_file) do
+      Tempfile.new(%w[rspec_flaky_report .json]).tap do |f|
+        f.write(JSON.pretty_generate(suite_flaky_example_report))
+        f.rewind
+      end
+    end
+
+    after do
+      report_file.close
+      report_file.unlink
+    end
+
+    it 'loads the report file' do
+      expect(described_class.load(report_file.path).flaky_examples.to_h).to eq(suite_flaky_example_report)
+    end
+  end
+
+  describe '.load_json' do
+    let(:report_json) do
+      JSON.pretty_generate(suite_flaky_example_report)
+    end
+
+    it 'loads the report file' do
+      expect(described_class.load_json(report_json).flaky_examples.to_h).to eq(suite_flaky_example_report)
+    end
+  end
+
+  describe '#initialize' do
+    it 'accepts a RspecFlaky::FlakyExamplesCollection' do
+      expect { report }.not_to raise_error
+    end
+
+    it 'does not accept anything else' do
+      expect { described_class.new([1, 2, 3]) }.to raise_error(ArgumentError, "`flaky_examples` must be a RspecFlaky::FlakyExamplesCollection, Array given!")
+    end
+  end
+
+  it 'delegates to #flaky_examples using SimpleDelegator' do
+    expect(report.__getobj__).to eq(flaky_examples)
+  end
+
+  describe '#write' do
+    let(:report_file_path) { Rails.root.join('tmp', 'rspec_flaky_report.json') }
+
+    before do
+      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 RspecFlaky::Config.generate_report? is false' do
+      before do
+        allow(RspecFlaky::Config).to receive(:generate_report?).and_return(false)
+      end
+
+      it 'does not write any report file' do
+        report.write(report_file_path)
+
+        expect(File.exist?(report_file_path)).to be(false)
+      end
+    end
+
+    context 'when RspecFlaky::Config.generate_report? is true' do
+      before do
+        allow(RspecFlaky::Config).to receive(:generate_report?).and_return(true)
+      end
+
+      it 'delegates the writes to RspecFlaky::Report' do
+        report.write(report_file_path)
+
+        expect(File.exist?(report_file_path)).to be(true)
+        expect(File.read(report_file_path))
+          .to eq(JSON.pretty_generate(report.flaky_examples.to_h))
+      end
+    end
+  end
+
+  describe '#prune_outdated' do
+    it 'returns a new collection without the examples older than 90 days by default' do
+      new_report = flaky_examples.to_h.dup.tap { |r| r.delete(:b) }
+      new_flaky_examples = report.prune_outdated
+
+      expect(new_flaky_examples).to be_a(described_class)
+      expect(new_flaky_examples.to_h).to eq(new_report)
+      expect(flaky_examples).to have_key(:b)
+    end
+
+    it 'accepts a given number of days' do
+      new_flaky_examples = report.prune_outdated(days: 200)
+
+      expect(new_flaky_examples.to_h).to eq(report.to_h)
+    end
+  end
+end