Return a multimap instead of a list of pairs from TestRunnerAction#getTestOutputsMapping.

Cleanup CL in preparation for fixing bazelbuild#17911.

PiperOrigin-RevId: 671735596
Change-Id: I2f477d4d49fd298895a59ef88fc98d72cad1f974

Author: tjgq

src/main/java/com/google/devtools/build/lib/actions/ImportantOutputHandler.java

@@ -22,7 +22,7 @@
 import com.google.devtools.build.lib.vfs.Path;
 import com.google.devtools.build.lib.vfs.PathFragment;
 import java.time.Duration;
-import java.util.List;
+import java.util.Collection;
 import java.util.Map;
 
 /** Context to be informed of top-level outputs and their runfiles. */
@@ -82,7 +82,7 @@ LostArtifacts processRunfilesAndGetLostArtifacts(
    * Actions#dependsOnBuildId}), so outputs passed to this method come from a just-executed test
    * action.
    */
-  void processTestOutputs(List<Path> testOutputs)
+  void processTestOutputs(Collection<Path> testOutputs)
       throws ImportantOutputException, InterruptedException;
 
   /**

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

@@ -18,6 +18,7 @@
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Preconditions;
 import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableMultimap;
 import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.buildeventstream.BuildEvent.LocalFile.LocalFileType;
 import com.google.devtools.build.lib.buildeventstream.BuildEventContext;
@@ -29,14 +30,14 @@
 import com.google.devtools.build.lib.buildeventstream.GenericBuildEvent;
 import com.google.devtools.build.lib.buildeventstream.PathConverter;
 import com.google.devtools.build.lib.runtime.BuildEventStreamerUtils;
-import com.google.devtools.build.lib.util.Pair;
 import com.google.devtools.build.lib.vfs.Path;
 import com.google.devtools.build.lib.view.test.TestStatus.BlazeTestStatus;
 import com.google.devtools.build.lib.view.test.TestStatus.TestResultData;
 import com.google.protobuf.util.Durations;
 import com.google.protobuf.util.Timestamps;
 import java.util.Collection;
 import java.util.List;
+import java.util.Map;
 
 /** This event is raised whenever an individual test attempt is completed. */
 public class TestAttempt implements BuildEventWithOrderConstraint {
@@ -47,7 +48,7 @@ public class TestAttempt implements BuildEventWithOrderConstraint {
   private final boolean cachedLocally;
   private final int attempt;
   private final boolean lastAttempt;
-  private final Collection<Pair<String, Path>> files;
+  private final ImmutableMultimap<String, Path> files;
   private final List<String> testWarnings;
   private final long durationMillis;
   private final long startTimeMillis;
@@ -69,7 +70,7 @@ private TestAttempt(
       String statusDetails,
       long startTimeMillis,
       long durationMillis,
-      Collection<Pair<String, Path>> files,
+      ImmutableMultimap<String, Path> files,
       List<String> testWarnings,
       boolean lastAttempt) {
     this.testAction = testAction;
@@ -93,7 +94,7 @@ public static TestAttempt forExecutedTestResult(
       TestRunnerAction testAction,
       TestResultData attemptData,
       int attempt,
-      Collection<Pair<String, Path>> files,
+      ImmutableMultimap<String, Path> files,
       BuildEventStreamProtos.TestResult.ExecutionInfo executionInfo,
       boolean lastAttempt) {
     return new TestAttempt(
@@ -118,7 +119,7 @@ public static TestAttempt fromCachedTestResult(
       TestRunnerAction testAction,
       TestResultData attemptData,
       int attempt,
-      Collection<Pair<String, Path>> files,
+      ImmutableMultimap<String, Path> files,
       BuildEventStreamProtos.TestResult.ExecutionInfo executionInfo,
       boolean lastAttempt) {
     return new TestAttempt(
@@ -153,7 +154,7 @@ public static TestAttempt forUnstartableTestResult(
         attemptData.getStatusDetails(),
         attemptData.getStartTimeMillisEpoch(),
         attemptData.getRunDurationMillis(),
-        /* files= */ ImmutableList.of(),
+        /* files= */ ImmutableMultimap.of(),
         attemptData.getWarningList(),
         /* lastAttempt= */ true);
   }
@@ -164,7 +165,7 @@ public Artifact getTestStatusArtifact() {
   }
 
   @VisibleForTesting
-  public Collection<Pair<String, Path>> getFiles() {
+  public ImmutableMultimap<String, Path> getFiles() {
     return files;
   }
 
@@ -227,12 +228,12 @@ public ImmutableList<LocalFile> referencedLocalFiles() {
             ? LocalFileType.SUCCESSFUL_TEST_OUTPUT
             : LocalFileType.FAILED_TEST_OUTPUT;
     ImmutableList.Builder<LocalFile> localFiles = ImmutableList.builder();
-    for (Pair<String, Path> file : files) {
-      if (file.getSecond() != null) {
+    for (Map.Entry<String, Path> file : files.entries()) {
+      if (file.getValue() != null) {
         // TODO(b/199940216): Can we populate metadata for these files?
         localFiles.add(
             new LocalFile(
-                file.getSecond(),
+                file.getValue(),
                 localFileType,
                 /* artifact= */ null,
                 /* artifactMetadata= */ null));
@@ -264,11 +265,11 @@ public BuildEventStreamProtos.TestResult asTestResult(BuildEventContext converte
     }
     builder.setTestAttemptDurationMillis(durationMillis);
     builder.addAllWarning(testWarnings);
-    for (Pair<String, Path> file : files) {
-      String uri = pathConverter.apply(file.getSecond());
+    for (Map.Entry<String, Path> file : files.entries()) {
+      String uri = pathConverter.apply(file.getValue());
       if (uri != null) {
         builder.addTestActionOutput(
-            BuildEventStreamProtos.File.newBuilder().setName(file.getFirst()).setUri(uri).build());
+            BuildEventStreamProtos.File.newBuilder().setName(file.getKey()).setUri(uri).build());
       }
     }
     return builder.build();

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

@@ -18,6 +18,7 @@
 
 import com.google.common.base.Preconditions;
 import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableMultimap;
 import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.actions.ArtifactPathResolver;
 import com.google.devtools.build.lib.buildeventstream.BuildEventStreamProtos;
@@ -26,12 +27,10 @@
 import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe;
 import com.google.devtools.build.lib.events.ExtendedEventHandler;
 import com.google.devtools.build.lib.util.DetailedExitCode;
-import com.google.devtools.build.lib.util.Pair;
 import com.google.devtools.build.lib.vfs.FileSystem;
 import com.google.devtools.build.lib.vfs.Path;
 import com.google.devtools.build.lib.view.test.TestStatus.BlazeTestStatus;
 import com.google.devtools.build.lib.view.test.TestStatus.TestResultData;
-import java.util.Collection;
 import java.util.List;
 import javax.annotation.Nullable;
 
@@ -183,7 +182,7 @@ public DetailedExitCode getSystemFailure() {
    * Returns the collection of files created by the test, tagged by their name indicating usage
    * (e.g., "test.log").
    */
-  private Collection<Pair<String, Path>> getFiles() {
+  private ImmutableMultimap<String, Path> getFiles() {
     // TODO(ulfjack): Cache the set of generated files in the TestResultData.
     return testAction.getTestOutputsMapping(ArtifactPathResolver.forExecRoot(execRoot), execRoot);
   }

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

@@ -22,8 +22,8 @@
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Preconditions;
 import com.google.common.base.Strings;
-import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.ImmutableMultimap;
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Iterators;
 import com.google.common.flogger.GoogleLogger;
@@ -65,7 +65,6 @@
 import com.google.devtools.build.lib.server.FailureDetails.TestAction;
 import com.google.devtools.build.lib.util.DetailedExitCode;
 import com.google.devtools.build.lib.util.Fingerprint;
-import com.google.devtools.build.lib.util.Pair;
 import com.google.devtools.build.lib.vfs.FileSystemUtils;
 import com.google.devtools.build.lib.vfs.Path;
 import com.google.devtools.build.lib.vfs.PathFragment;
@@ -401,73 +400,64 @@ public List<ActionInput> getSpawnOutputs() {
    * 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(
+  public ImmutableMultimap<String, Path> getTestOutputsMapping(
       ArtifactPathResolver resolver, Path execRoot) {
-    ImmutableList.Builder<Pair<String, Path>> builder = ImmutableList.builder();
+    ImmutableMultimap.Builder<String, Path> builder = ImmutableMultimap.builder();
     if (resolver.toPath(getTestLog()).exists()) {
-      builder.add(Pair.of(TestFileNameConstants.TEST_LOG, resolver.toPath(getTestLog())));
+      builder.put(TestFileNameConstants.TEST_LOG, resolver.toPath(getTestLog()));
     }
     if (getCoverageData() != null && resolver.toPath(getCoverageData()).exists()) {
-      builder.add(Pair.of(TestFileNameConstants.TEST_COVERAGE, resolver.toPath(getCoverageData())));
+      builder.put(TestFileNameConstants.TEST_COVERAGE, resolver.toPath(getCoverageData()));
     }
     if (execRoot != null) {
       ResolvedPaths resolvedPaths = resolve(execRoot);
       if (resolvedPaths.getTestStderr().exists()) {
-        builder.add(Pair.of(TestFileNameConstants.TEST_STDERR, resolvedPaths.getTestStderr()));
+        builder.put(TestFileNameConstants.TEST_STDERR, resolvedPaths.getTestStderr());
       }
       if (resolvedPaths.getXmlOutputPath().exists()) {
-        builder.add(Pair.of(TestFileNameConstants.TEST_XML, resolvedPaths.getXmlOutputPath()));
+        builder.put(TestFileNameConstants.TEST_XML, resolvedPaths.getXmlOutputPath());
       }
       if (resolvedPaths.getSplitLogsPath().exists()) {
-        builder.add(Pair.of(TestFileNameConstants.SPLIT_LOGS, resolvedPaths.getSplitLogsPath()));
+        builder.put(TestFileNameConstants.SPLIT_LOGS, resolvedPaths.getSplitLogsPath());
       }
       if (resolvedPaths.getTestWarningsPath().exists()) {
-        builder.add(
-            Pair.of(TestFileNameConstants.TEST_WARNINGS, resolvedPaths.getTestWarningsPath()));
+        builder.put(TestFileNameConstants.TEST_WARNINGS, resolvedPaths.getTestWarningsPath());
       }
       if (testConfiguration.getZipUndeclaredTestOutputs()
           && resolvedPaths.getUndeclaredOutputsZipPath().exists()) {
-        builder.add(
-            Pair.of(
-                TestFileNameConstants.UNDECLARED_OUTPUTS_ZIP,
-                resolvedPaths.getUndeclaredOutputsZipPath()));
+        builder.put(
+            TestFileNameConstants.UNDECLARED_OUTPUTS_ZIP,
+            resolvedPaths.getUndeclaredOutputsZipPath());
       }
       if (!testConfiguration.getZipUndeclaredTestOutputs()
           && resolvedPaths.getUndeclaredOutputsDir().exists()) {
-        builder.add(
-            Pair.of(
-                TestFileNameConstants.UNDECLARED_OUTPUTS_DIR,
-                resolvedPaths.getUndeclaredOutputsDir()));
+
+        builder.put(
+            TestFileNameConstants.UNDECLARED_OUTPUTS_DIR, resolvedPaths.getUndeclaredOutputsDir());
       }
       if (resolvedPaths.getUndeclaredOutputsManifestPath().exists()) {
-        builder.add(
-            Pair.of(
-                TestFileNameConstants.UNDECLARED_OUTPUTS_MANIFEST,
-                resolvedPaths.getUndeclaredOutputsManifestPath()));
+        builder.put(
+            TestFileNameConstants.UNDECLARED_OUTPUTS_MANIFEST,
+            resolvedPaths.getUndeclaredOutputsManifestPath());
       }
       if (resolvedPaths.getUndeclaredOutputsAnnotationsPath().exists()) {
-        builder.add(
-            Pair.of(
-                TestFileNameConstants.UNDECLARED_OUTPUTS_ANNOTATIONS,
-                resolvedPaths.getUndeclaredOutputsAnnotationsPath()));
+        builder.put(
+            TestFileNameConstants.UNDECLARED_OUTPUTS_ANNOTATIONS,
+            resolvedPaths.getUndeclaredOutputsAnnotationsPath());
       }
       if (resolvedPaths.getUndeclaredOutputsAnnotationsPbPath().exists()) {
-        builder.add(
-            Pair.of(
-                TestFileNameConstants.UNDECLARED_OUTPUTS_ANNOTATIONS_PB,
-                resolvedPaths.getUndeclaredOutputsAnnotationsPbPath()));
+        builder.put(
+            TestFileNameConstants.UNDECLARED_OUTPUTS_ANNOTATIONS_PB,
+            resolvedPaths.getUndeclaredOutputsAnnotationsPbPath());
       }
       if (resolvedPaths.getUnusedRunfilesLogPath().exists()) {
-        builder.add(
-            Pair.of(
-                TestFileNameConstants.UNUSED_RUNFILES_LOG,
-                resolvedPaths.getUnusedRunfilesLogPath()));
+        builder.put(
+            TestFileNameConstants.UNUSED_RUNFILES_LOG, resolvedPaths.getUnusedRunfilesLogPath());
       }
       if (resolvedPaths.getInfrastructureFailureFile().exists()) {
-        builder.add(
-            Pair.of(
-                TestFileNameConstants.TEST_INFRASTRUCTURE_FAILURE,
-                resolvedPaths.getInfrastructureFailureFile()));
+        builder.put(
+            TestFileNameConstants.TEST_INFRASTRUCTURE_FAILURE,
+            resolvedPaths.getInfrastructureFailureFile());
       }
     }
     return builder.build();

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

@@ -19,6 +19,7 @@
 import com.google.common.base.Verify;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.ImmutableMultimap;
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Maps;
 import com.google.common.io.ByteStreams;
@@ -51,7 +52,6 @@
 import com.google.devtools.build.lib.server.FailureDetails.Execution.Code;
 import com.google.devtools.build.lib.server.FailureDetails.FailureDetail;
 import com.google.devtools.build.lib.server.FailureDetails.TestAction;
-import com.google.devtools.build.lib.util.Pair;
 import com.google.devtools.build.lib.util.io.FileOutErr;
 import com.google.devtools.build.lib.vfs.FileStatus;
 import com.google.devtools.build.lib.vfs.FileSystemUtils;
@@ -146,10 +146,10 @@ public TestRunnerSpawn createTestRunnerSpawn(
     return new StandaloneTestRunnerSpawn(action, actionExecutionContext, spawn, tmpDir, execRoot);
   }
 
-  private static ImmutableList<Pair<String, Path>> renameOutputs(
+  private static ImmutableMultimap<String, Path> renameOutputs(
       ActionExecutionContext actionExecutionContext,
       TestRunnerAction action,
-      ImmutableList<Pair<String, Path>> testOutputs,
+      ImmutableMultimap<String, Path> testOutputs,
       int attemptId)
       throws IOException {
     // Rename outputs
@@ -163,12 +163,12 @@ private static ImmutableList<Pair<String, Path>> renameOutputs(
 
     // Get the normal test output paths, and then update them to use "attempt_N" names, and
     // attemptDir, before adding them to the outputs.
-    ImmutableList.Builder<Pair<String, Path>> testOutputsBuilder = new ImmutableList.Builder<>();
-    for (Pair<String, Path> testOutput : testOutputs) {
+    ImmutableMultimap.Builder<String, Path> testOutputsBuilder = ImmutableMultimap.builder();
+    for (Map.Entry<String, Path> testOutput : testOutputs.entries()) {
       // e.g. /testRoot/test.dir/file, an example we follow throughout this loop's comments.
-      Path testOutputPath = testOutput.getSecond();
+      Path testOutputPath = testOutput.getValue();
       Path destinationPath;
-      if (testOutput.getFirst().equals(TestFileNameConstants.TEST_LOG)) {
+      if (testOutput.getKey().equals(TestFileNameConstants.TEST_LOG)) {
         // The rename rules for the test log are different than for all the other files.
         destinationPath = testLog;
       } else {
@@ -188,7 +188,7 @@ private static ImmutableList<Pair<String, Path>> renameOutputs(
       // Move to the destination.
       testOutputPath.renameTo(destinationPath);
 
-      testOutputsBuilder.add(Pair.of(testOutput.getFirst(), destinationPath));
+      testOutputsBuilder.put(testOutput.getKey(), destinationPath);
     }
     return testOutputsBuilder.build();
   }
@@ -240,7 +240,7 @@ private StandaloneFailedAttemptResult processTestAttempt(
       TestRunnerAction action,
       StandaloneTestResult result)
       throws IOException {
-    ImmutableList<Pair<String, Path>> testOutputs =
+    ImmutableMultimap<String, Path> testOutputs =
         action.getTestOutputsMapping(
             actionExecutionContext.getPathResolver(), actionExecutionContext.getExecRoot());
     if (!isLastAttempt) {
@@ -249,10 +249,10 @@ private StandaloneFailedAttemptResult processTestAttempt(
 
     // Recover the test log path, which may have been renamed, and add it to the data builder.
     Path renamedTestLog = null;
-    for (Pair<String, Path> pair : testOutputs) {
-      if (TestFileNameConstants.TEST_LOG.equals(pair.getFirst())) {
+    for (Map.Entry<String, Path> pair : testOutputs.entries()) {
+      if (TestFileNameConstants.TEST_LOG.equals(pair.getKey())) {
         Preconditions.checkState(renamedTestLog == null, "multiple test_log matches");
-        renamedTestLog = pair.getSecond();
+        renamedTestLog = pair.getValue();
       }
     }
 

src/test/java/com/google/devtools/build/lib/runtime/TestResultAggregatorTest.java

@@ -20,6 +20,7 @@
 import static org.mockito.Mockito.when;
 
 import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableMultimap;
 import com.google.common.eventbus.EventBus;
 import com.google.devtools.build.lib.actions.Artifact.DerivedArtifact;
 import com.google.devtools.build.lib.actions.ArtifactRoot;
@@ -264,13 +265,13 @@ private TestResultAggregator createAggregatorWithTestRuns(int testRuns) {
 
   private static TestResult testResult(TestResultData.Builder data, boolean locallyCached) {
     TestRunnerAction mockTestAction = mock(TestRunnerAction.class);
-    when(mockTestAction.getTestOutputsMapping(any(), any())).thenReturn(ImmutableList.of());
+    when(mockTestAction.getTestOutputsMapping(any(), any())).thenReturn(ImmutableMultimap.of());
     return new TestResult(mockTestAction, data.build(), locallyCached, /*systemFailure=*/ null);
   }
 
   private static TestResult shardedTestResult(TestResultData.Builder data, int shardNum) {
     TestRunnerAction mockTestAction = mock(TestRunnerAction.class);
-    when(mockTestAction.getTestOutputsMapping(any(), any())).thenReturn(ImmutableList.of());
+    when(mockTestAction.getTestOutputsMapping(any(), any())).thenReturn(ImmutableMultimap.of());
     when(mockTestAction.getShardNum()).thenReturn(shardNum);
     return new TestResult(mockTestAction, data.build(), /*cached=*/ false, /*systemFailure=*/ null);
   }

src/test/java/com/google/devtools/build/lib/skyframe/rewinding/LostImportantOutputHandlerModule.java

@@ -39,7 +39,7 @@
 import com.google.devtools.build.lib.vfs.PathFragment;
 import com.google.errorprone.annotations.ForOverride;
 import java.io.IOException;
-import java.util.List;
+import java.util.Collection;
 import java.util.Map;
 import java.util.Set;
 import java.util.function.Function;
@@ -115,7 +115,7 @@ public LostArtifacts processRunfilesAndGetLostArtifacts(
     }
 
     @Override
-    public void processTestOutputs(List<Path> testOutputs) {
+    public void processTestOutputs(Collection<Path> testOutputs) {
       throw new UnsupportedOperationException();
     }