Preserve analysis cache through --test_env changes

src/main/java/com/google/devtools/build/lib/BUILD

@@ -358,6 +358,7 @@ java_library(
         "//src/main/java/com/google/devtools/build/lib/analysis:template_expansion_exception",
         "//src/main/java/com/google/devtools/build/lib/analysis:test/coverage_artifacts_known_event",
         "//src/main/java/com/google/devtools/build/lib/analysis:test/coverage_report_action_factory",
+        "//src/main/java/com/google/devtools/build/lib/analysis:test/test_configuration",
         "//src/main/java/com/google/devtools/build/lib/analysis:top_level_artifact_context",
         "//src/main/java/com/google/devtools/build/lib/analysis:transitive_info_collection",
         "//src/main/java/com/google/devtools/build/lib/analysis:view_creation_failed_exception",

src/main/java/com/google/devtools/build/lib/analysis/BUILD

@@ -1701,6 +1701,7 @@ java_library(
         ":config/run_under",
         ":config/starlark_defined_config_transition",
         ":platform_options",
+        ":test/test_configuration",
         "//src/main/java/com/google/devtools/build/lib/actions:action_environment",
         "//src/main/java/com/google/devtools/build/lib/actions:artifacts",
         "//src/main/java/com/google/devtools/build/lib/actions:build_configuration_event",

src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfigurationValue.java

@@ -27,6 +27,7 @@
 import com.google.devtools.build.lib.actions.CommandLineLimits;
 import com.google.devtools.build.lib.analysis.BlazeDirectories;
 import com.google.devtools.build.lib.analysis.PlatformOptions;
+import com.google.devtools.build.lib.analysis.test.TestConfiguration.TestOptions;
 import com.google.devtools.build.lib.buildeventstream.BuildEvent;
 import com.google.devtools.build.lib.buildeventstream.BuildEventIdUtil;
 import com.google.devtools.build.lib.buildeventstream.BuildEventStreamProtos;
@@ -165,9 +166,13 @@ public void reportInvalidOptions(EventHandler reporter) {
    * be inherited from the client environment.
    */
   private ActionEnvironment setupTestEnvironment() {
-    // We make a copy first to remove duplicate entries; last one wins.
+    if (!buildOptions.contains(TestOptions.class)) {
+      // TestOptions have been trimmed.
+      return ActionEnvironment.EMPTY;
+    }
+    // Order doesn't matter here as ActionEnvironment sorts by key.
     Map<String, String> testEnv = new HashMap<>();
-    for (Map.Entry<String, String> entry : options.testEnvironment) {
+    for (Map.Entry<String, String> entry : buildOptions.get(TestOptions.class).testEnvironment) {
       testEnv.put(entry.getKey(), entry.getValue());
     }
     return ActionEnvironment.split(testEnv);

src/main/java/com/google/devtools/build/lib/analysis/config/CoreOptions.java

@@ -34,11 +34,9 @@
 import com.google.devtools.common.options.OptionMetadataTag;
 import com.google.devtools.common.options.OptionsParsingException;
 import com.google.devtools.common.options.TriState;
-import java.util.AbstractMap.SimpleEntry;
 import java.util.ArrayList;
 import java.util.Comparator;
 import java.util.HashMap;
-import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Map;
 import java.util.TreeSet;
@@ -420,24 +418,6 @@ public ExecConfigurationDistinguisherSchemeConverter() {
       help = "Specifies a suffix to be added to the configuration directory.")
   public String platformSuffix;
 
-  // TODO(bazel-team): The test environment is actually computed in BlazeRuntime and this option
-  // is not read anywhere else. Thus, it should be in a different options class, preferably one
-  // specific to the "test" command or maybe in its own configuration fragment.
-  @Option(
-      name = "test_env",
-      converter = Converters.OptionalAssignmentConverter.class,
-      allowMultiple = true,
-      defaultValue = "null",
-      documentationCategory = OptionDocumentationCategory.TESTING,
-      effectTags = {OptionEffectTag.TEST_RUNNER},
-      help =
-          "Specifies additional environment variables to be injected into the test runner "
-              + "environment. Variables can be either specified by name, in which case its value "
-              + "will be read from the Bazel client environment, or by the name=value pair. "
-              + "This option can be used multiple times to specify several variables. "
-              + "Used only by the 'bazel test' command.")
-  public List<Map.Entry<String, String>> testEnvironment;
-
   // TODO(bazel-team): The set of available variables from the client environment for actions
   // is computed independently in CommandEnvironment to inject a more restricted client
   // environment to skyframe.
@@ -985,20 +965,6 @@ public ImmutableMap<String, String> getNormalizedCommandLineBuildVariables() {
         .collect(toImmutableMap(Map.Entry::getKey, Map.Entry::getValue));
   }
 
-  // Normalizes list of map entries by keeping only the last entry for each key.
-  private static List<Map.Entry<String, String>> normalizeEntries(
-      List<Map.Entry<String, String>> entries) {
-    LinkedHashMap<String, String> normalizedEntries = new LinkedHashMap<>();
-    for (Map.Entry<String, String> entry : entries) {
-      normalizedEntries.put(entry.getKey(), entry.getValue());
-    }
-    // If we made no changes, return the same instance we got to reduce churn.
-    if (normalizedEntries.size() == entries.size()) {
-      return entries;
-    }
-    return normalizedEntries.entrySet().stream().map(SimpleEntry::new).collect(toImmutableList());
-  }
-
   // Sort the map entries by key.
   private static List<Map.Entry<String, String>> sortEntries(
       List<Map.Entry<String, String>> entries) {
@@ -1056,7 +1022,6 @@ public CoreOptions getNormalized() {
 
     result.actionEnvironment = normalizeEntries(actionEnvironment);
     result.hostActionEnvironment = normalizeEntries(hostActionEnvironment);
-    result.testEnvironment = normalizeEntries(testEnvironment);
     result.commandLineFlagAliases = sortEntries(normalizeEntries(commandLineFlagAliases));
 
     return result;

src/main/java/com/google/devtools/build/lib/analysis/config/FragmentOptions.java

@@ -20,7 +20,10 @@
 import com.google.devtools.common.options.Option;
 import com.google.devtools.common.options.Options;
 import com.google.devtools.common.options.OptionsBase;
+import java.util.AbstractMap;
+import java.util.LinkedHashMap;
 import java.util.List;
+import java.util.Map;
 import javax.annotation.Nullable;
 
 /** Command-line build options for a Blaze module. */
@@ -89,6 +92,25 @@ protected static ImmutableList<String> dedupAndSort(@Nullable List<String> value
     return result.equals(values) ? ImmutableList.copyOf(values) : result;
   }
 
+  /**
+   * Helper method for subclasses to normalize list of map entries by keeping only the last entry
+   * for each key. The order of the entries is preserved.
+   */
+  protected static List<Map.Entry<String, String>> normalizeEntries(
+      List<Map.Entry<String, String>> entries) {
+    LinkedHashMap<String, String> normalizedEntries = new LinkedHashMap<>();
+    for (Map.Entry<String, String> entry : entries) {
+      normalizedEntries.put(entry.getKey(), entry.getValue());
+    }
+    // If we made no changes, return the same instance we got to reduce churn.
+    if (normalizedEntries.size() == entries.size()) {
+      return entries;
+    }
+    return normalizedEntries.entrySet().stream()
+        .map(AbstractMap.SimpleEntry::new)
+        .collect(toImmutableList());
+  }
+
   /** Tracks limitations on referring to an option in a {@code config_setting}. */
   // TODO(bazel-team): There will likely also be a need to customize whether or not an option is
   // visible to users for setting on the command line (or perhaps even in a test of a Starlark

src/main/java/com/google/devtools/build/lib/analysis/test/TestConfiguration.java

@@ -32,6 +32,7 @@
 import com.google.devtools.build.lib.packages.TestTimeout;
 import com.google.devtools.build.lib.util.Pair;
 import com.google.devtools.build.lib.util.RegexFilter;
+import com.google.devtools.common.options.Converters;
 import com.google.devtools.common.options.Option;
 import com.google.devtools.common.options.OptionDefinition;
 import com.google.devtools.common.options.OptionDocumentationCategory;
@@ -73,6 +74,21 @@ public static class TestOptions extends FragmentOptions {
             OptionsParser.getOptionDefinitionByName(
                 TestOptions.class, "experimental_retain_test_configuration_across_testonly"));
 
+    @Option(
+        name = "test_env",
+        converter = Converters.OptionalAssignmentConverter.class,
+        allowMultiple = true,
+        defaultValue = "null",
+        documentationCategory = OptionDocumentationCategory.TESTING,
+        effectTags = {OptionEffectTag.TEST_RUNNER},
+        help =
+            "Specifies additional environment variables to be injected into the test runner "
+                + "environment. Variables can be either specified by name, in which case its value "
+                + "will be read from the Bazel client environment, or by the name=value pair. "
+                + "This option can be used multiple times to specify several variables. "
+                + "Used only by the 'bazel test' command.")
+    public List<Map.Entry<String, String>> testEnvironment;
+
     @Option(
         name = "test_timeout",
         defaultValue = "-1",
@@ -335,6 +351,13 @@ public static class TestOptions extends FragmentOptions {
         effectTags = {OptionEffectTag.EXECUTION},
         help = "If true, Bazel will allow local tests to run.")
     public boolean allowLocalTests;
+
+    @Override
+    public TestOptions getNormalized() {
+      TestOptions result = (TestOptions) clone();
+      result.testEnvironment = normalizeEntries(testEnvironment);
+      return result;
+    }
   }
 
   private final TestOptions options;

src/main/java/com/google/devtools/build/lib/runtime/CommandEnvironment.java

@@ -32,6 +32,7 @@
 import com.google.devtools.build.lib.analysis.BuildInfoEvent;
 import com.google.devtools.build.lib.analysis.config.AdditionalConfigurationChangeEvent;
 import com.google.devtools.build.lib.analysis.config.CoreOptions;
+import com.google.devtools.build.lib.analysis.test.TestConfiguration.TestOptions;
 import com.google.devtools.build.lib.bazel.repository.downloader.DelegatingDownloader;
 import com.google.devtools.build.lib.bazel.repository.downloader.HttpDownloader;
 import com.google.devtools.build.lib.buildtool.BuildRequestOptions;
@@ -329,7 +330,7 @@ public void exit(AbruptExitException exception) {
         }
       }
       for (Map.Entry<String, String> entry :
-          options.getOptions(CoreOptions.class).testEnvironment) {
+          options.getOptions(TestOptions.class).testEnvironment) {
         if (entry.getValue() == null) {
           visibleTestEnv.add(entry.getKey());
         }

src/test/java/com/google/devtools/build/lib/analysis/config/BuildConfigurationStarlarkTest.java

@@ -42,22 +42,28 @@ public void testStarlarkWithTestEnvOptions() throws Exception {
         MyInfo = provider()
 
         def _test_rule_impl(ctx):
-            return MyInfo(test_env = ctx.configuration.test_env)
-
-        test_rule = rule(
+            out = ctx.actions.declare_file(ctx.label.name)
+            ctx.actions.write(out, "exit 0", is_executable = True)
+            return [
+                DefaultInfo(executable = out),
+                MyInfo(test_env = ctx.configuration.test_env),
+            ]
+
+        my_test = rule(
             implementation = _test_rule_impl,
             attrs = {},
+            test = True,
         )
         """);
 
     scratch.file(
         "examples/config_starlark/BUILD",
         """
-        load("//examples/rule:config_test.bzl", "test_rule")
+        load("//examples/rule:config_test.bzl", "my_test")
 
         package(default_visibility = ["//visibility:public"])
 
-        test_rule(
+        my_test(
             name = "my_target",
         )
         """);

src/test/java/com/google/devtools/build/lib/remote/BUILD

@@ -87,6 +87,7 @@ java_library(
         "//src/main/java/com/google/devtools/build/lib/analysis:constraints/constraint_constants",
         "//src/main/java/com/google/devtools/build/lib/analysis:server_directories",
         "//src/main/java/com/google/devtools/build/lib/analysis:symlink_entry",
+        "//src/main/java/com/google/devtools/build/lib/analysis:test/test_configuration",
         "//src/main/java/com/google/devtools/build/lib/analysis/platform",
         "//src/main/java/com/google/devtools/build/lib/authandtls",
         "//src/main/java/com/google/devtools/build/lib/authandtls/credentialhelper",

src/test/java/com/google/devtools/build/lib/remote/RemoteModuleTest.java

@@ -36,6 +36,7 @@
 import com.google.devtools.build.lib.analysis.BlazeDirectories;
 import com.google.devtools.build.lib.analysis.ServerDirectories;
 import com.google.devtools.build.lib.analysis.config.CoreOptions;
+import com.google.devtools.build.lib.analysis.test.TestConfiguration.TestOptions;
 import com.google.devtools.build.lib.authandtls.AuthAndTLSOptions;
 import com.google.devtools.build.lib.authandtls.credentialhelper.CredentialHelperEnvironment;
 import com.google.devtools.build.lib.authandtls.credentialhelper.CredentialModule;
@@ -139,6 +140,7 @@ private static CommandEnvironment createTestCommandEnvironment(
     PackageOptions packageOptions = Options.getDefaults(PackageOptions.class);
     ClientOptions clientOptions = Options.getDefaults(ClientOptions.class);
     ExecutionOptions executionOptions = Options.getDefaults(ExecutionOptions.class);
+    TestOptions testOptions = Options.getDefaults(TestOptions.class);
 
     AuthAndTLSOptions authAndTLSOptions = Options.getDefaults(AuthAndTLSOptions.class);
 
@@ -150,6 +152,7 @@ private static CommandEnvironment createTestCommandEnvironment(
     when(options.getOptions(RemoteOptions.class)).thenReturn(remoteOptions);
     when(options.getOptions(AuthAndTLSOptions.class)).thenReturn(authAndTLSOptions);
     when(options.getOptions(ExecutionOptions.class)).thenReturn(executionOptions);
+    when(options.getOptions(TestOptions.class)).thenReturn(testOptions);
 
     String productName = "bazel";
     Scratch scratch = new Scratch(new InMemoryFileSystem(DigestHashFunction.SHA256));

src/test/shell/bazel/bazel_test_test.sh

@@ -1116,4 +1116,31 @@ EOF
   expect_log "-- Test exited prematurely (TEST_PREMATURE_EXIT_FILE exists) --"
 }
 
+function test_test_env_change_preserves_cache() {
+  local -r pkg="pkg${LINENO}"
+  mkdir -p "${pkg}"
+  cat > "$pkg/BUILD" <<'EOF'
+load(":defs.bzl", "my_rule")
+my_rule(
+  name = "my_rule",
+)
+EOF
+  cat > "$pkg/defs.bzl" <<'EOF'
+def _my_rule_impl(ctx):
+  print("my_rule is being analyzed")
+  out = ctx.actions.declare_file(ctx.label.name)
+  ctx.actions.write(out, "hi")
+  return [DefaultInfo(files = depset([out]))]
+my_rule = rule(_my_rule_impl)
+EOF
+
+  bazel build "${pkg}:my_rule" >$TEST_log 2>&1 \
+   || fail "expected build to pass"
+  expect_log "my_rule is being analyzed"
+
+  bazel build --test_env=FOO=bar "${pkg}:my_rule" >$TEST_log 2>&1 \
+   || fail "expected build to pass"
+  expect_not_log "my_rule is being analyzed"
+}
+
 run_suite "bazel test tests"