fix(rspec): quarantine and abort edge cases (retries, dry-run, before(:context), hook order)

rspec-trunk-flaky-tests/lib/trunk_spec_helper.rb

@@ -71,13 +71,25 @@ def setup
 
         run = @current_run = Run.new
         RSpec::Core::Example.prepend(ExampleExtension)
-        RSpec.configure do |config|
-          config.before(:example) do
-            skip('Quarantine lookup failed, skipping test run') if run.abort_remaining?
-          end
-          config.around(:each) { |example| Trunk.run_counting_attempts(example) }
-          config.reporter.register_listener AnalyticsListener.new(run), :example_finished, :close
+        RSpec::Core::ExampleGroup.singleton_class.prepend(ExampleGroupExtension)
+        RSpec.configure { |config| install(config, run) }
+      end
+
+      def install(config, run)
+        # prepend_before, to also run ahead of before hooks configured before this file was required.
+        config.prepend_before(:example) do |example|
+          run.start_attempt(example)
+          next unless run.abort_remaining?
+
+          # RSpec starts no new examples once it wants to quit, so this is an in-place
+          # re-run (e.g. rspec-retry), where a skip would hide the failure.
+          failure = run.abort_failure(example)
+          raise failure if failure
+
+          skip('Quarantine lookup failed, skipping test run')
         end
+        config.around(:each) { |example| Trunk.run_counting_attempts(example) }
+        config.reporter.register_listener AnalyticsListener.new(run), :example_finished, :close
       end
 
       def disabled?
@@ -163,6 +175,8 @@ def initialize(report = TestReport.new('rspec', Trunk.command_line, nil))
         @report = report
         @quarantining_disabled = false
         @lookup_failed = false
+        @abort_failures = {}.compare_by_identity
+        @quarantined_failures = {}.compare_by_identity
       end
 
       def quarantining_disabled?
@@ -177,9 +191,20 @@ def abort_remaining?
         lookup_failed? && Trunk.quarantine_query_failure_exit?
       end
 
+      def abort_failure(example)
+        @abort_failures[example]
+      end
+
+      def start_attempt(example)
+        @quarantined_failures.delete(example)
+      end
+
       # Whether a failure of `example` with `exception` is quarantined. If the
       # quarantine machinery itself blows up, the failure must stand.
       def quarantine?(example, exception)
+        # A replayed abort failure didn't happen on this attempt, so it can't be quarantined.
+        return false if abort_failure(example).equal?(exception)
+
         check_quarantine(example, exception)
       rescue StandardError => e
         puts Colors.yellow("Quarantine check errored (#{e.class}: #{e.message}), treating test as not quarantined")
@@ -202,10 +227,7 @@ def check_quarantine(example, exception)
             puts Colors.yellow('Failed to check quarantining status, no failures will be quarantined')
             @lookup_failed = true
           end
-          if Trunk.quarantine_query_failure_exit?
-            puts Colors.red('Quarantine lookup failed, exiting early')
-            RSpec.world.wants_to_quit = true
-          end
+          abort_run(example, exception) if Trunk.quarantine_query_failure_exit?
           false
         elsif result.quarantining_disabled_for_repo
           unless quarantining_disabled?
@@ -214,15 +236,27 @@ def check_quarantine(example, exception)
           end
           false
         elsif result.test_is_quarantined
-          # monitor the override in the metadata
-          Trunk.write_metadata(example.metadata, :trunk_quarantined_exception, exception)
+          record_quarantined(example, exception)
           puts Colors.green("Test is quarantined, overriding exception: #{exception}")
           true
         else
           puts Colors.red('Test is not quarantined, continuing')
           false
         end
       end
+
+      def abort_run(example, exception)
+        puts Colors.red('Quarantine lookup failed, exiting early')
+        # The first: a replay's after hooks raise their own errors again anyway.
+        @abort_failures[example] ||= exception
+        RSpec.world.wants_to_quit = true
+      end
+
+      def record_quarantined(example, exception)
+        failures = (@quarantined_failures[example] ||= []) << exception
+        recorded = failures.one? ? exception : RSpec::Core::MultipleExceptionError.new(*failures)
+        Trunk.write_metadata(example.metadata, :trunk_quarantined_exception, recorded)
+      end
     end
 
     # Prepended to RSpec::Core::Example. RSpec uses the existence of an exception
@@ -268,6 +302,22 @@ def pending_example_fixed?(exception)
       end
     end
 
