Compute the list of test output files in TestRunnerAction

This allows us to slightly simplify the renaming logic in StandaloneTestStrategy.

PiperOrigin-RevId: 190645674

3 files changed, 72 insertions, 91 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/analysis/test/TestResult.java b/src/main/java/com/google/devtools/build/lib/analysis/test/TestResult.java
index 39cbcdb363..b48649a16f 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/test/TestResult.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/test/TestResult.java
@@ -15,10 +15,7 @@
 package com.google.devtools.build.lib.analysis.test;
 
 import com.google.common.base.Preconditions;
-import com.google.common.collect.ImmutableList;
 import com.google.devtools.build.lib.actions.Artifact;
-import com.google.devtools.build.lib.analysis.test.TestRunnerAction.ResolvedPaths;
-import com.google.devtools.build.lib.buildeventstream.TestFileNameConstants;
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable;
 import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe;
@@ -67,42 +64,6 @@ public class TestResult {
     return status == BlazeTestStatus.PASSED || status == BlazeTestStatus.FLAKY;
   }
 
-  public static ImmutableList<Pair<String, Path>> testOutputsFromPaths(
-      ResolvedPaths resolvedPaths) {
-    ImmutableList.Builder<Pair<String, Path>> builder = new ImmutableList.Builder<>();
-    if (resolvedPaths.getXmlOutputPath().exists()) {
-      builder.add(Pair.of(TestFileNameConstants.TEST_XML, resolvedPaths.getXmlOutputPath()));
-    }
-    if (resolvedPaths.getSplitLogsPath().exists()) {
-      builder.add(Pair.of(TestFileNameConstants.SPLIT_LOGS, resolvedPaths.getSplitLogsPath()));
-    }
-    if (resolvedPaths.getTestWarningsPath().exists()) {
-      builder.add(Pair.of(TestFileNameConstants.TEST_WARNINGS,
-            resolvedPaths.getTestWarningsPath()));
-    }
-    if (resolvedPaths.getUndeclaredOutputsZipPath().exists()) {
-      builder.add(Pair.of(TestFileNameConstants.UNDECLARED_OUTPUTS_ZIP,
-          resolvedPaths.getUndeclaredOutputsZipPath()));
-    }
-    if (resolvedPaths.getUndeclaredOutputsManifestPath().exists()) {
-      builder.add(Pair.of(TestFileNameConstants.UNDECLARED_OUTPUTS_MANIFEST,
-          resolvedPaths.getUndeclaredOutputsManifestPath()));
-    }
-    if (resolvedPaths.getUndeclaredOutputsAnnotationsPath().exists()) {
-      builder.add(Pair.of(TestFileNameConstants.UNDECLARED_OUTPUTS_ANNOTATIONS,
-          resolvedPaths.getUndeclaredOutputsAnnotationsPath()));
-    }
-    if (resolvedPaths.getUnusedRunfilesLogPath().exists()) {
-      builder.add(Pair.of(TestFileNameConstants.UNUSED_RUNFILES_LOG,
-          resolvedPaths.getUnusedRunfilesLogPath()));
-    }
-    if (resolvedPaths.getInfrastructureFailureFile().exists()) {
-      builder.add(Pair.of(TestFileNameConstants.TEST_INFRASTRUCTURE_FAILURE,
-          resolvedPaths.getInfrastructureFailureFile()));
-    }
-    return builder.build();
-  }
-
   /**
    * @return The test action.
    */
@@ -186,17 +147,6 @@ public class TestResult {
    *     "test.log").
    */
   public Collection<Pair<String, Path>> getFiles() {
-    ImmutableList.Builder<Pair<String, Path>> builder = new ImmutableList.Builder<>();
-    if (testAction.getTestLog().getPath().exists()) {
-      builder.add(Pair.of(TestFileNameConstants.TEST_LOG, testAction.getTestLog().getPath()));
-    }
-    if (testAction.getCoverageData() != null && testAction.getCoverageData().getPath().exists()) {
-      builder.add(
-          Pair.of(TestFileNameConstants.TEST_COVERAGE, testAction.getCoverageData().getPath()));
-    }
-    if (execRoot != null) {
-      builder.addAll(testOutputsFromPaths(testAction.resolve(execRoot)));
-    }
-    return builder.build();
+    return testAction.getTestOutputsMapping(execRoot);
   }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/test/TestRunnerAction.java b/src/main/java/com/google/devtools/build/lib/analysis/test/TestRunnerAction.java
