Have computed --instrumentation_filter default match packages exactly
This changes //foo to //foo[/:]. The latter matches exactly the set of targets in //foo and its subpackages. The former also matches sibling packages that have that package's name as a prefix (e.g. //foo also matches targets under //foobar). That avoids a confusing (though presumably rare) edge-case that doesn't match the documented behavior.
This also changes that logic to handle targets in the top-level package consistently. Previously, if all test targets were under the top-level package, the computed default for --instrumentation_filter would match everything, but it would not be output. If some of the test targets were in the top-level package, those targets would be ignored when computing the --instrumentation_filter default.
Also adds a bit more debugging output to some related integration tests.
RELNOTES: When computing --instrumentation_filter, end filter patterns with "[/:]" to match non-top-level packages exactly and treat top-level targets consistently.
PiperOrigin-RevId: 210757102
diff --git a/src/main/java/com/google/devtools/build/lib/buildtool/InstrumentationFilterSupport.java b/src/main/java/com/google/devtools/build/lib/buildtool/InstrumentationFilterSupport.java
index 5a58315..3cc4c2a 100644
--- a/src/main/java/com/google/devtools/build/lib/buildtool/InstrumentationFilterSupport.java
+++ b/src/main/java/com/google/devtools/build/lib/buildtool/InstrumentationFilterSupport.java
@@ -58,7 +58,17 @@
collectInstrumentedPackages(testTargets, packageFilters);
optimizeFilterSet(packageFilters);
- String instrumentationFilter = "//" + Joiner.on(",//").join(packageFilters);
+ String instrumentationFilter =
+ Joiner.on("[/:],//")
+ .appendTo(new StringBuilder("//"), packageFilters)
+ .append("[/:]")
+ .toString();
+ // Fix up if one of the test targets is a top-level target. "//foo[/:]" matches everything
+ // under //foo and subpackages, but "//[/:]" only matches targets directly under the top-level
+ // package.
+ if (instrumentationFilter.equals("//[/:]")) {
+ instrumentationFilter = "//";
+ }
if (!packageFilters.isEmpty()) {
eventHandler.handle(
Event.info("Using default value for --instrumentation_filter: \""
@@ -73,10 +83,7 @@
Collection<Target> targets, Set<String> packageFilters) {
for (Target target : targets) {
// Add package-based filters for every test target.
- String prefix = getInstrumentedPrefix(target.getLabel().getPackageName());
- if (!prefix.isEmpty()) {
- packageFilters.add(prefix);
- }
+ packageFilters.add(getInstrumentedPrefix(target.getLabel().getPackageName()));
if (TargetUtils.isTestSuiteRule(target)) {
AttributeMap attributes = NonconfigurableAttributeMapper.of((Rule) target);
// We don't need to handle $implicit_tests attribute since we already added
@@ -122,14 +129,15 @@
Set<String> parentFilters = Sets.newTreeSet();
String filterString = iterator.next();
PathFragment parent = PathFragment.create(filterString).getParentDirectory();
+ String parentPath = (parent == null) ? "" : parent.getPathString();
while (iterator.hasNext()) {
String current = iterator.next();
- if (parent != null && parent.getPathString().length() > 0
- && !current.startsWith(filterString) && current.startsWith(parent.getPathString())) {
- parentFilters.add(parent.getPathString());
+ if (!current.startsWith(filterString) && current.startsWith(parentPath)) {
+ parentFilters.add(parentPath);
} else {
filterString = current;
parent = PathFragment.create(filterString).getParentDirectory();
+ parentPath = (parent == null) ? "" : parent.getPathString();
}
}
packageFilters.addAll(parentFilters);
diff --git a/src/test/shell/bazel/bazel_coverage_test.sh b/src/test/shell/bazel/bazel_coverage_test.sh
index ae2b148..8e0c57f 100755
--- a/src/test/shell/bazel/bazel_coverage_test.sh
+++ b/src/test/shell/bazel/bazel_coverage_test.sh
@@ -198,7 +198,7 @@
}
EOF
- bazel coverage //:test &>$TEST_log || fail "Coverage for //:test failed"
+ bazel coverage --test_output=all //:test &>$TEST_log || fail "Coverage for //:test failed"
cat $TEST_log
ending_part=$(sed -n -e '/PASSED/,$p' $TEST_log)
@@ -290,7 +290,7 @@
}
EOF
- bazel coverage //:test --coverage_report_generator=@bazel_tools//tools/test/LcovMerger/java/com/google/devtools/lcovmerger:Main --combined_report=lcov &>$TEST_log \
+ bazel coverage --test_output=all //:test --coverage_report_generator=@bazel_tools//tools/test/LcovMerger/java/com/google/devtools/lcovmerger:Main --combined_report=lcov &>$TEST_log \
|| echo "Coverage for //:test failed"
cat <<EOF > result.dat
@@ -383,7 +383,7 @@
}
EOF
- bazel coverage --experimental_java_coverage //:test &>$TEST_log || fail "Coverage for //:test failed"
+ bazel coverage --test_output=all --experimental_java_coverage //:test &>$TEST_log || fail "Coverage for //:test failed"
ending_part=$(sed -n -e '/PASSED/,$p' $TEST_log)
coverage_file_path=$(grep -Eo "/[/a-zA-Z0-9\.\_\-]+\.dat$" <<< "$ending_part")
@@ -470,7 +470,7 @@
}
EOF
- bazel coverage //:orange-sh &>$TEST_log || fail "Coverage for //:orange-sh failed"
+ bazel coverage --test_output=all //:orange-sh &>$TEST_log || fail "Coverage for //:orange-sh failed"
ending_part=$(sed -n -e '/PASSED/,$p' $TEST_log)
