Fix TestNG retry listener ordering problem

nikita-tkachenko-datadog · nikita-tkachenko-datadog · commit f5c4b8f301db · 2025-02-03T14:49:14.000+01:00
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
@@ -58,7 +58,8 @@ public void onTestStart(
       @Nullable String testParameters,
       @Nullable Collection<String> categories,
       @Nonnull TestSourceData testSourceData,
-      @Nullable Long startTime) {
+      @Nullable Long startTime,
+      @Nullable TestExecutionHistory testExecutionHistory) {
     // 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
@@ -140,7 +140,8 @@ public void onTestStart(
       final @Nullable String testParameters,
       final @Nullable Collection<String> categories,
       final @Nonnull TestSourceData testSourceData,
-      final @Nullable Long startTime) {
+      final @Nullable Long startTime,
+      final @Nullable TestExecutionHistory testExecutionHistory) {
     if (skipTrace(testSourceData.getTestClass())) {
       return;
     }
@@ -170,6 +171,14 @@ public void onTestStart(
       test.setTag(Tags.TEST_MANAGEMENT_IS_QUARANTINED, true);
     }
 
+    if (testExecutionHistory != null) {
+      RetryReason retryReason = testExecutionHistory.currentExecutionRetryReason();
+      if (retryReason != null) {
+        test.setTag(Tags.TEST_IS_RETRY, true);
+        test.setTag(Tags.TEST_RETRY_REASON, retryReason);
+      }
+    }
+
     if (testFramework != null) {
       test.setTag(Tags.TEST_FRAMEWORK, testFramework);
       if (testFrameworkVersion != null) {
@@ -237,12 +246,6 @@ public void onTestFinish(
     }
 
     if (testExecutionHistory != null) {
-      RetryReason retryReason = testExecutionHistory.currentExecutionRetryReason();
-      if (retryReason != null) {
-        test.setTag(Tags.TEST_IS_RETRY, true);
-        test.setTag(Tags.TEST_RETRY_REASON, retryReason);
-      }
-
       if (test.hasFailed() && testExecutionHistory.hasFailedAllRetries()) {
         test.setTag(Tags.TEST_HAS_FAILED_ALL_RETRIES, true);
       }
@@ -271,6 +274,7 @@ public void onTestIgnore(
         testParameters,
         categories,
         testSourceData,
+        null,
         null);
     onTestSkip(testDescriptor, reason);
     onTestFinish(testDescriptor, null, null);
diff --git a/dd-java-agent/instrumentation/junit-4.10/cucumber-junit-4/src/main/java/datadog/trace/instrumentation/junit4/CucumberTracingListener.java b/dd-java-agent/instrumentation/junit-4.10/cucumber-junit-4/src/main/java/datadog/trace/instrumentation/junit4/CucumberTracingListener.java
@@ -80,7 +80,8 @@ public void testStarted(final Description description) {
         null,
         categories,
         TestSourceData.UNKNOWN,
-        null);
+        null,
+        executionHistories.get(description));
 
     recordFeatureFileCodeCoverage(description);
   }
diff --git a/dd-java-agent/instrumentation/junit-4.10/munit-junit-4/src/main/java/datadog/trace/instrumentation/junit4/MUnitTracingListener.java b/dd-java-agent/instrumentation/junit-4.10/munit-junit-4/src/main/java/datadog/trace/instrumentation/junit4/MUnitTracingListener.java
@@ -85,7 +85,8 @@ public void testStarted(final Description description) {
         null,
         categories,
         JUnit4Utils.toTestSourceData(description),
-        null);
+        null,
+        executionHistories.get(description));
   }
 
   @Override
@@ -160,6 +161,7 @@ public void testIgnored(final Description description) {
             null,
             categories,
             JUnit4Utils.toTestSourceData(description),
+            null,
             null);
       }
       TestEventsHandlerHolder.TEST_EVENTS_HANDLER.onTestSkip(testDescriptor, null);
diff --git a/dd-java-agent/instrumentation/junit-4.10/src/main/java/datadog/trace/instrumentation/junit4/JUnit4TracingListener.java b/dd-java-agent/instrumentation/junit-4.10/src/main/java/datadog/trace/instrumentation/junit4/JUnit4TracingListener.java
@@ -84,7 +84,8 @@ public void testStarted(final Description description) {
         testParameters,
         categories,
         testSourceData,
-        null);
+        null,
+        executionHistories.get(description));
   }
 
   @Override
diff --git a/dd-java-agent/instrumentation/junit-5.3/cucumber-junit-5/src/main/java/datadog/trace/instrumentation/junit5/CucumberTracingListener.java b/dd-java-agent/instrumentation/junit-5.3/cucumber-junit-5/src/main/java/datadog/trace/instrumentation/junit5/CucumberTracingListener.java
@@ -124,7 +124,8 @@ private void testResourceExecutionStarted(
         null,
         tags,
         TestSourceData.UNKNOWN,
-        null);
+        null,
+        TestEventsHandlerHolder.getExecutionHistory(testDescriptor));
 
     CoveragePerTestBridge.recordCoverage(classpathResourceName);
   }
