Skip to content

Commit

Permalink
Allow Java coverage collection for external targets.
Browse files Browse the repository at this point in the history
A roll-forward of #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
  • Loading branch information
c-mita authored and copybara-github committed Jan 30, 2023
1 parent e29ff7f commit 00e9af1
Show file tree
Hide file tree
Showing 3 changed files with 192 additions and 11 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -29,41 +29,46 @@
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.
*
* <p>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";

private final NestedSet<Artifact> files;
private final ImmutableSet<Artifact> filesToIgnore;
private final boolean includeDerivedArtifacts;
private final Function<Artifact, String> converter;

public LazyWritePathsFileAction(
ActionOwner owner,
Artifact output,
NestedSet<Artifact> files,
ImmutableSet<Artifact> 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<Artifact> files,
NestedSet<Artifact> files,
ImmutableSet<Artifact> filesToIgnore,
boolean includeDerivedArtifacts) {
boolean includeDerivedArtifacts,
Function<Artifact, String> 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.<Artifact>stableOrder().addAll(files).build();
this.files = files;
this.includeDerivedArtifacts = includeDerivedArtifacts;
this.filesToIgnore = filesToIgnore;
this.converter = converter;
}

@Override
Expand Down Expand Up @@ -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<Artifact> getFiles() {
return files;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -365,7 +365,7 @@ && getJavaConfiguration().experimentalEnableJspecify()
new LazyWritePathsFileAction(
ruleContext.getActionOwner(),
coverageArtifact,
sourceFiles,
NestedSetBuilder.<Artifact>stableOrder().addAll(sourceFiles).build(),
/* filesToIgnore= */ ImmutableSet.of(),
false));
}
Expand Down
172 changes: 172 additions & 0 deletions src/test/shell/bazel/bazel_coverage_java_test.sh
Original file line number Diff line number Diff line change
Expand Up @@ -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::<init> ()V
FN:6,com/example/Math::isEven (I)Z
FNDA:0,com/example/Math::<init> ()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::<init> ()V
FN:6,com/example/Collatz::getCollatzFinal (I)I
FNDA:0,com/example/Collatz::<init> ()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::<init> ()V
FN:6,com/example/Math::isEven (I)Z
FNDA:0,com/example/Math::<init> ()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"

0 comments on commit 00e9af1

Please sign in to comment.