commit fbdf39bc235e4aafa21b4e8ea91debd5046d0766
author ulfjack <ulfjack@google.com> Tue May 21 08:50:23 2019 -0700
committer Danna Kelmer <dannark@google.com> Tue May 21 17:00:05 2019 -0400
tree 886b7d5582338889a2e739af4ceb045c6234226a
parent 318aebb523b3c91275855e35f0a67814e0d2231f

Fix StandaloneTestStrategy.appendStderr

As of 4a5e1b7, it was using getErrorPath twice, which could cause it to
loop indefinitely, trying to append the error file to itself. This
happened rarely, as the test runner script redirects stderr to stdout.
However, it could happen if the SpawnRunner wrote any extra output to
stderr, which the RemoteSpawnRunner does in some cases.

I have manually checked that this fixes the issue, and also added a
regression test.

Fixes #8320.

PiperOrigin-RevId: 249258656

src/main/java/com/google/devtools/build/lib/exec/StandaloneTestStrategy.java

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 0483952..86a00bb 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
@@ -270,7 +270,7 @@
     if (stat != null) {
       try {
         if (stat.getSize() > 0) {
-          Path stdOut = outErr.getErrorPath();
+          Path stdOut = outErr.getOutputPath();
           if (stdOut.exists()) {
             stdOut.setWritable(true);
           }

src/test/java/com/google/devtools/build/lib/exec/StandaloneTestStrategyTest.java

diff --git a/src/test/java/com/google/devtools/build/lib/exec/StandaloneTestStrategyTest.java b/src/test/java/com/google/devtools/build/lib/exec/StandaloneTestStrategyTest.java
index 1ebc38c..dfc6433 100644
--- a/src/test/java/com/google/devtools/build/lib/exec/StandaloneTestStrategyTest.java
+++ b/src/test/java/com/google/devtools/build/lib/exec/StandaloneTestStrategyTest.java
@@ -655,4 +655,49 @@
     }
     assertThat(outErr.getErrorPath().exists()).isFalse();
   }
+
+  @Test
+  public void testAppendStdErrDoesNotBusyLoop() throws Exception {
+    ExecutionOptions executionOptions = Options.getDefaults(ExecutionOptions.class);
+    executionOptions.testOutput = TestOutputFormat.ALL;
+    Path tmpDirRoot = TestStrategy.getTmpRoot(rootDirectory, outputBase, executionOptions);
+    BinTools binTools = BinTools.forUnitTesting(directories, analysisMock.getEmbeddedTools());
+    TestedStandaloneTestStrategy standaloneTestStrategy =
+        new TestedStandaloneTestStrategy(executionOptions, binTools, tmpDirRoot);
+
+    // setup a test action
+    scratch.file("standalone/empty_test.sh", "this does not get executed, it is mocked out");
+    scratch.file(
+        "standalone/BUILD",
+        "sh_test(",
+        "    name = \"empty_test\",",
+        "    size = \"small\",",
+        "    srcs = [\"empty_test.sh\"],",
+        ")");
+    TestRunnerAction testRunnerAction = getTestAction("//standalone:empty_test");
+
+    SpawnResult expectedSpawnResult =
+        new SpawnResult.Builder().setStatus(Status.SUCCESS).setRunnerName("test").build();
+    when(spawnActionContext.beginExecution(any(), any()))
+        .then(
+            (invocation) -> {
+              ((ActionExecutionContext) invocation.getArgument(1)).getFileOutErr().printErr("Foo");
+              return SpawnContinuation.immediate(expectedSpawnResult);
+            });
+
+    FileOutErr outErr = createTempOutErr(tmpDirRoot);
+    ActionExecutionContext actionExecutionContext =
+        new FakeActionExecutionContext(outErr, spawnActionContext);
+
+    // actual StandaloneTestStrategy execution
+    execute(testRunnerAction, actionExecutionContext, standaloneTestStrategy);
+
+    // check that the test stdout contains all the expected output
+    try {
+      String outData = FileSystemUtils.readContent(outErr.getOutputPath(), UTF_8);
+      assertThat(outData).contains("Foo");
+    } catch (IOException e) {
+      fail("Test stdout file missing: " + outErr.getOutputPath());
+    }
+  }
 }