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"