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

src/main/java/com/google/devtools/build/lib/analysis/BaseRuleClasses.java

@@ -174,6 +174,11 @@ public Object getDefault(AttributeMap rule) {
                   .cfg(HostTransition.INSTANCE)
                   .singleArtifact()
                   .value(env.getToolsLabel("//tools/test:test_setup")))
+          .add(
+              attr("$xml_generator_script", LABEL)
+                  .cfg(HostTransition.INSTANCE)
+                  .singleArtifact()
+                  .value(env.getToolsLabel("//tools/test:test_xml_generator")))
           .add(
               attr("$collect_coverage_script", LABEL)
                   .cfg(HostTransition.INSTANCE)

src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkRuleClassFunctions.java

@@ -176,6 +176,12 @@ public static final RuleClass getTestBaseRule(String toolsRepository) {
                 .cfg(HostTransition.INSTANCE)
                 .singleArtifact()
                 .value(labelCache.getUnchecked(toolsRepository + "//tools/test:test_setup")))
+        .add(
+            attr("$xml_generator_script", LABEL)
+                .cfg(HostTransition.INSTANCE)
+                .singleArtifact()
+                .value(
+                    labelCache.getUnchecked(toolsRepository + "//tools/test:test_xml_generator")))
         .add(
             attr("$collect_coverage_script", LABEL)
                 .cfg(HostTransition.INSTANCE)

src/main/java/com/google/devtools/build/lib/analysis/test/TestActionBuilder.java

@@ -204,6 +204,9 @@ private TestParams createTestAction(int shards) {
 
     Artifact testSetupScript = ruleContext.getHostPrerequisiteArtifact("$test_setup_script");
     inputsBuilder.add(testSetupScript);
+    Artifact testXmlGeneratorScript =
+        ruleContext.getHostPrerequisiteArtifact("$xml_generator_script");
+    inputsBuilder.add(testXmlGeneratorScript);
 
     Artifact collectCoverageScript = null;
     TreeMap<String, String> extraTestEnv = new TreeMap<>();
@@ -307,6 +310,7 @@ private TestParams createTestAction(int shards) {
                 ruleContext.getActionOwner(),
                 inputs,
                 testSetupScript,
+                testXmlGeneratorScript,
                 collectCoverageScript,
                 testLog,
                 cacheStatus,

src/main/java/com/google/devtools/build/lib/analysis/test/TestRunnerAction.java

@@ -73,6 +73,7 @@ public class TestRunnerAction extends AbstractAction implements NotifyOnActionCa
   private static final String GUID = "cc41f9d0-47a6-11e7-8726-eb6ce83a8cc8";
 
   private final Artifact testSetupScript;
+  private final Artifact testXmlGeneratorScript;
   private final Artifact collectCoverageScript;
   private final BuildConfiguration configuration;
   private final TestConfiguration testConfiguration;
@@ -135,6 +136,7 @@ private static ImmutableList<Artifact> list(Artifact... artifacts) {
       ActionOwner owner,
       Iterable<Artifact> inputs,
       Artifact testSetupScript, // Must be in inputs
+      Artifact testXmlGeneratorScript, // Must be in inputs
       @Nullable Artifact collectCoverageScript, // Must be in inputs, if not null
       Artifact testLog,
       Artifact cacheStatus,
@@ -157,6 +159,7 @@ private static ImmutableList<Artifact> list(Artifact... artifacts) {
         configuration.getActionEnvironment());
     Preconditions.checkState((collectCoverageScript == null) == (coverageArtifact == null));
     this.testSetupScript = testSetupScript;
+    this.testXmlGeneratorScript = testXmlGeneratorScript;
     this.collectCoverageScript = collectCoverageScript;
     this.configuration = Preconditions.checkNotNull(configuration);
     this.testConfiguration =
@@ -741,6 +744,10 @@ public Artifact getTestSetupScript() {
     return testSetupScript;
   }
 
+  public Artifact getTestXmlGeneratorScript() {
+    return testXmlGeneratorScript;
+  }
+
   @Nullable
   public Artifact getCollectCoverageScript() {
     return collectCoverageScript;

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

@@ -309,6 +309,17 @@ public boolean usingLocalTestJobs() {
   )
   public String executionLogFile;
 
+  @Option(
+    name = "experimental_split_xml_generation",
+    defaultValue = "false",
+    documentationCategory = OptionDocumentationCategory.EXECUTION_STRATEGY,
+    effectTags = {OptionEffectTag.EXECUTION},
+    help = "If this flag is set, and a test action does not generate a test.xml file, then "
+        + "Bazel uses a separate action to generate a dummy test.xml file containing the test log. "
+        + "Otherwise, Bazel generates the test.xml as part of the test action."
+  )
+  public boolean splitXmlGeneration;
+
   /** Converter for the --flaky_test_attempts option. */
   public static class TestAttemptsConverter extends PerLabelOptions.PerLabelOptionsConverter {
     private static final int MIN_VALUE = 1;

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

@@ -17,7 +17,9 @@
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.Iterables;
+import com.google.common.collect.Lists;
 import com.google.devtools.build.lib.actions.ActionExecutionContext;
+import com.google.devtools.build.lib.actions.ActionInputHelper;
 import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.actions.EnvironmentalExecException;
 import com.google.devtools.build.lib.actions.ExecException;
@@ -30,15 +32,16 @@
 import com.google.devtools.build.lib.actions.SpawnResult;
 import com.google.devtools.build.lib.actions.TestExecException;
 import com.google.devtools.build.lib.analysis.RunfilesSupplierImpl;
+import com.google.devtools.build.lib.analysis.actions.SpawnAction;
 import com.google.devtools.build.lib.analysis.test.TestActionContext;
 import com.google.devtools.build.lib.analysis.test.TestResult;
 import com.google.devtools.build.lib.analysis.test.TestRunnerAction;
-import com.google.devtools.build.lib.analysis.test.TestRunnerAction.ResolvedPaths;
 import com.google.devtools.build.lib.buildeventstream.BuildEventStreamProtos;
 import com.google.devtools.build.lib.buildeventstream.TestFileNameConstants;
 import com.google.devtools.build.lib.events.Event;
 import com.google.devtools.build.lib.events.EventKind;
 import com.google.devtools.build.lib.events.Reporter;
+import com.google.devtools.build.lib.util.OS;
 import com.google.devtools.build.lib.util.Pair;
 import com.google.devtools.build.lib.util.io.FileOutErr;
 import com.google.devtools.build.lib.vfs.FileSystemUtils;
@@ -50,6 +53,7 @@
 import java.io.Closeable;
 import java.io.IOException;
 import java.time.Duration;
+import java.util.ArrayList;
 import java.util.List;
 import java.util.Map;
 import java.util.TreeMap;
@@ -99,10 +103,11 @@ public List<SpawnResult> exec(
     Path tmpDir = tmpDirRoot.getChild(TestStrategy.getTmpDirName(action));
     Map<String, String> env = setupEnvironment(
         action, actionExecutionContext.getClientEnv(), execRoot, runfilesDir, tmpDir);
+    if (executionOptions.splitXmlGeneration) {
+      env.put("EXPERIMENTAL_SPLIT_XML_GENERATION", "1");
+    }
     Path workingDirectory = runfilesDir.getRelative(action.getRunfilesPrefix());
 
-    ResolvedPaths resolvedPaths = action.resolve(execRoot);
-
     Map<String, String> executionInfo =
         new TreeMap<>(action.getTestProperties().getExecutionInfo());
     if (!action.shouldCacheResult()) {
@@ -336,7 +341,8 @@ protected StandaloneTestResult executeTest(
     long startTime = actionExecutionContext.getClock().currentTimeMillis();
     SpawnActionContext spawnActionContext =
         actionExecutionContext.getContext(SpawnActionContext.class);
-    List<SpawnResult> spawnResults = ImmutableList.of();
+    Path xmlOutputPath = action.resolve(actionExecutionContext.getExecRoot()).getXmlOutputPath();
+    List<SpawnResult> spawnResults = new ArrayList<>();
     BuildEventStreamProtos.TestResult.ExecutionInfo.Builder executionInfo =
         BuildEventStreamProtos.TestResult.ExecutionInfo.newBuilder();
     try {
@@ -347,36 +353,48 @@ protected StandaloneTestResult executeTest(
                   Reporter.outErrForReporter(actionExecutionContext.getEventHandler()),
                   testLogPath);
         }
-        spawnResults = spawnActionContext.exec(spawn, actionExecutionContext);
-
-        builder
-            .setTestPassed(true)
-            .setStatus(BlazeTestStatus.PASSED)
-            .setPassedLog(testLogPath.getPathString());
-      } catch (SpawnExecException e) {
-        // If this method returns normally, then the higher level will rerun the test (up to
-        // --flaky_test_attempts times).
-        if (e.isCatastrophic()) {
-          // Rethrow as the error was catastrophic and thus the build has to be halted.
-          throw e;
+        try {
+          spawnResults.addAll(spawnActionContext.exec(spawn, actionExecutionContext));
+          builder
+              .setTestPassed(true)
+              .setStatus(BlazeTestStatus.PASSED)
+              .setPassedLog(testLogPath.getPathString());
+        } catch (SpawnExecException e) {
+          // If this method returns normally, then the higher level will rerun the test (up to
+          // --flaky_test_attempts times).
+          if (e.isCatastrophic()) {
+            // Rethrow as the error was catastrophic and thus the build has to be halted.
+            throw e;
+          }
+          if (!e.getSpawnResult().setupSuccess()) {
+            // Rethrow as the test could not be run and thus there's no point in retrying.
+            throw e;
+          }
+          builder
+              .setTestPassed(false)
+              .setStatus(e.hasTimedOut() ? BlazeTestStatus.TIMEOUT : BlazeTestStatus.FAILED)
+              .addFailedLogs(testLogPath.getPathString());
+          spawnResults.add(e.getSpawnResult());
         }
-        if (!e.getSpawnResult().setupSuccess()) {
-          // Rethrow as the test could not be run and thus there's no point in retrying.
-          throw e;
+        // If the test did not create a test.xml, and --experimental_split_xml_generation is
+        // enabled, then we run a separate action to create a test.xml from test.log.
+        if (executionOptions.splitXmlGeneration
+            && action.getTestLog().getPath().exists()
+            && !xmlOutputPath.exists()) {
+          SpawnResult result = Iterables.getOnlyElement(spawnResults);
+          Spawn xmlGeneratingSpawn = createXmlGeneratingSpawn(action, result);
+          // We treat all failures to generate the test.xml here as catastrophic, and won't rerun
+          // the test if this fails.
+          spawnResults.addAll(spawnActionContext.exec(xmlGeneratingSpawn, actionExecutionContext));
         }
-        builder
-            .setTestPassed(false)
-            .setStatus(e.hasTimedOut() ? BlazeTestStatus.TIMEOUT : BlazeTestStatus.FAILED)
-            .addFailedLogs(testLogPath.getPathString());
-        spawnResults = ImmutableList.of(e.getSpawnResult());
       } finally {
         long endTime = actionExecutionContext.getClock().currentTimeMillis();
         long duration = endTime - startTime;
         // If execution fails with an exception other SpawnExecException, there is no result here.
         if (!spawnResults.isEmpty()) {
           // The SpawnResult of a remotely cached or remotely executed action may not have walltime
           // set. We fall back to the time measured here for backwards compatibility.
-          SpawnResult primaryResult = Iterables.getOnlyElement(spawnResults);
+          SpawnResult primaryResult = spawnResults.iterator().next();
           duration = primaryResult.getWallTime().orElse(Duration.ofMillis(duration)).toMillis();
           extractExecutionInfo(primaryResult, builder, executionInfo);
         }
@@ -390,11 +408,7 @@ protected StandaloneTestResult executeTest(
         }
       }
 
-      TestCase details =
-          parseTestResult(
-              action
-                  .resolve(actionExecutionContext.getExecRoot())
-                  .getXmlOutputPath());
+      TestCase details = parseTestResult(xmlOutputPath);
       if (details != null) {
         builder.setTestCase(details);
       }
@@ -431,6 +445,43 @@ private static void extractExecutionInfo(
     }
   }
 
+  /**
+   * A spawn to generate a test.xml file from the test log. This is only used if the test does not
+   * generate a test.xml file itself.
+   */
+  private Spawn createXmlGeneratingSpawn(TestRunnerAction action, SpawnResult result) {
+    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().getExecOS() == OS.WINDOWS
+    if (OS.getCurrent() == OS.WINDOWS) {
+      args.add(action.getShExecutable().getPathString());
+      args.add("-c");
+      args.add("$0 $*");
+    }
+    args.add(action.getTestXmlGeneratorScript().getExecPath().getCallablePathString());
+    args.add(action.getTestLog().getExecPathString());
+    args.add(action.getXmlOutputPath().getPathString());
+    args.add(Long.toString(result.getWallTime().orElse(Duration.ZERO).getSeconds()));
+    args.add(Integer.toString(result.exitCode()));
+
+    return new SimpleSpawn(
+        action,
+        ImmutableList.copyOf(args),
+        ImmutableMap.of(
+            "PATH", "/usr/bin:/bin",
+            "TEST_SHARD_INDEX", Integer.toString(action.getShardNum()),
+            "TEST_TOTAL_SHARDS", Integer.toString(action.getExecutionSettings().getTotalShards()),
+            "TEST_NAME", action.getTestName()),
+        ImmutableMap.of(),
+        null,
+        ImmutableMap.of(),
+        /*inputs=*/ ImmutableList.of(action.getTestXmlGeneratorScript(), action.getTestLog()),
+        /*tools=*/ ImmutableList.<Artifact>of(),
+        /*outputs=*/ ImmutableList.of(ActionInputHelper.fromPath(action.getXmlOutputPath())),
+        SpawnAction.DEFAULT_RESOURCE_SET);
+  }
+
   /**
    * Outputs test result to the stdout after test has finished (e.g. for --test_output=all or
    * --test_output=errors). Will also try to group output lines together (up to 10000 lines) so

src/test/java/com/google/devtools/build/lib/analysis/mock/BazelAnalysisMock.java

@@ -126,8 +126,9 @@ public void setupMockClient(MockToolsConfig config) throws IOException {
         "/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'])");