Do not report code coverage for skipped tests (#7244)

DataDog · Jun 26, 2024 · 2f63f03 · 2f63f03
1 parent 9e325ef
commit 2f63f03
Show file tree

Hide file tree

Showing 3 changed files with 41 additions and 22 deletions.
diff --git a/...a-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/domain/TestImpl.java b/...a-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/domain/TestImpl.java
@@ -220,11 +220,15 @@ public void end(@Nullable Long endTime) {
     InstrumentationTestBridge.fireBeforeTestEnd(context);
 
     CoverageBridge.removeThreadLocalCoverageProbeStore();
-    boolean coveragesGathered =
-        context.getCoverageProbeStore().report(sessionId, suiteId, span.getSpanId());
-    if (!coveragesGathered && !TestStatus.skip.equals(span.getTag(Tags.TEST_STATUS))) {
-      // test is not skipped, but no coverages were gathered
-      metricCollector.add(CiVisibilityCountMetric.CODE_COVERAGE_IS_EMPTY, 1);
+
+    // do not process coverage reports for skipped tests
+    if (span.getTag(Tags.TEST_STATUS) != TestStatus.skip) {
+      CoverageProbeStore coverageStore = context.getCoverageProbeStore();
+      boolean coveragesGathered = coverageStore.report(sessionId, suiteId, span.getSpanId());
+      if (!coveragesGathered && !TestStatus.skip.equals(span.getTag(Tags.TEST_STATUS))) {
+        // test is not skipped, but no coverages were gathered
+        metricCollector.add(CiVisibilityCountMetric.CODE_COVERAGE_IS_EMPTY, 1);
+      }
     }
 
     scope.close();

diff --git a/...-agent/agent-ci-visibility/src/test/groovy/datadog/trace/civisibility/TestImplTest.groovy b/...-agent/agent-ci-visibility/src/test/groovy/datadog/trace/civisibility/TestImplTest.groovy
@@ -5,9 +5,11 @@ import datadog.trace.agent.tooling.TracerInstaller
 import datadog.trace.api.Config
 import datadog.trace.api.DDSpanTypes
 import datadog.trace.api.IdGenerationStrategy
+import datadog.trace.api.civisibility.coverage.CoverageProbeStore
 import datadog.trace.api.civisibility.telemetry.tag.TestFrameworkInstrumentation
 import datadog.trace.bootstrap.instrumentation.api.AgentTracer
 import datadog.trace.civisibility.codeowners.NoCodeowners
+import datadog.trace.civisibility.coverage.CoverageProbeStoreFactory
 import datadog.trace.civisibility.coverage.NoopCoverageProbeStore
 import datadog.trace.civisibility.decorator.TestDecoratorImpl
 import datadog.trace.civisibility.domain.TestImpl
@@ -104,7 +106,24 @@ class TestImplTest extends DDSpecification {
     })
   }
 
-  private TestImpl givenATest() {
+  def "test coverage is not reported if test was skipped"() {
+    setup:
+    def coverageStore = Mock(CoverageProbeStore)
+    def coveageStoreFactory = Stub(CoverageProbeStoreFactory)
+    coveageStoreFactory.create(_, _) >> coverageStore
+
+    def test = givenATest(coveageStoreFactory)
+
+    when:
+    test.setSkipReason("skipped")
+    test.end(null)
+
+    then:
+    0 * coverageStore.report(_, _, _)
+  }
+
+  private TestImpl givenATest(
+    CoverageProbeStoreFactory coverageProbeStoreFactory = new NoopCoverageProbeStore.NoopCoverageProbeStoreFactory()) {
     def sessionId = 123
     def moduleId = 456
     def suiteId = 789
@@ -115,7 +134,6 @@ class TestImplTest extends DDSpecification {
     def testDecorator = new TestDecoratorImpl("component", [:])
     def methodLinesResolver = { it -> MethodLinesResolver.MethodLines.EMPTY }
     def codeowners = NoCodeowners.INSTANCE
-    def coverageProbeStoreFactory = new NoopCoverageProbeStore.NoopCoverageProbeStoreFactory()
     new TestImpl(
       sessionId,
       moduleId,

diff --git a/...race-core/src/main/java/datadog/trace/civisibility/writer/ddintake/CiTestCovMapperV2.java b/...race-core/src/main/java/datadog/trace/civisibility/writer/ddintake/CiTestCovMapperV2.java
@@ -30,8 +30,6 @@
 import java.util.Collection;
 import java.util.Collections;
 import java.util.List;
-import java.util.Objects;
-import java.util.stream.Collectors;
 import okhttp3.MultipartBody;
 import okhttp3.RequestBody;
 
@@ -68,17 +66,16 @@ private CiTestCovMapperV2(int size, boolean compressionEnabled) {
   public void map(List<? extends CoreSpan<?>> trace, Writable writable) {
     long serializationStartTimestamp = System.currentTimeMillis();
 
-    List<TestReport> testReports =
-        trace.stream()
-            // only consider test spans, since children spans
-            // share test reports with their parents
-            .filter(CiTestCovMapperV2::isTestSpan)
-            .map(CiTestCovMapperV2::getTestReport)
-            .filter(Objects::nonNull)
-            .filter(TestReport::isNotEmpty)
-            .collect(Collectors.toList());
-
-    for (TestReport testReport : testReports) {
+    for (CoreSpan<?> span : trace) {
+      if (!isTestSpan(span)) {
+        continue;
+      }
+
+      TestReport testReport = getTestReport(span);
+      if (testReport == null || !testReport.isNotEmpty()) {
+        continue;
+      }
+
       Long testSessionId = testReport.getTestSessionId();
       Long testSuiteId = testReport.getTestSuiteId();
 
@@ -124,9 +121,9 @@ public void map(List<? extends CoreSpan<?>> trace, Writable writable) {
           writable.writeInt(segment.getNumberOfExecutions());
         }
       }
-    }
 
-    eventCount += testReports.size();
+      eventCount++;
+    }
     serializationTimeMillis += (int) (System.currentTimeMillis() - serializationStartTimestamp);
   }