index 22638c7ce9..336ca69140 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/test/TestRunnerAction.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/test/TestRunnerAction.java
@@ -35,10 +35,12 @@ import com.google.devtools.build.lib.actions.NotifyOnActionCacheHit;
 import com.google.devtools.build.lib.analysis.RunfilesSupplierImpl;
 import com.google.devtools.build.lib.analysis.config.BuildConfiguration;
 import com.google.devtools.build.lib.analysis.config.RunUnder;
+import com.google.devtools.build.lib.buildeventstream.TestFileNameConstants;
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.collect.ImmutableIterable;
 import com.google.devtools.build.lib.util.Fingerprint;
 import com.google.devtools.build.lib.util.LoggingUtil;
+import com.google.devtools.build.lib.util.Pair;
 import com.google.devtools.build.lib.vfs.FileSystem;
 import com.google.devtools.build.lib.vfs.FileSystemUtils;
 import com.google.devtools.build.lib.vfs.Path;
@@ -231,6 +233,55 @@ public class TestRunnerAction extends AbstractAction implements NotifyOnActionCa
     return outputs;
   }
 
+  /**
+   * Returns the list of mappings from file name constants to output files. This method checks the
+   * file system for existence of these output files, so it must only be used after test execution.
+   */
+  // TODO(ulfjack): Instead of going to local disk here, use SpawnResult (add list of files there).
+  public ImmutableList<Pair<String, Path>> getTestOutputsMapping(Path execRoot) {
+    ImmutableList.Builder<Pair<String, Path>> builder = ImmutableList.builder();
+    if (getTestLog().getPath().exists()) {
+      builder.add(Pair.of(TestFileNameConstants.TEST_LOG, getTestLog().getPath()));
+    }
+    if (getCoverageData() != null && getCoverageData().getPath().exists()) {
+      builder.add(Pair.of(TestFileNameConstants.TEST_COVERAGE, getCoverageData().getPath()));
+    }
+    if (execRoot != null) {
+      ResolvedPaths resolvedPaths = resolve(execRoot);
+      if (resolvedPaths.getXmlOutputPath().exists()) {
+        builder.add(Pair.of(TestFileNameConstants.TEST_XML, resolvedPaths.getXmlOutputPath()));
+      }
+      if (resolvedPaths.getSplitLogsPath().exists()) {
+        builder.add(Pair.of(TestFileNameConstants.SPLIT_LOGS, resolvedPaths.getSplitLogsPath()));
+      }
+      if (resolvedPaths.getTestWarningsPath().exists()) {
+        builder.add(Pair.of(TestFileNameConstants.TEST_WARNINGS,
+              resolvedPaths.getTestWarningsPath()));
+      }
+      if (resolvedPaths.getUndeclaredOutputsZipPath().exists()) {
+        builder.add(Pair.of(TestFileNameConstants.UNDECLARED_OUTPUTS_ZIP,
+            resolvedPaths.getUndeclaredOutputsZipPath()));
+      }
+      if (resolvedPaths.getUndeclaredOutputsManifestPath().exists()) {
+        builder.add(Pair.of(TestFileNameConstants.UNDECLARED_OUTPUTS_MANIFEST,
+            resolvedPaths.getUndeclaredOutputsManifestPath()));
+      }
+      if (resolvedPaths.getUndeclaredOutputsAnnotationsPath().exists()) {
+        builder.add(Pair.of(TestFileNameConstants.UNDECLARED_OUTPUTS_ANNOTATIONS,
+            resolvedPaths.getUndeclaredOutputsAnnotationsPath()));
+      }
+      if (resolvedPaths.getUnusedRunfilesLogPath().exists()) {
+        builder.add(Pair.of(TestFileNameConstants.UNUSED_RUNFILES_LOG,
+            resolvedPaths.getUnusedRunfilesLogPath()));
+      }
+      if (resolvedPaths.getInfrastructureFailureFile().exists()) {
+        builder.add(Pair.of(TestFileNameConstants.TEST_INFRASTRUCTURE_FAILURE,
+            resolvedPaths.getInfrastructureFailureFile()));
+      }
+    }
+    return builder.build();
+  }
+
   @Override
   protected void computeKey(ActionKeyContext actionKeyContext, Fingerprint fp)
       throws CommandLineExpansionException {
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 c3828d3c0c..aa83718d33 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
@@ -163,21 +163,7 @@ public class StandaloneTestStrategy extends TestStrategy {
                 workingDirectory);
       }
       processLastTestAttempt(attempt, dataBuilder, standaloneTestResult.testResultData());