+    # RSpec fails the run when a before(:context) hook raises, even if every example's failure was
+    # quarantined. A green status alone isn't enough: a pending example hides the error in its
+    # pending_exception and still ends up :passed.
+    module ExampleGroupExtension
+      def run(reporter = RSpec::Core::NullReporter)
+        passed = super
+        return passed unless passed == false && Trunk.current_run
+
+        examples = descendant_filtered_examples
+        examples.any? && examples.all? do |example|
+          example.metadata[:trunk_quarantined_exception] &&
+            %i[passed pending].include?(example.execution_result.status)
+        end
+      end
+    end
+
     # Formats failures for the report as plain text suitable for storage and the web UI.
     module FailureFormatter
       # A no-op colorizer passed to RSpec's ExceptionPresenter.
@@ -362,12 +412,15 @@ def initialize(run)
         @run = run
       end
 
+      # --dry-run reports every example as passed without running it.
       def example_finished(notification)
-        add_test_case(notification.example)
+        add_test_case(notification.example) unless RSpec.configuration.dry_run?
       end
 
       # trunk-ignore(rubocop/Metrics/MethodLength,rubocop/Metrics/AbcSize)
       def close(_notification)
+        return if RSpec.configuration.dry_run?
+
         if @run.quarantining_disabled?
           puts Colors.yellow('Note: Quarantining is disabled for this repo. Test failures were not quarantined.')
         end
@@ -407,8 +460,8 @@ def add_test_case(example)
         name = example.full_description
         file, classname = Trunk.file_and_classname(example)
         line = example.metadata[:line_number]
-        started_at = example.execution_result.started_at.to_i
-        finished_at = example.execution_result.finished_at.to_i
+        started_at = example.execution_result.started_at.to_f
+        finished_at = example.execution_result.finished_at.to_f
 
         attempt_number = example.metadata[:retry_attempts] || example.metadata[:trunk_attempt_number] || 0
         # set the status to failure, but mark it as quarantined

rspec-trunk-flaky-tests/test/abort_hook_spec.rb

@@ -0,0 +1,42 @@
+# frozen_string_literal: true
+
+require 'rspec_trunk_flaky_tests'
+require_relative '../spec/spec_helper'
+require 'rspec/core/sandbox'
+
+# trunk-ignore(rubocop/Metrics/BlockLength)
+RSpec.describe 'abort hook' do
+  # trunk-ignore(rubocop/Metrics/MethodLength,rubocop/Metrics/AbcSize)
+  def run_example(abort_remaining:)
+    run = Object.new
+    run.define_singleton_method(:abort_remaining?) { abort_remaining }
+    run.define_singleton_method(:abort_failure) { |_example| nil }
+    run.define_singleton_method(:start_attempt) { |_example| nil }
+    hooks_ran = []
+    example = nil
+    RSpec::Core::Sandbox.sandboxed do |config|
+      config.before(:example) { hooks_ran << :suite }
+      RSpec::Trunk.install(config, run)
+      group = RSpec.describe('sandboxed') do
+        before { hooks_ran << :group }
+        it('e') { hooks_ran << :body }
+      end
+      group.run(RSpec::Core::NullReporter)
+      example = group.examples.first
+    end
+    [example, hooks_ran]
+  end
+
+  it 'skips before any other before hook runs' do
+    example, hooks_ran = run_example(abort_remaining: true)
+    expect(example.execution_result.status).to eq(:pending)
+    expect(example.execution_result.pending_message).to eq('Quarantine lookup failed, skipping test run')
+    expect(hooks_ran).to be_empty
+  end
+
+  it 'leaves the example alone otherwise' do
+    example, hooks_ran = run_example(abort_remaining: false)
+    expect(example.execution_result.status).to eq(:passed)
+    expect(hooks_ran).to eq(%i[suite group body])
+  end
+end

rspec-trunk-flaky-tests/test/quarantine_spec.rb

