Avoid final field modifications in Karate instrumentation (#11753)

feat: avoid final field modifications in karate instrumentation

fix: revert changes

feat: propagate failed results to original

fix: spotless

nit: comment

Merge branch 'master' into daniel.mohedano/jep-500-karate

Merge branch 'master' into daniel.mohedano/jep-500-karate

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

Author: daniel-mohedano

buildSrc/src/main/kotlin/dd-trace-java.instrumentation.testing-framework-tests.gradle.kts

@@ -6,6 +6,7 @@ logger.info("Avoid executing classes used to test testing frameworks instrumenta
 
 tasks.withType<Test>().configureEach {
   exclude("**/TestAssumption*", "**/TestSuiteSetUpAssumption*")
+  exclude("**/TestContinueOnStepFailure*")
   exclude("**/TestDisableTestTrace*")
   exclude("**/TestError*")
   exclude("**/TestFactory*")

dd-java-agent/instrumentation/karate-1.0/src/main/java/datadog/trace/instrumentation/karate/ExecutionContext.java

@@ -19,9 +19,7 @@ public void setSuppressFailures(boolean suppressFailures) {
     this.suppressFailures = suppressFailures;
   }
 
-  public boolean getAndResetSuppressFailures() {
-    boolean suppressFailures = this.suppressFailures;
-    this.suppressFailures = false;
+  public boolean shouldSuppressFailures() {
     return suppressFailures;
   }
 

dd-java-agent/instrumentation/karate-1.0/src/main/java/datadog/trace/instrumentation/karate/KarateExecutionInstrumentation.java

@@ -103,7 +103,8 @@ public static void afterExecute(@Advice.This ScenarioRuntime scenarioRuntime) {
         return;
       }
 
-      ScenarioResult finalResult = scenarioRuntime.result;
+      ScenarioResult originalResult = scenarioRuntime.result;
+      ScenarioResult finalResult = originalResult;
 
       TestExecutionPolicy executionPolicy = context.getExecutionPolicy();
       while (executionPolicy.applicable()) {
@@ -115,7 +116,12 @@ public static void afterExecute(@Advice.This ScenarioRuntime scenarioRuntime) {
         finalResult = retry.result;
       }
 
-      KarateUtils.setResult(scenarioRuntime, finalResult);
+      // When the scenario is retried, the original runtime's result must reflect the final
+      // attempt's outcome. To avoid final field modifications, the final attempt's failure is
+      // reflected onto the original result via addStepResult
+      if (finalResult.isFailed() && !originalResult.isFailed()) {
+        originalResult.addStepResult(finalResult.getFailedStep());
+      }
 
       CallDepthThreadLocalMap.reset(ScenarioRuntime.class);
     }
@@ -140,7 +146,10 @@ public static void onAddingStepResult(
           return;
         }
 
-        if (executionContext.getAndResetSuppressFailures()) {
+        // Suppress every failing step of a to-be-retried attempt (not just the first): with
+        // continueOnStepFailure a single attempt can add multiple failing steps, and any leak would
+        // mark the original runtime's result failed
+        if (executionContext.shouldSuppressFailures()) {
           stepResult = new StepResult(stepResult.getStep(), KarateUtils.abortedResult());
           stepResult.setFailedReason(result.getError());
           stepResult.setErrorIgnored(true);

dd-java-agent/instrumentation/karate-1.0/src/main/java/datadog/trace/instrumentation/karate/KarateUtils.java

@@ -7,7 +7,6 @@
 import com.intuit.karate.core.FeatureRuntime;
 import com.intuit.karate.core.Result;
 import com.intuit.karate.core.Scenario;
-import com.intuit.karate.core.ScenarioResult;
 import com.intuit.karate.core.ScenarioRuntime;
 import com.intuit.karate.core.Tag;
 import datadog.trace.api.civisibility.config.LibraryCapability;
@@ -46,8 +45,6 @@ private KarateUtils() {}
   // static method to create aborted result has a different signature starting with Karate 1.4.1
   private static final MethodHandle ABORTED_RESULT_STARTTIME_DURATION_NANOS =
       METHOD_HANDLES.method(Result.class, "aborted", long.class, long.class);
-  private static final MethodHandle SCENARIO_RUNTIME_RESULT_SETTER =
-      METHOD_HANDLES.privateFieldSetter(ScenarioRuntime.class, "result");
 
   private static final ComparableVersion karateV12 = new ComparableVersion("1.2.0");
   private static final ComparableVersion karateV13 = new ComparableVersion("1.3.0");
@@ -153,10 +150,6 @@ public static void resetBeforeHook(FeatureRuntime featureRuntime) {
     METHOD_HANDLES.invoke(FEATURE_RUNTIME_BEFORE_HOOK_DONE_SETTER, featureRuntime, false);
   }
 
-  public static void setResult(ScenarioRuntime runtime, ScenarioResult result) {
-    METHOD_HANDLES.invoke(SCENARIO_RUNTIME_RESULT_SETTER, runtime, result);
-  }
-
   public static String getKarateVersion() {
     return FileUtils.KARATE_VERSION;
   }

dd-java-agent/instrumentation/karate-1.0/src/test/groovy/KarateTest.groovy

@@ -72,11 +72,14 @@ class KarateTest extends CiVisibilityInstrumentationTest {
     assertSpansData(testcaseName)
 
     where:
-    testcaseName               | success | tests                           | retriedTests
-    "test-failed"              | false   | [TestFailedKarate]              | []
-    "test-retry-failed"        | false   | [TestFailedKarate]              | [new TestFQN("[org/example/test_failed] test failed", "second scenario")]
-    "test-failed-then-succeed" | true    | [TestFailedThenSucceedKarate]   | [new TestFQN("[org/example/test_failed_then_succeed] test failed", "flaky scenario")]
-    "test-retry-parameterized" | false   | [TestFailedParameterizedKarate] | [
+    testcaseName                 | success | tests                              | retriedTests
+    "test-failed"                | false   | [TestFailedKarate]                 | []
+    "test-retry-failed"          | false   | [TestFailedKarate]                 | [new TestFQN("[org/example/test_failed] test failed", "second scenario")]
+    "test-failed-then-succeed"   | true    | [TestFailedThenSucceedKarate]      | [new TestFQN("[org/example/test_failed_then_succeed] test failed", "flaky scenario")]
+    "test-retry-continue-on-step-failure" | true | [TestContinueOnStepFailureKarate] | [
+      new TestFQN("[org/example/test_continue_on_step_failure] test continue on step failure", "flaky scenario")
+    ]
+    "test-retry-parameterized"   | false   | [TestFailedParameterizedKarate]    | [
       new TestFQN("[org/example/test_failed_parameterized] test parameterized", "first scenario as an outline")
     ]
   }

dd-java-agent/instrumentation/karate-1.0/src/test/java/org/example/Flaky.java

@@ -5,8 +5,16 @@
 public class Flaky {
 
   private static int counter = 0;
+  private static int stepCounter = 0;
 
+  // Fails the first two attempts, passes from the third onwards.
   public static void flake() {
     assertTrue(++counter >= 3);
   }
+
+  // Same flaky behavior exposed as a value, for continueOnStepFailure scenarios that assert it in
+  // several steps. Uses a separate counter because both helpers run in the same test JVM.
+  public static boolean shouldPass() {
+    return ++stepCounter >= 3;
+  }
 }

dd-java-agent/instrumentation/karate-1.0/src/test/java/org/example/TestContinueOnStepFailureKarate.java

@@ -0,0 +1,11 @@
+package org.example;
+
+import com.intuit.karate.junit5.Karate;
+
+public class TestContinueOnStepFailureKarate {
+
+  @Karate.Test
+  public Karate test() {
+    return Karate.run("classpath:org/example/test_continue_on_step_failure.feature");
+  }
+}

dd-java-agent/instrumentation/karate-1.0/src/test/java/org/example/test_continue_on_step_failure.feature

@@ -0,0 +1,7 @@
+Feature: test continue on step failure
+
+  Scenario: flaky scenario
+    * configure continueOnStepFailure = { enabled: true, continueAfter: true }
+    * def pass = Java.type('org.example.Flaky').shouldPass()
+    * match pass == true
+    * match pass == true

dd-java-agent/instrumentation/karate-1.0/src/test/resources/test-retry-continue-on-step-failure/coverages.ftl

@@ -0,0 +1 @@
+[ ]