Ensure RSpecFlaky doesn't automatically update flaky examples

Previously, instantiating a RspecFlaky::FlakyExample object would
automatically update its first_flaky_at, last_flaky_at and
last_flaky_job.

That was wrong because we would overwrite every time the suite report
with this false data.

We now:

- Get the suite report and only read from it
- Write only the currently detected flaky examples in the report, so
  that the final report is only updated with flaky examples that were
  actually detected in each job. Before, job1 could overwrite the legit
  report from job2!
- Write the newly detected flaky examples by rejecting the already
  tracked flaky specs instead of using another hash.
Signed-off-by: Rémy Coutable <remy@rymai.me>

lib/rspec_flaky/flaky_example.rb

@@ -9,24 +9,21 @@ module RspecFlaky
           line: example.line,
           description: example.description,
           last_attempts_count: example.attempts,
-          flaky_reports: 1)
+          flaky_reports: 0)
       else
         super
       end
     end
 
-    def first_flaky_at
-      self[:first_flaky_at] || Time.now
-    end
-
-    def last_flaky_at
-      Time.now
-    end
+    def update_flakiness!(last_attempts_count: nil)
+      self.first_flaky_at ||= Time.now
+      self.last_flaky_at = Time.now
+      self.flaky_reports += 1
+      self.last_attempts_count = last_attempts_count if last_attempts_count
 
-    def last_flaky_job
-      return unless ENV['CI_PROJECT_URL'] && ENV['CI_JOB_ID']
-
-      "#{ENV['CI_PROJECT_URL']}/-/jobs/#{ENV['CI_JOB_ID']}"
+      if ENV['CI_PROJECT_URL'] && ENV['CI_JOB_ID']
+        self.last_flaky_job = "#{ENV['CI_PROJECT_URL']}/-/jobs/#{ENV['CI_JOB_ID']}"
+      end
     end
 
     def to_h

lib/rspec_flaky/listener.rb

@@ -2,11 +2,15 @@ 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
+    # - suite_flaky_examples: contains all the currently tracked flacky example
+    #   for the whole RSpec suite
+    # - flaky_examples: contains the examples detected as flaky during the
+    #   current RSpec run
+    attr_reader :suite_flaky_examples, :flaky_examples
+
+    def initialize(suite_flaky_examples_json = nil)
+      @flaky_examples = {}
+      @suite_flaky_examples = init_suite_flaky_examples(suite_flaky_examples_json)
     end
 
     def example_passed(notification)
@@ -14,24 +18,16 @@ module RspecFlaky
 
       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
