Skip to content

Commit 8100e2b

Browse files
daniel-mohedanodaniel-mohedano
authored
Include all candidate paths for duplicate keys in coverage reports (#10997)
fix: report duplicate entries in coverage instead of nulling the report fix: refactor source path resolvers to always return collections fix: remove unused variable fix: pre-size structures nit: add @nonnull annotations to overriden methods style: spotless fix: remove unused logging Merge branch 'master' into daniel.mohedano/coverage-report-duplicate-keys Merge branch 'master' into daniel.mohedano/coverage-report-duplicate-keys Co-authored-by: daniel.mohedano <daniel.mohedano@datadoghq.com>
1 parent 65b9b8a commit 8100e2b

23 files changed

Lines changed: 486 additions & 312 deletions

‎dd-java-agent/agent-ci-visibility/build.gradle‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,7 @@ dependencies {
3737
testImplementation group: 'org.skyscreamer', name: 'jsonassert', version: '1.5.1'
3838
testImplementation group: 'org.freemarker', name: 'freemarker', version: '2.3.31'
3939
testImplementation group: 'org.msgpack', name: 'jackson-dataformat-msgpack', version: '0.9.6'
40+
testImplementation libs.bundles.mockito
4041
}
4142

4243
tasks.named("shadowJar", ShadowJar) {

‎dd-java-agent/agent-ci-visibility/civisibility-instrumentation-test-fixtures/src/main/groovy/datadog/trace/civisibility/CiVisibilityInstrumentationTest.groovy‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -128,9 +128,9 @@ abstract class CiVisibilityInstrumentationTest extends InstrumentationSpecificat
128128
def metricCollector = Stub(CiVisibilityMetricCollectorImpl)
129129

130130
def sourcePathResolver = Stub(SourcePathResolver)
131-
sourcePathResolver.getSourcePath(_ as Class) >> DUMMY_SOURCE_PATH
132-
sourcePathResolver.getResourcePath(_ as String) >> {
133-
String path -> path
131+
sourcePathResolver.getSourcePaths(_ as Class) >> [DUMMY_SOURCE_PATH]
132+
sourcePathResolver.getResourcePaths(_ as String) >> {
133+
String path -> [path]
134134
}
135135

136136
def codeowners = Stub(Codeowners)

‎dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/coverage/ConcurrentCoverageStore.java‎

Lines changed: 3 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,6 @@
44
import datadog.trace.api.civisibility.coverage.CoverageProbes;
55
import datadog.trace.api.civisibility.coverage.CoverageStore;
66
import datadog.trace.api.civisibility.coverage.TestReport;
7-
import datadog.trace.civisibility.source.SourceResolutionException;
87
import java.util.Collection;
98
import java.util.Map;
109
import java.util.concurrent.ConcurrentHashMap;
@@ -37,18 +36,13 @@ private T create(Thread thread) {
3736

3837
@Override
3938
public boolean report(DDTraceId testSessionId, Long testSuiteId, long testSpanId) {
40-
try {
41-
report = report(testSessionId, testSuiteId, testSpanId, probes.values());
42-
return report != null && report.isNotEmpty();
43-
} catch (SourceResolutionException e) {
44-
return false;
45-
}
39+
report = report(testSessionId, testSuiteId, testSpanId, probes.values());
40+
return report != null && report.isNotEmpty();
4641
}
4742

4843
@Nullable
4944
protected abstract TestReport report(
50-
DDTraceId testSessionId, Long testSuiteId, long testSpanId, Collection<T> probes)
51-
throws SourceResolutionException;
45+
DDTraceId testSessionId, Long testSuiteId, long testSpanId, Collection<T> probes);
5246

5347
@Nullable
5448
@Override

‎dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/coverage/file/FileCoverageStore.java‎

Lines changed: 40 additions & 48 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,6 @@
1111
import datadog.trace.api.civisibility.telemetry.tag.CoverageErrorType;
1212
import datadog.trace.civisibility.coverage.ConcurrentCoverageStore;
1313
import datadog.trace.civisibility.source.SourcePathResolver;
14-
import datadog.trace.civisibility.source.SourceResolutionException;
1514
import java.util.ArrayList;
1615
import java.util.Collection;
1716
import java.util.Collections;
@@ -47,61 +46,54 @@ private FileCoverageStore(
4746
@Nullable
4847
@Override
4948
protected TestReport report(
50-
DDTraceId testSessionId, Long testSuiteId, long testSpanId, Collection<FileProbes> probes)
51-
throws SourceResolutionException {
52-
try {
53-
Set<Class<?>> combinedClasses = Collections.newSetFromMap(new IdentityHashMap<>());
54-
Collection<String> combinedNonCodeResources = new HashSet<>();
55-
56-
for (FileProbes probe : probes) {
57-
combinedClasses.addAll(probe.getCoveredClasses());
58-
combinedNonCodeResources.addAll(probe.getNonCodeResources());
59-
}
49+
DDTraceId testSessionId, Long testSuiteId, long testSpanId, Collection<FileProbes> probes) {
50+
Set<Class<?>> combinedClasses = Collections.newSetFromMap(new IdentityHashMap<>());
51+
Collection<String> combinedNonCodeResources = new HashSet<>();
6052

61-
if (combinedClasses.isEmpty() && combinedNonCodeResources.isEmpty()) {
62-
return null;
63-
}
53+
for (FileProbes probe : probes) {
54+
combinedClasses.addAll(probe.getCoveredClasses());
55+
combinedNonCodeResources.addAll(probe.getNonCodeResources());
56+
}
6457

65-
Set<String> coveredPaths = set(combinedClasses.size() + combinedNonCodeResources.size());
66-
for (Class<?> clazz : combinedClasses) {
67-
String sourcePath = sourcePathResolver.getSourcePath(clazz);
68-
if (sourcePath == null) {
69-
log.debug(
70-
"Skipping coverage reporting for {} because source path could not be determined",
71-
clazz);
72-
metrics.add(CiVisibilityCountMetric.CODE_COVERAGE_ERRORS, 1, CoverageErrorType.PATH);
73-
continue;
74-
}
75-
coveredPaths.add(sourcePath);
76-
}
58+
if (combinedClasses.isEmpty() && combinedNonCodeResources.isEmpty()) {
59+
return null;
60+
}
7761

78-
for (String nonCodeResource : combinedNonCodeResources) {
79-
String resourcePath = sourcePathResolver.getResourcePath(nonCodeResource);
80-
if (resourcePath == null) {
81-
log.debug(
82-
"Skipping coverage reporting for {} because resource path could not be determined",
83-
nonCodeResource);
84-
metrics.add(CiVisibilityCountMetric.CODE_COVERAGE_ERRORS, 1, CoverageErrorType.PATH);
85-
continue;
86-
}
87-
coveredPaths.add(resourcePath);
62+
Set<String> coveredPaths = set(combinedClasses.size() + combinedNonCodeResources.size());
63+
for (Class<?> clazz : combinedClasses) {
64+
Collection<String> sourcePaths = sourcePathResolver.getSourcePaths(clazz);
65+
if (sourcePaths.isEmpty()) {
66+
log.debug(
67+
"Skipping coverage reporting for {} because source path could not be determined",
68+
clazz);
69+
metrics.add(CiVisibilityCountMetric.CODE_COVERAGE_ERRORS, 1, CoverageErrorType.PATH);
70+
continue;
8871
}
72+
coveredPaths.addAll(sourcePaths);
73+
}
8974

90-
List<TestReportFileEntry> fileEntries = new ArrayList<>(coveredPaths.size());
91-
for (String path : coveredPaths) {
92-
fileEntries.add(new TestReportFileEntry(path, null));
75+
for (String nonCodeResource : combinedNonCodeResources) {
76+
Collection<String> resourcePaths = sourcePathResolver.getResourcePaths(nonCodeResource);
77+
if (resourcePaths.isEmpty()) {
78+
log.debug(
79+
"Skipping coverage reporting for {} because resource path could not be determined",
80+
nonCodeResource);
81+
metrics.add(CiVisibilityCountMetric.CODE_COVERAGE_ERRORS, 1, CoverageErrorType.PATH);
82+
continue;
9383
}
84+
coveredPaths.addAll(resourcePaths);
85+
}
9486

95-
TestReport report = new TestReport(testSessionId, testSuiteId, testSpanId, fileEntries);
96-
metrics.add(
97-
CiVisibilityDistributionMetric.CODE_COVERAGE_FILES,
98-
report.getTestReportFileEntries().size());
99-
return report;
100-
101-
} catch (Exception e) {
102-
metrics.add(CiVisibilityCountMetric.CODE_COVERAGE_ERRORS, 1);
103-
throw e;
87+
List<TestReportFileEntry> fileEntries = new ArrayList<>(coveredPaths.size());
88+
for (String path : coveredPaths) {
89+
fileEntries.add(new TestReportFileEntry(path, null));
10490
}
91+
92+
TestReport report = new TestReport(testSessionId, testSuiteId, testSpanId, fileEntries);
93+
metrics.add(
94+
CiVisibilityDistributionMetric.CODE_COVERAGE_FILES,
95+
report.getTestReportFileEntries().size());
96+
return report;
10597
}
10698

10799
private static <T> Set<T> set(int size) {

‎dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/coverage/line/LineCoverageStore.java‎

Lines changed: 69 additions & 74 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,6 @@
1111
import datadog.trace.api.civisibility.telemetry.tag.CoverageErrorType;
1212
import datadog.trace.civisibility.coverage.ConcurrentCoverageStore;
1313
import datadog.trace.civisibility.source.SourcePathResolver;
14-
import datadog.trace.civisibility.source.SourceResolutionException;
1514
import datadog.trace.civisibility.source.Utils;
1615
import java.io.InputStream;
1716
import java.util.ArrayList;
@@ -53,88 +52,84 @@ private LineCoverageStore(
5352
@Nullable
5453
@Override
5554
protected TestReport report(
56-
DDTraceId testSessionId, Long testSuiteId, long testSpanId, Collection<LineProbes> probes)
57-
throws SourceResolutionException {
58-
try {
59-
Map<Class<?>, ExecutionDataAdapter> combinedExecutionData = new IdentityHashMap<>();
60-
Collection<String> combinedNonCodeResources = new HashSet<>();
61-
62-
for (LineProbes probe : probes) {
63-
for (Map.Entry<Class<?>, ExecutionDataAdapter> e : probe.getExecutionData().entrySet()) {
64-
combinedExecutionData.merge(e.getKey(), e.getValue(), ExecutionDataAdapter::merge);
65-
}
66-
combinedNonCodeResources.addAll(probe.getNonCodeResources());
67-
}
55+
DDTraceId testSessionId, Long testSuiteId, long testSpanId, Collection<LineProbes> probes) {
56+
Map<Class<?>, ExecutionDataAdapter> combinedExecutionData = new IdentityHashMap<>();
57+
Collection<String> combinedNonCodeResources = new HashSet<>();
6858

69-
if (combinedExecutionData.isEmpty() && combinedNonCodeResources.isEmpty()) {
70-
return null;
59+
for (LineProbes probe : probes) {
60+
for (Map.Entry<Class<?>, ExecutionDataAdapter> e : probe.getExecutionData().entrySet()) {
61+
combinedExecutionData.merge(e.getKey(), e.getValue(), ExecutionDataAdapter::merge);
7162
}
63+
combinedNonCodeResources.addAll(probe.getNonCodeResources());
64+
}
7265

73-
Map<String, BitSet> coveredLinesBySourcePath = new HashMap<>();
74-
for (Map.Entry<Class<?>, ExecutionDataAdapter> e : combinedExecutionData.entrySet()) {
75-
ExecutionDataAdapter executionDataAdapter = e.getValue();
76-
String className = executionDataAdapter.getClassName();
77-
78-
Class<?> clazz = e.getKey();
79-
String sourcePath = sourcePathResolver.getSourcePath(clazz);
80-
if (sourcePath == null) {
81-
log.debug(
82-
"Skipping coverage reporting for {} because source path could not be determined",
83-
className);
84-
metrics.add(CiVisibilityCountMetric.CODE_COVERAGE_ERRORS, 1, CoverageErrorType.PATH);
85-
continue;
86-
}
87-
88-
try (InputStream is = Utils.getClassStream(clazz)) {
89-
BitSet coveredLines =
90-
coveredLinesBySourcePath.computeIfAbsent(sourcePath, key -> new BitSet());
91-
ExecutionDataStore store = new ExecutionDataStore();
92-
store.put(executionDataAdapter.toExecutionData());
93-
94-
// TODO optimize this part to avoid parsing
95-
// the same class multiple times for different test cases
96-
Analyzer analyzer = new Analyzer(store, new SourceAnalyzer(coveredLines));
97-
analyzer.analyzeClass(is, null);
98-
99-
} catch (Exception exception) {
100-
log.debug(
101-
"Skipping coverage reporting for {} ({}) because of error",
102-
className,
103-
sourcePath,
104-
exception);
105-
metrics.add(CiVisibilityCountMetric.CODE_COVERAGE_ERRORS, 1);
106-
}
107-
}
66+
if (combinedExecutionData.isEmpty() && combinedNonCodeResources.isEmpty()) {
67+
return null;
68+
}
10869

109-
List<TestReportFileEntry> fileEntries = new ArrayList<>(coveredLinesBySourcePath.size());
110-
for (Map.Entry<String, BitSet> e : coveredLinesBySourcePath.entrySet()) {
111-
String sourcePath = e.getKey();
112-
BitSet coveredLines = e.getValue();
113-
fileEntries.add(new TestReportFileEntry(sourcePath, coveredLines));
70+
Map<String, BitSet> coveredLinesBySourcePath = new HashMap<>();
71+
for (Map.Entry<Class<?>, ExecutionDataAdapter> e : combinedExecutionData.entrySet()) {
72+
ExecutionDataAdapter executionDataAdapter = e.getValue();
73+
String className = executionDataAdapter.getClassName();
74+
75+
Class<?> clazz = e.getKey();
76+
Collection<String> sourcePaths = sourcePathResolver.getSourcePaths(clazz);
77+
if (sourcePaths.size() != 1) {
78+
log.debug(
79+
"Skipping coverage reporting for {} because source path could not be determined",
80+
className);
81+
metrics.add(CiVisibilityCountMetric.CODE_COVERAGE_ERRORS, 1, CoverageErrorType.PATH);
82+
continue;
11483
}
115-
116-
for (String nonCodeResource : combinedNonCodeResources) {
117-
String resourcePath = sourcePathResolver.getResourcePath(nonCodeResource);
118-
if (resourcePath == null) {
119-
log.debug(
120-
"Skipping coverage reporting for {} because resource path could not be determined",
121-
nonCodeResource);
122-
metrics.add(CiVisibilityCountMetric.CODE_COVERAGE_ERRORS, 1, CoverageErrorType.PATH);
123-
continue;
124-
}
125-
fileEntries.add(new TestReportFileEntry(resourcePath, null));
84+
String sourcePath = sourcePaths.iterator().next();
85+
86+
try (InputStream is = Utils.getClassStream(clazz)) {
87+
BitSet coveredLines =
88+
coveredLinesBySourcePath.computeIfAbsent(sourcePath, key -> new BitSet());
89+
ExecutionDataStore store = new ExecutionDataStore();
90+
store.put(executionDataAdapter.toExecutionData());
91+
92+
// TODO optimize this part to avoid parsing
93+
// the same class multiple times for different test cases
94+
Analyzer analyzer = new Analyzer(store, new SourceAnalyzer(coveredLines));
95+
analyzer.analyzeClass(is, null);
96+
97+
} catch (Exception exception) {
98+
log.debug(
99+
"Skipping coverage reporting for {} ({}) because of error",
100+
className,
101+
sourcePath,
102+
exception);
103+
metrics.add(CiVisibilityCountMetric.CODE_COVERAGE_ERRORS, 1);
126104
}
105+
}
127106

128-
TestReport report = new TestReport(testSessionId, testSuiteId, testSpanId, fileEntries);
129-
metrics.add(
130-
CiVisibilityDistributionMetric.CODE_COVERAGE_FILES,
131-
report.getTestReportFileEntries().size());
132-
return report;
107+
List<TestReportFileEntry> fileEntries = new ArrayList<>(coveredLinesBySourcePath.size());
108+
for (Map.Entry<String, BitSet> e : coveredLinesBySourcePath.entrySet()) {
109+
String sourcePath = e.getKey();
110+
BitSet coveredLines = e.getValue();
111+
fileEntries.add(new TestReportFileEntry(sourcePath, coveredLines));
112+
}
133113

134-
} catch (Exception e) {
135-
metrics.add(CiVisibilityCountMetric.CODE_COVERAGE_ERRORS, 1);
136-
throw e;
114+
for (String nonCodeResource : combinedNonCodeResources) {
115+
Collection<String> resourcePaths = sourcePathResolver.getResourcePaths(nonCodeResource);
116+
if (resourcePaths.isEmpty()) {
117+
log.debug(
118+
"Skipping coverage reporting for {} because resource path could not be determined",
119+
nonCodeResource);
120+
metrics.add(CiVisibilityCountMetric.CODE_COVERAGE_ERRORS, 1, CoverageErrorType.PATH);
121+
continue;
122+
}
123+
for (String resourcePath : resourcePaths) {
124+
fileEntries.add(new TestReportFileEntry(resourcePath, null));
125+
}
137126
}
127+
128+
TestReport report = new TestReport(testSessionId, testSuiteId, testSpanId, fileEntries);
129+
metrics.add(
130+
CiVisibilityDistributionMetric.CODE_COVERAGE_FILES,
131+
report.getTestReportFileEntries().size());
132+
return report;
138133
}
139134

140135
public static final class Factory implements CoverageStore.Factory {

‎dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/coverage/report/JacocoCoverageProcessor.java‎

Lines changed: 12 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,6 @@
99
import datadog.trace.civisibility.ipc.ModuleCoverageDataJacoco;
1010
import datadog.trace.civisibility.ipc.SignalResponse;
1111
import datadog.trace.civisibility.ipc.SignalType;
12-
import datadog.trace.civisibility.source.SourceResolutionException;
1312
import datadog.trace.civisibility.source.index.RepoIndex;
1413
import datadog.trace.civisibility.source.index.RepoIndexProvider;
1514
import datadog.trace.util.Strings;
@@ -355,19 +354,15 @@ private RepoIndexFileLocator(RepoIndex repoIndex, @Nonnull String repoRoot) {
355354

356355
@Override
357356
protected InputStream getSourceStream(String path) throws IOException {
358-
try {
359-
String relativePath = repoIndex.getSourcePath(path);
360-
if (relativePath == null) {
361-
return null;
362-
}
363-
String absolutePath =
364-
repoRoot + (!repoRoot.endsWith(File.separator) ? File.separator : "") + relativePath;
365-
return new BufferedInputStream(Files.newInputStream(Paths.get(absolutePath)));
366-
367-
} catch (SourceResolutionException e) {
368-
LOGGER.debug("Could not resolve source for path {}", path, e);
357+
Collection<String> relativePaths = repoIndex.getSourcePaths(path);
358+
if (relativePaths.size() != 1) {
359+
LOGGER.debug("Could not resolve source for path {}", path);
369360
return null;
370361
}
362+
String relativePath = relativePaths.iterator().next();
363+
String absolutePath =
364+
repoRoot + (!repoRoot.endsWith(File.separator) ? File.separator : "") + relativePath;
365+
return new BufferedInputStream(Files.newInputStream(Paths.get(absolutePath)));
371366
}
372367
}
373368

@@ -437,19 +432,16 @@ private long mergeAndUploadCoverageReport(IBundleCoverage coverageBundle) {
437432
String fileName = sourceFile.getName();
438433
String pathRelativeToSourceRoot =
439434
(Strings.isNotBlank(packageName) ? packageName + "/" : "") + fileName;
440-
String pathRelativeToIndexRoot;
441-
try {
442-
pathRelativeToIndexRoot = repoIndex.getSourcePath(pathRelativeToSourceRoot);
443-
} catch (SourceResolutionException e) {
444-
LOGGER.debug("Could not resolve source for path {}", pathRelativeToSourceRoot, e);
445-
continue;
446-
}
435+
Collection<String> pathsRelativeToIndexRoot =
436+
repoIndex.getSourcePaths(pathRelativeToSourceRoot);
447437

448-
if (pathRelativeToIndexRoot == null) {
438+
if (pathsRelativeToIndexRoot.size() != 1) {
449439
LOGGER.debug("Could not resolve source for path {}", pathRelativeToSourceRoot);
450440
continue;
451441
}
452442

443+
String pathRelativeToIndexRoot = pathsRelativeToIndexRoot.iterator().next();
444+
453445
LinesCoverage linesCoverage = getLinesCoverage(sourceFile);
454446
// backendCoverageData contains data for all modules in the repo,
455447
// but coverageBundle bundle only has source files that are relevant for the given module,

0 commit comments

Comments
 (0)