diff options
author | 2017-03-06 09:38:43 +0000 | |
---|---|---|
committer | 2017-03-06 09:48:17 +0000 | |
commit | 5c2f39bce83dbdaf5ae9bf938809ad6c08d4a1ec (patch) | |
tree | 4e758f91f420d1dfcd6fbe470766328be2bf7501 /src/main/java | |
parent | c3c25dc3754193b413a6dc11032f1fdb86e0500e (diff) |
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
Diffstat (limited to 'src/main/java')
3 files changed, 44 insertions, 31 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 904bfb2cd3..1a31d5c232 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 @@ 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; @@ -33,7 +32,6 @@ 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; @@ -49,7 +47,6 @@ 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; @@ -159,10 +156,14 @@ 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"); @@ -170,41 +171,46 @@ public abstract class TestStrategy implements TestActionContext { } Artifact testSetup = testAction.getRuntimeArtifact(TEST_SETUP_BASENAME); - args.add(testSetup.getExecPathString()); - TestTargetExecutionSettings execSettings = testAction.getExecutionSettings(); + args.add(testSetup.getExecPath().getCallablePathString()); - List<String> execArgs = new ArrayList<>(); - if (!coverageScript.isEmpty() && testAction.isCoverageMode()) { - execArgs.add(coverageScript); + if (testAction.isCoverageMode()) { + args.add(coverageScript); } - // 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"); + TestTargetExecutionSettings execSettings = testAction.getExecutionSettings(); - String runUnderCommand = ShellEscaper.escapeString(execSettings.getRunUnder().getCommand()); - args.add( - runUnderCommand - + ' ' - + ShellEscaper.escapeJoinAll( - Iterables.concat(execSettings.getRunUnder().getOptions(), execArgs))); + // 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. + 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 06cb42116c..ac9ba6afdf 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 @@ public class TestRunnerAction extends AbstractAction implements NotifyOnActionCa 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 d4a0fa8140..36ab7bf8e5 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 @@ 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( @@ -192,7 +196,7 @@ public class WorkerTestStrategy extends StandaloneTestStrategy { } 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"); |