+      flaky_example = suite_flaky_examples.fetch(current_example.uid) { 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(all_flaky_examples, all_flaky_examples_report_path)
+      write_report_file(flaky_examples, flaky_examples_report_path)
 
+      new_flaky_examples = _new_flaky_examples
       if new_flaky_examples.any?
         Rails.logger.warn "\nNew flaky examples detected:\n"
         Rails.logger.warn JSON.pretty_generate(to_report(new_flaky_examples))
@@ -46,12 +42,24 @@ module RspecFlaky
 
     private
 
-    def init_all_flaky_examples
-      return {} unless File.exist?(all_flaky_examples_report_path)
+    def init_suite_flaky_examples(suite_flaky_examples_json = nil)
+      unless suite_flaky_examples_json
+        return {} unless File.exist?(suite_flaky_examples_report_path)
+
+        suite_flaky_examples_json = File.read(suite_flaky_examples_report_path)
+      end
+
+      suite_flaky_examples = JSON.parse(suite_flaky_examples_json)
 
-      all_flaky_examples = JSON.parse(File.read(all_flaky_examples_report_path))
+      Hash[(suite_flaky_examples || {}).map { |k, ex| [k, FlakyExample.new(ex)] }].freeze
+    end
+
+    def _new_flaky_examples
+      flaky_examples.reject { |uid, _| already_flaky?(uid) }
+    end
 
-      Hash[(all_flaky_examples || {}).map { |k, ex| [k, FlakyExample.new(ex)] }]
+    def already_flaky?(example_uid)
+      suite_flaky_examples.key?(example_uid)
     end
 
     def write_report_file(examples, file_path)
@@ -62,9 +70,14 @@ module RspecFlaky
       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")
+    def suite_flaky_examples_report_path
+      @suite_flaky_examples_report_path ||= ENV['SUITE_FLAKY_RSPEC_REPORT_PATH'] ||
+        Rails.root.join("rspec_flaky/suite-report.json")
+    end
+
+    def flaky_examples_report_path
+      @flaky_examples_report_path ||= ENV['FLAKY_RSPEC_REPORT_PATH'] ||
+        Rails.root.join("rspec_flaky/report.json")
     end
 
     def new_flaky_examples_report_path

spec/lib/rspec_flaky/flaky_example_spec.rb

 require 'spec_helper'
 
-describe RspecFlaky::FlakyExample do
+describe RspecFlaky::FlakyExample, :aggregate_failures do
   let(:flaky_example_attrs) do
     {
       example_id: 'spec/foo/bar_spec.rb:2',
@@ -9,6 +9,7 @@ describe RspecFlaky::FlakyExample do
       description: 'hello world',
       first_flaky_at: 1234,
       last_flaky_at: 2345,
+      last_flaky_job: 'https://gitlab.com/gitlab-org/gitlab-ce/-/jobs/12',
       last_attempts_count: 2,
       flaky_reports: 1
     }
@@ -27,57 +28,78 @@ describe RspecFlaky::FlakyExample do
   end
   let(:example) { double(example_attrs) }
 
+  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 FlakyExample instance' do
-      it 'returns valid attributes' do
-        flaky_example = described_class.new(args)
+      let(:flaky_example) { described_class.new(args) }
 
+      it 'returns valid attributes' do
         expect(flaky_example.uid).to eq(flaky_example_attrs[:uid])
-        expect(flaky_example.example_id).to eq(flaky_example_attrs[:example_id])
+        expect(flaky_example.file).to eq(flaky_example_attrs[:file])
+        expect(flaky_example.line).to eq(flaky_example_attrs[:line])
+        expect(flaky_example.description).to eq(flaky_example_attrs[:description])
+        expect(flaky_example.first_flaky_at).to eq(expected_first_flaky_at)
+        expect(flaky_example.last_flaky_at).to eq(expected_last_flaky_at)
+        expect(flaky_example.last_attempts_count).to eq(flaky_example_attrs[:last_attempts_count])
+        expect(flaky_example.flaky_reports).to eq(expected_flaky_reports)
       end
     end
 
     context 'when given an Rspec::Example' do
-      let(:args) { example }
-
-      it_behaves_like 'a valid FlakyExample instance'
+      it_behaves_like 'a valid FlakyExample instance' do
+        let(:args) { example }
+        let(:expected_first_flaky_at) { nil }
+        let(:expected_last_flaky_at) { nil }
+        let(:expected_flaky_reports) { 0 }
+      end
     end
 
     context 'when given a hash' do
-      let(:args) { flaky_example_attrs }
-
-      it_behaves_like 'a valid FlakyExample instance'
+      it_behaves_like 'a valid FlakyExample instance' do
+        let(:args) { flaky_example_attrs }
+        let(:expected_flaky_reports) { flaky_example_attrs[:flaky_reports] }
+        let(:expected_first_flaky_at) { flaky_example_attrs[:first_flaky_at] }
+        let(:expected_last_flaky_at) { flaky_example_attrs[:last_flaky_at] }
+      end
     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
+  describe '#update_flakiness!' do
+    shared_examples 'an up-to-date FlakyExample instance' do
+      let(:flaky_example) { described_class.new(args) }
 
-    shared_examples 'a valid FlakyExample hash' do
-      let(:additional_attrs) { {} }
+      it 'updates the first_flaky_at' do
+        now = Time.now
+        expected_first_flaky_at = flaky_example.first_flaky_at ? flaky_example.first_flaky_at : now
+        Timecop.freeze(now) { flaky_example.update_flakiness! }
 
-      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.first_flaky_at).to eq(expected_first_flaky_at)
+      end
+
+      it 'updates the last_flaky_at' do
+        now = Time.now
+        Timecop.freeze(now) { flaky_example.update_flakiness! }
 
-        expect(flaky_example.to_h).to match(hash_including(final_hash))
+        expect(flaky_example.last_flaky_at).to eq(now)
       end
-    end
 
