Skip to content

Commit 00e9af1

Browse files
c-mitacopybara-github
c-mita
authored and
copybara-github
committed
Allow Java coverage collection for external targets.
A roll-forward of bazelbuild#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
1 parent e29ff7f commit 00e9af1

File tree

3 files changed

+192
-11
lines changed

3 files changed

+192
-11
lines changed

‎src/main/java/com/google/devtools/build/lib/analysis/actions/LazyWritePathsFileAction.java‎

Lines changed: 19 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -29,41 +29,46 @@
2929
import com.google.devtools.build.lib.util.Fingerprint;
3030
import java.io.IOException;
3131
import java.io.OutputStream;
32+
import java.util.function.Function;
3233
import javax.annotation.Nullable;
3334

3435
/**
35-
* Lazily writes the exec path of the given files separated by newline into a specified output file.
36+
* Lazily writes the path of the given files separated by newline into a specified output file.
37+
*
38+
* <p>By default the exec path is written but this behaviour can be customized by providing an
39+
* alternative converter function.
3640
*/
3741
public final class LazyWritePathsFileAction extends AbstractFileWriteAction {
3842
private static final String GUID = "6be94d90-96f3-4bec-8104-1fb08abc2546";
3943

4044
private final NestedSet<Artifact> files;
4145
private final ImmutableSet<Artifact> filesToIgnore;
4246
private final boolean includeDerivedArtifacts;
47+
private final Function<Artifact, String> converter;
4348

4449
public LazyWritePathsFileAction(
4550
ActionOwner owner,
4651
Artifact output,
4752
NestedSet<Artifact> files,
4853
ImmutableSet<Artifact> filesToIgnore,
4954
boolean includeDerivedArtifacts) {
50-
// TODO(ulfjack): It's a bad idea to have these two constructors do slightly different things.
51-
super(owner, files, output, false);
52-
this.files = files;
53-
this.includeDerivedArtifacts = includeDerivedArtifacts;
54-
this.filesToIgnore = filesToIgnore;
55+
this(owner, output, files, filesToIgnore, includeDerivedArtifacts, Artifact::getExecPathString);
5556
}
5657

5758
public LazyWritePathsFileAction(
5859
ActionOwner owner,
5960
Artifact output,
60-
ImmutableSet<Artifact> files,
61+
NestedSet<Artifact> files,
6162
ImmutableSet<Artifact> filesToIgnore,
62-
boolean includeDerivedArtifacts) {
63+
boolean includeDerivedArtifacts,
64+
Function<Artifact, String> converter) {
65+
// We don't need to pass the given files as explicit inputs to this action; we don't care about
66+
// them, we only need their names, which we already know.
6367
super(owner, NestedSetBuilder.emptySet(Order.STABLE_ORDER), output, false);
64-
this.files = NestedSetBuilder.<Artifact>stableOrder().addAll(files).build();
68+
this.files = files;
6569
this.includeDerivedArtifacts = includeDerivedArtifacts;
6670
this.filesToIgnore = filesToIgnore;
71+
this.converter = converter;
6772
}
6873

6974
@Override
@@ -94,10 +99,14 @@ private String getContents() {
9499
continue;
95100
}
96101
if (file.isSourceArtifact() || includeDerivedArtifacts) {
97-
stringBuilder.append(file.getRootRelativePathString());
102+
stringBuilder.append(converter.apply(file));
98103
stringBuilder.append("\n");
99104
}
100105
}
101106
return stringBuilder.toString();
102107
}
108+
109+
public NestedSet<Artifact> getFiles() {
110+
return files;
111+
}
103112
}

‎src/main/java/com/google/devtools/build/lib/rules/java/JavaCompilationHelper.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -365,7 +365,7 @@ && getJavaConfiguration().experimentalEnableJspecify()
365365
new LazyWritePathsFileAction(
366366
ruleContext.getActionOwner(),
367367
coverageArtifact,
368-
sourceFiles,
368+
NestedSetBuilder.<Artifact>stableOrder().addAll(sourceFiles).build(),
369369
/* filesToIgnore= */ ImmutableSet.of(),
370370
false));
371371
}

‎src/test/shell/bazel/bazel_coverage_java_test.sh‎

Lines changed: 172 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1143,4 +1143,176 @@ end_of_record"
11431143
assert_coverage_result "$coverage_result_num_lib_header" "$coverage_file_path"
11441144
}
11451145