@@ -0,0 +1,136 @@
+# frozen_string_literal: true
+
+require 'rspec_trunk_flaky_tests'
+require_relative '../spec/spec_helper'
+require_relative 'support/trunk_harness'
+
+# trunk-ignore(rubocop/Metrics/BlockLength)
+RSpec.describe 'quarantine outcomes' do
+  def run_group(lookup, &block)
+    outcome = nil
+    TrunkHarness.with_trunk(lookup) do |_config, run|
+      group = RSpec.describe('sandboxed', &block)
+      outcome = [group.run(RSpec::Core::NullReporter), group.examples, run]
+    end
+    outcome
+  end
+
+  def listener_records(examples, run)
+    listener = RSpec::Trunk::AnalyticsListener.new(run)
+    examples.each { |example| listener.example_finished(RSpec::Core::Notifications::ExampleNotification.for(example)) }
+    listener.close(nil)
+  end
+
+  # trunk-ignore(rubocop/Metrics/BlockLength)
+  context 'when a lookup failure aborts the run' do
+    around do |example|
+      previous = ENV.fetch('TRUNK_QUARANTINE_QUERY_FAILURE_EXIT', nil)
+      ENV['TRUNK_QUARANTINE_QUERY_FAILURE_EXIT'] = 'true'
+      example.run
+    ensure
+      ENV['TRUNK_QUARANTINE_QUERY_FAILURE_EXIT'] = previous
+    end
+
+    it 'keeps failing an example that is re-run in place' do
+      body_runs = 0
+      passed, examples, run = run_group(:failed) do
+        around(&TrunkHarness::RERUN_IN_PLACE)
+        it('fails') do
+          body_runs += 1
+          raise 'the real failure'
+        end
+      end
+
+      expect(passed).to be(false)
+      expect(examples.first.execution_result.status).to eq(:failed)
+      expect(examples.first.exception.message).to eq('the real failure')
+      expect(body_runs).to eq(1)
+      expect(run.report.lookups).to eq(1)
+    end
+
+    it 'replays the failure that aborted the run, not a later after-hook error' do
+      _, examples, = run_group(:failed) do
+        around(&TrunkHarness::RERUN_IN_PLACE)
+        after { raise 'cleanup error' }
+        it('fails') { raise 'the real failure' }
+      end
+
+      expect(examples.first.exception.all_exceptions.map(&:message)).to eq(['the real failure', 'cleanup error'])
+    end
+  end
+
+  context 'when a before(:context) hook raises' do
+    let(:group_body) do
+      proc do
+        before(:context) { raise 'boom in before(:context)' }
+        it('a') { expect(1).to eq(1) }
+        it('b') { expect(1).to eq(1) }
+      end
+    end
+
+    it 'passes the group when every example is quarantined' do
+      passed, examples, = run_group(:quarantined, &group_body)
+      expect(examples.map { |example| example.execution_result.status }).to eq(%i[passed passed])
+      expect(passed).to be(true)
+    end
+
+    it 'still fails the group when a pending example hides the error' do
+      passed, examples, = run_group(:quarantined) do
+        before(:context) { raise 'db down' }
+        it('p', :pending) { expect(1).to eq(2) }
+      end
+      expect(examples.first.execution_result.status).to eq(:passed)
+      expect(passed).to be(false)
+    end
+
+    it 'still fails the group when the examples are not quarantined' do
+      passed, examples, = run_group(:not_quarantined, &group_body)
+      expect(examples.map { |example| example.execution_result.status }).to eq(%i[failed failed])
+      expect(passed).to be(false)
+    end
+  end
+
+  it 'records every failure of a quarantined example, not just the last' do
+    passed, examples, = run_group(:quarantined) do
+      after { raise 'cleanup error' }
+      it('fails') { raise 'the real failure' }
+    end
+
+    expect(passed).to be(true)
+    recorded = examples.first.metadata[:trunk_quarantined_exception]
+    expect(recorded).to be_a(RSpec::Core::MultipleExceptionError)
+    expect(recorded.all_exceptions.map(&:message)).to eq(['the real failure', 'cleanup error'])
+  end
+
+  it 'records only the latest attempt when an example is re-run in place' do
+    attempt = 0
+    _, examples, = run_group(:quarantined) do
+      around { |ex| 2.times { ex.run } }
+      it('fails') { raise "attempt #{attempt += 1}" }
+    end
+    expect(examples.first.metadata[:trunk_quarantined_exception].message).to eq('attempt 2')
+  end
+
+  it 'records and submits nothing for --dry-run' do
+    run = nil
+    TrunkHarness.with_trunk(:not_quarantined) do |config, trunk_run|
+      config.dry_run = true
+      group = RSpec.describe('sandboxed') { it('fails') { raise 'never runs' } }
+      group.run(RSpec::Core::NullReporter)
+      listener_records(group.examples, trunk_run)
+      run = trunk_run
+    end
+    expect(run.report.added).to be_empty
+    expect(run.report.submitted).to be(false)
+  end
+
+  it 'records start and finish times with their sub-second part' do
+    _, examples, run = run_group(:not_quarantined) { it('passes') { expect(1).to eq(1) } }
+    listener_records(examples, run)
+
+    result = examples.first.execution_result
+    started_at, finished_at = run.report.added.first.values_at(8, 9)
+    expect([started_at, finished_at]).to eq([result.started_at.to_f, result.finished_at.to_f])
+    expect(run.report.submitted).to be(true)
+  end
+end