-    context 'when given an Rspec::Example' do
-      let(:args) { example }
+      it 'updates the flaky_reports' do
+        expected_flaky_reports = flaky_example.first_flaky_at ? flaky_example.flaky_reports + 1 : 1
+
+        expect { flaky_example.update_flakiness! }.to change { flaky_example.flaky_reports }.by(1)
+        expect(flaky_example.flaky_reports).to eq(expected_flaky_reports)
+      end
+
+      context 'when passed a :last_attempts_count' do
+        it 'updates the last_attempts_count' do
+          flaky_example.update_flakiness!(last_attempts_count: 42)
 
-      context 'when run locally' do
-        it_behaves_like 'a valid FlakyExample hash' do
-          let(:additional_attrs) do
-            { first_flaky_at: instance_of(Time) }
-          end
+          expect(flaky_example.last_attempts_count).to eq(42)
         end
       end
 
@@ -87,10 +109,45 @@ describe RspecFlaky::FlakyExample do
           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
+        it 'updates the last_flaky_job' do
+          flaky_example.update_flakiness!
+
+          expect(flaky_example.last_flaky_job).to eq('https://gitlab.com/gitlab-org/gitlab-ce/-/jobs/42')
+        end
+      end
+    end
+
+    context 'when given an Rspec::Example' do
+      it_behaves_like 'an up-to-date FlakyExample instance' do
+        let(:args) { example }
+      end
+    end
+
+    context 'when given a hash' do
+      it_behaves_like 'an up-to-date FlakyExample instance' do
+        let(:args) { flaky_example_attrs }
+      end
+    end
+  end
+
+  describe '#to_h' do
+    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(additional_attrs)
+
+        expect(flaky_example.to_h).to eq(final_hash)
+      end
+    end
+
+    context 'when given an Rspec::Example' do
+      let(:args) { example }
+
+      it_behaves_like 'a valid FlakyExample hash' do
+        let(:additional_attrs) do
+          { first_flaky_at: nil, last_flaky_at: nil, last_flaky_job: nil, flaky_reports: 0 }
         end
       end
     end

spec/lib/rspec_flaky/listener_spec.rb

 require 'spec_helper'
 
-describe RspecFlaky::Listener do
-  let(:flaky_example_report) do
+describe RspecFlaky::Listener, :aggregate_failures do
+  let(:already_flaky_example_uid) { '6e869794f4cfd2badd93eb68719371d1' }
+  let(:suite_flaky_example_report) do
     {
-      'abc123' => {
+      already_flaky_example_uid => {
         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,
+        last_flaky_at: 4321,
+        last_attempts_count: 3,
         flaky_reports: 1,
         last_flaky_job: nil
       }
     }
   end
-  let(:example_attrs) do
+  let(:already_flaky_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: nil)
+    }
+  end
+  let(:already_flaky_example) { RspecFlaky::FlakyExample.new(suite_flaky_example_report[already_flaky_example_uid]) }
+  let(:new_example_attrs) do
     {
       id: 'spec/foo/baz_spec.rb:3',
       metadata: {
@@ -36,14 +49,15 @@ describe RspecFlaky::Listener do
 
   describe '#initialize' do
     shared_examples 'a valid Listener instance' do
-      let(:expected_all_flaky_examples) { {} }
+      let(:expected_suite_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({})
+        expect(listener.to_report(listener.suite_flaky_examples))
+          .to eq(expected_suite_flaky_examples)
+        expect(listener.__send__(:_new_flaky_examples)).to eq({})
+        expect(listener.flaky_examples).to eq({})
       end
     end
 
@@ -51,16 +65,16 @@ describe RspecFlaky::Listener do
       it_behaves_like 'a valid Listener instance'
     end
 
-    context 'when a report file exists and set by ALL_FLAKY_RSPEC_REPORT_PATH' do
+    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(flaky_example_report))
+          f.write(JSON.pretty_generate(suite_flaky_example_report))
           f.rewind
         end
       end
 
       before do
-        stub_env('ALL_FLAKY_RSPEC_REPORT_PATH', report_file.path)
+        stub_env('SUITE_FLAKY_RSPEC_REPORT_PATH', report_file.path)
       end
 
       after do
@@ -69,74 +83,122 @@ describe RspecFlaky::Listener do
       end
 
       it_behaves_like 'a valid Listener instance' do
-        let(:expected_all_flaky_examples) { flaky_example_report }
+        let(:expected_suite_flaky_examples) { suite_flaky_example_report }
       end
     end
   end
 
   describe '#example_passed' do
-    let(:rspec_example) { double(example_attrs) }
+    let(:rspec_example) { double(new_example_attrs) }
     let(:notification) { double(example: rspec_example) }
+    let(:listener) { described_class.new(suite_flaky_example_report.to_json) }
 
     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 }
+        expect { listener.example_passed(notification) }
+          .not_to change { listener.flaky_examples }
       end
     end
 
-    describe 'when the RSpec example does not respond to attempts' do
-      it_behaves_like 'a non-flaky example'
-    end
+    shared_examples 'an existing flaky example' do
+      let(:expected_flaky_example) 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_attempts_count: 2,
+          flaky_reports: 2,
+          last_flaky_job: nil
+        }
+      end
 