-      ImmutableList.Builder<Pair<String, Path>> testOutputsBuilder = new ImmutableList.Builder<>();
-      if (actionExecutionContext.getInputPath(action.getTestLog()).exists()) {
-        testOutputsBuilder.add(
-            Pair.of(
-                TestFileNameConstants.TEST_LOG,
-                actionExecutionContext.getInputPath(action.getTestLog())));
-      }
-      if (action.getCoverageData() != null
-          && actionExecutionContext.getInputPath(action.getCoverageData()).exists()) {
-        testOutputsBuilder.add(
-            Pair.of(
-                TestFileNameConstants.TEST_COVERAGE,
-                actionExecutionContext.getInputPath(action.getCoverageData())));
-      }
-      testOutputsBuilder.addAll(TestResult.testOutputsFromPaths(resolvedPaths));
+      ImmutableList<Pair<String, Path>> testOutputs = action.getTestOutputsMapping(execRoot);
       actionExecutionContext
           .getEventBus()
           .post(
@@ -187,7 +173,7 @@ public class StandaloneTestStrategy extends TestStrategy {
                   standaloneTestResult.testResultData().getStatus(),
                   standaloneTestResult.testResultData().getStartTimeMillisEpoch(),
                   standaloneTestResult.testResultData().getRunDurationMillis(),
-                  testOutputsBuilder.build(),
+                  testOutputs,
                   standaloneTestResult.testResultData().getWarningList(),
                   true));
       finalizeTest(actionExecutionContext, action, dataBuilder.build());
@@ -217,39 +203,33 @@ public class StandaloneTestStrategy extends TestStrategy {
     attemptsDir.createDirectory();
     String attemptPrefix = "attempt_" + attempt;
     Path testLog = attemptsDir.getChild(attemptPrefix + ".log");
-    if (actionExecutionContext.getInputPath(action.getTestLog()).exists()) {
-      actionExecutionContext.getInputPath(action.getTestLog()).renameTo(testLog);
-      testOutputsBuilder.add(Pair.of(TestFileNameConstants.TEST_LOG, testLog));
-    }
-    if (action.getCoverageData() != null
-        && actionExecutionContext.getInputPath(action.getCoverageData()).exists()) {
-      testOutputsBuilder.add(
-          Pair.of(
-              TestFileNameConstants.TEST_COVERAGE,
-              actionExecutionContext.getInputPath(action.getCoverageData())));
-    }
 
     // Get the normal test output paths, and then update them to use "attempt_N" names, and
     // attemptDir, before adding them to the outputs.
-    ResolvedPaths resolvedPaths = action.resolve(actionExecutionContext.getExecRoot());
-    ImmutableList<Pair<String, Path>> testOutputs = TestResult.testOutputsFromPaths(resolvedPaths);
+    ImmutableList<Pair<String, Path>> testOutputs =
+        action.getTestOutputsMapping(actionExecutionContext.getExecRoot());
     for (Pair<String, Path> testOutput : testOutputs) {
       // e.g. /testRoot/test.dir/file, an example we follow throughout this loop's comments.
       Path testOutputPath = testOutput.getSecond();
+      Path destinationPath;
+      if (testOutput.getFirst().equals(TestFileNameConstants.TEST_LOG)) {
+        // The rename rules for the test log are different than for all the other files.
+        destinationPath = testLog;
+      } else {
+        // e.g. test.dir/file
+        PathFragment relativeToTestDirectory = testOutputPath.relativeTo(testRoot);
 
-      // e.g. test.dir/file
-      PathFragment relativeToTestDirectory = testOutputPath.relativeTo(testRoot);
-
-      // e.g. attempt_1.dir/file
-      String destinationPathFragmentStr =
-          relativeToTestDirectory.getSafePathString().replaceFirst("test", attemptPrefix);
-      PathFragment destinationPathFragment = PathFragment.create(destinationPathFragmentStr);
+        // e.g. attempt_1.dir/file
+        String destinationPathFragmentStr =
+            relativeToTestDirectory.getSafePathString().replaceFirst("test", attemptPrefix);
+        PathFragment destinationPathFragment = PathFragment.create(destinationPathFragmentStr);
 
-      // e.g. /attemptsDir/attempt_1.dir/file
-      Path destinationPath = attemptsDir.getRelative(destinationPathFragment);
-      destinationPath.getParentDirectory().createDirectory();
+        // e.g. /attemptsDir/attempt_1.dir/file
+        destinationPath = attemptsDir.getRelative(destinationPathFragment);
+        destinationPath.getParentDirectory().createDirectory();
+      }
 
-      // Copy to the destination.
+      // Move to the destination.
       testOutputPath.renameTo(destinationPath);
 
       testOutputsBuilder.add(Pair.of(testOutput.getFirst(), destinationPath));