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 coverage is
enabled at the same time. Inside Google, it's very rare for --run_under to be
used in combination with coverage, and it seems likely to be rare externally
as well, so I don't think it warrants covering it in the release notes.
Also set TEST_BINARY to the root-relative path of the test executable for
all tests (in TestRunnerAction.java).
--
PiperOrigin-RevId: 149275688
MOS_MIGRATED_REVID=149275688
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 904bfb2..1a31d5c 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,7 +16,6 @@
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;
@@ -33,7 +32,6 @@
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;
@@ -49,7 +47,6 @@
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;
@@ -159,10 +156,14 @@
* 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");
@@ -170,41 +171,46 @@
}
Artifact testSetup = testAction.getRuntimeArtifact(TEST_SETUP_BASENAME);
- args.add(testSetup.getExecPathString());
+ args.add(testSetup.getExecPath().getCallablePathString());
+
+ if (testAction.isCoverageMode()) {
+ args.add(coverageScript);
+ }
+
TestTargetExecutionSettings execSettings = testAction.getExecutionSettings();
- List<String> execArgs = new ArrayList<>();
- if (!coverageScript.isEmpty() && testAction.isCoverageMode()) {
- execArgs.add(coverageScript);
+ // Insert the command prefix specified by the "--run_under=<command-prefix>" option, if any.
+ if (execSettings.getRunUnder() != null) {
+ addRunUnderArgs(testAction, args);
}
- // 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 {
- 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)));
- }
+ // 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());
+ } 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.addAll(testAction.getExecutionSettings().getRunUnder().getOptions());
+ }
+
/**
* Returns the number of attempts specific test action can be retried.
*
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 06cb421..ac9ba6a 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
@@ -362,6 +362,9 @@
env.put("TEST_SIZE", getTestProperties().getSize().toString());
env.put("TEST_TIMEOUT", Integer.toString(timeoutInSeconds));
env.put("TEST_WORKSPACE", getRunfilesPrefix());
+ env.put(
+ "TEST_BINARY",
+ getExecutionSettings().getExecutable().getRootRelativePath().getCallablePathString());
// When we run test multiple times, set different TEST_RANDOM_SEED values for each run.
// Don't override any previous setting.
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 d4a0fa8..36ab7bf 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,6 +87,10 @@
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(
@@ -192,7 +196,7 @@
}
private List<String> getStartUpArgs(TestRunnerAction action) throws ExecException {
- List<String> args = getArgs(/*coverageScript=*/ "", action);
+ List<String> args = getArgs(/*coverageScript=*/ "coverage-is-not-supported", 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");