-    describe 'when the RSpec example has 1 attempt' do
-      let(:rspec_example) { double(example_attrs.merge(attempts: 1)) }
+      it 'changes the flaky examples hash' do
+        new_example = RspecFlaky::Example.new(rspec_example)
 
-      it_behaves_like 'a non-flaky example'
+        now = Time.now
+        Timecop.freeze(now) do
+          expect { listener.example_passed(notification) }
+            .to change { listener.flaky_examples[new_example.uid].to_h }
+        end
+
+        expect(listener.flaky_examples[new_example.uid].to_h)
+          .to eq(expected_flaky_example.merge(last_flaky_at: now))
+      end
     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
+    shared_examples 'a new flaky example' do
+      let(:expected_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 }
-
+      it 'changes the all flaky examples hash' do
         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))
+        now = Time.now
+        Timecop.freeze(now) do
+          expect { listener.example_passed(notification) }
+            .to change { listener.flaky_examples[new_example.uid].to_h }
+        end
+
+        expect(listener.flaky_examples[new_example.uid].to_h)
+          .to eq(expected_flaky_example.merge(first_flaky_at: now, last_flaky_at: now))
+      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(new_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(new_example_attrs.merge(attempts: 2)) }
+
+      it_behaves_like 'a new flaky example'
+
+      context 'with an existing flaky example' do
+        let(:rspec_example) { double(already_flaky_example_attrs.merge(attempts: 2)) }
+
+        it_behaves_like 'an existing flaky example'
       end
     end
   end
 
   describe '#dump_summary' do
-    let(:rspec_example) { double(example_attrs) }
-    let(:notification) { double(example: rspec_example) }
+    let(:listener) { described_class.new(suite_flaky_example_report.to_json) }
+    let(:new_flaky_rspec_example) { double(new_example_attrs.merge(attempts: 2)) }
+    let(:already_flaky_rspec_example) { double(already_flaky_example_attrs.merge(attempts: 2)) }
+    let(:notification_new_flaky_rspec_example) { double(example: new_flaky_rspec_example) }
+    let(:notification_already_flaky_rspec_example) { double(example: already_flaky_rspec_example) }
 
-    context 'when a report file path is set by ALL_FLAKY_RSPEC_REPORT_PATH' do
+    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('ALL_FLAKY_RSPEC_REPORT_PATH', report_file_path)
+        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
@@ -144,12 +206,13 @@ describe RspecFlaky::Listener do
           stub_env('FLAKY_RSPEC_GENERATE_REPORT', 'false')
         end
 
-        it 'does not write the report file' do
-          subject.example_passed(notification)
+        it 'does not write any report file' do
+          listener.example_passed(notification_new_flaky_rspec_example)
 
-          subject.dump_summary(nil)
+          listener.dump_summary(nil)
 
           expect(File.exist?(report_file_path)).to be(false)
+          expect(File.exist?(new_report_file_path)).to be(false)
         end
       end
 
@@ -158,21 +221,39 @@ describe RspecFlaky::Listener do
           stub_env('FLAKY_RSPEC_GENERATE_REPORT', 'true')
         end
 
-        it 'writes the report file' do
-          subject.example_passed(notification)
+        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)
 
-          subject.dump_summary(nil)
+          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(subject.to_report('abc123' => RspecFlaky::FlakyExample.new(flaky_example_report['abc123'])))
-        .to match(hash_including(flaky_example_report))
+      expect(listener.to_report(already_flaky_example_uid => already_flaky_example))
+        .to match(hash_including(suite_flaky_example_report))
     end
   end
 end