More detailed error message for sandbox failures in test, when --sandbox_debug and --verbose_failures are on. See discussion in #1049.

RELNOTES:

--
MOS_MIGRATED_REVID=119635080

7 files changed, 54 insertions, 4 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/actions/SpawnActionContext.java b/src/main/java/com/google/devtools/build/lib/actions/SpawnActionContext.java
index 5e20f85684..e0deb85505 100644
--- a/src/main/java/com/google/devtools/build/lib/actions/SpawnActionContext.java
+++ b/src/main/java/com/google/devtools/build/lib/actions/SpawnActionContext.java
@@ -33,4 +33,11 @@ public interface SpawnActionContext extends Executor.ActionContext {
    * execute spawns remotely, i.e., force remote execution.
    */
   boolean willExecuteRemotely(boolean remotable);
+
+  /**
+   * If an ExecException should be rethrown by the strategy that executed this.
+   * Currently only works for LinuxSandboxedStrategy:
+   * If true, will throw ExecException and give reproduction instruction for sandbox.
+   */
+  boolean shouldPropagateExecException();
 }
diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnStrategy.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnStrategy.java
index b5f4326ba2..f688fd2191 100644
--- a/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnStrategy.java
+++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnStrategy.java
@@ -259,4 +259,9 @@ final class RemoteSpawnStrategy implements SpawnActionContext {
     // Returning true here just helps to estimate the cost of this computation is zero.
     return remotable;
   }
+
+  @Override
+  public boolean shouldPropagateExecException() {
+    return false;
+  }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/rules/test/StandaloneTestStrategy.java b/src/main/java/com/google/devtools/build/lib/rules/test/StandaloneTestStrategy.java
index a474521544..a12084a813 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/test/StandaloneTestStrategy.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/test/StandaloneTestStrategy.java
@@ -26,6 +26,7 @@ import com.google.devtools.build.lib.actions.ResourceManager;
 import com.google.devtools.build.lib.actions.ResourceManager.ResourceHandle;
 import com.google.devtools.build.lib.actions.ResourceSet;
 import com.google.devtools.build.lib.actions.Spawn;
