From 00e9af1985cc0227599516fe7568785ca4334050 Mon Sep 17 00:00:00 2001 From: Googler Date: Mon, 30 Jan 2023 06:43:37 -0800 Subject: [PATCH] Allow Java coverage collection for external targets. A roll-forward of https://github.com/bazelbuild/bazel/pull/16268. LazyWritePathsFileAction has been changed to accept a custom converter parameter that can be used to specify exactly what path should be written out to the file. This is required to support the use case of an internal rule that still needs root-relative paths written out. The default case (when no converter is specified) is to output the exec path, as per the original PR. PiperOrigin-RevId: 505678128 Change-Id: Ib1d8547dcb359a9e0fb7e462850424ec4218f7fb --- .../actions/LazyWritePathsFileAction.java | 29 ++- .../lib/rules/java/JavaCompilationHelper.java | 2 +- .../shell/bazel/bazel_coverage_java_test.sh | 172 ++++++++++++++++++ 3 files changed, 192 insertions(+), 11 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/analysis/actions/LazyWritePathsFileAction.java b/src/main/java/com/google/devtools/build/lib/analysis/actions/LazyWritePathsFileAction.java index e3462bab2c8093..d37dde8014438c 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/actions/LazyWritePathsFileAction.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/actions/LazyWritePathsFileAction.java @@ -29,10 +29,14 @@ import com.google.devtools.build.lib.util.Fingerprint; import java.io.IOException; import java.io.OutputStream; +import java.util.function.Function; import javax.annotation.Nullable; /** - * Lazily writes the exec path of the given files separated by newline into a specified output file. + * Lazily writes the path of the given files separated by newline into a specified output file. + * + *

