Add a flag to split test.xml generation into a separate Spawn

At this time, this is only implemented for the StandaloneTestStrategy.

This solves a race condition on Posix-like systems, where we cannot guarantee that the pipes are actually fully flushed to disk when the test process exits, and this can cause the test.xml to be empty, which makes it hard to debug issues. (The test.log files do not show up in normal CI systems, only the test.xml files.)

Progress on #4608.

PiperOrigin-RevId: 206292179

2 files changed, 128 insertions, 1 deletions

diff --git a/src/test/java/com/google/devtools/build/lib/analysis/mock/BazelAnalysisMock.java b/src/test/java/com/google/devtools/build/lib/analysis/mock/BazelAnalysisMock.java
index 0d399f2db3..123b58d3ba 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/mock/BazelAnalysisMock.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/mock/BazelAnalysisMock.java
@@ -126,8 +126,9 @@ public final class BazelAnalysisMock extends AnalysisMock {
         "/bazel_tools_workspace/tools/genrule/BUILD", "exports_files(['genrule-setup.sh'])");
 
     config.create("/bazel_tools_workspace/tools/test/BUILD",
-        "filegroup(name = 'runtime', srcs = ['test-setup.sh'])",
+        "filegroup(name = 'runtime', srcs = ['test-setup.sh', 'test-xml-generator.sh'])",
         "filegroup(name = 'test_setup', srcs = ['test-setup.sh'])",
+        "filegroup(name = 'test_xml_generator', srcs = ['test-xml-generator.sh'])",
         "filegroup(name = 'collect_coverage', srcs = ['collect_coverage.sh'])",
         "filegroup(name='coverage_support', srcs=['collect_coverage.sh'])",
         "filegroup(name = 'coverage_report_generator', srcs = ['coverage_report_generator.sh'])");
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 ab07910d60..5351a956c3 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
@@ -19,6 +19,7 @@ import static com.google.devtools.build.lib.testutil.TestConstants.WORKSPACE_NAM
 import static java.nio.charset.StandardCharsets.UTF_8;
 import static org.junit.Assert.fail;
 import static org.mockito.Matchers.any;
+import static org.mockito.Matchers.argThat;
 import static org.mockito.Matchers.same;
 import static org.mockito.Mockito.when;
 
@@ -30,6 +31,7 @@ import com.google.devtools.build.lib.actions.ActionExecutionContext;
 import com.google.devtools.build.lib.actions.ActionInput;
 import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.actions.ArtifactPathResolver;
+import com.google.devtools.build.lib.actions.Spawn;
 import com.google.devtools.build.lib.actions.SpawnActionContext;
 import com.google.devtools.build.lib.actions.SpawnResult;
 import com.google.devtools.build.lib.actions.SpawnResult.Status;
@@ -55,6 +57,7 @@ import org.junit.Before;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.junit.runners.JUnit4;
+import org.mockito.ArgumentMatcher;
 import org.mockito.Mock;
 import org.mockito.MockitoAnnotations;
 import org.mockito.invocation.InvocationOnMock;
