Break out --test_filter into a test configuration fragment.

The other test configuration options will follow in subsequent CLs to keep size manageable.

PiperOrigin-RevId: 164068510

6 files changed, 177 insertions, 78 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/analysis/BaseRuleClasses.java b/src/main/java/com/google/devtools/build/lib/analysis/BaseRuleClasses.java
index b4553767c8..2c30c49ee6 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/BaseRuleClasses.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/BaseRuleClasses.java
@@ -49,6 +49,7 @@ import com.google.devtools.build.lib.packages.RuleClass.Builder;
 import com.google.devtools.build.lib.packages.RuleClass.Builder.RuleClassType;
 import com.google.devtools.build.lib.packages.TestSize;
 import com.google.devtools.build.lib.rules.MakeVariableProvider;
+import com.google.devtools.build.lib.rules.test.TestConfiguration;
 import com.google.devtools.build.lib.syntax.Type;
 import com.google.devtools.build.lib.util.FileTypeSet;
 import java.util.List;
@@ -110,45 +111,61 @@ public class BaseRuleClasses {
     @Override
     public RuleClass build(Builder builder, RuleDefinitionEnvironment env) {
       return builder
-          .add(attr("size", STRING).value("medium").taggable()
-              .nonconfigurable("policy decision: should be consistent across configurations"))
-          .add(attr("timeout", STRING).taggable()
-              .nonconfigurable("policy decision: should be consistent across configurations")
-              .value(new Attribute.ComputedDefault() {
-                @Override
-                public Object getDefault(AttributeMap rule) {
-                  TestSize size = TestSize.getTestSize(rule.get("size", Type.STRING));
-                  if (size != null) {
-                    String timeout = size.getDefaultTimeout().toString();
-                    if (timeout != null) {
-                      return timeout;
-                    }
-                  }
-                  return "illegal";
-                }
-              }))
-          .add(attr("flaky", BOOLEAN).value(false).taggable()
-              .nonconfigurable("policy decision: should be consistent across configurations"))
+          .requiresConfigurationFragments(TestConfiguration.class)
+          .add(
+              attr("size", STRING)
+                  .value("medium")
+                  .taggable()
+                  .nonconfigurable("policy decision: should be consistent across configurations"))
+          .add(
+              attr("timeout", STRING)
+                  .taggable()
+                  .nonconfigurable("policy decision: should be consistent across configurations")
+                  .value(
+                      new Attribute.ComputedDefault() {
+                        @Override
+                        public Object getDefault(AttributeMap rule) {
+                          TestSize size = TestSize.getTestSize(rule.get("size", Type.STRING));
+                          if (size != null) {
+                            String timeout = size.getDefaultTimeout().toString();
+                            if (timeout != null) {
+                              return timeout;
+                            }
+                          }
+                          return "illegal";
+                        }
+                      }))
+          .add(
+              attr("flaky", BOOLEAN)
+                  .value(false)
+                  .taggable()
+                  .nonconfigurable("policy decision: should be consistent across configurations"))
           .add(attr("shard_count", INTEGER).value(-1))
-          .add(attr("local", BOOLEAN).value(false).taggable()
-              .nonconfigurable("policy decision: should be consistent across configurations"))
+          .add(
+              attr("local", BOOLEAN)
+                  .value(false)
+                  .taggable()
+                  .nonconfigurable("policy decision: should be consistent across configurations"))
           .add(attr("args", STRING_LIST))
           // Input files for every test action
-          .add(attr("$test_runtime", LABEL_LIST).cfg(HOST).value(ImmutableList.of(
-              env.getToolsLabel("//tools/test:runtime"))))
+          .add(
+              attr("$test_runtime", LABEL_LIST)
+                  .cfg(HOST)
+                  .value(ImmutableList.of(env.getToolsLabel("//tools/test:runtime"))))
           // Input files for test actions collecting code coverage
-          .add(attr("$coverage_support", LABEL)
-              .value(env.getLabel("//tools/defaults:coverage_support")))
+          .add(
+              attr("$coverage_support", LABEL)
+                  .value(env.getLabel("//tools/defaults:coverage_support")))
           // Used in the one-per-build coverage report generation action.
-          .add(attr("$coverage_report_generator", LABEL)
-              .cfg(HOST)
-              .value(env.getLabel("//tools/defaults:coverage_report_generator"))
-              .singleArtifact())
+          .add(
+              attr("$coverage_report_generator", LABEL)
+                  .cfg(HOST)
+                  .value(env.getLabel("//tools/defaults:coverage_report_generator"))
+                  .singleArtifact())
 
           // The target itself and run_under both run on the same machine. We use the DATA config
           // here because the run_under acts like a data dependency (e.g. no LIPO optimization).
-          .add(attr(":run_under", LABEL).cfg(DATA).value(RUN_UNDER)
-              .skipPrereqValidatorCheck())
+          .add(attr(":run_under", LABEL).cfg(DATA).value(RUN_UNDER).skipPrereqValidatorCheck())
           .build();
     }
 
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 c301d63dcb..106a066666 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
@@ -875,19 +875,6 @@ public final class BuildConfiguration implements BuildEvent {
     public List<String> testArguments;
 
     @Option(
-      name = "test_filter",
-      allowMultiple = false,
-      defaultValue = "null",
-      category = "testing",
-      documentationCategory = OptionDocumentationCategory.UNCATEGORIZED,
-      effectTags = {OptionEffectTag.UNKNOWN},
-      help =
-          "Specifies a filter to forward to the test framework.  Used to limit "
-              + "the tests run. Note that this does not affect which targets are built."
-    )
-    public String testFilter;
-
-    @Option(
       name = "check_fileset_dependencies_recursively",
       defaultValue = "true",
       category = "semantics",
@@ -2539,10 +2526,6 @@ public final class BuildConfiguration implements BuildEvent {
     return options.testArguments;
   }
 
-  public String getTestFilter() {
-    return options.testFilter;
-  }
-
   /**
    * Returns user-specified test environment variables and their values, as set by the --test_env
    * options.
diff --git a/src/main/java/com/google/devtools/build/lib/rules/SkylarkRuleClassFunctions.java b/src/main/java/com/google/devtools/build/lib/rules/SkylarkRuleClassFunctions.java
index b71db3b8c8..41107725f4 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/SkylarkRuleClassFunctions.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/SkylarkRuleClassFunctions.java
@@ -72,6 +72,7 @@ import com.google.devtools.build.lib.packages.SkylarkProvider;
 import com.google.devtools.build.lib.packages.TargetUtils;
 import com.google.devtools.build.lib.packages.TestSize;
 import com.google.devtools.build.lib.rules.SkylarkAttr.Descriptor;
+import com.google.devtools.build.lib.rules.test.TestConfiguration;
 import com.google.devtools.build.lib.skylarkinterface.Param;
 import com.google.devtools.build.lib.skylarkinterface.SkylarkPrinter;
 import com.google.devtools.build.lib.skylarkinterface.SkylarkSignature;
@@ -142,41 +143,61 @@ public class SkylarkRuleClassFunctions {
   /** Parent rule class for test Skylark rules. */
   public static final RuleClass getTestBaseRule(String toolsRepository) {
     return new RuleClass.Builder("$test_base_rule", RuleClassType.ABSTRACT, true, baseRule)
-        .add(attr("size", STRING).value("medium").taggable()
-            .nonconfigurable("used in loading phase rule validation logic"))
-        .add(attr("timeout", STRING).taggable()
-            .nonconfigurable("used in loading phase rule validation logic").value(
-                new Attribute.ComputedDefault() {
-                  @Override
-                  public Object getDefault(AttributeMap rule) {
-                    TestSize size = TestSize.getTestSize(rule.get("size", Type.STRING));
-                    if (size != null) {
-                      String timeout = size.getDefaultTimeout().toString();
-                      if (timeout != null) {
-                        return timeout;
+        .requiresConfigurationFragments(TestConfiguration.class)
+        .add(
+            attr("size", STRING)
+                .value("medium")
+                .taggable()
+                .nonconfigurable("used in loading phase rule validation logic"))
+        .add(
+            attr("timeout", STRING)
+                .taggable()
+                .nonconfigurable("used in loading phase rule validation logic")
+                .value(
+                    new Attribute.ComputedDefault() {
+                      @Override
+                      public Object getDefault(AttributeMap rule) {
+                        TestSize size = TestSize.getTestSize(rule.get("size", Type.STRING));
+                        if (size != null) {
+                          String timeout = size.getDefaultTimeout().toString();
+                          if (timeout != null) {
+                            return timeout;
+                          }
+                        }
+                        return "illegal";
                       }
-                    }
-                    return "illegal";
-                  }
-                }))
-        .add(attr("flaky", BOOLEAN).value(false).taggable()
-            .nonconfigurable("taggable - called in Rule.getRuleTags"))
+                    }))
+        .add(
+            attr("flaky", BOOLEAN)
+                .value(false)
+                .taggable()
+                .nonconfigurable("taggable - called in Rule.getRuleTags"))
         .add(attr("shard_count", INTEGER).value(-1))
-        .add(attr("local", BOOLEAN).value(false).taggable()
-            .nonconfigurable("policy decision: this should be consistent across configurations"))
+        .add(
+            attr("local", BOOLEAN)
+                .value(false)
+                .taggable()
+                .nonconfigurable(
+                    "policy decision: this should be consistent across configurations"))
         .add(attr("args", STRING_LIST))
         // Input files for every test action
-        .add(attr("$test_runtime", LABEL_LIST).cfg(HOST).value(ImmutableList.of(
-            labelCache.getUnchecked(toolsRepository + "//tools/test:runtime"))))
+        .add(
+            attr("$test_runtime", LABEL_LIST)
+                .cfg(HOST)
+                .value(
+                    ImmutableList.of(
+                        labelCache.getUnchecked(toolsRepository + "//tools/test:runtime"))))
         // Input files for test actions collecting code coverage
-        .add(attr("$coverage_support", LABEL)
-            .cfg(HOST)
-            .value(labelCache.getUnchecked("//tools/defaults:coverage_support")))
+        .add(
+            attr("$coverage_support", LABEL)
+                .cfg(HOST)
+                .value(labelCache.getUnchecked("//tools/defaults:coverage_support")))
         // Used in the one-per-build coverage report generation action.
-        .add(attr("$coverage_report_generator", LABEL)
-            .cfg(HOST)
-            .value(labelCache.getUnchecked("//tools/defaults:coverage_report_generator"))
-            .singleArtifact())
+        .add(
+            attr("$coverage_report_generator", LABEL)
+                .cfg(HOST)
+                .value(labelCache.getUnchecked("//tools/defaults:coverage_report_generator"))
+                .singleArtifact())
         .add(attr(":run_under", LABEL).cfg(DATA).value(RUN_UNDER))
         .build();
   }
diff --git a/src/main/java/com/google/devtools/build/lib/rules/core/CoreRules.java b/src/main/java/com/google/devtools/build/lib/rules/core/CoreRules.java
index 9494b4edd5..2ab890d20a 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/core/CoreRules.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/core/CoreRules.java
@@ -21,6 +21,7 @@ import com.google.devtools.build.lib.analysis.ConfiguredRuleClassProvider.RuleSe
 import com.google.devtools.build.lib.analysis.featurecontrol.FeaturePolicyLoader;
 import com.google.devtools.build.lib.analysis.featurecontrol.FeaturePolicyOptions;
 import com.google.devtools.build.lib.rules.config.ConfigFeatureFlag;
+import com.google.devtools.build.lib.rules.test.TestConfiguration;
 
 /** A set of basic rules - Bazel won't work correctly without these. */
 public final class CoreRules implements RuleSet {
@@ -39,6 +40,7 @@ public final class CoreRules implements RuleSet {
     builder.addConfigurationFragment(new FeaturePolicyLoader(FEATURE_POLICY_FEATURES));
     builder.addDynamicTransitionMaps(BaseRuleClasses.DYNAMIC_TRANSITIONS_MAP);
 
+    builder.addConfig(TestConfiguration.TestOptions.class, new TestConfiguration.Loader());
     builder.addRuleDefinition(new BaseRuleClasses.RootRule());
     builder.addRuleDefinition(new BaseRuleClasses.BaseRule());
     builder.addRuleDefinition(new BaseRuleClasses.RuleBase());
diff --git a/src/main/java/com/google/devtools/build/lib/rules/test/TestConfiguration.java b/src/main/java/com/google/devtools/build/lib/rules/test/TestConfiguration.java
new file mode 100644
index 0000000000..936d2af98a
--- /dev/null
+++ b/src/main/java/com/google/devtools/build/lib/rules/test/TestConfiguration.java
@@ -0,0 +1,75 @@
+// Copyright 2017 The Bazel Authors. All rights reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//    http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.devtools.build.lib.rules.test;
+
+import com.google.common.collect.ImmutableSet;
+import com.google.devtools.build.lib.analysis.config.BuildConfiguration.Fragment;
+import com.google.devtools.build.lib.analysis.config.BuildOptions;
+import com.google.devtools.build.lib.analysis.config.ConfigurationEnvironment;
+import com.google.devtools.build.lib.analysis.config.ConfigurationFragmentFactory;
+import com.google.devtools.build.lib.analysis.config.FragmentOptions;
+import com.google.devtools.build.lib.analysis.config.InvalidConfigurationException;
+import com.google.devtools.common.options.Option;
+import com.google.devtools.common.options.OptionDocumentationCategory;
+import com.google.devtools.common.options.OptionEffectTag;
+
+/** Test-related options. */
+public class TestConfiguration extends Fragment {
+
+  /** Command-line options. */
+  public static class TestOptions extends FragmentOptions {
+    @Option(
+      name = "test_filter",
+      allowMultiple = false,
+      defaultValue = "null",
+      category = "testing",
+      documentationCategory = OptionDocumentationCategory.UNCATEGORIZED,
+      effectTags = {OptionEffectTag.UNKNOWN},
+      help =
+          "Specifies a filter to forward to the test framework.  Used to limit "
+              + "the tests run. Note that this does not affect which targets are built."
+    )
+    public String testFilter;
+  }
+
+  /** Configuration loader for test options */
+  public static class Loader implements ConfigurationFragmentFactory {
+    @Override
+    public Fragment create(ConfigurationEnvironment env, BuildOptions buildOptions)
+        throws InvalidConfigurationException {
+      return new TestConfiguration(buildOptions.get(TestOptions.class));
+    }
+
+    @Override
+    public Class<? extends Fragment> creates() {
+      return TestConfiguration.class;
+    }
+
+    @Override
+    public ImmutableSet<Class<? extends FragmentOptions>> requiredOptions() {
+      return ImmutableSet.of(TestOptions.class);
+    }
+  }
+
+  private final TestOptions options;
+
+  TestConfiguration(TestOptions options) {
+    this.options = options;
+  }
+
+  public String getTestFilter() {
+    return options.testFilter;
+  }
+}
diff --git a/src/main/java/com/google/devtools/build/lib/rules/test/TestTargetExecutionSettings.java b/src/main/java/com/google/devtools/build/lib/rules/test/TestTargetExecutionSettings.java
index 5a01bcb0ba..6a6109e53f 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/test/TestTargetExecutionSettings.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/test/TestTargetExecutionSettings.java
@@ -52,6 +52,7 @@ public final class TestTargetExecutionSettings {
     Preconditions.checkArgument(TargetUtils.isTestRule(ruleContext.getRule()));
     Preconditions.checkArgument(shards >= 0);
     BuildConfiguration config = ruleContext.getConfiguration();
+    TestConfiguration testConfig = config.getFragment(TestConfiguration.class);
 
     CommandLine targetArgs = runfilesSupport.getArgs();
     testArguments = CommandLine.concat(targetArgs, ImmutableList.copyOf(config.getTestArguments()));
@@ -60,7 +61,7 @@ public final class TestTargetExecutionSettings {
     runUnder = config.getRunUnder();
     runUnderExecutable = getRunUnderExecutable(ruleContext);
 
-    this.testFilter = config.getTestFilter();
+    this.testFilter = testConfig.getTestFilter();
     this.executable = executable;
     this.runfilesSymlinksCreated = runfilesSupport.getCreateSymlinks();
     this.runfilesDir = runfilesSupport.getRunfilesDirectory();