Fix additional outputs in BEP when remote_download_minimal is used

.bazelrc

@@ -35,4 +35,4 @@ build --java_language_version=11
 build --tool_java_language_version=11
 
 # User-specific .bazelrc
-try-import user.bazelrc
+try-import %workspace%/user.bazelrc

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

@@ -17,6 +17,7 @@
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Preconditions;
 import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableMap;
 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;
@@ -31,6 +32,7 @@
 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.PathMetadata;
 import com.google.devtools.build.lib.view.test.TestStatus.TestResultData;
 import com.google.protobuf.util.Durations;
 import com.google.protobuf.util.Timestamps;
@@ -48,6 +50,7 @@ public class TestAttempt implements BuildEventWithOrderConstraint {
   private final boolean lastAttempt;
   private final Collection<Pair<String, Path>> files;
   private final List<String> testWarnings;
+  private final ImmutableMap<String, PathMetadata> fileNameToMetadata;
   private final long durationMillis;
   private final long startTimeMillis;
   private final BuildEventStreamProtos.TestResult.ExecutionInfo executionInfo;
@@ -69,6 +72,7 @@ private TestAttempt(
       long startTimeMillis,
       long durationMillis,
       Collection<Pair<String, Path>> files,
+      ImmutableMap<String, PathMetadata> fileNameToMetadata,
       List<String> testWarnings,
       boolean lastAttempt) {
     this.testAction = testAction;
@@ -80,6 +84,7 @@ private TestAttempt(
     this.startTimeMillis = startTimeMillis;
     this.durationMillis = durationMillis;
     this.files = Preconditions.checkNotNull(files);
+    this.fileNameToMetadata = Preconditions.checkNotNull(fileNameToMetadata);
     this.testWarnings = Preconditions.checkNotNull(testWarnings);
     this.lastAttempt = lastAttempt;
   }
@@ -105,6 +110,7 @@ public static TestAttempt forExecutedTestResult(
         attemptData.getStartTimeMillisEpoch(),
         attemptData.getRunDurationMillis(),
         files,
+        attemptData.getAttemptsList().size() > 0 ? ImmutableMap.copyOf(attemptData.getAttempts(0).getPathMetadataMap()) : ImmutableMap.of(),
         attemptData.getWarningList(),
         lastAttempt);
   }
@@ -126,6 +132,7 @@ public static TestAttempt fromCachedTestResult(
         attemptData.getStartTimeMillisEpoch(),
         attemptData.getRunDurationMillis(),
         files,
+        attemptData.getAttemptsList().size() > 0 ? ImmutableMap.copyOf(attemptData.getAttempts(0).getPathMetadataMap()) : ImmutableMap.of(),
         attemptData.getWarningList(),
         lastAttempt);
   }