@@ -429,6 +432,7 @@ public final class StandaloneTestStrategyTest extends BuildViewTestCase {
 
     ExecutionOptions executionOptions = Options.getDefaults(ExecutionOptions.class);
     executionOptions.testOutput = TestOutputFormat.ERRORS;
+    executionOptions.splitXmlGeneration = false;
     Path tmpDirRoot = TestStrategy.getTmpRoot(rootDirectory, outputBase, executionOptions);
     BinTools binTools = BinTools.forUnitTesting(directories, analysisMock.getEmbeddedTools());
     TestedStandaloneTestStrategy standaloneTestStrategy =
@@ -534,6 +538,128 @@ public final class StandaloneTestStrategyTest extends BuildViewTestCase {
   }
 
   @Test
+  public void testThatTestLogAndOutputAreReturnedWithSplitXmlGeneration() throws Exception {
+
+    // setup a StandaloneTestStrategy
+
+    ExecutionOptions executionOptions = Options.getDefaults(ExecutionOptions.class);
+    executionOptions.testOutput = TestOutputFormat.ERRORS;
+    executionOptions.splitXmlGeneration = true;
+    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/failing_test.sh", "this does not get executed, it is mocked out");
+
+    scratch.file(
+        "standalone/BUILD",
+        "sh_test(",
+        "    name = \"failing_test\",",
+        "    size = \"small\",",
+        "    srcs = [\"failing_test.sh\"],",
+        ")");
+
+    ConfiguredTarget configuredTarget = getConfiguredTarget("//standalone:failing_test");
+    List<Artifact> testStatusArtifacts =
+        configuredTarget.getProvider(TestProvider.class).getTestParams().getTestStatusArtifacts();
+    Artifact testStatusArtifact = Iterables.getOnlyElement(testStatusArtifacts);
+    TestRunnerAction testRunnerAction = (TestRunnerAction) getGeneratingAction(testStatusArtifact);
+    FileSystemUtils.createDirectoryAndParents(
+        testRunnerAction.getTestLog().getPath().getParentDirectory());
+    // setup a mock ActionExecutionContext
+
+    when(actionExecutionContext.getClock()).thenReturn(BlazeClock.instance());
+    when(actionExecutionContext.withFileOutErr(any()))
+        .thenAnswer(
+            new Answer<ActionExecutionContext>() {
+              @SuppressWarnings("unchecked")
+              @Override
+              public ActionExecutionContext answer(InvocationOnMock invocation) throws Throwable {
+                FileOutErr outErr = (FileOutErr) invocation.getArguments()[0];
+                try (OutputStream stream = outErr.getOutputStream()) {
+                  stream.write("This will not appear in the test output: bla\n".getBytes(UTF_8));
+                  stream.write((TestLogHelper.HEADER_DELIMITER + "\n").getBytes(UTF_8));
+                  stream.write("This will appear in the test output: foo\n".getBytes(UTF_8));
+                }
+                return actionExecutionContext;
+              }
+            });
+    reporter.removeHandler(failFastHandler);
+    when(actionExecutionContext.getExecRoot()).thenReturn(outputBase.getRelative("execroot"));
+    when(actionExecutionContext.getClientEnv()).thenReturn(ImmutableMap.of());
+    when(actionExecutionContext.getEventHandler()).thenReturn(reporter);
+    when(actionExecutionContext.getEventBus()).thenReturn(eventBus);
+    when(actionExecutionContext.getInputPath(any())).thenAnswer(this::getInputPathMock);
+    when(actionExecutionContext.getPathResolver()).thenReturn(ArtifactPathResolver.IDENTITY);
+
+    Path outPath = tmpDirRoot.getRelative("test-out.txt");
+    Path errPath = tmpDirRoot.getRelative("test-err.txt");
+    FileOutErr outErr = new FileOutErr(outPath, errPath);
+    when(actionExecutionContext.getFileOutErr()).thenReturn(outErr);
+
+    SpawnResult testSpawnResult =
+        new SpawnResult.Builder()
+            .setStatus(Status.NON_ZERO_EXIT)
+            .setExitCode(1)
+            .setRunnerName("test")
+            .build();
+    when(spawnActionContext.exec(argThat(new ArgumentMatcher<Spawn>() {
+          @Override
+          public boolean matches(Object argument) {
+            return (argument instanceof Spawn) && ((Spawn) argument).getOutputFiles().size() != 1;
+          }
+        }), any()))
+        .thenThrow(
+            new SpawnExecException(
+                "Failure!!",
+                testSpawnResult,
+                /*forciblyRunRemotely=*/ false,
+                /*catastrophe=*/ false));
+
+    SpawnResult xmlGeneratorSpawnResult =
+        new SpawnResult.Builder()
+            .setStatus(Status.SUCCESS)
+            .setRunnerName("test")
+            .build();
+    when(spawnActionContext.exec(argThat(new ArgumentMatcher<Spawn>() {
+          @Override
+          public boolean matches(Object argument) {
+            return (argument instanceof Spawn) && ((Spawn) argument).getOutputFiles().size() == 1;
+          }
+        }), any()))
+        .thenReturn(ImmutableList.of(xmlGeneratorSpawnResult));
+    when(actionExecutionContext.getContext(same(SpawnActionContext.class)))
+        .thenReturn(spawnActionContext);
+
+    // actual StandaloneTestStrategy execution
+    List<SpawnResult> spawnResults =
+        standaloneTestStrategy.exec(testRunnerAction, actionExecutionContext);
+
+    // check that the rigged SpawnResult was returned
+    assertThat(spawnResults).containsExactly(testSpawnResult, xmlGeneratorSpawnResult);
+    // check that the test log contains all the output
+    String logData = FileSystemUtils.readContent(testRunnerAction.getTestLog().getPath(), UTF_8);
+    assertThat(logData).contains("bla");
+    assertThat(logData).contains(TestLogHelper.HEADER_DELIMITER);
+    assertThat(logData).contains("foo");
+    // check that the test stdout contains all the expected output
+    outErr.close(); // Create the output files.
+    String outData = FileSystemUtils.readContent(outPath, UTF_8);
+    assertThat(outData)
+        .contains("==================== Test output for //standalone:failing_test:");
+    assertThat(outData).doesNotContain("bla");
+    assertThat(outData).doesNotContain(TestLogHelper.HEADER_DELIMITER);
+    assertThat(outData).contains("foo");
+    assertThat(outData)
+        .contains(
+            "================================================================================");
+    assertThat(errPath.exists()).isFalse();
+  }
+
+  @Test
   public void testEmptyOutputCreatesEmptyLogFile() throws Exception {
     // setup a StandaloneTestStrategy
     ExecutionOptions executionOptions = Options.getDefaults(ExecutionOptions.class);