diff --git a/dd-java-agent/instrumentation/junit-5.3/spock-junit-5/src/main/java/datadog/trace/instrumentation/junit5/SpockTracingListener.java b/dd-java-agent/instrumentation/junit-5.3/spock-junit-5/src/main/java/datadog/trace/instrumentation/junit5/SpockTracingListener.java
@@ -125,7 +125,8 @@ private void testMethodExecutionStarted(TestDescriptor testDescriptor, MethodSou
         testParameters,
         tags,
         testSourceData,
-        null);
+        null,
+        TestEventsHandlerHolder.getExecutionHistory(testDescriptor));
   }
 
   private void testCaseExecutionFinished(
diff --git a/dd-java-agent/instrumentation/junit-5.3/src/main/java/datadog/trace/instrumentation/junit5/TracingListener.java b/dd-java-agent/instrumentation/junit-5.3/src/main/java/datadog/trace/instrumentation/junit5/TracingListener.java
@@ -125,7 +125,8 @@ private void testMethodExecutionStarted(TestDescriptor testDescriptor, MethodSou
         testParameters,
         tags,
         testSourceData,
-        null);
+        null,
+        TestEventsHandlerHolder.getExecutionHistory(testDescriptor));
   }
 
   private void testCaseExecutionFinished(
diff --git a/dd-java-agent/instrumentation/karate/src/main/java/datadog/trace/instrumentation/karate/KarateExecutionInstrumentation.java b/dd-java-agent/instrumentation/karate/src/main/java/datadog/trace/instrumentation/karate/KarateExecutionInstrumentation.java
@@ -104,7 +104,7 @@ public static void afterExecute(@Advice.This ScenarioRuntime scenarioRuntime) {
       while (executionPolicy.retry(!context.getAndResetFailed(), duration)) {
         ScenarioRuntime retry =
             new ScenarioRuntime(scenarioRuntime.featureRuntime, scenarioRuntime.scenario);
-        retry.magicVariables.put(KarateUtils.EXECUTION_HISTORY_MAGIC_VARIABLE, executionPolicy);
+        retry.magicVariables.put(KarateUtils.EXECUTION_HISTORY_MAGICVARIABLE, executionPolicy);
         retry.run();
         retry.featureRuntime.result.addResult(retry.result);
         finalResult = retry.result;
diff --git a/dd-java-agent/instrumentation/karate/src/main/java/datadog/trace/instrumentation/karate/KarateTracingHook.java b/dd-java-agent/instrumentation/karate/src/main/java/datadog/trace/instrumentation/karate/KarateTracingHook.java
@@ -144,7 +144,8 @@ public boolean beforeScenario(ScenarioRuntime sr) {
         parameters,
         categories,
         TestSourceData.UNKNOWN,
-        null);
+        null,
+        (TestExecutionHistory) sr.magicVariables.get(KarateUtils.EXECUTION_HISTORY_MAGICVARIABLE));
     return true;
   }
 
@@ -163,7 +164,7 @@ public void afterScenario(ScenarioRuntime sr) {
     }
 
     TestExecutionHistory executionHistory =
-        (TestExecutionHistory) sr.magicVariables.get(KarateUtils.EXECUTION_HISTORY_MAGIC_VARIABLE);
+        (TestExecutionHistory) sr.magicVariables.get(KarateUtils.EXECUTION_HISTORY_MAGICVARIABLE);
     TestEventsHandlerHolder.TEST_EVENTS_HANDLER.onTestFinish(
         testDescriptor, null, executionHistory);
 
diff --git a/dd-java-agent/instrumentation/karate/src/main/java/datadog/trace/instrumentation/karate/KarateUtils.java b/dd-java-agent/instrumentation/karate/src/main/java/datadog/trace/instrumentation/karate/KarateUtils.java
@@ -21,7 +21,7 @@
 
 public abstract class KarateUtils {
 
-  public static final String EXECUTION_HISTORY_MAGIC_VARIABLE = "__datadog_execution_history";
+  public static final String EXECUTION_HISTORY_MAGICVARIABLE = "__datadog_execution_history";
 
   private KarateUtils() {}
 
diff --git a/dd-java-agent/instrumentation/scalatest/src/main/java/datadog/trace/instrumentation/scalatest/DatadogReporter.java b/dd-java-agent/instrumentation/scalatest/src/main/java/datadog/trace/instrumentation/scalatest/DatadogReporter.java
@@ -155,7 +155,8 @@ private static void onTestStart(TestStarting event) {
         testParameters,
         categories,
         new TestSourceData(testClass, null, null),
-        null);
+        null,
+        context.popExecutionHistory(testIdentifier));
   }
 
   private static void onTestSuccess(TestSucceeded event) {
diff --git a/dd-java-agent/instrumentation/scalatest/src/main/java/datadog/trace/instrumentation/scalatest/RunContext.java b/dd-java-agent/instrumentation/scalatest/src/main/java/datadog/trace/instrumentation/scalatest/RunContext.java
@@ -135,6 +135,11 @@ public TestExecutionPolicy getOrCreateExecutionPolicy(
         testIdentifier, test -> eventHandler.executionPolicy(test, testSourceData));
   }
 
