From 95b3762f7d57452f5463b598e35f501924c1c121 Mon Sep 17 00:00:00 2001 From: Nikita Tkachenko Date: Fri, 28 Feb 2025 17:35:00 +0100 Subject: [PATCH] Consider modified tests when applying fail-fast tests ordering --- .../domain/TestFrameworkModule.java | 8 +++++- .../domain/buildsystem/ProxyTestModule.java | 7 ++++- .../domain/headless/HeadlessTestModule.java | 7 ++++- .../events/NoOpTestEventsHandler.java | 6 ++++ .../events/TestEventsHandlerImpl.java | 5 ++++ .../civisibility/test/ExecutionStrategy.java | 26 ++++++++++++++++- .../junit5/order/FailFastClassOrderer.java | 28 +++++++++++-------- .../junit5/order/FailFastMethodOrderer.java | 14 ++++++---- .../instrumentation/testng/TestNGUtils.java | 4 +++ .../order/FailFastOrderInterceptor.java | 15 ++++++---- .../events/TestEventsHandler.java | 6 ++++ 11 files changed, 98 insertions(+), 28 deletions(-) diff --git a/dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/domain/TestFrameworkModule.java b/dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/domain/TestFrameworkModule.java index 05d9a0130f3..ff07f324cf0 100644 --- a/dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/domain/TestFrameworkModule.java +++ b/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); } diff --git a/dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/domain/buildsystem/ProxyTestModule.java b/dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/domain/buildsystem/ProxyTestModule.java index 8784278770b..aa05502ffe9 100644 --- a/dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/domain/buildsystem/ProxyTestModule.java +++ b/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, diff --git a/dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/domain/headless/HeadlessTestModule.java b/dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/domain/headless/HeadlessTestModule.java index 997b8fbc020..f9aa26314b8 100644 --- a/dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/domain/headless/HeadlessTestModule.java +++ b/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(); diff --git a/dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/events/NoOpTestEventsHandler.java b/dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/events/NoOpTestEventsHandler.java index 2a0f408a56e..024d25049f1 100644 --- a/dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/events/NoOpTestEventsHandler.java +++ b/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 diff --git a/dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/events/TestEventsHandlerImpl.java b/dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/events/TestEventsHandlerImpl.java index d94cfa89008..22a1ccf84ee 100644 --- a/dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/events/TestEventsHandlerImpl.java +++ b/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); diff --git a/dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/test/ExecutionStrategy.java b/dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/test/ExecutionStrategy.java index 7f2f794120e..334428e6c59 100644 --- a/dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/test/ExecutionStrategy.java +++ b/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; + } } diff --git a/dd-java-agent/instrumentation/junit-5.3/junit-5.8/src/main/java/datadog/trace/instrumentation/junit5/order/FailFastClassOrderer.java b/dd-java-agent/instrumentation/junit-5.3/junit-5.8/src/main/java/datadog/trace/instrumentation/junit5/order/FailFastClassOrderer.java index 32e89b901c3..01709b6211f 100644 --- a/dd-java-agent/instrumentation/junit-5.3/junit-5.8/src/main/java/datadog/trace/instrumentation/junit5/order/FailFastClassOrderer.java +++ b/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 testEventsHandler; private final @Nullable ClassOrderer delegate; - private final Comparator knownTestSuitesComparator; + private final Comparator executionOrderComparator; public FailFastClassOrderer( TestEventsHandler 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 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); } } diff --git a/dd-java-agent/instrumentation/junit-5.3/junit-5.8/src/main/java/datadog/trace/instrumentation/junit5/order/FailFastMethodOrderer.java b/dd-java-agent/instrumentation/junit-5.3/junit-5.8/src/main/java/datadog/trace/instrumentation/junit5/order/FailFastMethodOrderer.java index 2b1d1385c89..4c990fec64f 100644 --- a/dd-java-agent/instrumentation/junit-5.3/junit-5.8/src/main/java/datadog/trace/instrumentation/junit5/order/FailFastMethodOrderer.java +++ b/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 testEventsHandler; private final @Nullable MethodOrderer delegate; - private final Comparator knownTestMethodsComparator; + private final Comparator executionOrderComparator; public FailFastMethodOrderer( TestEventsHandler 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); } } diff --git a/dd-java-agent/instrumentation/testng/src/main/java/datadog/trace/instrumentation/testng/TestNGUtils.java b/dd-java-agent/instrumentation/testng/src/main/java/datadog/trace/instrumentation/testng/TestNGUtils.java index 3fd6df4794f..76c34f13fda 100644 --- a/dd-java-agent/instrumentation/testng/src/main/java/datadog/trace/instrumentation/testng/TestNGUtils.java +++ b/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(); diff --git a/dd-java-agent/instrumentation/testng/testng-7/src/main/java/datadog/trace/instrumentation/testng7/order/FailFastOrderInterceptor.java b/dd-java-agent/instrumentation/testng/testng-7/src/main/java/datadog/trace/instrumentation/testng7/order/FailFastOrderInterceptor.java index 62c7734f246..f01337e1723 100644 --- a/dd-java-agent/instrumentation/testng/testng-7/src/main/java/datadog/trace/instrumentation/testng7/order/FailFastOrderInterceptor.java +++ b/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 testEventsHandler; - private final Comparator knownAndStableTestsComparator; + private final Comparator executionOrderComparator; public FailFastOrderInterceptor( TestEventsHandler 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 intercept(List list, ITestContext iTestContext) { - list.sort(knownAndStableTestsComparator); + list.sort(executionOrderComparator); return list; } } diff --git a/internal-api/src/main/java/datadog/trace/api/civisibility/events/TestEventsHandler.java b/internal-api/src/main/java/datadog/trace/api/civisibility/events/TestEventsHandler.java index 0d40bb98201..bc7c7fd8730 100644 --- a/internal-api/src/main/java/datadog/trace/api/civisibility/events/TestEventsHandler.java +++ b/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. *