From 0bd199a228c4f7d2b6547aa308874331fcd02735 Mon Sep 17 00:00:00 2001 From: Nikita Tkachenko Date: Mon, 10 Mar 2025 13:58:44 +0100 Subject: [PATCH 1/2] Fix Test Optimization init when repo root cannot be determined --- .../CiVisibilityRepoServices.java | 13 ++--- .../config/ExecutionSettingsFactoryImpl.java | 12 ++--- .../percentage/JacocoCoverageCalculator.java | 49 ++++++++++++++----- .../civisibility/git/tree/GitClient.java | 2 +- .../civisibility/git/tree/ShellGitClient.java | 13 +++-- .../index/CachingRepoIndexBuilderFactory.java | 3 +- .../source/index/RepoIndexBuilder.java | 3 +- .../source/index/RepoIndexProvider.java | 4 +- 8 files changed, 67 insertions(+), 32 deletions(-) diff --git a/dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/CiVisibilityRepoServices.java b/dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/CiVisibilityRepoServices.java index 6a269fa1964..6078e61880e 100644 --- a/dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/CiVisibilityRepoServices.java +++ b/dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/CiVisibilityRepoServices.java @@ -39,6 +39,7 @@ import java.nio.file.Paths; import java.util.Map; import java.util.concurrent.CompletableFuture; +import javax.annotation.Nullable; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -47,7 +48,7 @@ public class CiVisibilityRepoServices { private static final Logger LOGGER = LoggerFactory.getLogger(CiVisibilityRepoServices.class); - final String repoRoot; + @Nullable final String repoRoot; final String moduleName; final Provider ciProvider; final Map ciTags; @@ -135,7 +136,7 @@ private static String appendSlashIfNeeded(String repoRoot) { } } - static String getModuleName(Config config, String repoRoot, Path path) { + static String getModuleName(Config config, @Nullable String repoRoot, Path path) { // if parent process is instrumented, it will provide build system's module name String parentModuleName = config.getCiVisibilityModuleName(); if (parentModuleName != null) { @@ -175,7 +176,7 @@ private static ExecutionSettingsFactory buildExecutionSettingsFactory( GitRepoUnshallow gitRepoUnshallow, GitDataUploader gitDataUploader, PullRequestInfo pullRequestInfo, - String repoRoot) { + @Nullable String repoRoot) { ConfigurationApi configurationApi; if (backendApi == null) { LOGGER.warn( @@ -208,7 +209,7 @@ private static GitDataUploader buildGitDataUploader( GitClient gitClient, GitRepoUnshallow gitRepoUnshallow, BackendApi backendApi, - String repoRoot) { + @Nullable String repoRoot) { if (!config.isCiVisibilityGitUploadEnabled()) { return () -> CompletableFuture.completedFuture(null); } @@ -239,7 +240,7 @@ private static GitDataUploader buildGitDataUploader( } private static SourcePathResolver buildSourcePathResolver( - String repoRoot, RepoIndexProvider indexProvider) { + @Nullable String repoRoot, RepoIndexProvider indexProvider) { SourcePathResolver compilerAidedResolver = repoRoot != null ? new CompilerAidedSourcePathResolver(repoRoot) @@ -248,7 +249,7 @@ private static SourcePathResolver buildSourcePathResolver( return new BestEffortSourcePathResolver(compilerAidedResolver, indexResolver); } - private static Codeowners buildCodeowners(String repoRoot) { + private static Codeowners buildCodeowners(@Nullable String repoRoot) { if (repoRoot != null) { return new CodeownersProvider().build(repoRoot); } else { diff --git a/dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/config/ExecutionSettingsFactoryImpl.java b/dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/config/ExecutionSettingsFactoryImpl.java index cab997654b1..c0beee97019 100644 --- a/dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/config/ExecutionSettingsFactoryImpl.java +++ b/dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/config/ExecutionSettingsFactoryImpl.java @@ -54,7 +54,7 @@ public class ExecutionSettingsFactoryImpl implements ExecutionSettingsFactory { private final GitRepoUnshallow gitRepoUnshallow; private final GitDataUploader gitDataUploader; private final PullRequestInfo pullRequestInfo; - private final String repositoryRoot; + @Nullable private final String repositoryRoot; public ExecutionSettingsFactoryImpl( Config config, @@ -63,7 +63,7 @@ public ExecutionSettingsFactoryImpl( GitRepoUnshallow gitRepoUnshallow, GitDataUploader gitDataUploader, PullRequestInfo pullRequestInfo, - String repositoryRoot) { + @Nullable String repositoryRoot) { this.config = config; this.configurationApi = configurationApi; this.gitClient = gitClient; @@ -75,21 +75,19 @@ public ExecutionSettingsFactoryImpl( /** @return Executions settings by module name */ public Map create(@Nonnull JvmInfo jvmInfo) { - TracerEnvironment tracerEnvironment = buildTracerEnvironment(repositoryRoot, jvmInfo, null); + TracerEnvironment tracerEnvironment = buildTracerEnvironment(jvmInfo, null); return create(tracerEnvironment); } @Override public ExecutionSettings create(@Nonnull JvmInfo jvmInfo, @Nullable String moduleName) { - TracerEnvironment tracerEnvironment = - buildTracerEnvironment(repositoryRoot, jvmInfo, moduleName); + TracerEnvironment tracerEnvironment = buildTracerEnvironment(jvmInfo, moduleName); Map settingsByModule = create(tracerEnvironment); ExecutionSettings settings = settingsByModule.get(moduleName); return settings != null ? settings : settingsByModule.get(DEFAULT_SETTINGS); } - private TracerEnvironment buildTracerEnvironment( - String repositoryRoot, JvmInfo jvmInfo, @Nullable String moduleName) { + private TracerEnvironment buildTracerEnvironment(JvmInfo jvmInfo, @Nullable String moduleName) { GitInfo gitInfo = GitInfoProvider.INSTANCE.getGitInfo(repositoryRoot); TracerEnvironment.Builder builder = TracerEnvironment.builder(); diff --git a/dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/coverage/percentage/JacocoCoverageCalculator.java b/dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/coverage/percentage/JacocoCoverageCalculator.java index b957575718e..21c2360844f 100644 --- a/dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/coverage/percentage/JacocoCoverageCalculator.java +++ b/dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/coverage/percentage/JacocoCoverageCalculator.java @@ -19,6 +19,7 @@ import java.io.FileOutputStream; import java.io.IOException; import java.io.InputStream; +import java.io.Reader; import java.nio.file.Files; import java.nio.file.Paths; import java.util.BitSet; @@ -42,9 +43,11 @@ import org.jacoco.core.data.SessionInfoStore; import org.jacoco.report.FileMultiReportOutput; import org.jacoco.report.IReportVisitor; +import org.jacoco.report.ISourceFileLocator; import org.jacoco.report.InputStreamSourceFileLocator; import org.jacoco.report.html.HTMLFormatter; import org.jacoco.report.xml.XMLFormatter; +import org.jetbrains.annotations.NotNull; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -55,13 +58,13 @@ public static final class Factory implements CoverageCalculator.Factory { private final Config config; private final RepoIndexProvider repoIndexProvider; - private final String repoRoot; + @Nullable private final String repoRoot; private final ModuleSignalRouter moduleSignalRouter; public Factory( Config config, RepoIndexProvider repoIndexProvider, - String repoRoot, + @Nullable String repoRoot, ModuleSignalRouter moduleSignalRouter) { this.config = config; this.repoIndexProvider = repoIndexProvider; @@ -100,7 +103,7 @@ public JacocoCoverageCalculator moduleCoverage( private final RepoIndexProvider repoIndexProvider; - private final String repoRoot; + @Nullable private final String repoRoot; private final long eventId; @@ -116,7 +119,10 @@ public JacocoCoverageCalculator moduleCoverage( private final Collection outputClassesDirs = new HashSet<>(); private JacocoCoverageCalculator( - Config config, RepoIndexProvider repoIndexProvider, String repoRoot, long sessionId) { + Config config, + RepoIndexProvider repoIndexProvider, + @Nullable String repoRoot, + long sessionId) { this.parent = null; this.config = config; this.repoIndexProvider = repoIndexProvider; @@ -128,7 +134,7 @@ private JacocoCoverageCalculator( Config config, RepoIndexProvider repoIndexProvider, ExecutionSettings executionSettings, - String repoRoot, + @Nullable String repoRoot, long moduleId, @Nullable BuildModuleLayout moduleLayout, ModuleSignalRouter moduleSignalRouter, @@ -297,8 +303,7 @@ private void dumpCoverageReport(IBundleCoverage coverageBundle, File reportFolde final IReportVisitor htmlVisitor = htmlFormatter.createVisitor(new FileMultiReportOutput(reportFolder)); htmlVisitor.visitInfo(Collections.emptyList(), Collections.emptyList()); - htmlVisitor.visitBundle( - coverageBundle, new RepoIndexFileLocator(repoIndexProvider.getIndex(), repoRoot)); + htmlVisitor.visitBundle(coverageBundle, createSourceFileLocator()); htmlVisitor.visitEnd(); File xmlReport = new File(reportFolder, "jacoco.xml"); @@ -306,8 +311,7 @@ private void dumpCoverageReport(IBundleCoverage coverageBundle, File reportFolde XMLFormatter xmlFormatter = new XMLFormatter(); IReportVisitor xmlVisitor = xmlFormatter.createVisitor(xmlReportStream); xmlVisitor.visitInfo(Collections.emptyList(), Collections.emptyList()); - xmlVisitor.visitBundle( - coverageBundle, new RepoIndexFileLocator(repoIndexProvider.getIndex(), repoRoot)); + xmlVisitor.visitBundle(coverageBundle, createSourceFileLocator()); xmlVisitor.visitEnd(); } } catch (Exception e) { @@ -315,11 +319,18 @@ private void dumpCoverageReport(IBundleCoverage coverageBundle, File reportFolde } } + @NotNull + private ISourceFileLocator createSourceFileLocator() { + return repoRoot != null + ? new RepoIndexFileLocator(repoIndexProvider.getIndex(), repoRoot) + : NoOpFileLocator.INSTANCE; + } + private static final class RepoIndexFileLocator extends InputStreamSourceFileLocator { private final RepoIndex repoIndex; - private final String repoRoot; + @Nonnull private final String repoRoot; - private RepoIndexFileLocator(RepoIndex repoIndex, String repoRoot) { + private RepoIndexFileLocator(RepoIndex repoIndex, @Nonnull String repoRoot) { super("utf-8", 4); this.repoIndex = repoIndex; this.repoRoot = repoRoot; @@ -343,6 +354,22 @@ protected InputStream getSourceStream(String path) throws IOException { } } + private static final class NoOpFileLocator implements ISourceFileLocator { + private static final NoOpFileLocator INSTANCE = new NoOpFileLocator(); + + private NoOpFileLocator() {} + + @Override + public Reader getSourceFile(String s, String s1) { + return null; + } + + @Override + public int getTabWidth() { + return 0; + } + } + private long getCoveragePercentage(IBundleCoverage coverageBundle) { if (backendCoverageData.isEmpty()) { return getLocalCoveragePercentage(coverageBundle); diff --git a/dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/git/tree/GitClient.java b/dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/git/tree/GitClient.java index a6bffa37771..41e9cc9ee10 100644 --- a/dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/git/tree/GitClient.java +++ b/dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/git/tree/GitClient.java @@ -77,6 +77,6 @@ LineDiff getGitDiff(String baseCommit, String targetCommit) throws IOException, TimeoutException, InterruptedException; interface Factory { - GitClient create(String repoRoot); + GitClient create(@Nullable String repoRoot); } } diff --git a/dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/git/tree/ShellGitClient.java b/dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/git/tree/ShellGitClient.java index 56c3cb4949d..4d78112a646 100644 --- a/dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/git/tree/ShellGitClient.java +++ b/dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/git/tree/ShellGitClient.java @@ -51,7 +51,7 @@ public class ShellGitClient implements GitClient { */ ShellGitClient( CiVisibilityMetricCollector metricCollector, - String repoRoot, + @Nonnull String repoRoot, String latestCommitsSince, int latestCommitsLimit, long timeoutMillis) { @@ -651,10 +651,15 @@ public Factory(Config config, CiVisibilityMetricCollector metricCollector) { } @Override - public GitClient create(String repoRoot) { + public GitClient create(@Nullable String repoRoot) { long commandTimeoutMillis = config.getCiVisibilityGitCommandTimeoutMillis(); - return new ShellGitClient( - metricCollector, repoRoot, "1 month ago", 1000, commandTimeoutMillis); + if (repoRoot != null) { + return new ShellGitClient( + metricCollector, repoRoot, "1 month ago", 1000, commandTimeoutMillis); + } else { + LOGGER.debug("Could not determine repository root, using no-op git client"); + return NoOpGitClient.INSTANCE; + } } } } diff --git a/dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/source/index/CachingRepoIndexBuilderFactory.java b/dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/source/index/CachingRepoIndexBuilderFactory.java index d3b34ebbba2..152422f6efe 100644 --- a/dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/source/index/CachingRepoIndexBuilderFactory.java +++ b/dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/source/index/CachingRepoIndexBuilderFactory.java @@ -4,6 +4,7 @@ import datadog.trace.api.cache.DDCache; import datadog.trace.api.cache.DDCaches; import java.nio.file.FileSystem; +import javax.annotation.Nullable; public class CachingRepoIndexBuilderFactory implements RepoIndexProvider.Factory { @@ -25,7 +26,7 @@ public CachingRepoIndexBuilderFactory( } @Override - public RepoIndexProvider create(String repoRoot) { + public RepoIndexProvider create(@Nullable String repoRoot) { if (repoRoot == null) { return () -> RepoIndex.EMPTY; } diff --git a/dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/source/index/RepoIndexBuilder.java b/dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/source/index/RepoIndexBuilder.java index b5253ae0738..805cb4e56c1 100644 --- a/dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/source/index/RepoIndexBuilder.java +++ b/dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/source/index/RepoIndexBuilder.java @@ -19,6 +19,7 @@ import java.util.HashSet; import java.util.Map; import java.util.concurrent.atomic.AtomicInteger; +import javax.annotation.Nonnull; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -37,7 +38,7 @@ public class RepoIndexBuilder implements RepoIndexProvider { public RepoIndexBuilder( Config config, - String repoRoot, + @Nonnull String repoRoot, PackageResolver packageResolver, ResourceResolver resourceResolver, FileSystem fileSystem) { diff --git a/dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/source/index/RepoIndexProvider.java b/dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/source/index/RepoIndexProvider.java index 0a012c9516b..0244526b105 100644 --- a/dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/source/index/RepoIndexProvider.java +++ b/dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/source/index/RepoIndexProvider.java @@ -1,9 +1,11 @@ package datadog.trace.civisibility.source.index; +import javax.annotation.Nullable; + public interface RepoIndexProvider { RepoIndex getIndex(); interface Factory { - RepoIndexProvider create(String repoRoot); + RepoIndexProvider create(@Nullable String repoRoot); } } From c186d4a4e354994836a96889f4134a4d32f62214 Mon Sep 17 00:00:00 2001 From: Nikita Tkachenko Date: Mon, 10 Mar 2025 14:03:26 +0100 Subject: [PATCH 2/2] Use javax Nullable/Nonnull annotations instead of Jetbrains ones --- .../coverage/percentage/JacocoCoverageCalculator.java | 2 -- .../civisibility/events/NoOpTestEventsHandler.java | 3 +-- .../trace/civisibility/git/tree/NoOpGitClient.java | 10 +++++----- 3 files changed, 6 insertions(+), 9 deletions(-) diff --git a/dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/coverage/percentage/JacocoCoverageCalculator.java b/dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/coverage/percentage/JacocoCoverageCalculator.java index 21c2360844f..c275d18a7eb 100644 --- a/dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/coverage/percentage/JacocoCoverageCalculator.java +++ b/dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/coverage/percentage/JacocoCoverageCalculator.java @@ -47,7 +47,6 @@ import org.jacoco.report.InputStreamSourceFileLocator; import org.jacoco.report.html.HTMLFormatter; import org.jacoco.report.xml.XMLFormatter; -import org.jetbrains.annotations.NotNull; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -319,7 +318,6 @@ private void dumpCoverageReport(IBundleCoverage coverageBundle, File reportFolde } } - @NotNull private ISourceFileLocator createSourceFileLocator() { return repoRoot != null ? new RepoIndexFileLocator(repoIndexProvider.getIndex(), repoRoot) 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 index cd7d292aab6..99ac9994829 100644 --- 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 @@ -15,7 +15,6 @@ import java.util.Collection; import javax.annotation.Nonnull; import javax.annotation.Nullable; -import org.jetbrains.annotations.NotNull; public class NoOpTestEventsHandler implements TestEventsHandler { @@ -99,7 +98,7 @@ public SkipReason skipReason(TestIdentifier test) { return null; } - @NotNull + @Nonnull @Override public TestExecutionPolicy executionPolicy( TestIdentifier test, TestSourceData source, Collection testTags) { diff --git a/dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/git/tree/NoOpGitClient.java b/dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/git/tree/NoOpGitClient.java index 2e09358c40b..c030fc7442f 100644 --- a/dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/git/tree/NoOpGitClient.java +++ b/dd-java-agent/agent-ci-visibility/src/main/java/datadog/trace/civisibility/git/tree/NoOpGitClient.java @@ -5,8 +5,8 @@ import java.util.Collection; import java.util.Collections; import java.util.List; -import org.jetbrains.annotations.NotNull; -import org.jetbrains.annotations.Nullable; +import javax.annotation.Nonnull; +import javax.annotation.Nullable; public class NoOpGitClient implements GitClient { @@ -54,7 +54,7 @@ public String getCurrentBranch() { return null; } - @NotNull + @Nonnull @Override public List getTags(String commit) { return Collections.emptyList(); @@ -108,13 +108,13 @@ public String getCommitterDate(String commit) { return null; } - @NotNull + @Nonnull @Override public List getLatestCommits() { return Collections.emptyList(); } - @NotNull + @Nonnull @Override public List getObjects( Collection commitsToSkip, Collection commitsToInclude) {