Move most test options from BuildConfiguration to TestConfiguration.

--test_env isn't moved in this CL since it's exposed to Skylark via BuildConfiguration, making it a somewhat riskier refactor.

PiperOrigin-RevId: 164266168

7 files changed, 149 insertions, 129 deletions

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 a96156aa42..1a462df820 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
@@ -64,7 +64,6 @@ import com.google.devtools.build.lib.packages.RuleClass;
 import com.google.devtools.build.lib.packages.RuleClassProvider;
 import com.google.devtools.build.lib.packages.RuleTransitionFactory;
 import com.google.devtools.build.lib.packages.Target;
-import com.google.devtools.build.lib.rules.test.TestActionBuilder;
 import com.google.devtools.build.lib.skylarkinterface.SkylarkCallable;
 import com.google.devtools.build.lib.skylarkinterface.SkylarkModule;
 import com.google.devtools.build.lib.skylarkinterface.SkylarkModuleCategory;
@@ -767,75 +766,6 @@ public final class BuildConfiguration implements BuildEvent {
     public boolean useLLVMCoverageMapFormat;
 
     @Option(
-      name = "cache_test_results",
-      defaultValue = "auto",
-      category = "testing",
-      abbrev = 't', // it's useful to toggle this on/off quickly
-      documentationCategory = OptionDocumentationCategory.UNCATEGORIZED,
-      effectTags = {OptionEffectTag.UNKNOWN},
-      help =
-          "If set to 'auto', Bazel reruns a test if and only if: "
-              + "(1) Bazel detects changes in the test or its dependencies, "
-              + "(2) the test is marked as external, "
-              + "(3) multiple test runs were requested with --runs_per_test, or"
-              + "(4) the test previously failed. "
-              + "If set to 'yes', Bazel caches all test results except for tests marked as "
-              + "external. If set to 'no', Bazel does not cache any test results."
-    )
-    public TriState cacheTestResults;
-
-    @Deprecated
-    @Option(
-      name = "test_result_expiration",
-      defaultValue = "-1", // No expiration by defualt.
-      category = "testing",
-      documentationCategory = OptionDocumentationCategory.UNCATEGORIZED,
-      effectTags = {OptionEffectTag.UNKNOWN},
-      help = "This option is deprecated and has no effect."
-    )
-    public int testResultExpiration;
-
-    @Option(
-      name = "test_sharding_strategy",
-      defaultValue = "explicit",
-      category = "testing",
-      converter = TestActionBuilder.ShardingStrategyConverter.class,
-      documentationCategory = OptionDocumentationCategory.UNCATEGORIZED,
-      effectTags = {OptionEffectTag.UNKNOWN},
-      help =
-          "Specify strategy for test sharding: "
-              + "'explicit' to only use sharding if the 'shard_count' BUILD attribute is present. "
-              + "'disabled' to never use test sharding. "
-              + "'experimental_heuristic' to enable sharding on remotely executed tests without an "
-              + "explicit  'shard_count' attribute which link in a supported framework. Considered "
-              + "experimental."
-    )
-    public TestActionBuilder.TestShardingStrategy testShardingStrategy;
-
-    @Option(
-      name = "runs_per_test",
-      allowMultiple = true,
-      defaultValue = "1",
-      category = "testing",
-      converter = RunsPerTestConverter.class,
-      documentationCategory = OptionDocumentationCategory.UNCATEGORIZED,
-      effectTags = {OptionEffectTag.UNKNOWN},
-      help =
-          "Specifies number of times to run each test. If any of those attempts "
-              + "fail for any reason, the whole test would be considered failed. "
-              + "Normally the value specified is just an integer. Example: --runs_per_test=3 "
-              + "will run all tests 3 times. "
-              + "Alternate syntax: regex_filter@runs_per_test. Where runs_per_test stands for "
-              + "an integer value and regex_filter stands "
-              + "for a list of include and exclude regular expression patterns (Also see "
-              + "--instrumentation_filter). Example: "
-              + "--runs_per_test=//foo/.*,-//foo/bar/.*@3 runs all tests in //foo/ "
-              + "except those under foo/bar three times. "
-              + "This option can be passed multiple times. "
-    )
-    public List<PerLabelOptions> runsPerTest;
-
-    @Option(
       name = "build_runfile_links",
       defaultValue = "true",
       category = "strategy",
@@ -860,21 +790,6 @@ public final class BuildConfiguration implements BuildEvent {
     public boolean legacyExternalRunfiles;
 
     @Option(
-      name = "test_arg",
-      allowMultiple = true,
-      defaultValue = "",
-      category = "testing",
-      documentationCategory = OptionDocumentationCategory.UNCATEGORIZED,
-      effectTags = {OptionEffectTag.UNKNOWN},
-      help =
-          "Specifies additional options and arguments that should be passed to the test "
-              + "executable. Can be used multiple times to specify several arguments. "
-              + "If multiple tests are executed, each of them will receive identical arguments. "
-              + "Used only by the 'bazel test' command."
-    )
-    public List<String> testArguments;
-
-    @Option(
       name = "check_fileset_dependencies_recursively",
       defaultValue = "true",
       category = "semantics",
@@ -1446,14 +1361,6 @@ public final class BuildConfiguration implements BuildEvent {
       reporter.handle(Event.error(
           "The internal '--output directory name' option cannot be used on the command line"));
     }
-
-    if (options.testShardingStrategy
-        == TestActionBuilder.TestShardingStrategy.EXPERIMENTAL_HEURISTIC) {
-      reporter.handle(Event.warn(
-          "Heuristic sharding is intended as a one-off experimentation tool for determing the "
-              + "benefit from sharding certain tests. Please don't keep this option in your "
-              + ".blazerc or continuous build"));
-    }
   }
 
   /**
@@ -2534,10 +2441,6 @@ public final class BuildConfiguration implements BuildEvent {
     return options.skyframeNativeFileset;
   }
 
-  public List<String> getTestArguments() {
-    return options.testArguments;
-  }
-
   /**
    * Returns user-specified test environment variables and their values, as set by the --test_env
    * options.
@@ -2565,10 +2468,6 @@ public final class BuildConfiguration implements BuildEvent {
     return testEnv;
   }
 
-  public TriState cacheTestResults() {
-    return options.cacheTestResults;
-  }
-
   public int getMinParamFileSize() {
     return options.minParamFileSize;
   }
@@ -2591,23 +2490,6 @@ public final class BuildConfiguration implements BuildEvent {
     return actionsEnabled;
   }
 
-  public TestActionBuilder.TestShardingStrategy testShardingStrategy() {
-    return options.testShardingStrategy;
-  }
-
-  /**
-   * @return number of times the given test should run.
-   * If the test doesn't match any of the filters, runs it once.
-   */
-  public int getRunsPerTestForLabel(Label label) {
-    for (PerLabelOptions perLabelRuns : options.runsPerTest) {
-      if (perLabelRuns.isIncluded(label)) {
-        return Integer.parseInt(Iterables.getOnlyElement(perLabelRuns.getOptions()));
-      }
-    }
-    return 1;
-  }
-
   public RunUnder getRunUnder() {
     return options.runUnder;
   }
diff --git a/src/main/java/com/google/devtools/build/lib/bazel/rules/java/BazelJavaSemantics.java b/src/main/java/com/google/devtools/build/lib/bazel/rules/java/BazelJavaSemantics.java
index 49fface0ea..3fd9bb9334 100644
--- a/src/main/java/com/google/devtools/build/lib/bazel/rules/java/BazelJavaSemantics.java
+++ b/src/main/java/com/google/devtools/build/lib/bazel/rules/java/BazelJavaSemantics.java
@@ -56,6 +56,7 @@ import com.google.devtools.build.lib.rules.java.JavaTargetAttributes;
 import com.google.devtools.build.lib.rules.java.JavaUtil;
 import com.google.devtools.build.lib.rules.java.Jvm;
 import com.google.devtools.build.lib.rules.java.proto.GeneratedExtensionRegistryProvider;
+import com.google.devtools.build.lib.rules.test.TestConfiguration;
 import com.google.devtools.build.lib.syntax.Type;
 import com.google.devtools.build.lib.util.OS;
 import com.google.devtools.build.lib.util.Preconditions;
@@ -664,7 +665,9 @@ public class BazelJavaSemantics implements JavaSemantics {
   public List<String> getExtraArguments(RuleContext ruleContext, ImmutableList<Artifact> sources) {
     if (ruleContext.getRule().getRuleClass().equals("java_test")) {
       if (useLegacyJavaTest(ruleContext)) {
-        if (ruleContext.getConfiguration().getTestArguments().isEmpty()
+        TestConfiguration testConfiguration =
+            ruleContext.getConfiguration().getFragment(TestConfiguration.class);
+        if (testConfiguration.getTestArguments().isEmpty()
             && !ruleContext.attributes().isAttributeValueExplicitlySpecified("args")) {
           ImmutableList.Builder<String> builder = ImmutableList.builder();
           for (Artifact artifact : sources) {
diff --git a/src/main/java/com/google/devtools/build/lib/rules/test/TestActionBuilder.java b/src/main/java/com/google/devtools/build/lib/rules/test/TestActionBuilder.java
index 6de3553c34..8b3b1f8aef 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/test/TestActionBuilder.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/test/TestActionBuilder.java
@@ -73,7 +73,8 @@ public final class TestActionBuilder {
   public TestParams build() {
     Preconditions.checkState(runfilesSupport != null);
     boolean local = TargetUtils.isTestRuleAndRunsLocally(ruleContext.getRule());
-    TestShardingStrategy strategy = ruleContext.getConfiguration().testShardingStrategy();
+    TestShardingStrategy strategy =
+        ruleContext.getConfiguration().getFragment(TestConfiguration.class).testShardingStrategy();
     int shards = strategy.getNumberOfShards(
         local, explicitShardCount, isTestShardingCompliant(),
         TestSize.getTestSize(ruleContext.getRule()));
@@ -259,7 +260,8 @@ public final class TestActionBuilder {
       }
     }
 
-    int runsPerTest = config.getRunsPerTestForLabel(ruleContext.getLabel());
+    int runsPerTest =
+        config.getFragment(TestConfiguration.class).getRunsPerTestForLabel(ruleContext.getLabel());
 
     Iterable<Artifact> inputs = inputsBuilder.build();
     int shardRuns = (shards > 0 ? shards : 1);
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
index 936d2af98a..b7bb2f5b12 100644
--- 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
@@ -15,15 +15,23 @@
 package com.google.devtools.build.lib.rules.test;
 
 import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.Iterables;
 import com.google.devtools.build.lib.analysis.config.BuildConfiguration.Fragment;
+import com.google.devtools.build.lib.analysis.config.BuildConfiguration.RunsPerTestConverter;
 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.build.lib.analysis.config.PerLabelOptions;
+import com.google.devtools.build.lib.cmdline.Label;
+import com.google.devtools.build.lib.events.Event;
+import com.google.devtools.build.lib.events.EventHandler;
 import com.google.devtools.common.options.Option;
 import com.google.devtools.common.options.OptionDocumentationCategory;
 import com.google.devtools.common.options.OptionEffectTag;
+import com.google.devtools.common.options.TriState;
+import java.util.List;
 
 /** Test-related options. */
 public class TestConfiguration extends Fragment {
@@ -42,6 +50,90 @@ public class TestConfiguration extends Fragment {
               + "the tests run. Note that this does not affect which targets are built."
     )
     public String testFilter;
+
+    @Option(
+      name = "cache_test_results",
+      defaultValue = "auto",
+      category = "testing",
+      abbrev = 't', // it's useful to toggle this on/off quickly
+      documentationCategory = OptionDocumentationCategory.UNCATEGORIZED,
+      effectTags = {OptionEffectTag.UNKNOWN},
+      help =
+          "If set to 'auto', Bazel reruns a test if and only if: "
+              + "(1) Bazel detects changes in the test or its dependencies, "
+              + "(2) the test is marked as external, "
+              + "(3) multiple test runs were requested with --runs_per_test, or"
+              + "(4) the test previously failed. "
+              + "If set to 'yes', Bazel caches all test results except for tests marked as "
+              + "external. If set to 'no', Bazel does not cache any test results."
+    )
+    public TriState cacheTestResults;
+
+    @Deprecated
+    @Option(
+      name = "test_result_expiration",
+      defaultValue = "-1", // No expiration by defualt.
+      category = "testing",
+      documentationCategory = OptionDocumentationCategory.UNCATEGORIZED,
+      effectTags = {OptionEffectTag.UNKNOWN},
+      help = "This option is deprecated and has no effect."
+    )
+    public int testResultExpiration;
+
+    @Option(
+      name = "test_arg",
+      allowMultiple = true,
+      defaultValue = "",
+      category = "testing",
+      documentationCategory = OptionDocumentationCategory.UNCATEGORIZED,
+      effectTags = {OptionEffectTag.UNKNOWN},
+      help =
+          "Specifies additional options and arguments that should be passed to the test "
+              + "executable. Can be used multiple times to specify several arguments. "
+              + "If multiple tests are executed, each of them will receive identical arguments. "
+              + "Used only by the 'bazel test' command."
+    )
+    public List<String> testArguments;
+
+    @Option(
+      name = "test_sharding_strategy",
+      defaultValue = "explicit",
+      category = "testing",
+      converter = TestActionBuilder.ShardingStrategyConverter.class,
+      documentationCategory = OptionDocumentationCategory.UNCATEGORIZED,
+      effectTags = {OptionEffectTag.UNKNOWN},
+      help =
+          "Specify strategy for test sharding: "
+              + "'explicit' to only use sharding if the 'shard_count' BUILD attribute is present. "
+              + "'disabled' to never use test sharding. "
+              + "'experimental_heuristic' to enable sharding on remotely executed tests without an "
+              + "explicit  'shard_count' attribute which link in a supported framework. Considered "
+              + "experimental."
+    )
+    public TestActionBuilder.TestShardingStrategy testShardingStrategy;
+
+    @Option(
+      name = "runs_per_test",
+      allowMultiple = true,
+      defaultValue = "1",
+      category = "testing",
+      converter = RunsPerTestConverter.class,
+      documentationCategory = OptionDocumentationCategory.UNCATEGORIZED,
+      effectTags = {OptionEffectTag.UNKNOWN},
+      help =
+          "Specifies number of times to run each test. If any of those attempts "
+              + "fail for any reason, the whole test would be considered failed. "
+              + "Normally the value specified is just an integer. Example: --runs_per_test=3 "
+              + "will run all tests 3 times. "
+              + "Alternate syntax: regex_filter@runs_per_test. Where runs_per_test stands for "
+              + "an integer value and regex_filter stands "
+              + "for a list of include and exclude regular expression patterns (Also see "
+              + "--instrumentation_filter). Example: "
+              + "--runs_per_test=//foo/.*,-//foo/bar/.*@3 runs all tests in //foo/ "
+              + "except those under foo/bar three times. "
+              + "This option can be passed multiple times. "
+    )
+    public List<PerLabelOptions> runsPerTest;
   }
 
   /** Configuration loader for test options */
@@ -69,7 +161,44 @@ public class TestConfiguration extends Fragment {
     this.options = options;
   }
 
+  @Override
+  public void reportInvalidOptions(EventHandler reporter, BuildOptions buildOptions) {
+    if (options.testShardingStrategy
+        == TestActionBuilder.TestShardingStrategy.EXPERIMENTAL_HEURISTIC) {
+      reporter.handle(
+          Event.warn(
+              "Heuristic sharding is intended as a one-off experimentation tool for determing the "
+                  + "benefit from sharding certain tests. Please don't keep this option in your "
+                  + ".blazerc or continuous build"));
+    }
+  }
+
   public String getTestFilter() {
     return options.testFilter;
   }
+
+  public TriState cacheTestResults() {
+    return options.cacheTestResults;
+  }
+
+  public List<String> getTestArguments() {
+    return options.testArguments;
+  }
+
+  public TestActionBuilder.TestShardingStrategy testShardingStrategy() {
+    return options.testShardingStrategy;
+  }
+
+  /**
+   * @return number of times the given test should run. If the test doesn't match any of the
+   *     filters, runs it once.
+   */
+  public int getRunsPerTestForLabel(Label label) {
+    for (PerLabelOptions perLabelRuns : options.runsPerTest) {
+      if (perLabelRuns.isIncluded(label)) {
+        return Integer.parseInt(Iterables.getOnlyElement(perLabelRuns.getOptions()));
+      }
+    }
+    return 1;
+  }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/rules/test/TestRunnerAction.java b/src/main/java/com/google/devtools/build/lib/rules/test/TestRunnerAction.java
index 30333f504e..38d61a1ae8 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/test/TestRunnerAction.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/test/TestRunnerAction.java
@@ -68,6 +68,7 @@ public class TestRunnerAction extends AbstractAction implements NotifyOnActionCa
 
   private final NestedSet<Artifact> runtime;
   private final BuildConfiguration configuration;
+  private final TestConfiguration testConfiguration;
   private final Artifact testLog;
   private final Artifact cacheStatus;
   private final PathFragment testWarningsPath;
@@ -147,6 +148,8 @@ public class TestRunnerAction extends AbstractAction implements NotifyOnActionCa
         list(testLog, cacheStatus, coverageArtifact));
     this.runtime = runtime;
     this.configuration = Preconditions.checkNotNull(configuration);
+    this.testConfiguration =
+        Preconditions.checkNotNull(configuration.getFragment(TestConfiguration.class));
     this.testLog = testLog;
     this.cacheStatus = cacheStatus;
     this.coverageData = coverageArtifact;
@@ -242,7 +245,7 @@ public class TestRunnerAction extends AbstractAction implements NotifyOnActionCa
     f.addInt(shardNum);
     f.addInt(executionSettings.getTotalShards());
     f.addInt(runNumber);
-    f.addInt(configuration.getRunsPerTestForLabel(getOwner().getLabel()));
+    f.addInt(testConfiguration.getRunsPerTestForLabel(getOwner().getLabel()));
     f.addInt(configuration.isCodeCoverageEnabled() ? 1 : 0);
     return f.hexDigestAndReset();
   }
@@ -285,10 +288,10 @@ public class TestRunnerAction extends AbstractAction implements NotifyOnActionCa
 
   private boolean computeExecuteUnconditionallyFromTestStatus() {
     return !canBeCached(
-        configuration.cacheTestResults(),
+        testConfiguration.cacheTestResults(),
         readCacheStatus(),
         testProperties.isExternal(),
-        configuration.getRunsPerTestForLabel(getOwner().getLabel()));
+        testConfiguration.getRunsPerTestForLabel(getOwner().getLabel()));
   }
 
   @VisibleForTesting
@@ -408,7 +411,7 @@ public class TestRunnerAction extends AbstractAction implements NotifyOnActionCa
 
     // When we run test multiple times, set different TEST_RANDOM_SEED values for each run.
     // Don't override any previous setting.
-    if (getConfiguration().getRunsPerTestForLabel(getOwner().getLabel()) > 1
+    if (testConfiguration.getRunsPerTestForLabel(getOwner().getLabel()) > 1
         && !env.containsKey("TEST_RANDOM_SEED")) {
       env.put("TEST_RANDOM_SEED", Integer.toString(getRunNumber() + 1));
     }
@@ -476,7 +479,7 @@ public class TestRunnerAction extends AbstractAction implements NotifyOnActionCa
   public String getTestSuffix() {
     int totalShards = executionSettings.getTotalShards();
     // Use a 1-based index for user friendliness.
-    int runsPerTest = configuration.getRunsPerTestForLabel(getOwner().getLabel());
+    int runsPerTest = testConfiguration.getRunsPerTestForLabel(getOwner().getLabel());
     if (totalShards > 1 && runsPerTest > 1) {
       return String.format("(shard %d of %d, run %d of %d)", shardNum + 1, totalShards,
           runNumber + 1, runsPerTest);
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 6a6109e53f..7678c842a7 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
@@ -55,7 +55,8 @@ public final class TestTargetExecutionSettings {
     TestConfiguration testConfig = config.getFragment(TestConfiguration.class);
 
     CommandLine targetArgs = runfilesSupport.getArgs();
-    testArguments = CommandLine.concat(targetArgs, ImmutableList.copyOf(config.getTestArguments()));
+    testArguments =
+        CommandLine.concat(targetArgs, ImmutableList.copyOf(testConfig.getTestArguments()));
 
     totalShards = shards;
     runUnder = config.getRunUnder();
diff --git a/src/main/java/com/google/devtools/build/lib/sandbox/SandboxHelpers.java b/src/main/java/com/google/devtools/build/lib/sandbox/SandboxHelpers.java
index e26f37bb43..8bc53bfeae 100644
--- a/src/main/java/com/google/devtools/build/lib/sandbox/SandboxHelpers.java
+++ b/src/main/java/com/google/devtools/build/lib/sandbox/SandboxHelpers.java
@@ -22,10 +22,10 @@ import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.actions.Artifact.ArtifactExpander;
 import com.google.devtools.build.lib.actions.ExecException;
 import com.google.devtools.build.lib.actions.Spawn;
-import com.google.devtools.build.lib.analysis.config.BuildConfiguration;
 import com.google.devtools.build.lib.exec.SpawnInputExpander;
 import com.google.devtools.build.lib.exec.SpawnRunner.SpawnExecutionPolicy;
 import com.google.devtools.build.lib.rules.fileset.FilesetActionContext;
+import com.google.devtools.build.lib.rules.test.TestConfiguration;
 import com.google.devtools.build.lib.standalone.StandaloneSpawnStrategy;
 import com.google.devtools.build.lib.util.Preconditions;
 import com.google.devtools.build.lib.vfs.Path;
@@ -136,7 +136,7 @@ public final class SandboxHelpers {
     // remote debug server of the test. This intentionally overrides the "block-network" execution
     // tag.
     return buildOptions
-        .getOptions(BuildConfiguration.Options.class)
+        .getOptions(TestConfiguration.TestOptions.class)
         .testArguments
         .contains("--wrapper_script_flag=--debug");
   }