+  @Nullable
+  public TestExecutionHistory getExecutionHistory(TestIdentifier testIdentifier) {
+    return executionPolicies.get(testIdentifier);
+  }
+
   @Nullable
   public TestExecutionHistory popExecutionHistory(TestIdentifier testIdentifier) {
     TestExecutionPolicy[] holder = new TestExecutionPolicy[1];
diff --git a/dd-java-agent/instrumentation/testng/src/main/java/datadog/trace/instrumentation/testng/TracingListener.java b/dd-java-agent/instrumentation/testng/src/main/java/datadog/trace/instrumentation/testng/TracingListener.java
@@ -92,7 +92,8 @@ public void onTestStart(final ITestResult result) {
         testParameters,
         groups,
         testSourceData,
-        null);
+        null,
+        executionHistory(result));
   }
 
   @Nullable
diff --git a/dd-java-agent/instrumentation/weaver/src/main/java/datadog/trace/instrumentation/weaver/DatadogWeaverReporter.java b/dd-java-agent/instrumentation/weaver/src/main/java/datadog/trace/instrumentation/weaver/DatadogWeaverReporter.java
@@ -106,7 +106,8 @@ public static void onTestFinished(TestFinished event, TaskDef taskDef) {
         testParameters,
         categories,
         new TestSourceData(testClass, testMethod, testMethodName),
-        startMicros);
+        startMicros,
+        executionHistory);
 
     if (testOutcome.result() instanceof Result.Ignored) {
       Result.Ignored result = (Result.Ignored) testOutcome.result();
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
@@ -53,6 +53,7 @@ void onTestSuiteStart(
    * @param categories test categories (or test tags) if the test case is marked with any
    * @param testSourceData metadata for locating the source code for the test case
    * @param startTime the timestamp of the test execution start ({@code null} for current timestamp)
+   * @param testExecutionHistory the history of executions of this test case
    */
   void onTestStart(
       SuiteKey suiteDescriptor,
@@ -63,7 +64,8 @@ void onTestStart(
       @Nullable String testParameters,
       @Nullable Collection<String> categories,
       @Nonnull TestSourceData testSourceData,
-      @Nullable Long startTime);
+      @Nullable Long startTime,
+      @Nullable TestExecutionHistory testExecutionHistory);
 
   void onTestSkip(TestKey descriptor, @Nullable String reason);
 

Original file line number	Diff line number	Diff line change
`@@ -58,7 +58,8 @@ public void onTestStart(`
`58`	`58`	`@Nullable String testParameters,`
`59`	`59`	`@Nullable Collection<String> categories,`
`60`	`60`	`@Nonnull TestSourceData testSourceData,`
`61`		`- @Nullable Long startTime) {`
	`61`	`+ @Nullable Long startTime,`
	`62`	`+ @Nullable TestExecutionHistory testExecutionHistory) {`
`62`	`63`	`// do nothing`
`63`	`64`	`}`
`64`	`65`
Original file line number	Diff line number	Diff line change
`@@ -80,7 +80,8 @@ public void testStarted(final Description description) {`
`80`	`80`	`null,`
`81`	`81`	`categories,`
`82`	`82`	`TestSourceData.UNKNOWN,`
`83`		`- null);`
	`83`	`+ null,`
	`84`	`+ executionHistories.get(description));`
`84`	`85`
`85`	`86`	`recordFeatureFileCodeCoverage(description);`
`86`	`87`	`}`
Original file line number	Diff line number	Diff line change
`@@ -84,7 +84,8 @@ public void testStarted(final Description description) {`
`84`	`84`	`testParameters,`
`85`	`85`	`categories,`
`86`	`86`	`testSourceData,`
`87`		`- null);`
	`87`	`+ null,`
	`88`	`+ executionHistories.get(description));`
`88`	`89`	`}`
`89`	`90`
`90`	`91`	`@Override`
Original file line number	Diff line number	Diff line change
`@@ -124,7 +124,8 @@ private void testResourceExecutionStarted(`
`124`	`124`	`null,`
`125`	`125`	`tags,`
`126`	`126`	`TestSourceData.UNKNOWN,`
`127`		`- null);`
	`127`	`+ null,`
	`128`	`+ TestEventsHandlerHolder.getExecutionHistory(testDescriptor));`
`128`	`129`
`129`	`130`	`CoveragePerTestBridge.recordCoverage(classpathResourceName);`
`130`	`131`	`}`
Original file line number	Diff line number	Diff line change
`@@ -125,7 +125,8 @@ private void testMethodExecutionStarted(TestDescriptor testDescriptor, MethodSou`
`125`	`125`	`testParameters,`
`126`	`126`	`tags,`
`127`	`127`	`testSourceData,`
`128`		`- null);`
	`128`	`+ null,`
	`129`	`+ TestEventsHandlerHolder.getExecutionHistory(testDescriptor));`
`129`	`130`	`}`
`130`	`131`
`131`	`132`	`private void testCaseExecutionFinished(`