Rollback of commit 1cb5059396e46d9ff2c317b8a0a046af32b3a1d8.

*** Reason for rollback ***

Breaks TEST_BINARY env setting if --run_under is used.

*** Original change description ***

Rewrite TestStrategy.getArgs

This changes command-line computation for tests:
- run the coverage collector before the run_under command
- no shell escaping: if all tools just call "$@", then this should work

Note that we still wrap the command in a sub-shell to support shell built-ins
and PATH lookup if the command does not contain a slash character '/'.

A side effect of this change is that the --run_under command now executes in
the test's runfiles directory, rather than in the exec root, if...

***

--
PiperOrigin-RevId: 148003525
MOS_MIGRATED_REVID=148003525

2 files changed, 31 insertions, 41 deletions

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 d7f94ec1d2..4c94254613 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
@@ -16,6 +16,7 @@ package com.google.devtools.build.lib.exec;
 
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.Iterables;
 import com.google.common.collect.Lists;
 import com.google.common.io.ByteStreams;
 import com.google.common.io.Closeables;
@@ -32,6 +33,7 @@ import com.google.devtools.build.lib.rules.test.TestResult;
 import com.google.devtools.build.lib.rules.test.TestRunnerAction;
 import com.google.devtools.build.lib.rules.test.TestTargetExecutionSettings;
 import com.google.devtools.build.lib.util.OS;
+import com.google.devtools.build.lib.util.ShellEscaper;
 import com.google.devtools.build.lib.util.io.FileWatcher;
 import com.google.devtools.build.lib.util.io.OutErr;
 import com.google.devtools.build.lib.vfs.FileStatus;
@@ -47,6 +49,7 @@ import java.io.Closeable;
 import java.io.IOException;
 import java.io.InputStream;
 import java.io.OutputStream;
+import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.HashMap;
 import java.util.List;
@@ -156,14 +159,10 @@ public abstract class TestStrategy implements TestActionContext {
    *     collect coverage data. If this is an empty string, it is ignored.
    * @param testAction The test action.
    * @return the command line as string list.
-   * @throws ExecException 
    */
   protected List<String> getArgs(String coverageScript, TestRunnerAction testAction)
       throws ExecException {
     List<String> args = Lists.newArrayList();
-    // TODO(ulfjack): This is incorrect for remote execution, where we need to consider the target
-    // configuration, not the machine Bazel happens to run on. Change this to something like:
-    // testAction.getConfiguration().getTargetOS() == OS.WINDOWS
     if (OS.getCurrent() == OS.WINDOWS) {
       args.add(testAction.getShExecutable().getPathString());
       args.add("-c");
@@ -171,44 +170,39 @@ public abstract class TestStrategy implements TestActionContext {
     }
 
     Artifact testSetup = testAction.getRuntimeArtifact(TEST_SETUP_BASENAME);
-    args.add(testSetup.getExecPath().getCallablePathString());
-
-    if (testAction.isCoverageMode()) {
-      args.add(coverageScript);
-    }
-
+    args.add(testSetup.getExecPathString());
     TestTargetExecutionSettings execSettings = testAction.getExecutionSettings();
 
-    // Insert the command prefix specified by the "--run_under=<command-prefix>" option, if any.
-    if (execSettings.getRunUnder() != null) {
-      addRunUnderArgs(testAction, args);
+    List<String> execArgs = new ArrayList<>();
+    if (!coverageScript.isEmpty() && testAction.isCoverageMode()) {
+      execArgs.add(coverageScript);
     }
 
-    // Execute the test using the alias in the runfiles tree, as mandated by the Test Encyclopedia.
-    args.add(execSettings.getExecutable().getRootRelativePath().getCallablePathString());
-    args.addAll(execSettings.getArgs());
-    return args;
-  }
-
-  private static void addRunUnderArgs(TestRunnerAction testAction, List<String> args) {
-    TestTargetExecutionSettings execSettings = testAction.getExecutionSettings();
-    if (execSettings.getRunUnderExecutable() != null) {
-      args.add(execSettings.getRunUnderExecutable().getRootRelativePath().getCallablePathString());
+    // Execute the test using the alias in the runfiles tree, as mandated by
+    // the Test Encyclopedia.
+    execArgs.add(execSettings.getExecutable().getRootRelativePath().getPathString());
+    execArgs.addAll(execSettings.getArgs());
+
+    // Insert the command prefix specified by the "--run_under=<command-prefix>" option,
+    // if any.
+    if (execSettings.getRunUnder() == null) {
+      args.addAll(execArgs);
+    } else if (execSettings.getRunUnderExecutable() != null) {
+      args.add(execSettings.getRunUnderExecutable().getRootRelativePath().getPathString());
+      args.addAll(execSettings.getRunUnder().getOptions());
+      args.addAll(execArgs);
     } else {
-      String command = execSettings.getRunUnder().getCommand();
-      // --run_under commands that do not contain '/' are either shell built-ins or need to be
-      // located on the PATH env, so we wrap them in a shell invocation. Note that we shell tokenize
-      // the --run_under parameter and getCommand only returns the first such token.
-      boolean needsShell = !command.contains("/");
-      if (needsShell) {
-        args.add(testAction.getConfiguration().getShellExecutable().getPathString());
-        args.add("-c");
-        args.add("\"$@\"");
-        args.add("/bin/sh"); // Sets $0.
-      }
-      args.add(command);
+      args.add(testAction.getConfiguration().getShellExecutable().getPathString());
+      args.add("-c");
+
+      String runUnderCommand = ShellEscaper.escapeString(execSettings.getRunUnder().getCommand());
+      args.add(
+          runUnderCommand
+              + ' '
+              + ShellEscaper.escapeJoinAll(
+                  Iterables.concat(execSettings.getRunUnder().getOptions(), execArgs)));
     }
-    args.addAll(testAction.getExecutionSettings().getRunUnder().getOptions());
+    return args;
   }
 
   /**
diff --git a/src/main/java/com/google/devtools/build/lib/worker/WorkerTestStrategy.java b/src/main/java/com/google/devtools/build/lib/worker/WorkerTestStrategy.java
index 36ab7bf8e5..d4a0fa8140 100644
--- a/src/main/java/com/google/devtools/build/lib/worker/WorkerTestStrategy.java
+++ b/src/main/java/com/google/devtools/build/lib/worker/WorkerTestStrategy.java
@@ -87,10 +87,6 @@ public class WorkerTestStrategy extends StandaloneTestStrategy {
       throw new UserExecException("Tests that do not use the default test runner are incompatible"
           + " with the persistent worker test strategy. Please use another test strategy");
     }
-    if (!action.isCoverageMode()) {
-      throw new UserExecException("Coverage is currently incompatible"
-          + " with the persistent worker test strategy. Please use another test strategy");
-    }
     List<String> startupArgs = getStartUpArgs(action);
 
     return execInWorker(
@@ -196,7 +192,7 @@ public class WorkerTestStrategy extends StandaloneTestStrategy {
   }
 
   private List<String> getStartUpArgs(TestRunnerAction action) throws ExecException {
-    List<String> args = getArgs(/*coverageScript=*/ "coverage-is-not-supported", action);
+    List<String> args = getArgs(/*coverageScript=*/ "", action);
     ImmutableList.Builder<String> startupArgs = ImmutableList.builder();
     // Add test setup with no echo to prevent stdout corruption.
     startupArgs.add(args.get(0)).add("--no_echo");