1146+
function setup_external_java_target() {
1147+
cat >> WORKSPACE <<'EOF'
1148+
local_repository(
1149+
name = "other_repo",
1150+
path = "other_repo",
1151+
)
1152+
EOF
1153+
1154+
cat > BUILD <<'EOF'
1155+
java_library(
1156+
name = "math",
1157+
srcs = ["src/main/com/example/Math.java"],
1158+
visibility = ["//visibility:public"],
1159+
)
1160+
EOF
1161+
1162+
mkdir -p src/main/com/example
1163+
cat > src/main/com/example/Math.java <<'EOF'
1164+
package com.example;
1165+
1166+
public class Math {
1167+
1168+
public static boolean isEven(int n) {
1169+
return n % 2 == 0;
1170+
}
1171+
}
1172+
EOF
1173+
1174+
mkdir -p other_repo
1175+
touch other_repo/WORKSPACE
1176+
1177+
cat > other_repo/BUILD <<'EOF'
1178+
java_library(
1179+
name = "collatz",
1180+
srcs = ["src/main/com/example/Collatz.java"],
1181+
deps = ["@//:math"],
1182+
)
1183+
1184+
java_test(
1185+
name = "test",
1186+
srcs = ["src/test/com/example/TestCollatz.java"],
1187+
test_class = "com.example.TestCollatz",
1188+
deps = [":collatz"],
1189+
)
1190+
EOF
1191+
1192+
mkdir -p other_repo/src/main/com/example
1193+
cat > other_repo/src/main/com/example/Collatz.java <<'EOF'
1194+
package com.example;
1195+
1196+
public class Collatz {
1197+
1198+
public static int getCollatzFinal(int n) {
1199+
if (n == 1) {
1200+
return 1;
1201+
}
1202+
if (Math.isEven(n)) {
1203+
return getCollatzFinal(n / 2);
1204+
} else {
1205+
return getCollatzFinal(n * 3 + 1);
1206+
}
1207+
}
1208+
1209+
}
1210+
EOF
1211+
1212+
mkdir -p other_repo/src/test/com/example
1213+
cat > other_repo/src/test/com/example/TestCollatz.java <<'EOF'
1214+
package com.example;
1215+
1216+
import static org.junit.Assert.assertEquals;
1217+
import org.junit.Test;
1218+
1219+
public class TestCollatz {
1220+
1221+
@Test
1222+
public void testGetCollatzFinal() {
1223+
assertEquals(Collatz.getCollatzFinal(1), 1);
1224+
assertEquals(Collatz.getCollatzFinal(5), 1);
1225+
assertEquals(Collatz.getCollatzFinal(10), 1);
1226+
assertEquals(Collatz.getCollatzFinal(21), 1);
1227+
}
1228+
1229+
}
1230+
EOF
1231+
}
1232+
1233+
function test_external_java_target_can_collect_coverage() {
1234+
setup_external_java_target
1235+
1236+
bazel coverage --test_output=all @other_repo//:test --combined_report=lcov \
1237+
--instrumentation_filter=// &>$TEST_log \
1238+
|| echo "Coverage for //:test failed"
1239+
1240+
local coverage_file_path="$(get_coverage_file_path_from_test_log)"
1241+
local expected_result_math='SF:src/main/com/example/Math.java
1242+
FN:3,com/example/Math::<init> ()V
1243+
FN:6,com/example/Math::isEven (I)Z
1244+
FNDA:0,com/example/Math::<init> ()V
1245+
FNDA:1,com/example/Math::isEven (I)Z
1246+
FNF:2
1247+
FNH:1
1248+
BRDA:6,0,0,1
1249+
BRDA:6,0,1,1
1250+
BRF:2
1251+
BRH:2
1252+
DA:3,0
1253+
DA:6,1
1254+
LH:1
1255+
LF:2
1256+
end_of_record'
1257+
local expected_result_collatz="SF:external/other_repo/src/main/com/example/Collatz.java
1258+
FN:3,com/example/Collatz::<init> ()V
1259+
FN:6,com/example/Collatz::getCollatzFinal (I)I
1260+
FNDA:0,com/example/Collatz::<init> ()V
1261+
FNDA:1,com/example/Collatz::getCollatzFinal (I)I
1262+
FNF:2
1263+
FNH:1
1264+
BRDA:6,0,0,1
1265+
BRDA:6,0,1,1
1266+
BRDA:9,0,0,1
1267+
BRDA:9,0,1,1
1268+
BRF:4
1269+
BRH:4
1270+
DA:3,0
1271+
DA:6,1
1272+
DA:7,1
1273+
DA:9,1
1274+
DA:10,1
1275+
DA:12,1
1276+
LH:5
1277+
LF:6
1278+
end_of_record"
1279+
1280+
assert_coverage_result "$expected_result_math" "$coverage_file_path"
1281+
assert_coverage_result "$expected_result_collatz" "$coverage_file_path"
1282+
1283+
assert_coverage_result "$expected_result_math" bazel-out/_coverage/_coverage_report.dat
1284+
assert_coverage_result "$expected_result_collatz" bazel-out/_coverage/_coverage_report.dat
1285+
}
1286+
1287+
function test_external_java_target_coverage_not_collected_by_default() {
1288+
setup_external_java_target
1289+
1290+
bazel coverage --test_output=all @other_repo//:test --combined_report=lcov &>$TEST_log \
1291+
|| echo "Coverage for //:test failed"
1292+
1293+
local coverage_file_path="$(get_coverage_file_path_from_test_log)"
1294+
local expected_result_math='SF:src/main/com/example/Math.java
1295+
FN:3,com/example/Math::<init> ()V
1296+
FN:6,com/example/Math::isEven (I)Z
1297+
FNDA:0,com/example/Math::<init> ()V
1298+
FNDA:1,com/example/Math::isEven (I)Z
1299+
FNF:2
1300+
FNH:1
1301+
BRDA:6,0,0,1
1302+
BRDA:6,0,1,1
1303+
BRF:2
1304+
BRH:2
1305+
DA:3,0
1306+
DA:6,1
1307+
LH:1
1308+
LF:2
1309+
end_of_record'
1310+
1311+
assert_coverage_result "$expected_result_math" "$coverage_file_path"
1312+
assert_not_contains "SF:external/other_repo/" "$coverage_file_path"
1313+
1314+
assert_coverage_result "$expected_result_math" bazel-out/_coverage/_coverage_report.dat
1315+
assert_not_contains "SF:external/other_repo/" bazel-out/_coverage/_coverage_report.dat
1316+
}
1317+
11461318
run_suite "test tests"

0 commit comments

Comments
 (0)