Add --[no]instrument_test_targets option to Blaze

When true (the default), test rules included by --instrumentation_filter are instrumented. When false, test rules are excluded from instrumentation (whether or not their names would be matched by --instrumentation_filter).

This option provides a superior way to exclude test rules from instrumentation, compared to setting --instrumentation_filter exclusions based on test rule naming conventions. Naming conventions vary from language to language and are not always followed. Blaze already has enforced semantics for test targets (test rule build function names _must_ end in "_test", see TargetUtils.isTestRule). In general, when either option is available, it's better to rely on enforced semantics than unenforced conventions.

Note that this default preserves present behavior. In the future, I think it would make sense to:
* Change the default value of --instrument_test_targets to false.
* Change the default value of --instrumentation_filter to an empty string (match everything).

RELNOTES: Add --instrument_test_targets option.

--
MOS_MIGRATED_REVID=124732226

3 files changed, 31 insertions, 4 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java b/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java
index f7671a2655..3977caf460 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java
@@ -1284,6 +1284,13 @@ public final class RuleContext extends TargetContext
   }
 
   /**
+   * Returns true if the target for this context is a test target.
+   */
+  public boolean isTestTarget() {
+    return TargetUtils.isTestRule(getTarget());
+  }
+
+  /**
    * Returns true if runfiles support should create the runfiles tree, or
    * false if it should just create the manifest.
    */
@@ -1298,7 +1305,7 @@ public final class RuleContext extends TargetContext
     //  b. host tools could potentially use data files, but currently don't
     //     (they're run from the execution root, not a runfiles tree).
     //     Currently hostConfiguration.buildRunfiles() returns true.
-    if (TargetUtils.isTestRule(getTarget())) {
+    if (isTestTarget()) {
       // Tests are only executed during testing (duh),
       // and their runfiles are generated lazily on local
       // execution (see LocalTestStrategy). Therefore, it
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfiguration.java b/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfiguration.java
index 3f8c6be31f..f3d7b7f470 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfiguration.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfiguration.java
@@ -559,7 +559,7 @@ public final class BuildConfiguration {
     public boolean stampBinaries;
 
     // TODO(bazel-team): delete from OSS tree
-    // This value is always overwritten in the case of "blaze coverage" by :
+    // This default value is always overwritten in the case of "blaze coverage" by
     // CoverageCommand.setDefaultInstrumentationFilter()
     @Option(name = "instrumentation_filter",
         converter = RegexFilter.RegexFilterConverter.class,
@@ -571,6 +571,15 @@ public final class BuildConfiguration {
             + "'javatests' or ending with '_test' will not be instrumented.")
     public RegexFilter instrumentationFilter;
 
+    @Option(name = "instrument_test_targets",
+        defaultValue = "true",
+        category = "semantics",
+        help = "When coverage is enabled, specifies whether to consider instrumenting test rules. "
+            + "When true (the default), test rules included by --instrumentation_filter are "
+            + "instrumented. When false, test rules are always excluded from coverage "
+            + "instrumentation.")
+    public boolean instrumentTestTargets;
+
     @Option(name = "show_cached_analysis_results",
         defaultValue = "true",
         category = "undocumented",
@@ -2045,6 +2054,15 @@ public final class BuildConfiguration {
   }
 
   /**
+   * Returns a boolean of whether to include targets created by *_test rules in the set of targets
+   * matched by --instrumentation_filter. If this is false, all test targets are excluded from
+   * instrumentation.
+   */
+  public boolean shouldInstrumentTestTargets() {
+    return options.instrumentTestTargets;
+  }
+
+  /**
    * Returns the set of labels for coverage.
    */
   public Set<Label> getCoverageLabels() {
diff --git a/src/main/java/com/google/devtools/build/lib/rules/test/InstrumentedFilesCollector.java b/src/main/java/com/google/devtools/build/lib/rules/test/InstrumentedFilesCollector.java
index 9951f63d46..bd20a529fd 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/test/InstrumentedFilesCollector.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/test/InstrumentedFilesCollector.java
@@ -21,6 +21,7 @@ import com.google.devtools.build.lib.analysis.FileProvider;
 import com.google.devtools.build.lib.analysis.RuleConfiguredTarget.Mode;
 import com.google.devtools.build.lib.analysis.RuleContext;
 import com.google.devtools.build.lib.analysis.TransitiveInfoCollection;
+import com.google.devtools.build.lib.analysis.config.BuildConfiguration;
 import com.google.devtools.build.lib.collect.nestedset.NestedSet;
 import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
 import com.google.devtools.build.lib.collect.nestedset.Order;
@@ -250,8 +251,9 @@ public final class InstrumentedFilesCollector {
   public static final LocalMetadataCollector NO_METADATA_COLLECTOR = null;
 
   private static boolean shouldIncludeLocalSources(RuleContext ruleContext) {
-    return ruleContext.getConfiguration().getInstrumentationFilter().isIncluded(
-        ruleContext.getLabel().toString());
+    BuildConfiguration config = ruleContext.getConfiguration();
+    return ((config.shouldInstrumentTestTargets() || !ruleContext.isTestTarget())
+        && config.getInstrumentationFilter().isIncluded(ruleContext.getLabel().toString()));
   }
 
   private static Iterable<TransitiveInfoCollection> getAllPrerequisites(