don't fail rspec test suite if test has passed at least once during r…

…etries

lib/datadog/ci/contrib/rspec/example.rb

@@ -64,16 +64,18 @@ def run(*args)
 
                   result = super
 
-                  # In case when test job is canceled and RSpec is quitting we don't want to report the last test
+                  # When test job is canceled and RSpec is quitting we don't want to report the last test
                   # before RSpec context unwinds. This test might have some unrelated errors that we don't want to
-                  # report.
+                  # see in Datadog.
                   return result if ::RSpec.world.wants_to_quit
 
                   case execution_result.status
                   when :passed
                     test_span&.passed!
                   when :failed
                     test_span&.failed!(exception: execution_result.exception)
+                    # if any of the retries passed, we don't fail the test run
+                    @exception = nil if test_span&.any_retry_passed?
                   else
                     # :pending or nil
                     test_span&.skipped!(
@@ -84,21 +86,20 @@ def run(*args)
                 end
               end
 
-              # after retries are done, we can report the test to RSpec
-              @skip_reporting = false
-
               # this is a special case for ci-queue, we need to finish the test suite span
               ci_queue_test_span&.finish
 
-              # Finish spec with latest retry's result
-              # TODO: when implementing new test retries make sure to clean @exception before calling this method
-              # if test passed at least once
+              # after retries are done, we can finally report the test to RSpec
+              @skip_reporting = false
               finish(reporter)
             end
 
             def finish(reporter)
-              # by default finish test but do not report it to RSpec::Core::Reporter
-              # it is going to be reported once after retries are done
+              # By default finish test but do not report it to RSpec::Core::Reporter
+              # it is going to be reported once after retries are done.
+              #
+              # We need to do this because RSpec breaks when we try to report the same example multiple times with different
+              # results.
               return super unless @skip_reporting
 
               super(::RSpec::Core::NullReporter)

lib/datadog/ci/test.rb

@@ -145,11 +145,18 @@ def parameters
         get_tag(Ext::Test::TAG_PARAMETERS)
       end
 
+      # @internal
+      def any_retry_passed?
+        !!test_suite&.any_test_retry_passed?(test_id)
+      end
+
       private
 
-      def record_test_result(datadog_status)
-        test_id = Utils::TestRun.datadog_test_id(name, test_suite_name, parameters)
+      def test_id
+        @test_id ||= Utils::TestRun.datadog_test_id(name, test_suite_name, parameters)
+      end
 
+      def record_test_result(datadog_status)
         # if this test was already executed in this test suite, mark it as retried
         if test_suite&.test_executed?(test_id)
           set_tag(Ext::Test::TAG_IS_RETRY, "true")

lib/datadog/ci/test_suite.rb

@@ -51,6 +51,14 @@ def any_passed?
         end
       end
 
+      # @internal
+      def any_test_retry_passed?(test_id)
+        synchronize do
+          stats = @execution_stats_per_test[test_id]
+          stats && stats[Ext::Test::Status::PASS] > 0
+        end
+      end
+
       # @internal
       def test_executed?(test_id)
         synchronize do

sig/datadog/ci/test.rbs

@@ -1,6 +1,8 @@
 module Datadog
   module CI
     class Test < Span
+      @test_id: String
+
       def finish: () -> void
       def test_suite: () -> Datadog::CI::TestSuite?
       def test_suite_id: () -> String?
@@ -12,9 +14,11 @@ module Datadog
       def source_file: () -> String?
       def parameters: () -> String?
       def is_retry?: () -> bool
+      def any_retry_passed?: () -> bool
 
       private
 
+      def test_id: () -> String
       def record_test_result: (String datadog_status) -> void
     end
   end

sig/datadog/ci/test_suite.rbs

@@ -9,6 +9,8 @@ module Datadog
 
       def test_executed?: (String test_id) -> bool
 
+      def any_test_retry_passed?: (String) -> bool
+
       private
 
       def set_status_from_stats!: () -> void

spec/datadog/ci/contrib/rspec/instrumentation_spec.rb

@@ -33,6 +33,7 @@ def rspec_session_run(
     with_shared_context: false,
     with_flaky_test: false,
     with_canceled_test: false,
+    with_flaky_test_that_fails_once: false,
     unskippable: {
       test: false,
       context: false,
@@ -47,6 +48,9 @@ def rspec_session_run(
     max_flaky_test_failures = 4
     flaky_test_failures = 0
 
+    max_flaky_test_that_fails_once_passes = 10
+    flaky_test_that_fails_once_passes = 0
+
     current_let_value = 0
 
     with_new_rspec_environment do
@@ -87,6 +91,17 @@ def rspec_session_run(
             end
           end
 
+          if with_flaky_test_that_fails_once
+            it "flaky that fails once" do
+              if flaky_test_that_fails_once_passes < max_flaky_test_that_fails_once_passes
+                flaky_test_that_fails_once_passes += 1
+                expect(1 + 1).to eq(2)
+              else
+                expect(1 + 1).to eq(3)
+              end
+            end
+          end
+
           if with_canceled_test
             it "canceled during execution" do
               RSpec.world.wants_to_quit = true
@@ -151,7 +166,7 @@ def rspec_session_run(
         :source_file,
         "spec/datadog/ci/contrib/rspec/instrumentation_spec.rb"
       )
-      expect(first_test_span).to have_test_tag(:source_start, "124")
+      expect(first_test_span).to have_test_tag(:source_start, "139")
       expect(first_test_span).to have_test_tag(
         :codeowners,
         "[\"@DataDog/ruby-guild\", \"@DataDog/ci-app-libraries\"]"
@@ -1222,4 +1237,42 @@ def rspec_skipped_session_run
       expect(test_session_span).to have_test_tag(:early_flake_abort_reason, "faulty")
     end
   end
+
+  context "session with early flake detection enabled and test fails on last retry" do
+    include_context "CI mode activated" do
+      let(:integration_name) { :rspec }
+
+      let(:early_flake_detection_enabled) { true }
+      let(:unique_tests_set) { Set.new(["SomeTest at ./spec/datadog/ci/contrib/rspec/instrumentation_spec.rb.nested foo."]) }
+    end
+
+    it "retries the new test 10 times" do
+      rspec_session_run(with_flaky_test_that_fails_once: true)
+
+      # 1 passing test + 1 flaky test run + 10 new test retries = 12 spans
+      expect(test_spans).to have(12).items
+
+      failed_spans, passed_spans = test_spans.partition { |span| span.get_tag("test.status") == "fail" }
+      expect(failed_spans).to have(1).items
+      expect(passed_spans).to have(11).items
+
+      test_spans_by_test_name = test_spans.group_by { |span| span.get_tag("test.name") }
+      expect(test_spans_by_test_name["nested foo"]).to have(1).item
+      expect(test_spans_by_test_name["nested flaky that fails once"]).to have(11).items
+
+      # count how many tests were marked as retries
+      retries_count = test_spans.count { |span| span.get_tag("test.is_retry") == "true" }
+      expect(retries_count).to eq(10)
+
+      # count how many tests were marked as new
+      new_tests_count = test_spans.count { |span| span.get_tag("test.is_new") == "true" }
+      expect(new_tests_count).to eq(11)
+
+      expect(test_suite_spans).to have(1).item
+      expect(test_suite_spans.first).to have_pass_status
+
+      expect(test_session_span).to have_pass_status
+      expect(test_session_span).to have_test_tag(:early_flake_enabled, "true")
+    end
+  end
 end

spec/datadog/ci/test_spec.rb

@@ -169,9 +169,9 @@
   describe "#passed!" do
     before do
       allow(ci_test).to receive(:test_suite).and_return(test_suite)
-      expect(tracer_span).to receive(:get_tag).with("test.name").and_return("test name")
-      expect(tracer_span).to receive(:get_tag).with("test.suite").and_return("test suite name")
-      expect(tracer_span).to receive(:get_tag).with("test.parameters").and_return(nil)
+      allow(tracer_span).to receive(:get_tag).with("test.name").and_return("test name")
+      allow(tracer_span).to receive(:get_tag).with("test.suite").and_return("test suite name")
+      allow(tracer_span).to receive(:get_tag).with("test.parameters").and_return(nil)
     end
 
     context "when test suite is set" do
@@ -216,9 +216,9 @@
   describe "#skipped!" do
     before do
       allow(ci_test).to receive(:test_suite).and_return(test_suite)
-      expect(tracer_span).to receive(:get_tag).with("test.name").and_return("test name")
-      expect(tracer_span).to receive(:get_tag).with("test.suite").and_return("test suite name")
-      expect(tracer_span).to receive(:get_tag).with("test.parameters").and_return(nil)
+      allow(tracer_span).to receive(:get_tag).with("test.name").and_return("test name")
+      allow(tracer_span).to receive(:get_tag).with("test.suite").and_return("test suite name")
+      allow(tracer_span).to receive(:get_tag).with("test.parameters").and_return(nil)
     end
 
     context "when test suite is set" do
@@ -264,9 +264,9 @@
     before do
       allow(ci_test).to receive(:test_suite).and_return(test_suite)
 
-      expect(tracer_span).to receive(:get_tag).with("test.name").and_return("test name")
-      expect(tracer_span).to receive(:get_tag).with("test.suite").and_return("test suite name")
-      expect(tracer_span).to receive(:get_tag).with("test.parameters").and_return(nil)
+      allow(tracer_span).to receive(:get_tag).with("test.name").and_return("test name")
+      allow(tracer_span).to receive(:get_tag).with("test.suite").and_return("test suite name")
+      allow(tracer_span).to receive(:get_tag).with("test.parameters").and_return(nil)
     end
 
     context "when test suite is set" do