By default the exec path is written but this behaviour can be customized by providing an + * alternative converter function. */ public final class LazyWritePathsFileAction extends AbstractFileWriteAction { private static final String GUID = "6be94d90-96f3-4bec-8104-1fb08abc2546"; @@ -40,6 +44,7 @@ public final class LazyWritePathsFileAction extends AbstractFileWriteAction { private final NestedSet files; private final ImmutableSet filesToIgnore; private final boolean includeDerivedArtifacts; + private final Function converter; public LazyWritePathsFileAction( ActionOwner owner, @@ -47,23 +52,23 @@ public LazyWritePathsFileAction( NestedSet files, ImmutableSet filesToIgnore, boolean includeDerivedArtifacts) { - // TODO(ulfjack): It's a bad idea to have these two constructors do slightly different things. - super(owner, files, output, false); - this.files = files; - this.includeDerivedArtifacts = includeDerivedArtifacts; - this.filesToIgnore = filesToIgnore; + this(owner, output, files, filesToIgnore, includeDerivedArtifacts, Artifact::getExecPathString); } public LazyWritePathsFileAction( ActionOwner owner, Artifact output, - ImmutableSet files, + NestedSet files, ImmutableSet filesToIgnore, - boolean includeDerivedArtifacts) { + boolean includeDerivedArtifacts, + Function converter) { + // We don't need to pass the given files as explicit inputs to this action; we don't care about + // them, we only need their names, which we already know. super(owner, NestedSetBuilder.emptySet(Order.STABLE_ORDER), output, false); - this.files = NestedSetBuilder.stableOrder().addAll(files).build(); + this.files = files; this.includeDerivedArtifacts = includeDerivedArtifacts; this.filesToIgnore = filesToIgnore; + this.converter = converter; } @Override @@ -94,10 +99,14 @@ private String getContents() { continue; } if (file.isSourceArtifact() || includeDerivedArtifacts) { - stringBuilder.append(file.getRootRelativePathString()); + stringBuilder.append(converter.apply(file)); stringBuilder.append("\n"); } } return stringBuilder.toString(); } + + public NestedSet getFiles() { + return files; + } } diff --git a/src/main/java/com/google/devtools/build/lib/rules/java/JavaCompilationHelper.java b/src/main/java/com/google/devtools/build/lib/rules/java/JavaCompilationHelper.java index 8ae3fad9cf94f2..efca3d34269412 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/java/JavaCompilationHelper.java +++ b/src/main/java/com/google/devtools/build/lib/rules/java/JavaCompilationHelper.java @@ -365,7 +365,7 @@ && getJavaConfiguration().experimentalEnableJspecify() new LazyWritePathsFileAction( ruleContext.getActionOwner(), coverageArtifact, - sourceFiles, + NestedSetBuilder.stableOrder().addAll(sourceFiles).build(), /* filesToIgnore= */ ImmutableSet.of(), false)); } diff --git a/src/test/shell/bazel/bazel_coverage_java_test.sh b/src/test/shell/bazel/bazel_coverage_java_test.sh index 27f86db018b32c..a7b0e31619be8f 100755 --- a/src/test/shell/bazel/bazel_coverage_java_test.sh +++ b/src/test/shell/bazel/bazel_coverage_java_test.sh @@ -1143,4 +1143,176 @@ end_of_record" assert_coverage_result "$coverage_result_num_lib_header" "$coverage_file_path" } +function setup_external_java_target() { + cat >> WORKSPACE <<'EOF' +local_repository( + name = "other_repo", + path = "other_repo", +) +EOF + + cat > BUILD <<'EOF' +java_library( + name = "math", + srcs = ["src/main/com/example/Math.java"], + visibility = ["//visibility:public"], +) +EOF + + mkdir -p src/main/com/example + cat > src/main/com/example/Math.java <<'EOF' +package com.example; + +public class Math { + + public static boolean isEven(int n) { + return n % 2 == 0; + } +} +EOF + + mkdir -p other_repo + touch other_repo/WORKSPACE + + cat > other_repo/BUILD <<'EOF' +java_library( + name = "collatz", + srcs = ["src/main/com/example/Collatz.java"], + deps = ["@//:math"], +) + +java_test( + name = "test", + srcs = ["src/test/com/example/TestCollatz.java"], + test_class = "com.example.TestCollatz", + deps = [":collatz"], +) +EOF + + mkdir -p other_repo/src/main/com/example + cat > other_repo/src/main/com/example/Collatz.java <<'EOF' +package com.example; + +public class Collatz { + + public static int getCollatzFinal(int n) { + if (n == 1) { + return 1; + } + if (Math.isEven(n)) { + return getCollatzFinal(n / 2); + } else { + return getCollatzFinal(n * 3 + 1); + } + } + +} +EOF + + mkdir -p other_repo/src/test/com/example + cat > other_repo/src/test/com/example/TestCollatz.java <<'EOF' +package com.example; + +import static org.junit.Assert.assertEquals; +import org.junit.Test; + +public class TestCollatz { + + @Test + public void testGetCollatzFinal() { + assertEquals(Collatz.getCollatzFinal(1), 1); + assertEquals(Collatz.getCollatzFinal(5), 1); + assertEquals(Collatz.getCollatzFinal(10), 1); + assertEquals(Collatz.getCollatzFinal(21), 1); + } + +} +EOF +} + +function test_external_java_target_can_collect_coverage() { + setup_external_java_target + + bazel coverage --test_output=all @other_repo//:test --combined_report=lcov \ + --instrumentation_filter=// &>$TEST_log \ + || echo "Coverage for //:test failed" + + local coverage_file_path="$(get_coverage_file_path_from_test_log)" + local expected_result_math='SF:src/main/com/example/Math.java +FN:3,com/example/Math:: ()V +FN:6,com/example/Math::isEven (I)Z +FNDA:0,com/example/Math:: ()V +FNDA:1,com/example/Math::isEven (I)Z +FNF:2 +FNH:1 +BRDA:6,0,0,1 +BRDA:6,0,1,1 +BRF:2 +BRH:2 +DA:3,0 +DA:6,1 +LH:1 +LF:2 +end_of_record' + local expected_result_collatz="SF:external/other_repo/src/main/com/example/Collatz.java +FN:3,com/example/Collatz:: ()V +FN:6,com/example/Collatz::getCollatzFinal (I)I +FNDA:0,com/example/Collatz:: ()V +FNDA:1,com/example/Collatz::getCollatzFinal (I)I +FNF:2 +FNH:1 +BRDA:6,0,0,1 +BRDA:6,0,1,1 +BRDA:9,0,0,1 +BRDA:9,0,1,1 +BRF:4 +BRH:4 +DA:3,0 +DA:6,1 +DA:7,1 +DA:9,1 +DA:10,1 +DA:12,1 +LH:5 +LF:6 +end_of_record" + + assert_coverage_result "$expected_result_math" "$coverage_file_path" + assert_coverage_result "$expected_result_collatz" "$coverage_file_path" + + assert_coverage_result "$expected_result_math" bazel-out/_coverage/_coverage_report.dat + assert_coverage_result "$expected_result_collatz" bazel-out/_coverage/_coverage_report.dat +} + +function test_external_java_target_coverage_not_collected_by_default() { + setup_external_java_target + + bazel coverage --test_output=all @other_repo//:test --combined_report=lcov &>$TEST_log \ + || echo "Coverage for //:test failed" + + local coverage_file_path="$(get_coverage_file_path_from_test_log)" + local expected_result_math='SF:src/main/com/example/Math.java +FN:3,com/example/Math:: ()V +FN:6,com/example/Math::isEven (I)Z +FNDA:0,com/example/Math:: ()V +FNDA:1,com/example/Math::isEven (I)Z +FNF:2 +FNH:1 +BRDA:6,0,0,1 +BRDA:6,0,1,1 +BRF:2 +BRH:2 +DA:3,0 +DA:6,1 +LH:1 +LF:2 +end_of_record' + + assert_coverage_result "$expected_result_math" "$coverage_file_path" + assert_not_contains "SF:external/other_repo/" "$coverage_file_path" + + assert_coverage_result "$expected_result_math" bazel-out/_coverage/_coverage_report.dat + assert_not_contains "SF:external/other_repo/" bazel-out/_coverage/_coverage_report.dat +} + run_suite "test tests"