Remove skip and shouldBeSkipped methods from TestEventsHandler in favor of isSkippable (#8286)

Author: nikita-tkachenko-datadog

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

@@ -31,21 +31,12 @@ TestSuiteImpl testSuiteStart(
   boolean isModified(TestSourceData testSourceData);
 
   /**
-   * Checks if a given test should be skipped with Intelligent Test Runner or not
+   * Checks if a given test can be skipped with Intelligent Test Runner or not.
    *
    * @param test Test to be checked
    * @return {@code true} if the test can be skipped, {@code false} otherwise
    */
-  boolean shouldBeSkipped(TestIdentifier test);
-
-  /**
-   * Checks if a given test can be skipped with Intelligent Test Runner or not. If the test is
-   * considered skippable, the count of skippable tests is incremented.
-   *
-   * @param test Test to be checked
-   * @return {@code true} if the test can be skipped, {@code false} otherwise
-   */
-  boolean skip(TestIdentifier test);
+  boolean isSkippable(TestIdentifier test);
 
   @Nonnull
   TestRetryPolicy retryPolicy(TestIdentifier test, TestSourceData testSource);

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

@@ -37,6 +37,7 @@
 import datadog.trace.civisibility.source.LinesResolver;
 import datadog.trace.civisibility.source.SourcePathResolver;
 import datadog.trace.civisibility.source.SourceResolutionException;
+import datadog.trace.civisibility.test.ExecutionResults;
 import java.lang.reflect.Method;
 import java.util.Collection;
 import java.util.Collections;
@@ -50,6 +51,7 @@ public class TestImpl implements DDTest {
   private static final Logger log = LoggerFactory.getLogger(TestImpl.class);
 
   private final CiVisibilityMetricCollector metricCollector;
+  private final ExecutionResults executionResults;
   private final TestFrameworkInstrumentation instrumentation;
   private final AgentSpan span;
   private final DDTraceId sessionId;
@@ -78,11 +80,13 @@ public TestImpl(
       LinesResolver linesResolver,
       Codeowners codeowners,
       CoverageStore.Factory coverageStoreFactory,
+      ExecutionResults executionResults,
       Consumer<AgentSpan> onSpanFinish) {
     this.instrumentation = instrumentation;
     this.metricCollector = metricCollector;
     this.sessionId = moduleSpanContext.getTraceId();
     this.suiteId = suiteId;
+    this.executionResults = executionResults;
     this.onSpanFinish = onSpanFinish;
 
     this.identifier = new TestIdentifier(testSuiteName, testName, testParameters);
@@ -258,6 +262,10 @@ public void end(@Nullable Long endTime) {
       span.finish();
     }
 
+    if (InstrumentationBridge.ITR_SKIP_REASON.equals(span.getTag(Tags.TEST_SKIP_REASON))) {
+      executionResults.incrementTestsSkippedByItr();
+    }
+
     metricCollector.add(
         CiVisibilityCountMetric.EVENT_FINISHED,
         1,

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

@@ -22,6 +22,7 @@
 import datadog.trace.civisibility.source.LinesResolver;
 import datadog.trace.civisibility.source.SourcePathResolver;
 import datadog.trace.civisibility.source.SourceResolutionException;
+import datadog.trace.civisibility.test.ExecutionResults;
 import datadog.trace.civisibility.utils.SpanUtils;
 import java.lang.reflect.Method;
 import java.util.Collection;
@@ -49,6 +50,7 @@ public class TestSuiteImpl implements DDTestSuite {
   private final Codeowners codeowners;
   private final LinesResolver linesResolver;
   private final CoverageStore.Factory coverageStoreFactory;
+  private final ExecutionResults executionResults;
   private final boolean parallelized;
   private final Consumer<AgentSpan> onSpanFinish;
 
@@ -69,6 +71,7 @@ public TestSuiteImpl(
       Codeowners codeowners,
       LinesResolver linesResolver,
       CoverageStore.Factory coverageStoreFactory,
+      ExecutionResults executionResults,
       Consumer<AgentSpan> onSpanFinish) {
     this.moduleSpanContext = moduleSpanContext;
     this.moduleName = moduleName;
@@ -84,6 +87,7 @@ public TestSuiteImpl(
     this.codeowners = codeowners;
     this.linesResolver = linesResolver;
     this.coverageStoreFactory = coverageStoreFactory;
+    this.executionResults = executionResults;
     this.onSpanFinish = onSpanFinish;
 
     AgentTracer.SpanBuilder spanBuilder =
@@ -254,6 +258,7 @@ public TestImpl testStart(
         linesResolver,
         codeowners,
         coverageStoreFactory,
+        executionResults,
         SpanUtils.propagateCiVisibilityTagsTo(span));
   }
 }

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

@@ -25,6 +25,7 @@
 import datadog.trace.civisibility.ipc.TestFramework;
 import datadog.trace.civisibility.source.LinesResolver;
 import datadog.trace.civisibility.source.SourcePathResolver;
+import datadog.trace.civisibility.test.ExecutionResults;
 import datadog.trace.civisibility.test.ExecutionStrategy;
 import java.util.Collection;
 import java.util.TreeSet;
@@ -46,6 +47,7 @@ public class ProxyTestModule implements TestFrameworkModule {
   private final AgentSpanContext parentProcessModuleContext;
   private final String moduleName;
   private final ExecutionStrategy executionStrategy;
+  private final ExecutionResults executionResults;
   private final SignalClient.Factory signalClientFactory;
   private final ChildProcessCoverageReporter childProcessCoverageReporter;
   private final Config config;
@@ -73,6 +75,7 @@ public ProxyTestModule(
     this.parentProcessModuleContext = parentProcessModuleContext;
     this.moduleName = moduleName;
     this.executionStrategy = executionStrategy;
+    this.executionResults = new ExecutionResults();
     this.signalClientFactory = signalClientFactory;
     this.childProcessCoverageReporter = childProcessCoverageReporter;
     this.config = config;
@@ -100,13 +103,8 @@ public boolean isModified(TestSourceData testSourceData) {
   }
 
   @Override
-  public boolean shouldBeSkipped(TestIdentifier test) {
-    return executionStrategy.shouldBeSkipped(test);
-  }
-
-  @Override
-  public boolean skip(TestIdentifier test) {
-    return executionStrategy.skip(test);
+  public boolean isSkippable(TestIdentifier test) {
+    return executionStrategy.isSkippable(test);
   }
 
   @Override
@@ -143,7 +141,7 @@ private void sendModuleExecutionResult() {
       boolean earlyFlakeDetectionEnabled = earlyFlakeDetectionSettings.isEnabled();
       boolean earlyFlakeDetectionFaulty =
           earlyFlakeDetectionEnabled && executionStrategy.isEFDLimitReached();
-      long testsSkippedTotal = executionStrategy.getTestsSkipped();
+      long testsSkippedTotal = executionResults.getTestsSkippedByItr();
 
       signalClient.send(
           new ModuleExecutionResult(
@@ -185,6 +183,7 @@ public TestSuiteImpl testSuiteStart(
         codeowners,
         linesResolver,
         coverageStoreFactory,
+        executionResults,
         this::propagateTestFrameworkData);
   }
 

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

@@ -22,6 +22,7 @@
 import datadog.trace.civisibility.domain.TestSuiteImpl;
 import datadog.trace.civisibility.source.LinesResolver;
 import datadog.trace.civisibility.source.SourcePathResolver;
+import datadog.trace.civisibility.test.ExecutionResults;
 import datadog.trace.civisibility.test.ExecutionStrategy;
 import datadog.trace.civisibility.utils.SpanUtils;
 import java.util.function.Consumer;
@@ -39,6 +40,7 @@ public class HeadlessTestModule extends AbstractTestModule implements TestFramew
 
   private final CoverageStore.Factory coverageStoreFactory;
   private final ExecutionStrategy executionStrategy;
+  private final ExecutionResults executionResults;
 
   public HeadlessTestModule(
       AgentSpanContext sessionSpanContext,
@@ -67,6 +69,7 @@ public HeadlessTestModule(
         onSpanFinish);
     this.coverageStoreFactory = coverageStoreFactory;
     this.executionStrategy = executionStrategy;
+    this.executionResults = new ExecutionResults();
   }
 
   @Override
@@ -85,13 +88,8 @@ public boolean isModified(TestSourceData testSourceData) {
   }
 
   @Override
-  public boolean shouldBeSkipped(TestIdentifier test) {
-    return executionStrategy.shouldBeSkipped(test);
-  }
-
-  @Override
-  public boolean skip(TestIdentifier test) {
-    return executionStrategy.skip(test);
+  public boolean isSkippable(TestIdentifier test) {
+    return executionStrategy.isSkippable(test);
   }
 
   @Override
@@ -111,7 +109,7 @@ public void end(@Nullable Long endTime) {
       setTag(Tags.TEST_ITR_TESTS_SKIPPING_ENABLED, true);
       setTag(Tags.TEST_ITR_TESTS_SKIPPING_TYPE, "test");
 
-      long testsSkippedTotal = executionStrategy.getTestsSkipped();
+      long testsSkippedTotal = executionResults.getTestsSkippedByItr();
       setTag(Tags.TEST_ITR_TESTS_SKIPPING_COUNT, testsSkippedTotal);
       if (testsSkippedTotal > 0) {
         setTag(DDTags.CI_ITR_TESTS_SKIPPED, true);
@@ -154,6 +152,7 @@ public TestSuiteImpl testSuiteStart(
         codeowners,
         linesResolver,
         coverageStoreFactory,
+        executionResults,
         SpanUtils.propagateCiVisibilityTagsTo(span));
   }
 }

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

@@ -14,6 +14,7 @@
 import datadog.trace.civisibility.domain.TestSuiteImpl;
 import datadog.trace.civisibility.source.LinesResolver;
 import datadog.trace.civisibility.source.SourcePathResolver;
+import datadog.trace.civisibility.test.ExecutionResults;
 import datadog.trace.civisibility.utils.SpanUtils;
 import java.util.function.Consumer;
 import javax.annotation.Nullable;
@@ -25,6 +26,7 @@
 public class ManualApiTestModule extends AbstractTestModule implements DDTestModule {
 
   private final CoverageStore.Factory coverageStoreFactory;
+  private final ExecutionResults executionResults = new ExecutionResults();
 
   public ManualApiTestModule(
       AgentSpanContext sessionSpanContext,
@@ -76,6 +78,7 @@ public TestSuiteImpl testSuiteStart(
         codeowners,
         linesResolver,
         coverageStoreFactory,
+        executionResults,
         SpanUtils.propagateCiVisibilityTagsTo(span));
   }
 }

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

@@ -91,12 +91,7 @@ public void onTestIgnore(
   }
 
   @Override
-  public boolean skip(TestIdentifier test) {
-    return false;
-  }
-
-  @Override
-  public boolean shouldBeSkipped(TestIdentifier test) {
+  public boolean isSkippable(TestIdentifier test) {
     return false;
   }
 

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

@@ -187,7 +187,7 @@ public void onTestStart(
           test.setTag(Tags.TEST_ITR_UNSKIPPABLE, true);
           metricCollector.add(CiVisibilityCountMetric.ITR_UNSKIPPABLE, 1, EventType.TEST);
 
-          if (testModule.shouldBeSkipped(thisTest)) {
+          if (testModule.isSkippable(thisTest)) {
             test.setTag(Tags.TEST_ITR_FORCED_RUN, true);
             metricCollector.add(CiVisibilityCountMetric.ITR_FORCED_RUN, 1, EventType.TEST);
           }
@@ -260,16 +260,6 @@ public void onTestIgnore(
     onTestFinish(testDescriptor, null);
   }
 
-  @Override
-  public boolean skip(TestIdentifier test) {
-    return testModule.skip(test);
-  }
-
-  @Override
-  public boolean shouldBeSkipped(TestIdentifier test) {
-    return testModule.shouldBeSkipped(test);
-  }
-
   @Override
   @Nonnull
   public TestRetryPolicy retryPolicy(TestIdentifier test, TestSourceData testSource) {
@@ -286,6 +276,11 @@ public boolean isFlaky(TestIdentifier test) {
     return testModule.isFlaky(test);
   }
 
+  @Override
+  public boolean isSkippable(TestIdentifier test) {
+    return testModule.isSkippable(test);
+  }
+
   @Override
   public void close() {
     testModule.end(null);

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

@@ -0,0 +1,16 @@
+package datadog.trace.civisibility.test;
+
+import java.util.concurrent.atomic.LongAdder;
+
+public class ExecutionResults {
+
+  private final LongAdder testsSkippedByItr = new LongAdder();
+
+  public void incrementTestsSkippedByItr() {
+    testsSkippedByItr.increment();
+  }
+
+  public long getTestsSkippedByItr() {
+    return testsSkippedByItr.sum();
+  }
+}

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

@@ -16,7 +16,6 @@
 import java.util.Collection;
 import java.util.Map;
 import java.util.concurrent.atomic.AtomicInteger;
-import java.util.concurrent.atomic.LongAdder;
 import javax.annotation.Nonnull;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
@@ -25,7 +24,6 @@ public class ExecutionStrategy {
 
   private static final Logger LOGGER = LoggerFactory.getLogger(ExecutionStrategy.class);
 
-  private final LongAdder testsSkipped = new LongAdder();
   private final AtomicInteger earlyFlakeDetectionsUsed = new AtomicInteger(0);
   private final AtomicInteger autoRetriesUsed = new AtomicInteger(0);
 
@@ -50,10 +48,6 @@ public ExecutionSettings getExecutionSettings() {
     return executionSettings;
   }
 
-  public long getTestsSkipped() {
-    return testsSkipped.sum();
-  }
-
   public boolean isNew(TestIdentifier test) {
     Collection<TestIdentifier> knownTests = executionSettings.getKnownTests();
     return knownTests != null && !knownTests.contains(test.withoutParameters());
@@ -64,7 +58,7 @@ public boolean isFlaky(TestIdentifier test) {
     return flakyTests != null && flakyTests.contains(test.withoutParameters());
   }
 
-  public boolean shouldBeSkipped(TestIdentifier test) {
+  public boolean isSkippable(TestIdentifier test) {
     if (test == null) {
       return false;
     }
@@ -78,15 +72,6 @@ public boolean shouldBeSkipped(TestIdentifier test) {
             && testMetadata.isMissingLineCodeCoverage());
   }
 
-  public boolean skip(TestIdentifier test) {
-    if (shouldBeSkipped(test)) {
-      testsSkipped.increment();
-      return true;
-    } else {
-      return false;
-    }
-  }
-
   @Nonnull
   public TestRetryPolicy retryPolicy(TestIdentifier test, TestSourceData testSource) {
     if (test == null) {