@@ -230,10 +237,19 @@ public BuildEventStreamProtos.TestResult asTestResult(BuildEventContext converte
     builder.setTestAttemptDurationMillis(durationMillis);
     builder.addAllWarning(testWarnings);
     for (Pair<String, Path> file : files) {
-      String uri = pathConverter.apply(file.getSecond());
-      if (uri != null) {
-        builder.addTestActionOutput(
+      if (!fileNameToMetadata.containsKey(file.getFirst())) {
+        String uri = pathConverter.apply(file.getSecond());
+        if (uri != null) {
+          builder.addTestActionOutput(
+              BuildEventStreamProtos.File.newBuilder().setName(file.getFirst()).setUri(uri).build());
+        }
+      } else {
+        PathMetadata metadata = fileNameToMetadata.get(file.getFirst());
+        if (metadata != null) {
+          String uri = pathConverter.applyForDigest(metadata.getHash(), metadata.getSizeBytes());
+          builder.addTestActionOutput(
             BuildEventStreamProtos.File.newBuilder().setName(file.getFirst()).setUri(uri).build());
+        }
       }
     }
     return builder.build();

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

@@ -30,6 +30,8 @@
 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.File;
+import com.google.devtools.build.lib.view.test.TestStatus.PathMetadata;
 import com.google.devtools.build.lib.view.test.TestStatus.TestResultData;
 import java.util.Collection;
 import java.util.List;
@@ -183,7 +185,6 @@ public DetailedExitCode getSystemFailure() {
    * (e.g., "test.log").
    */
   private Collection<Pair<String, Path>> getFiles() {
-    // TODO(ulfjack): Cache the set of generated files in the TestResultData.
-    return testAction.getTestOutputsMapping(ArtifactPathResolver.forExecRoot(execRoot), execRoot);
+    return testAction.testResultDataToMapping(execRoot, data);
   }
 }

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

@@ -45,6 +45,7 @@
 import com.google.devtools.build.lib.actions.CommandLineExpansionException;
 import com.google.devtools.build.lib.actions.EnvironmentalExecException;
 import com.google.devtools.build.lib.actions.ExecException;
+import com.google.devtools.build.lib.actions.FileArtifactValue;
 import com.google.devtools.build.lib.actions.NotifyOnActionCacheHit;
 import com.google.devtools.build.lib.actions.RunfilesSupplier;
 import com.google.devtools.build.lib.actions.SpawnExecutedEvent;
@@ -74,6 +75,8 @@
 import com.google.devtools.build.lib.vfs.FileSystemUtils;
 import com.google.devtools.build.lib.vfs.Path;
 import com.google.devtools.build.lib.vfs.PathFragment;
+import com.google.devtools.build.lib.view.test.TestStatus.File;
+import com.google.devtools.build.lib.view.test.TestStatus.TestAttempt;
 import com.google.devtools.build.lib.view.test.TestStatus.TestResultData;
 import com.google.devtools.common.options.TriState;
 import com.google.protobuf.ExtensionRegistry;
@@ -92,6 +95,7 @@
 import java.util.function.Supplier;
 import javax.annotation.Nullable;
 
+
 /**
  * An Action representing a test with the associated environment (runfiles, environment variables,
  * test result, etc). It consumes test executable and runfiles artifacts and produces test result
@@ -366,78 +370,122 @@ public List<ActionInput> getSpawnOutputs() {
     return outputs;
   }
 
+  public static final class TestOutputInfo {
+    private final String key;
+    private final Path path;
+    private final FileArtifactValue fileArtifactValue;
+
+    public TestOutputInfo(String key, Path path, FileArtifactValue fileArtifactValue) {
+      this.key = key;
+      this.path = path;
+      this.fileArtifactValue = fileArtifactValue;
+    }
+
+    public String getKey() {
+      return key;
+    }
+
+    public Path getPath() {
+      return path;
+    }
+
+    public FileArtifactValue getFileArtifactValue() {
+      return fileArtifactValue;
+    }
+
+  }
+
   /**
    * 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(
-      ArtifactPathResolver resolver, Path execRoot) {
-    ImmutableList.Builder<Pair<String, Path>> builder = ImmutableList.builder();
+  public ImmutableList<TestOutputInfo> getTestOutputsMapping(
+      ArtifactPathResolver resolver, Path execRoot,
+      ImmutableMap<Artifact, FileArtifactValue> injectedOutputs) {
+    ImmutableList.Builder<TestOutputInfo> builder = ImmutableList.builder();
     if (resolver.toPath(getTestLog()).exists()) {
-      builder.add(Pair.of(TestFileNameConstants.TEST_LOG, resolver.toPath(getTestLog())));
+      builder.add(new TestOutputInfo(TestFileNameConstants.TEST_LOG, resolver.toPath(getTestLog()), null));
     }
     if (getCoverageData() != null && resolver.toPath(getCoverageData()).exists()) {
-      builder.add(Pair.of(TestFileNameConstants.TEST_COVERAGE, resolver.toPath(getCoverageData())));
+      builder.add(new TestOutputInfo(TestFileNameConstants.TEST_COVERAGE, resolver.toPath(getCoverageData()), null));
     }
+
     if (execRoot != null) {
       ResolvedPaths resolvedPaths = resolve(execRoot);
+      for (Map.Entry<Artifact, FileArtifactValue> entry : injectedOutputs.entrySet()) {
+        Path artifactPath = resolver.toPath(entry.getKey());
+        if (artifactPath.equals(resolvedPaths.getTestStderr())) {
+          builder.add(new TestOutputInfo(TestFileNameConstants.TEST_STDERR, artifactPath, entry.getValue()));
+        } else if (artifactPath.equals(resolvedPaths.getXmlOutputPath())) {
+          builder.add(new TestOutputInfo(TestFileNameConstants.TEST_XML, artifactPath, entry.getValue()));
+        } else if (artifactPath.equals(resolvedPaths.getSplitLogsPath())) {
+          builder.add(new TestOutputInfo(TestFileNameConstants.SPLIT_LOGS, artifactPath, entry.getValue()));
+        } else if (artifactPath.equals(resolvedPaths.getTestWarningsPath())) {
+          builder.add(new TestOutputInfo(TestFileNameConstants.TEST_WARNINGS, artifactPath, entry.getValue()));
+        } else if (artifactPath.equals(resolvedPaths.getUndeclaredOutputsZipPath())) {
+          builder.add(new TestOutputInfo(TestFileNameConstants.UNDECLARED_OUTPUTS_ZIP, artifactPath, entry.getValue()));
+        } else if (artifactPath.equals(resolvedPaths.getUndeclaredOutputsManifestPath())) {
+          builder.add(new TestOutputInfo(TestFileNameConstants.UNDECLARED_OUTPUTS_MANIFEST, artifactPath, entry.getValue()));
+        } else if (artifactPath.equals(resolvedPaths.getUndeclaredOutputsAnnotationsPath())) {
+          builder.add(new TestOutputInfo(TestFileNameConstants.UNDECLARED_OUTPUTS_ANNOTATIONS, artifactPath, entry.getValue()));
+        } else if (artifactPath.equals(resolvedPaths.getUndeclaredOutputsAnnotationsPbPath())) {
+          builder.add(new TestOutputInfo(TestFileNameConstants.UNDECLARED_OUTPUTS_ANNOTATIONS_PB, artifactPath, entry.getValue()));
+        } else if (artifactPath.equals(resolvedPaths.getUnusedRunfilesLogPath())) {
+          builder.add(new TestOutputInfo(TestFileNameConstants.UNUSED_RUNFILES_LOG, artifactPath, entry.getValue()));
+        } else if (artifactPath.equals(resolvedPaths.getInfrastructureFailureFile())) {
+          builder.add(new TestOutputInfo(TestFileNameConstants.TEST_INFRASTRUCTURE_FAILURE, artifactPath, entry.getValue()));
+        }
+      }
       if (resolvedPaths.getTestStderr().exists()) {
-        builder.add(Pair.of(TestFileNameConstants.TEST_STDERR, resolvedPaths.getTestStderr()));
+        builder.add(new TestOutputInfo(TestFileNameConstants.TEST_STDERR, resolvedPaths.getTestStderr(), null));
       }
       if (resolvedPaths.getXmlOutputPath().exists()) {
-        builder.add(Pair.of(TestFileNameConstants.TEST_XML, resolvedPaths.getXmlOutputPath()));
+        builder.add(new TestOutputInfo(TestFileNameConstants.TEST_XML, resolvedPaths.getXmlOutputPath(), null));
       }
       if (resolvedPaths.getSplitLogsPath().exists()) {
-        builder.add(Pair.of(TestFileNameConstants.SPLIT_LOGS, resolvedPaths.getSplitLogsPath()));
+        builder.add(new TestOutputInfo(TestFileNameConstants.SPLIT_LOGS, resolvedPaths.getSplitLogsPath(), null));
       }
       if (resolvedPaths.getTestWarningsPath().exists()) {
-        builder.add(
-            Pair.of(TestFileNameConstants.TEST_WARNINGS, resolvedPaths.getTestWarningsPath()));
+        builder.add(new TestOutputInfo(TestFileNameConstants.TEST_WARNINGS, resolvedPaths.getTestWarningsPath(), null));
       }
       if (testConfiguration.getZipUndeclaredTestOutputs()
           && resolvedPaths.getUndeclaredOutputsZipPath().exists()) {
-        builder.add(
-            Pair.of(
-                TestFileNameConstants.UNDECLARED_OUTPUTS_ZIP,
-                resolvedPaths.getUndeclaredOutputsZipPath()));
+        builder.add(new TestOutputInfo(TestFileNameConstants.UNDECLARED_OUTPUTS_ZIP, resolvedPaths.getUndeclaredOutputsZipPath(), null));
       }
       if (!testConfiguration.getZipUndeclaredTestOutputs()
           && resolvedPaths.getUndeclaredOutputsDir().exists()) {
-        builder.add(
-            Pair.of(
-                TestFileNameConstants.UNDECLARED_OUTPUTS_DIR,
-                resolvedPaths.getUndeclaredOutputsDir()));
+        builder.add(new TestOutputInfo(TestFileNameConstants.UNDECLARED_OUTPUTS_DIR, resolvedPaths.getUndeclaredOutputsDir(), null));
       }
       if (resolvedPaths.getUndeclaredOutputsManifestPath().exists()) {
-        builder.add(
-            Pair.of(
-                TestFileNameConstants.UNDECLARED_OUTPUTS_MANIFEST,
-                resolvedPaths.getUndeclaredOutputsManifestPath()));
+        builder.add(new TestOutputInfo(TestFileNameConstants.UNDECLARED_OUTPUTS_MANIFEST, resolvedPaths.getUndeclaredOutputsManifestPath(), null));
       }
       if (resolvedPaths.getUndeclaredOutputsAnnotationsPath().exists()) {
-        builder.add(
-            Pair.of(
-                TestFileNameConstants.UNDECLARED_OUTPUTS_ANNOTATIONS,
-                resolvedPaths.getUndeclaredOutputsAnnotationsPath()));
+        builder.add(new TestOutputInfo(TestFileNameConstants.UNDECLARED_OUTPUTS_ANNOTATIONS, resolvedPaths.getUndeclaredOutputsAnnotationsPath(), null));
       }
       if (resolvedPaths.getUndeclaredOutputsAnnotationsPbPath().exists()) {
-        builder.add(
-            Pair.of(
-                TestFileNameConstants.UNDECLARED_OUTPUTS_ANNOTATIONS_PB,
-                resolvedPaths.getUndeclaredOutputsAnnotationsPbPath()));
+        builder.add(new TestOutputInfo(TestFileNameConstants.UNDECLARED_OUTPUTS_ANNOTATIONS_PB, resolvedPaths.getUndeclaredOutputsAnnotationsPbPath(), null));
       }
       if (resolvedPaths.getUnusedRunfilesLogPath().exists()) {
-        builder.add(
-            Pair.of(
-                TestFileNameConstants.UNUSED_RUNFILES_LOG,
-                resolvedPaths.getUnusedRunfilesLogPath()));
+        builder.add(new TestOutputInfo(TestFileNameConstants.UNUSED_RUNFILES_LOG, resolvedPaths.getUnusedRunfilesLogPath(), null));
       }
       if (resolvedPaths.getInfrastructureFailureFile().exists()) {
-        builder.add(
-            Pair.of(
-                TestFileNameConstants.TEST_INFRASTRUCTURE_FAILURE,
-                resolvedPaths.getInfrastructureFailureFile()));
+        builder.add(new TestOutputInfo(TestFileNameConstants.TEST_INFRASTRUCTURE_FAILURE, resolvedPaths.getInfrastructureFailureFile(), null));
+      }
+    }
+    return builder.build();
+  }
+
+  public static ImmutableList<Pair<String, Path>> testResultDataToMapping(Path execRoot, TestResultData data) {
+    ImmutableList.Builder<Pair<String, Path>> builder = ImmutableList.builder();
+    List<TestAttempt> attempts = data.getAttemptsList();
+    if (attempts.size() > 0) {
+      for (File file : attempts.get(attempts.size()-1).getOutputsList()) {
+        Path path = null;
+        if (file.getPath() != null) {
+          path = execRoot.getRelative(file.getPath());
+        }
+        builder.add(new Pair<String, Path>(file.getName(), path));
       }
     }
     return builder.build();

src/main/java/com/google/devtools/build/lib/buildeventstream/LocalFilesArtifactUploader.java

@@ -72,5 +72,10 @@ public String apply(Path path) {
       }
       return FILE_URI_PATH_CONVERTER.apply(path);
     }
+
+    @Override
+    public String applyForDigest(String hash, long sizeBytes) {
+      throw new IllegalStateException("Can't formulate file:// path for a remote artifact");
+    }
   }
 }

src/main/java/com/google/devtools/build/lib/buildeventstream/PathConverter.java

@@ -25,14 +25,23 @@
  */
 public interface PathConverter {
   /** An implementation that throws on every call to {@link #apply(Path)}. */
-  PathConverter NO_CONVERSION =
-      path -> {
-        throw new IllegalStateException(
-            String.format(
-                "Can't convert '%s', as it has not been"
-                    + "declared as a referenced artifact of a build event",
-                path.getPathString()));
-      };
+  PathConverter NO_CONVERSION = new InvalidConverter();
+
+  final class InvalidConverter implements PathConverter {
+    @Override
+    public String apply(Path path) {
+      throw new IllegalStateException(
+        String.format(
+            "Can't convert '%s', as it has not been"
+                + "declared as a referenced artifact of a build event",
+            path.getPathString()));
+    }
+
+    @Override
+    public String applyForDigest(String hash, long sizeBytes) {
+      throw new IllegalStateException("Can't convert remote digest");
+    }
+  }
 
   /** A {@link PathConverter} that returns a path formatted as a URI with a {@code file} scheme. */
   // TODO(ulfjack): Make this a static final field.
@@ -43,6 +52,11 @@ public String apply(Path path) {
       return pathToUriString(path.getPathString());
     }
 
+    @Override
+    public String applyForDigest(String hash, long sizeBytes) {
+      throw new IllegalStateException("Can't formulate file:// path for a remote artifact");
+    }
+
     /**
      * Returns the path encoded as an {@link URI}.
      *
@@ -88,4 +102,6 @@ static String pathToUriString(String path) {
    */
   @Nullable
   String apply(Path path);
+
+  String applyForDigest(String hash, long sizeBytes);
 }