Test strategy unification - extract env setup to a new class

Introduce a new class TestPolicy that computes the test environment and use it
from all the test strategies instead of the previous duplicated code.

Note how the TestPolicy constructs the environment in four steps, each of which
can override variables set in a previous step:
1. set env variables for which we have a hard-coded policy
2. set env variables from --action_env
3. set env variables from --test_env
4. set env variables that are specific to the test (path locations, coverage
  collection protocol, test sharding protocol, etc.); the code for this lives
  in TestRunnerAction

This changes the order in which env variables are set, which can change the
resulting environment if --test_env or --action_env are set. We should consider
not allowing the command-line flags to override TEST_SRCDIR, RUNFILES_DIR, or
TEST_TMPDIR.

We should also try to find a way to drop the RUNTEST_PRESERVE_CWD variable in
coverage mode. I'm fairly sure that it's current use breaks the combination
of coverage and --run_under. My current thinking is that we change the test
setup script to source the coverage collector before changing the CWD. Another
option is to merge the coverage collector into the test setup script.

--
PiperOrigin-RevId: 146237448
MOS_MIGRATED_REVID=146237448

4 files changed, 134 insertions, 46 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 553c88c9db..6f58666fe3 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
@@ -1278,12 +1278,12 @@ public final class BuildConfiguration {
     // Shell environment variables specified via options take precedence over the
     // ones inherited from the fragments. In the long run, these fragments will
     // be replaced by appropriate default rc files anyway.
-    Map<String, String> shellEnv = new TreeMap(builder.build());
+    Map<String, String> shellEnv = new TreeMap<>(builder.build());
     for (Map.Entry<String, String> entry : options.actionEnvironment) {
       shellEnv.put(entry.getKey(), entry.getValue());
     }
-    Map<String, String> fixedShellEnv = new TreeMap(shellEnv);
-    Set<String> variableShellEnv = new HashSet();
+    Map<String, String> fixedShellEnv = new TreeMap<>(shellEnv);
+    Set<String> variableShellEnv = new HashSet<>();
     for (Map.Entry<String, String> entry : shellEnv.entrySet()) {
       if (entry.getValue() == null) {
         String key = entry.getKey();
diff --git a/src/main/java/com/google/devtools/build/lib/exec/StandaloneTestStrategy.java b/src/main/java/com/google/devtools/build/lib/exec/StandaloneTestStrategy.java
index 41004a7c6d..2f754a8e07 100644
--- a/src/main/java/com/google/devtools/build/lib/exec/StandaloneTestStrategy.java
+++ b/src/main/java/com/google/devtools/build/lib/exec/StandaloneTestStrategy.java
@@ -14,6 +14,7 @@
 
 package com.google.devtools.build.lib.exec;
 
+import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
 import com.google.devtools.build.lib.actions.ActionExecutionContext;
 import com.google.devtools.build.lib.actions.Artifact;
@@ -30,7 +31,6 @@ import com.google.devtools.build.lib.actions.SpawnActionContext;
 import com.google.devtools.build.lib.actions.TestExecException;
 import com.google.devtools.build.lib.analysis.RunfilesSupplierImpl;
 import com.google.devtools.build.lib.analysis.config.BinTools;
-import com.google.devtools.build.lib.analysis.config.BuildConfiguration;
 import com.google.devtools.build.lib.events.Event;
 import com.google.devtools.build.lib.events.EventKind;
 import com.google.devtools.build.lib.events.Reporter;
@@ -41,6 +41,7 @@ import com.google.devtools.build.lib.rules.test.TestRunnerAction.ResolvedPaths;
 import com.google.devtools.build.lib.util.io.FileOutErr;
 import com.google.devtools.build.lib.vfs.FileSystemUtils;
 import com.google.devtools.build.lib.vfs.Path;
+import com.google.devtools.build.lib.vfs.PathFragment;
 import com.google.devtools.build.lib.view.test.TestStatus.BlazeTestStatus;
 import com.google.devtools.build.lib.view.test.TestStatus.TestCase;
 import com.google.devtools.build.lib.view.test.TestStatus.TestResultData;
@@ -61,6 +62,20 @@ public class StandaloneTestStrategy extends TestStrategy {
   public static final String COLLECT_COVERAGE =
       "external/bazel_tools/tools/test/collect_coverage.sh";
 
+  private static final ImmutableMap<String, String> ENV_VARS =
+      ImmutableMap.<String, String>builder()
+          .put("TZ", "UTC")
+          .put("USER", TestPolicy.SYSTEM_USER_NAME)
+          .put("TEST_SRCDIR", TestPolicy.RUNFILES_DIR)
+          // TODO(lberki): Remove JAVA_RUNFILES and PYTHON_RUNFILES.
+          .put("JAVA_RUNFILES", TestPolicy.RUNFILES_DIR)
+          .put("PYTHON_RUNFILES", TestPolicy.RUNFILES_DIR)
+          .put("RUNFILES_DIR", TestPolicy.RUNFILES_DIR)
+          .put("TEST_TMPDIR", TestPolicy.TEST_TMP_DIR)
+          .build();
+
+  public static final TestPolicy DEFAULT_LOCAL_POLICY = new TestPolicy(ENV_VARS);
+
   protected final Path tmpDirRoot;
 
   public StandaloneTestStrategy(
@@ -231,27 +246,18 @@ public class StandaloneTestStrategy extends TestStrategy {
 
   private Map<String, String> setupEnvironment(
       TestRunnerAction action, Path execRoot, Path runfilesDir, Path tmpDir) {
-    Map<String, String> env = getDefaultTestEnvironment(action);
-    BuildConfiguration config = action.getConfiguration();
-
-    env.putAll(config.getLocalShellEnvironment());
-    env.putAll(action.getTestEnv());
-
-    String tmpDirString;
+    PathFragment relativeTmpDir;
     if (tmpDir.startsWith(execRoot)) {
-      tmpDirString = tmpDir.relativeTo(execRoot).getPathString();
+      relativeTmpDir = tmpDir.relativeTo(execRoot);
     } else {
-      tmpDirString = tmpDir.getPathString();
+      relativeTmpDir = tmpDir.asFragment();
     }
-
-    String testSrcDir = runfilesDir.relativeTo(execRoot).getPathString();
-    env.put("JAVA_RUNFILES", testSrcDir);
-    env.put("PYTHON_RUNFILES", testSrcDir);
-    env.put("TEST_SRCDIR", testSrcDir);
-    env.put("TEST_TMPDIR", tmpDirString);
-
-    action.setupEnvVariables(env, getTimeout(action));
-    return env;
+    return DEFAULT_LOCAL_POLICY.computeTestEnvironment(
+        action,
+        clientEnv,
+        getTimeout(action),
+        runfilesDir.relativeTo(execRoot),
+        relativeTmpDir);
   }
 
   protected TestResultData executeTest(
diff --git a/src/main/java/com/google/devtools/build/lib/exec/TestPolicy.java b/src/main/java/com/google/devtools/build/lib/exec/TestPolicy.java
new file mode 100644
index 0000000000..a58c666129
--- /dev/null
+++ b/src/main/java/com/google/devtools/build/lib/exec/TestPolicy.java
@@ -0,0 +1,106 @@
+// 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.exec;
+
+import com.google.common.collect.ImmutableMap;
+import com.google.devtools.build.lib.rules.test.TestRunnerAction;
+import com.google.devtools.build.lib.util.UserUtils;
+import com.google.devtools.build.lib.vfs.PathFragment;
+import java.util.HashMap;
+import java.util.Map;
+
+/**
+ * A policy for running tests. It currently only encompasses the environment computation for the
+ * test.
+ */
+public class TestPolicy {
+  /**
+   * The user name of the user running Bazel; this may differ from ${USER} for tests that are run
+   * remotely.
+   */
+  public static final String SYSTEM_USER_NAME = "${SYSTEM_USER_NAME}";
+
+  /** An absolute path to a writable directory that is reserved for the current test. */
+  public static final String TEST_TMP_DIR = "${TEST_TMP_DIR}";
+
+  /** The path of the runfiles directory. */
+  public static final String RUNFILES_DIR = "${RUNFILES_DIR}";
+
+  public static final String INHERITED = "${inherited}";
+
+  private final ImmutableMap<String, String> envVariables;
+
+  /**
+   * Creates a new instance. The map's keys are the names of the environment variables, while the
+   * values can be either fixed values, or one of the constants in this class, specifically {@link
+   * #SYSTEM_USER_NAME}, {@link #TEST_TMP_DIR}, {@link #RUNFILES_DIR}, or {@link #INHERITED}.
+   */
+  public TestPolicy(ImmutableMap<String, String> envVariables) {
+    this.envVariables = envVariables;
+  }
+
+  /**
+   * Returns a mutable map of the environment variables for a specific test. This is intended to be
+   * the final, complete environment - callers should avoid relying on the mutability of the return
+   * value, and instead change the policy itself.
+   */
+  public Map<String, String> computeTestEnvironment(
+      TestRunnerAction testAction,
+      ImmutableMap<String, String> clientEnv,
+      int timeoutInSeconds,
+      PathFragment relativeRunfilesDir,
+      PathFragment tmpDir) {
+    Map<String, String> env = new HashMap<>();
+
+    // Add all env variables, allow some string replacements and inheritance.
+    String userProp = UserUtils.getUserName();
+    String tmpDirPath = tmpDir.getPathString();
+    String runfilesDirPath = relativeRunfilesDir.getPathString();
+    for (Map.Entry<String, String> entry : envVariables.entrySet()) {
+      String val = entry.getValue();
+      if (val.contains("${")) {
+        if (val.equals(INHERITED)) {
+          if (!clientEnv.containsKey(entry.getKey())) {
+            continue;
+          }
+          val = clientEnv.get(entry.getKey());
+        } else {
+          val = val.replace(SYSTEM_USER_NAME, userProp);
+          val = val.replace(TEST_TMP_DIR, tmpDirPath);
+          val = val.replace(RUNFILES_DIR, runfilesDirPath);
+        }
+      }
+      env.put(entry.getKey(), val);
+    }
+
+    // Overwrite with the environment common to all actions, see --action_env.
+    // TODO(ulfjack): This also includes env variables from the configuration fragments, and it does
+    // not include the env variables which are supposed to be inherited, i.e., for with --action_env
+    // does not specify an explicit value.
+    env.putAll(testAction.getConfiguration().getLocalShellEnvironment());
+
+    // Overwrite with the environment common to all tests, see --test_env.
+    // TODO(ulfjack): This is handled differently from --action_env such that changing the
+    // --test_env flag (or any of the inherited env variables) requires a full re-analysis of
+    // everything, instead of triggering just the subset of actions that rely on inherited
+    // variables. Needless to say, that is not optimal, and we should fix it to use the same
+    // approach as --action_env.
+    env.putAll(testAction.getTestEnv());
+
+    // Setup any test-specific env variables; note that this does not overwrite existing values for
+    // TEST_RANDOM_SEED or TEST_SIZE if they're already set.
+    testAction.setupEnvVariables(env, timeoutInSeconds);
+    return env;
+  }
+}
diff --git a/src/main/java/com/google/devtools/build/lib/exec/TestStrategy.java b/src/main/java/com/google/devtools/build/lib/exec/TestStrategy.java
index a0f371c631..f84754f57f 100644
--- a/src/main/java/com/google/devtools/build/lib/exec/TestStrategy.java
+++ b/src/main/java/com/google/devtools/build/lib/exec/TestStrategy.java
@@ -152,20 +152,6 @@ public abstract class TestStrategy implements TestActionContext {
       throws ExecException, InterruptedException;
 
   /**
-   * Returns mutable map of default testing shell environment. By itself it is incomplete and is
-   * modified further by the specific test strategy implementations (mostly due to the fact that
-   * environments used locally and remotely are different).
-   */
-  protected Map<String, String> getDefaultTestEnvironment(TestRunnerAction action) {
-    Map<String, String> env = new HashMap<>();
-
-    env.putAll(action.getConfiguration().getLocalShellEnvironment());
-    env.remove("LANG");
-    env.put("TZ", "UTC");
-    return env;
-  }
-
-  /**
    * Generates a command line to run for the test action, taking into account coverage and {@code
    * --run_under} settings.
    *
@@ -253,16 +239,6 @@ public abstract class TestStrategy implements TestActionContext {
     return executionOptions.testTimeout.get(testAction.getTestProperties().getTimeout());
   }
 
-  /**
-   * Returns a subset of the environment from the current shell.
-   *
-   * <p>Warning: Since these variables are not part of the configuration's fingerprint, they MUST
-   * NOT be used by any rule or action in such a way as to affect the semantics of that build step.
-   */
-  public Map<String, String> getAdmissibleShellEnvironment(Iterable<String> variables) {
-    return getMapping(variables, clientEnv);
-  }
-
   /*
    * Finalize test run: persist the result, and post on the event bus.
    */