Address comments

src/main/java/com/google/devtools/build/lib/remote/BUILD

@@ -202,9 +202,12 @@ java_library(
         "//src/main/java/com/google/devtools/build/lib/actions:artifacts",
         "//src/main/java/com/google/devtools/build/lib/actions:file_metadata",
         "//src/main/java/com/google/devtools/build/lib/analysis:analysis_cluster",
+        "//src/main/java/com/google/devtools/build/lib/analysis:configured_target",
+        "//src/main/java/com/google/devtools/build/lib/analysis:configured_target_value",
         "//src/main/java/com/google/devtools/build/lib/remote/util",
         "//src/main/java/com/google/devtools/build/lib/skyframe:action_execution_value",
         "//src/main/java/com/google/devtools/build/lib/skyframe:tree_artifact_value",
+        "//src/main/java/com/google/devtools/build/lib/skyframe:configured_target_key",
         "//src/main/java/com/google/devtools/build/lib/util",
         "//src/main/java/com/google/devtools/build/lib/vfs",
         "//src/main/java/com/google/devtools/build/skyframe",

src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java

@@ -1010,8 +1010,11 @@ public InMemoryOutput downloadOutputs(RemoteAction action, RemoteActionResult re
     ImmutableList.Builder<ListenableFuture<FileMetadata>> downloadsBuilder =
         ImmutableList.builder();
     RemoteOutputsMode remoteOutputsMode = remoteOptions.remoteOutputsMode;
-    boolean downloadOutputs = shouldDownloadAllSpawnOutputs(remoteOutputsMode,
-        result.getExitCode());
+    boolean downloadOutputs = remoteOutputsMode.downloadAllOutputs()
+        ||
+        // In case the action failed, download all outputs. It might be helpful for debugging
+        // and there is no point in injecting output metadata of a failed action.
+        result.getExitCode() != 0;
 
     // Download into temporary paths, then move everything at the end.
     // This avoids holding the output lock while downloading, which would prevent the local branch

src/main/java/com/google/devtools/build/lib/remote/RemoteModule.java

@@ -931,7 +931,7 @@ public void executorInit(CommandEnvironment env, BuildRequest request, ExecutorB
 
       if (remoteOutputsMode.downloadToplevelOutputsOnly()) {
         toplevelArtifactsDownloader = new ToplevelArtifactsDownloader(
-            env.getCommand(),
+            env.getCommandName(),
             env.getSkyframeExecutor()
             .getEvaluator(), actionInputFetcher, (path) -> {
           FileSystem fileSystem = path.getFileSystem();

src/main/java/com/google/devtools/build/lib/remote/ToplevelArtifactsDownloader.java

@@ -28,14 +28,17 @@
 import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.actions.FileArtifactValue;
 import com.google.devtools.build.lib.analysis.AspectCompleteEvent;
+import com.google.devtools.build.lib.analysis.ConfiguredTarget;
+import com.google.devtools.build.lib.analysis.ConfiguredTargetValue;
 import com.google.devtools.build.lib.analysis.Runfiles;
 import com.google.devtools.build.lib.analysis.TargetCompleteEvent;
 import com.google.devtools.build.lib.analysis.TopLevelArtifactHelper.ArtifactsInOutputGroup;
+import com.google.devtools.build.lib.analysis.configuredtargets.RuleConfiguredTarget;
 import com.google.devtools.build.lib.analysis.test.TestAttempt;
 import com.google.devtools.build.lib.remote.AbstractActionInputPrefetcher.Priority;
 import com.google.devtools.build.lib.remote.util.StaticMetadataProvider;
-import com.google.devtools.build.lib.runtime.Command;
 import com.google.devtools.build.lib.skyframe.ActionExecutionValue;
+import com.google.devtools.build.lib.skyframe.ConfiguredTargetKey;
 import com.google.devtools.build.lib.skyframe.TreeArtifactValue;
 import com.google.devtools.build.lib.util.Pair;
 import com.google.devtools.build.lib.vfs.Path;
@@ -53,14 +56,6 @@ private enum CommandMode {
     BUILD,
     TEST,
     RUN;
-
-    boolean shouldDownloadTestOutputs() {
-      return this == TEST;
-    }
-
-    boolean shouldDownloadToplevelOutputs() {
-      return this == BUILD || this == RUN;
-    }
   }
 
   private static final GoogleLogger logger = GoogleLogger.forEnclosingClass();
@@ -71,11 +66,11 @@ boolean shouldDownloadToplevelOutputs() {
   private final PathToMetadataConverter pathToMetadataConverter;
 
   public ToplevelArtifactsDownloader(
-      Command command,
+      String commandName,
       MemoizingEvaluator memoizingEvaluator,
       AbstractActionInputPrefetcher actionInputPrefetcher,
       PathToMetadataConverter pathToMetadataConverter) {
-    switch (command.name()) {
+    switch (commandName) {
       case "build":
         this.commandMode = CommandMode.BUILD;
         break;
@@ -108,10 +103,6 @@ public interface PathToMetadataConverter {
   @Subscribe
   @AllowConcurrentEvents
   public void onTestAttempt(TestAttempt event) {
-    if (!commandMode.shouldDownloadTestOutputs()) {
-      return;
-    }
-
     for (Pair<String, Path> pair : event.getFiles()) {
       Path path = checkNotNull(pair.getSecond());
       // Since the event is fired within action execution, the skyframe doesn't know the outputs of
@@ -141,10 +132,6 @@ public void onFailure(Throwable throwable) {
   @Subscribe
   @AllowConcurrentEvents
   public void onAspectComplete(AspectCompleteEvent event) {
-    if (!commandMode.shouldDownloadToplevelOutputs()) {
-      return;
-    }
-
     if (event.failed()) {
       return;
     }
@@ -155,7 +142,7 @@ public void onAspectComplete(AspectCompleteEvent event) {
   @Subscribe
   @AllowConcurrentEvents
   public void onTargetComplete(TargetCompleteEvent event) {
-    if (!commandMode.shouldDownloadToplevelOutputs()) {
+    if (!shouldDownloadToplevelOutputsForTarget(event.getConfiguredTargetKey())) {
       return;
     }
 
@@ -168,6 +155,31 @@ public void onTargetComplete(TargetCompleteEvent event) {
         event.getExecutableTargetData().getRunfiles());
   }
 
+  private boolean shouldDownloadToplevelOutputsForTarget(ConfiguredTargetKey configuredTargetKey) {
+    if (commandMode != CommandMode.TEST) {
+      return true;
+    }
+
+    // Do not download test binary in test mode.
+    try {
+      var configuredTargetValue = (ConfiguredTargetValue) memoizingEvaluator.getExistingValue(
+          configuredTargetKey);
+      if (configuredTargetValue == null) {
+        return false;
+      }
+      ConfiguredTarget configuredTarget = configuredTargetValue.getConfiguredTarget();
+      if (configuredTarget instanceof RuleConfiguredTarget) {
+        var ruleConfiguredTarget = (RuleConfiguredTarget) configuredTarget;
+        var isTestRule = ruleConfiguredTarget.getRuleClassString().endsWith("_test");
+        return !isTestRule;
+      }
+      return true;
+    } catch (InterruptedException ignored) {
+      Thread.currentThread().interrupt();
+      return false;
+    }
+  }
+
   private void downloadTargetOutputs(
       ImmutableMap<String, ArtifactsInOutputGroup> outputGroups, @Nullable Runfiles runfiles) {
 

src/main/java/com/google/devtools/build/lib/remote/util/Utils.java

@@ -190,18 +190,6 @@ private static Instant timestampToInstant(Timestamp timestamp) {
     return Instant.ofEpochSecond(timestamp.getSeconds(), timestamp.getNanos());
   }
 
-  /**
-   * Returns {@code true} if all spawn outputs should be downloaded to disk.
-   */
-  public static boolean shouldDownloadAllSpawnOutputs(RemoteOutputsMode remoteOutputsMode,
-      int exitCode) {
-    return remoteOutputsMode.downloadAllOutputs()
-        ||
-        // In case the action failed, download all outputs. It might be helpful for debugging
-        // and there is no point in injecting output metadata of a failed action.
-        exitCode != 0;
-  }
-
   private static String statusName(int code) {
     // 'convert_underscores' to 'Convert Underscores'
     String name = Code.forNumber(code).getValueDescriptor().getName();

src/test/shell/bazel/remote/build_without_the_bytes_test.sh

@@ -253,10 +253,10 @@ EOF
     //a:foobar >& $TEST_log || fail "Failed to build //a:foobar"
 
   (! [[ -f bazel-bin/a/foo.txt ]]) \
-  || fail "Expected intermediate output bazel-bin/a/foo.txt to not be downloaded"
+    || fail "Expected intermediate output bazel-bin/a/foo.txt to not be downloaded"
 
   [[ -f bazel-bin/a/foobar.txt ]] \
-  || fail "Expected toplevel output bazel-bin/a/foobar.txt to be downloaded"
+    || fail "Expected toplevel output bazel-bin/a/foobar.txt to be downloaded"
 
   bazel build \
     --remote_executor=grpc://localhost:${worker_port} \
@@ -266,7 +266,7 @@ EOF
   expect_log "1 process: 1 internal"
 
   [[ -f bazel-bin/a/foo.txt ]] \
-  || fail "Expected toplevel output bazel-bin/a/foo.txt to be downloaded"
+    || fail "Expected toplevel output bazel-bin/a/foo.txt to be downloaded"
 }
 
 function test_downloads_toplevel_runfiles() {