Consider modified tests when applying fail-fast tests ordering

Author: nikita-tkachenko-datadog

dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/domain/TestFrameworkModule.java

@@ -29,7 +29,7 @@ TestSuiteImpl testSuiteStart(
 
   boolean isFlaky(@Nonnull TestIdentifier test);
 
-  boolean isModified(TestSourceData testSourceData);
+  boolean isModified(@Nonnull TestSourceData testSourceData);
 
   boolean isQuarantined(TestIdentifier test);
 
@@ -49,5 +49,11 @@ TestSuiteImpl testSuiteStart(
   @Nonnull
   TestExecutionPolicy executionPolicy(TestIdentifier test, TestSourceData testSource);
 
+  /**
+   * Returns the priority of the test execution that can be used for ordering tests. The higher the
+   * value, the higher the priority, meaning that the test should be executed earlier.
+   */
+  int executionPriority(@Nullable TestIdentifier test, @Nonnull TestSourceData testSource);
+
   void end(Long startTime);
 }

dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/domain/buildsystem/ProxyTestModule.java

@@ -100,7 +100,7 @@ public boolean isFlaky(@Nonnull TestIdentifier test) {
   }
 
   @Override
-  public boolean isModified(TestSourceData testSourceData) {
+  public boolean isModified(@Nonnull TestSourceData testSourceData) {
     return executionStrategy.isModified(testSourceData);
   }
 
@@ -131,6 +131,11 @@ public TestExecutionPolicy executionPolicy(TestIdentifier test, TestSourceData t
     return executionStrategy.executionPolicy(test, testSource);
   }
 
+  @Override
+  public int executionPriority(@Nullable TestIdentifier test, @Nonnull TestSourceData testSource) {
+    return executionStrategy.executionPriority(test, testSource);
+  }
+
   @Override
   public void end(@Nullable Long endTime) {
     // we have no span locally,

dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/domain/headless/HeadlessTestModule.java

@@ -85,7 +85,7 @@ public boolean isFlaky(@Nonnull TestIdentifier test) {
   }
 
   @Override
-  public boolean isModified(TestSourceData testSourceData) {
+  public boolean isModified(@Nonnull TestSourceData testSourceData) {
     return executionStrategy.isModified(testSourceData);
   }
 
@@ -116,6 +116,11 @@ public TestExecutionPolicy executionPolicy(TestIdentifier test, TestSourceData t
     return executionStrategy.executionPolicy(test, testSource);
   }
 
+  @Override
+  public int executionPriority(@Nullable TestIdentifier test, @Nonnull TestSourceData testSource) {
+    return executionStrategy.executionPriority(test, testSource);
+  }
+
   @Override
   public void end(@Nullable Long endTime) {
     ExecutionSettings executionSettings = executionStrategy.getExecutionSettings();

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

@@ -114,6 +114,12 @@ public boolean isFlaky(@Nonnull TestIdentifier test) {
     return false;
   }
 
+  @Override
+  public int executionPriority(
+      @Nullable TestIdentifier test, @Nonnull TestSourceData testSourceData) {
+    return 0;
+  }
+
   @Override
   public void close() {
     // do nothing

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

@@ -299,6 +299,11 @@ public TestExecutionPolicy executionPolicy(TestIdentifier test, TestSourceData t
     return testModule.executionPolicy(test, testSource);
   }
 
+  @Override
+  public int executionPriority(@Nullable TestIdentifier test, @Nonnull TestSourceData testSource) {
+    return testModule.executionPriority(test, testSource);
+  }
+
   @Override
   public boolean isNew(@Nonnull TestIdentifier test) {
     return testModule.isNew(test);

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

@@ -187,7 +187,7 @@ public boolean isEFDLimitReached() {
     return detectionsUsed > threshold;
   }
 
-  public boolean isModified(TestSourceData testSourceData) {
+  public boolean isModified(@Nonnull TestSourceData testSourceData) {
     Class<?> testClass = testSourceData.getTestClass();
     if (testClass == null) {
       return false;
@@ -219,4 +219,28 @@ private LinesResolver.Lines getLines(Method testMethod) {
       return linesResolver.getMethodLines(testMethod);
     }
   }
+
+  /**
+   * Returns the priority of the test execution that can be used for ordering tests. The higher the
+   * value, the higher the priority, meaning that the test should be executed earlier.
+   */
+  public int executionPriority(@Nullable TestIdentifier test, @Nonnull TestSourceData testSource) {
+    if (test == null) {
+      return 0;
+    }
+    if (isNew(test)) {
+      // execute new tests first
+      return 300;
+    }
+    if (isModified(testSource)) {
+      // then modified tests
+      return 200;
+    }
+    if (isFlaky(test)) {
+      // then tests known to be flaky
+      return 100;
+    }
+    // then the rest
+    return 0;
+  }
 }

dd-java-agent/instrumentation/junit-5.3/junit-5.8/src/main/java/datadog/trace/instrumentation/junit5/order/FailFastClassOrderer.java

@@ -1,6 +1,7 @@
 package datadog.trace.instrumentation.junit5.order;
 
 import datadog.trace.api.civisibility.config.TestIdentifier;
+import datadog.trace.api.civisibility.config.TestSourceData;
 import datadog.trace.api.civisibility.events.TestEventsHandler;
 import datadog.trace.instrumentation.junit5.JUnitPlatformUtils;
 import java.util.Comparator;
@@ -15,41 +16,44 @@ public class FailFastClassOrderer implements ClassOrderer {
 
   private final TestEventsHandler<TestDescriptor, TestDescriptor> testEventsHandler;
   private final @Nullable ClassOrderer delegate;
-  private final Comparator<ClassDescriptor> knownTestSuitesComparator;
+  private final Comparator<ClassDescriptor> executionOrderComparator;
 
   public FailFastClassOrderer(
       TestEventsHandler<TestDescriptor, TestDescriptor> testEventsHandler,
       @Nullable ClassOrderer delegate) {
     this.testEventsHandler = testEventsHandler;
     this.delegate = delegate;
-    this.knownTestSuitesComparator = Comparator.comparing(this::knownAndStableTestsPercentage);
+    this.executionOrderComparator = Comparator.comparing(this::classExecutionPriority).reversed();
   }
 
-  private int knownAndStableTestsPercentage(ClassDescriptor classDescriptor) {
+  private int classExecutionPriority(ClassDescriptor classDescriptor) {
     TestDescriptor testDescriptor = JUnit5OrderUtils.getTestDescriptor(classDescriptor);
-    return 100 - unknownAndFlakyTestsPercentage(testDescriptor);
+    return executionPriority(testDescriptor);
   }
 
-  private int unknownAndFlakyTestsPercentage(TestDescriptor testDescriptor) {
+  private int executionPriority(TestDescriptor testDescriptor) {
     if (testDescriptor == null) {
       return 0;
     }
+
     if (testDescriptor.isTest() || JUnitPlatformUtils.isParameterizedTest(testDescriptor)) {
       TestIdentifier testIdentifier = JUnitPlatformUtils.toTestIdentifier(testDescriptor);
-      return testEventsHandler.isNew(testIdentifier) || testEventsHandler.isFlaky(testIdentifier)
-          ? 100
-          : 0;
+      TestSourceData testSourceData = JUnitPlatformUtils.toTestSourceData(testDescriptor);
+      return testEventsHandler.executionPriority(testIdentifier, testSourceData);
     }
+
     Set<? extends TestDescriptor> children = testDescriptor.getChildren();
     if (children.isEmpty()) {
       return 0;
     }
 
-    int uknownTestsPercentage = 0;
+    // there's no specific meaning to the average of children priorities,
+    // it is just a heuristic to try to favor classes with higher percentage of failure-prone tests
+    int childrenPrioritySum = 0;
     for (TestDescriptor child : children) {
-      uknownTestsPercentage += unknownAndFlakyTestsPercentage(child);
+      childrenPrioritySum += executionPriority(child);
     }
-    return uknownTestsPercentage / children.size();
+    return childrenPrioritySum / children.size();
   }
 
   @Override
@@ -60,6 +64,6 @@ public void orderClasses(ClassOrdererContext classOrdererContext) {
     }
     // then apply our ordering (since sorting is stable, delegate's ordering will be preserved for
     // classes with the same "known/stable" status)
-    classOrdererContext.getClassDescriptors().sort(knownTestSuitesComparator);
+    classOrdererContext.getClassDescriptors().sort(executionOrderComparator);
   }
 }

dd-java-agent/instrumentation/junit-5.3/junit-5.8/src/main/java/datadog/trace/instrumentation/junit5/order/FailFastMethodOrderer.java

@@ -1,6 +1,7 @@
 package datadog.trace.instrumentation.junit5.order;
 
 import datadog.trace.api.civisibility.config.TestIdentifier;
+import datadog.trace.api.civisibility.config.TestSourceData;
 import datadog.trace.api.civisibility.events.TestEventsHandler;
 import datadog.trace.instrumentation.junit5.JUnitPlatformUtils;
 import java.util.Comparator;
@@ -14,23 +15,24 @@ public class FailFastMethodOrderer implements MethodOrderer {
 
   private final TestEventsHandler<TestDescriptor, TestDescriptor> testEventsHandler;
   private final @Nullable MethodOrderer delegate;
-  private final Comparator<MethodDescriptor> knownTestMethodsComparator;
+  private final Comparator<MethodDescriptor> executionOrderComparator;
 
   public FailFastMethodOrderer(
       TestEventsHandler<TestDescriptor, TestDescriptor> testEventsHandler,
       @Nullable MethodOrderer delegate) {
     this.testEventsHandler = testEventsHandler;
     this.delegate = delegate;
-    this.knownTestMethodsComparator = Comparator.comparing(this::isKnownAndStable);
+    this.executionOrderComparator = Comparator.comparing(this::executionPriority).reversed();
   }
 
-  private boolean isKnownAndStable(MethodDescriptor methodDescriptor) {
+  private int executionPriority(MethodDescriptor methodDescriptor) {
     TestDescriptor testDescriptor = JUnit5OrderUtils.getTestDescriptor(methodDescriptor);
     if (testDescriptor == null) {
-      return true;
+      return 0;
     }
     TestIdentifier testIdentifier = JUnitPlatformUtils.toTestIdentifier(testDescriptor);
-    return !testEventsHandler.isNew(testIdentifier) && !testEventsHandler.isFlaky(testIdentifier);
+    TestSourceData testSourceData = JUnitPlatformUtils.toTestSourceData(testDescriptor);
+    return testEventsHandler.executionPriority(testIdentifier, testSourceData);
   }
 
   @Override
@@ -41,6 +43,6 @@ public void orderMethods(MethodOrdererContext methodOrdererContext) {
     }
     // then apply our ordering (since sorting is stable, delegate's ordering will be preserved for
     // methods with the same "known/stable" status)
-    methodOrdererContext.getMethodDescriptors().sort(knownTestMethodsComparator);
+    methodOrdererContext.getMethodDescriptors().sort(executionOrderComparator);
   }
 }

dd-java-agent/instrumentation/testng/src/main/java/datadog/trace/instrumentation/testng/TestNGUtils.java

@@ -13,6 +13,7 @@
 import java.util.Collections;
 import java.util.List;
 import java.util.Properties;
+import javax.annotation.Nonnull;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 import org.testng.IClass;
@@ -234,6 +235,7 @@ public static IRetryAnalyzer getRetryAnalyzer(ITestResult result) {
     }
   }
 
+  @Nonnull
   public static TestIdentifier toTestIdentifier(
       Method method, Object instance, Object[] parameters) {
     Class<?> testClass = instance != null ? instance.getClass() : method.getDeclaringClass();
@@ -243,6 +245,7 @@ public static TestIdentifier toTestIdentifier(
     return new TestIdentifier(testSuiteName, testName, testParameters);
   }
 
+  @Nonnull
   public static TestIdentifier toTestIdentifier(ITestResult result) {
     String testSuiteName = result.getInstanceName();
     String testName =
@@ -251,6 +254,7 @@ public static TestIdentifier toTestIdentifier(ITestResult result) {
     return new TestIdentifier(testSuiteName, testName, testParameters);
   }
 
+  @Nonnull
   public static TestSuiteDescriptor toSuiteDescriptor(ITestClass testClass) {
     String testSuiteName = testClass.getName();
     Class<?> testSuiteClass = testClass.getRealClass();

dd-java-agent/instrumentation/testng/testng-7/src/main/java/datadog/trace/instrumentation/testng7/order/FailFastOrderInterceptor.java

@@ -1,6 +1,7 @@
 package datadog.trace.instrumentation.testng7.order;
 
 import datadog.trace.api.civisibility.config.TestIdentifier;
+import datadog.trace.api.civisibility.config.TestSourceData;
 import datadog.trace.api.civisibility.events.TestEventsHandler;
 import datadog.trace.api.civisibility.events.TestSuiteDescriptor;
 import datadog.trace.instrumentation.testng.TestNGUtils;
@@ -16,27 +17,29 @@
 public class FailFastOrderInterceptor implements IMethodInterceptor {
 
   private final TestEventsHandler<TestSuiteDescriptor, ITestResult> testEventsHandler;
-  private final Comparator<IMethodInstance> knownAndStableTestsComparator;
+  private final Comparator<IMethodInstance> executionOrderComparator;
 
   public FailFastOrderInterceptor(
       TestEventsHandler<TestSuiteDescriptor, ITestResult> testEventsHandler) {
     this.testEventsHandler = testEventsHandler;
-    this.knownAndStableTestsComparator = Comparator.comparing(this::isKnownAndStable);
+    this.executionOrderComparator = Comparator.comparing(this::executionPriority).reversed();
   }
 
-  private boolean isKnownAndStable(IMethodInstance methodInstance) {
+  private int executionPriority(IMethodInstance methodInstance) {
     ITestNGMethod method = methodInstance.getMethod();
     if (method == null) {
-      return true;
+      // not expected to happen, just a safeguard
+      return 0;
     }
     ITestResult testResult = TestResult.newTestResultFor(method);
     TestIdentifier testIdentifier = TestNGUtils.toTestIdentifier(testResult);
-    return !testEventsHandler.isNew(testIdentifier) && !testEventsHandler.isFlaky(testIdentifier);
+    TestSourceData testSourceData = TestNGUtils.toTestSourceData(testResult);
+    return testEventsHandler.executionPriority(testIdentifier, testSourceData);
   }
 
   @Override
   public List<IMethodInstance> intercept(List<IMethodInstance> list, ITestContext iTestContext) {
-    list.sort(knownAndStableTestsComparator);
+    list.sort(executionOrderComparator);
     return list;
   }
 }

internal-api/src/main/java/datadog/trace/api/civisibility/events/TestEventsHandler.java

@@ -90,6 +90,12 @@ void onTestIgnore(
   @Nonnull
   TestExecutionPolicy executionPolicy(TestIdentifier test, TestSourceData source);
 
+  /**
+   * Returns the priority of the test execution that can be used for ordering tests. The higher the
+   * value, the higher the priority, meaning that the test should be executed earlier.
+   */
+  int executionPriority(@Nullable TestIdentifier test, @Nonnull TestSourceData testSourceData);
+
   /**
    * Returns the reason for skipping a test IF it can be skipped.
    *