Implement early exit for retry mechanisms (#10604)

feat: implement early exit policy

chore: update test fixtures removing additional retries

chore: missed final status updates

fix: only tag "test.has_failed_all_retries" if test retried > 0

chore: lint

Co-authored-by: daniel.mohedano <daniel.mohedano@datadoghq.com>

Author: daniel-mohedano

dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/events/TestEventsHandlerImpl.java

@@ -269,7 +269,7 @@ public void onTestFinish(
       if (outcome.lastExecution()) {
         test.setTag(Tags.TEST_FINAL_STATUS, outcome.finalStatus());
 
-        if (outcome.failedAllRetries()) {
+        if (retryReason != null && outcome.failedAllRetries()) {
           test.setTag(Tags.TEST_HAS_FAILED_ALL_RETRIES, true);
         }
 

dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/execution/RunNTimes.java

@@ -4,29 +4,37 @@
 import datadog.trace.api.civisibility.execution.TestStatus;
 import datadog.trace.api.civisibility.telemetry.tag.RetryReason;
 import datadog.trace.civisibility.config.ExecutionsByDuration;
+import datadog.trace.civisibility.execution.exit.EarlyExitPolicy;
 import java.util.List;
 
-/** Runs a test case N times (N depends on test duration) regardless of success or failure. */
+/**
+ * Runs a test case N times (N depends on test duration) regardless of success or failure. The
+ * execution can also be terminated early if its ExitPolicy evaluates to {@code true}.
+ */
 public class RunNTimes implements TestExecutionPolicy {
 
   private final boolean suppressFailures;
   private final List<ExecutionsByDuration> executionsByDuration;
   private int executions;
   private int maxExecutions;
   private int successfulExecutionsSeen;
+  private int failedExecutionsSeen;
   private final RetryReason retryReason;
   private TestStatus lastStatus;
+  private final EarlyExitPolicy exitPolicy;
 
   public RunNTimes(
       List<ExecutionsByDuration> executionsByDuration,
       boolean suppressFailures,
-      RetryReason retryReason) {
+      RetryReason retryReason,
+      EarlyExitPolicy exitPolicy) {
     this.suppressFailures = suppressFailures;
     this.executionsByDuration = executionsByDuration;
     this.executions = 0;
     this.maxExecutions = getExecutions(0);
     this.successfulExecutionsSeen = 0;
     this.retryReason = retryReason;
+    this.exitPolicy = exitPolicy;
   }
 
   @Override
@@ -35,20 +43,21 @@ public ExecutionOutcome registerExecution(TestStatus status, long durationMillis
     ++executions;
     if (status != TestStatus.fail) {
       ++successfulExecutionsSeen;
+    } else {
+      ++failedExecutionsSeen;
     }
     int maxExecutionsForGivenDuration = getExecutions(durationMillis);
     maxExecutions = Math.min(maxExecutions, maxExecutionsForGivenDuration);
 
     boolean lastExecution = !retriesLeft();
     boolean retry = executions > 1; // first execution is not a retry
     boolean failureSuppressed = status == TestStatus.fail && suppressFailures();
-    boolean succeededAllRetries = lastExecution && successfulExecutionsSeen == executions;
-
+    boolean succeededAllExecutions = successfulExecutionsSeen == executions;
     TestStatus finalStatus = null;
     if (lastExecution) {
       // final status will only be "pass" if all retries pass (or the failures were suppressed)
       // also, the `suppressFailures()` call works because its value cannot change between retries
-      if (succeededAllRetries || suppressFailures()) {
+      if (succeededAllExecutions || suppressFailures()) {
         finalStatus = TestStatus.pass;
       } else {
         finalStatus = TestStatus.fail;
@@ -58,15 +67,17 @@ public ExecutionOutcome registerExecution(TestStatus status, long durationMillis
     return new ExecutionOutcomeImpl(
         failureSuppressed,
         lastExecution,
-        lastExecution && successfulExecutionsSeen == 0,
-        succeededAllRetries,
+        lastExecution && failedExecutionsSeen == executions,
+        lastExecution && succeededAllExecutions,
         retry ? retryReason : null,
         finalStatus);
   }
 
   private boolean retriesLeft() {
     // skipped tests (either by the framework or DD) should not be retried
-    return lastStatus != TestStatus.skip && executions < maxExecutions;
+    return lastStatus != TestStatus.skip
+        && executions < maxExecutions
+        && !exitPolicy.evaluate(failedExecutionsSeen != 0, successfulExecutionsSeen != 0);
   }
 
   @Override

dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/execution/exit/EarlyExitPolicy.java

@@ -0,0 +1,9 @@
+package datadog.trace.civisibility.execution.exit;
+
+public interface EarlyExitPolicy {
+
+  /**
+   * @return {@code true} if the policy indicates that the test should not be retried anymore.
+   */
+  boolean evaluate(boolean hasFailedExecutions, boolean hasPassedExecutions);
+}

dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/execution/exit/ExitOnFailure.java

@@ -0,0 +1,13 @@
+package datadog.trace.civisibility.execution.exit;
+
+/** Policy to avoid retries once a test fails once. Used for Attempt to Fix. */
+public class ExitOnFailure implements EarlyExitPolicy {
+  public static final EarlyExitPolicy INSTANCE = new ExitOnFailure();
+
+  private ExitOnFailure() {}
+
+  @Override
+  public boolean evaluate(boolean hasFailedExecutions, boolean hasPassedExecutions) {
+    return hasFailedExecutions;
+  }
+}

dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/execution/exit/ExitOnFlake.java

@@ -0,0 +1,16 @@
+package datadog.trace.civisibility.execution.exit;
+
+/**
+ * Policy to avoid retries once a test flakes (has at least one failed and passed execution). Used
+ * for Early Flake Detection.
+ */
+public class ExitOnFlake implements EarlyExitPolicy {
+  public static final EarlyExitPolicy INSTANCE = new ExitOnFlake();
+
+  private ExitOnFlake() {}
+
+  @Override
+  public boolean evaluate(boolean hasFailedExecutions, boolean hasPassedExecutions) {
+    return hasFailedExecutions && hasPassedExecutions;
+  }
+}

dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/execution/exit/NoExit.java

@@ -0,0 +1,13 @@
+package datadog.trace.civisibility.execution.exit;
+
+/** Policy that ignores early exit on retries. Currently only used in testing. */
+public class NoExit implements EarlyExitPolicy {
+  public static final EarlyExitPolicy INSTANCE = new NoExit();
+
+  private NoExit() {}
+
+  @Override
+  public boolean evaluate(boolean hasFailedExecutions, boolean hasPassedExecutions) {
+    return false;
+  }
+}

dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/test/ExecutionStrategy.java

@@ -16,6 +16,8 @@
 import datadog.trace.civisibility.execution.RetryUntilSuccessful;
 import datadog.trace.civisibility.execution.RunNTimes;
 import datadog.trace.civisibility.execution.RunOnceIgnoreOutcome;
+import datadog.trace.civisibility.execution.exit.ExitOnFailure;
+import datadog.trace.civisibility.execution.exit.ExitOnFlake;
 import datadog.trace.civisibility.source.LinesResolver;
 import datadog.trace.civisibility.source.SourcePathResolver;
 import java.lang.reflect.Method;
@@ -129,7 +131,8 @@ public TestExecutionPolicy executionPolicy(
       return new RunNTimes(
           executionSettings.getTestManagementSettings().getAttemptToFixExecutions(),
           isQuarantined(test) || isDisabled(test),
-          RetryReason.attemptToFix);
+          RetryReason.attemptToFix,
+          ExitOnFailure.INSTANCE);
     }
 
     if (isEFDApplicable(test, testSource, testTags)) {
@@ -139,7 +142,8 @@ public TestExecutionPolicy executionPolicy(
       return new RunNTimes(
           executionSettings.getEarlyFlakeDetectionSettings().getExecutionsByDuration(),
           isQuarantined(test),
-          RetryReason.efd);
+          RetryReason.efd,
+          ExitOnFlake.INSTANCE);
     }
 
     if (isAutoRetryApplicable(test)) {

dd-java-agent/agent-ci-visibility/src/test/groovy/datadog/trace/civisibility/execution/RunNTimesTest.groovy

@@ -3,13 +3,16 @@ package datadog.trace.civisibility.execution
 import datadog.trace.api.civisibility.execution.TestStatus
 import datadog.trace.api.civisibility.telemetry.tag.RetryReason
 import datadog.trace.civisibility.config.ExecutionsByDuration
+import datadog.trace.civisibility.execution.exit.ExitOnFailure
+import datadog.trace.civisibility.execution.exit.ExitOnFlake
+import datadog.trace.civisibility.execution.exit.NoExit
 import spock.lang.Specification
 
 class RunNTimesTest extends Specification {
 
   def "test run N times"() {
     setup:
-    def executionPolicy = new RunNTimes([new ExecutionsByDuration(Long.MAX_VALUE, 3)], false, RetryReason.efd)
+    def executionPolicy = new RunNTimes([new ExecutionsByDuration(Long.MAX_VALUE, 3)], false, RetryReason.efd, NoExit.INSTANCE)
 
     when:
     def outcome = executionPolicy.registerExecution(TestStatus.fail, 0)
@@ -47,7 +50,7 @@ class RunNTimesTest extends Specification {
 
   def "test failed all retries"() {
     setup:
-    def executionPolicy = new RunNTimes([new ExecutionsByDuration(Long.MAX_VALUE, 3)], false, RetryReason.efd)
+    def executionPolicy = new RunNTimes([new ExecutionsByDuration(Long.MAX_VALUE, 3)], false, RetryReason.efd, NoExit.INSTANCE)
 
     when:
     def outcome = executionPolicy.registerExecution(TestStatus.fail, 0)
@@ -85,7 +88,7 @@ class RunNTimesTest extends Specification {
 
   def "test succeeded all retries"() {
     setup:
-    def executionPolicy = new RunNTimes([new ExecutionsByDuration(Long.MAX_VALUE, 3)], false, RetryReason.efd)
+    def executionPolicy = new RunNTimes([new ExecutionsByDuration(Long.MAX_VALUE, 3)], false, RetryReason.efd, NoExit.INSTANCE)
 
     when:
     def outcome = executionPolicy.registerExecution(TestStatus.pass, 0)
@@ -123,7 +126,7 @@ class RunNTimesTest extends Specification {
 
   def "test suppress failures"() {
     setup:
-    def executionPolicy = new RunNTimes([new ExecutionsByDuration(Long.MAX_VALUE, 3)], true, RetryReason.attemptToFix)
+    def executionPolicy = new RunNTimes([new ExecutionsByDuration(Long.MAX_VALUE, 3)], true, RetryReason.attemptToFix, NoExit.INSTANCE)
 
     when:
     def outcome = executionPolicy.registerExecution(TestStatus.fail, 0)
@@ -159,17 +162,48 @@ class RunNTimesTest extends Specification {
     outcome3.finalStatus() == TestStatus.pass
   }
 
+  def "test exits on flake"(){
+    setup:
+    def executionPolicy = new RunNTimes([new ExecutionsByDuration(Long.MAX_VALUE, 3)], false, RetryReason.efd, ExitOnFlake.INSTANCE)
+
+    when:
+    def outcome = executionPolicy.registerExecution(TestStatus.fail, 0)
+
+    then:
+    !outcome.lastExecution()
+    outcome.finalStatus() == null
+
+    when:
+    def outcome2 = executionPolicy.registerExecution(TestStatus.pass, 0)
+
+    then:
+    outcome2.lastExecution()
+    outcome2.finalStatus() == TestStatus.fail
+  }
+
+  def "test exits on failure"() {
+    setup:
+    def executionPolicy = new RunNTimes([new ExecutionsByDuration(Long.MAX_VALUE, 3)], false, RetryReason.attemptToFix, ExitOnFailure.INSTANCE)
+
+    when:
+    def outcome = executionPolicy.registerExecution(TestStatus.fail, 0)
+
+    then:
+    outcome.lastExecution()
+    outcome.finalStatus() == TestStatus.fail
+  }
+
   def "test adaptive retry count"() {
     when:
-    def executionPolicy = new RunNTimes([new ExecutionsByDuration(100, 3), new ExecutionsByDuration(Long.MAX_VALUE, 1)], true, RetryReason.efd)
+    def executionPolicy = new RunNTimes([new ExecutionsByDuration(100, 3), new ExecutionsByDuration(Long.MAX_VALUE, 1)], true, RetryReason.efd, NoExit.INSTANCE)
 
     then:
     !executionPolicy.registerExecution(TestStatus.fail, 0).lastExecution()
     !executionPolicy.registerExecution(TestStatus.fail, 0).lastExecution()
     executionPolicy.registerExecution(TestStatus.fail, 0).lastExecution()
 
     when:
-    executionPolicy = new RunNTimes([new ExecutionsByDuration(100, 3), new ExecutionsByDuration(Long.MAX_VALUE, 1)], true, RetryReason.efd)
+    executionPolicy = new RunNTimes([new ExecutionsByDuration(100, 3), new ExecutionsByDuration(Long.MAX_VALUE, 1)], true, RetryReason.efd, NoExit.INSTANCE)
 
     then:
     !executionPolicy.registerExecution(TestStatus.fail, 0).lastExecution()

dd-java-agent/instrumentation/junit/junit-4/junit-4-cucumber-5.4/src/test/resources/test-attempt-to-fix-disabled-failed/coverages.ftl

@@ -5,32 +5,4 @@
   "span_id" : ${content_span_id},
   "test_session_id" : ${content_test_session_id},
   "test_suite_id" : ${content_test_suite_id}
-}, {
-  "files" : [ {
-    "filename" : "org/example/cucumber/calculator/basic_arithmetic_failed.feature"
-  } ],
-  "span_id" : ${content_span_id_2},
-  "test_session_id" : ${content_test_session_id},
-  "test_suite_id" : ${content_test_suite_id}
-}, {
-  "files" : [ {
-    "filename" : "org/example/cucumber/calculator/basic_arithmetic_failed.feature"
-  } ],
-  "span_id" : ${content_span_id_3},
-  "test_session_id" : ${content_test_session_id},
-  "test_suite_id" : ${content_test_suite_id}
-}, {
-  "files" : [ {
-    "filename" : "org/example/cucumber/calculator/basic_arithmetic_failed.feature"
-  } ],
-  "span_id" : ${content_span_id_4},
-  "test_session_id" : ${content_test_session_id},
-  "test_suite_id" : ${content_test_suite_id}
-}, {
-  "files" : [ {
-    "filename" : "org/example/cucumber/calculator/basic_arithmetic_failed.feature"
-  } ],
-  "span_id" : ${content_span_id_5},
-  "test_session_id" : ${content_test_session_id},
-  "test_suite_id" : ${content_test_suite_id}
 } ]