+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;
@@ -166,13 +167,14 @@ public class StandaloneTestStrategy extends TestStrategy {
 
   private TestResultData execute(
       ActionExecutionContext actionExecutionContext, Spawn spawn, TestRunnerAction action)
-      throws TestExecException, InterruptedException {
+      throws ExecException, InterruptedException {
     Executor executor = actionExecutionContext.getExecutor();
     Closeable streamed = null;
     Path testLogPath = action.getTestLog().getPath();
     TestResultData.Builder builder = TestResultData.newBuilder();
 
     long startTime = executor.getClock().currentTimeMillis();
+    SpawnActionContext spawnActionContext = executor.getSpawnActionContext(action.getMnemonic());
     try {
       try {
         if (executionOptions.testOutput.equals(TestOutputFormat.STREAMED)) {
@@ -180,7 +182,7 @@ public class StandaloneTestStrategy extends TestStrategy {
               Reporter.outErrForReporter(
                   actionExecutionContext.getExecutor().getEventHandler()), testLogPath);
         }
-        executor.getSpawnActionContext(action.getMnemonic()).exec(spawn, actionExecutionContext);
+        spawnActionContext.exec(spawn, actionExecutionContext);
 
         builder.setTestPassed(true)
             .setStatus(BlazeTestStatus.PASSED)
@@ -195,6 +197,9 @@ public class StandaloneTestStrategy extends TestStrategy {
             .setTestPassed(false)
             .setStatus(e.hasTimedOut() ? BlazeTestStatus.TIMEOUT : BlazeTestStatus.FAILED)
             .addFailedLogs(testLogPath.getPathString());
+        if (spawnActionContext.shouldPropagateExecException()) {
+          throw e;
+        }
       } finally {
         long duration = executor.getClock().currentTimeMillis() - startTime;
         builder.addTestTimes(duration);
diff --git a/src/main/java/com/google/devtools/build/lib/sandbox/LinuxSandboxedStrategy.java b/src/main/java/com/google/devtools/build/lib/sandbox/LinuxSandboxedStrategy.java
index 99d7183182..4aabf17897 100644
--- a/src/main/java/com/google/devtools/build/lib/sandbox/LinuxSandboxedStrategy.java
+++ b/src/main/java/com/google/devtools/build/lib/sandbox/LinuxSandboxedStrategy.java
@@ -68,8 +68,10 @@ import java.util.concurrent.atomic.AtomicInteger;
 /**
  * Strategy that uses sandboxing to execute a process.
  */
-@ExecutionStrategy(name = {"sandboxed"},
-                   contextType = SpawnActionContext.class)
+@ExecutionStrategy(
+  name = {"sandboxed"},
+  contextType = SpawnActionContext.class
+)
 public class LinuxSandboxedStrategy implements SpawnActionContext {
   private final ExecutorService backgroundWorkers;
 
@@ -684,4 +686,9 @@ public class LinuxSandboxedStrategy implements SpawnActionContext {
   public String toString() {
     return "sandboxed";
   }
+
+  @Override
+  public boolean shouldPropagateExecException() {
+    return verboseFailures && sandboxDebug;
+  }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/standalone/StandaloneSpawnStrategy.java b/src/main/java/com/google/devtools/build/lib/standalone/StandaloneSpawnStrategy.java
index 9c269633c9..6f009af507 100644
--- a/src/main/java/com/google/devtools/build/lib/standalone/StandaloneSpawnStrategy.java
+++ b/src/main/java/com/google/devtools/build/lib/standalone/StandaloneSpawnStrategy.java
@@ -196,4 +196,9 @@ public class StandaloneSpawnStrategy implements SpawnActionContext {
     }
     return AppleHostInfo.getSdkRoot(execRoot, developerDir, iosSdkVersion, appleSdkPlatform);
   }
+
+  @Override
+  public boolean shouldPropagateExecException() {
+    return false;
+  }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/worker/WorkerSpawnStrategy.java b/src/main/java/com/google/devtools/build/lib/worker/WorkerSpawnStrategy.java
index 3433e98706..dd07b80013 100644
--- a/src/main/java/com/google/devtools/build/lib/worker/WorkerSpawnStrategy.java
+++ b/src/main/java/com/google/devtools/build/lib/worker/WorkerSpawnStrategy.java
@@ -269,4 +269,9 @@ public final class WorkerSpawnStrategy implements SpawnActionContext {
   public boolean willExecuteRemotely(boolean remotable) {
     return false;
   }
+
+  @Override
+  public boolean shouldPropagateExecException() {
+    return false;
+  }
 }
diff --git a/src/test/shell/bazel/bazel_sandboxing_test.sh b/src/test/shell/bazel/bazel_sandboxing_test.sh
index d0f9089764..27097bc9c1 100755
--- a/src/test/shell/bazel/bazel_sandboxing_test.sh
+++ b/src/test/shell/bazel/bazel_sandboxing_test.sh
@@ -418,6 +418,22 @@ function test_sandbox_add_path_workspace_child() {
   expect_log "Mounting subdirectory of WORKSPACE or OUTPUTBASE to sandbox is not allowed"
 }
 
+function test_sandbox_fail_command() {
+  mkdir -p "javatests/orange"
+  echo "java_test(name = 'Orange', srcs = ['Orange.java'])" > javatests/orange/BUILD
+  cat > javatests/orange/Orange.java <<EOF
+package orange;
+import junit.framework.TestCase;
+public class Orange extends TestCase {
+  public void testFails() { fail("juice"); }
+}
+EOF
+  bazel test --sandbox_debug --verbose_failures //javatests/orange:Orange >& $TEST_log \
+    && fail "Expected failure" || true
+
+  expect_log "Sandboxed execution failed, which may be legitimate"
+}
+
 # The test shouldn't fail if the environment doesn't support running it.
 check_supported_platform || exit 0
 check_sandbox_allowed || exit 0