Make remote_download_outputs=toplevel work for tests too

This introduces two new behaviours:
 1) bazel test //:foo_test downloads
 bazel-testlogs/foo_test/test.{log|xml}
 2) bazel build //:foo_test downloads bazel-bin/foo_test

Fixes bazelbuild#8934

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

@@ -19,6 +19,7 @@ java_library(
         "//src/main/java/com/google/devtools/build/lib:events",
         "//src/main/java/com/google/devtools/build/lib:io",
         "//src/main/java/com/google/devtools/build/lib:out-err",
+        "//src/main/java/com/google/devtools/build/lib:packages-internal",
         "//src/main/java/com/google/devtools/build/lib:runtime",
         "//src/main/java/com/google/devtools/build/lib:util",
         "//src/main/java/com/google/devtools/build/lib/actions",

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

@@ -19,6 +19,7 @@
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableSet;
 import com.google.devtools.build.lib.actions.ActionContext;
+import com.google.devtools.build.lib.actions.ActionInput;
 import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.actions.ExecutionStrategy;
 import com.google.devtools.build.lib.actions.ExecutorInitException;
@@ -49,7 +50,7 @@ final class RemoteActionContextProvider extends ActionContextProvider {
   private final DigestUtil digestUtil;
   @Nullable private final Path logDir;
   private final AtomicReference<SpawnRunner> fallbackRunner = new AtomicReference<>();
-  private ImmutableSet<Artifact> topLevelOutputs = ImmutableSet.of();
+  private ImmutableSet<ActionInput> topLevelOutputs = ImmutableSet.of();
 
   private RemoteActionContextProvider(
       CommandEnvironment env,
@@ -171,7 +172,7 @@ AbstractRemoteActionCache getRemoteCache() {
     return cache;
   }
 
-  void setTopLevelOutputs(ImmutableSet<Artifact> topLevelOutputs) {
+  void setTopLevelOutputs(ImmutableSet<ActionInput> topLevelOutputs) {
     this.topLevelOutputs = Preconditions.checkNotNull(topLevelOutputs, "topLevelOutputs");
   }
 

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

@@ -22,10 +22,14 @@
 import com.google.common.collect.ImmutableSet;
 import com.google.common.util.concurrent.ListeningScheduledExecutorService;
 import com.google.common.util.concurrent.MoreExecutors;
+import com.google.devtools.build.lib.actions.ActionInput;
 import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.analysis.ConfiguredTarget;
+import com.google.devtools.build.lib.analysis.TopLevelArtifactContext;
 import com.google.devtools.build.lib.analysis.TopLevelArtifactHelper;
 import com.google.devtools.build.lib.analysis.config.BuildOptions;
+import com.google.devtools.build.lib.analysis.configuredtargets.RuleConfiguredTarget;
+import com.google.devtools.build.lib.analysis.test.TestProvider;
 import com.google.devtools.build.lib.authandtls.AuthAndTLSOptions;
 import com.google.devtools.build.lib.authandtls.GoogleAuthUtils;
 import com.google.devtools.build.lib.buildeventstream.BuildEventArtifactUploader;
@@ -34,6 +38,7 @@
 import com.google.devtools.build.lib.events.Event;
 import com.google.devtools.build.lib.events.Reporter;
 import com.google.devtools.build.lib.exec.ExecutorBuilder;
+import com.google.devtools.build.lib.packages.TargetUtils;
 import com.google.devtools.build.lib.remote.logging.LoggingInterceptor;
 import com.google.devtools.build.lib.remote.options.RemoteOptions;
 import com.google.devtools.build.lib.remote.options.RemoteOutputsMode;
@@ -45,6 +50,7 @@
 import com.google.devtools.build.lib.runtime.Command;
 import com.google.devtools.build.lib.runtime.CommandEnvironment;
 import com.google.devtools.build.lib.runtime.ServerBuilder;
+import com.google.devtools.build.lib.runtime.commands.TestCommand;
 import com.google.devtools.build.lib.skyframe.AspectValue;
 import com.google.devtools.build.lib.util.AbruptExitException;
 import com.google.devtools.build.lib.util.ExitCode;
@@ -321,27 +327,42 @@ public void afterAnalysis(
       ImmutableSet<AspectValue> aspects) {
     if (remoteOutputsMode != null && remoteOutputsMode.downloadToplevelOutputsOnly()) {
       Preconditions.checkState(actionContextProvider != null, "actionContextProvider was null");
-      // TODO(buchgr): Consider only storing the action owners instead of the artifacts
       // Collect all top level output artifacts of regular targets as well as aspects. This
       // information is used by remote spawn runners to decide whether to download an artifact
       // if --experimental_remote_download_outputs=toplevel is set
-      ImmutableSet.Builder<Artifact> topLevelOutputsBuilder = ImmutableSet.builder();
+      ImmutableSet.Builder<ActionInput> topLevelOutputsBuilder = ImmutableSet.builder();
       for (ConfiguredTarget configuredTarget : configuredTargets) {
-        topLevelOutputsBuilder.addAll(
-            TopLevelArtifactHelper.getAllArtifactsToBuild(
-                    configuredTarget, request.getTopLevelArtifactContext())
-                .getImportantArtifacts());
+        topLevelOutputsBuilder.addAll(getTopLevelTargetOutputs(configuredTarget,
+            request.getTopLevelArtifactContext(), env.getCommandName()));
       }
+      actionContextProvider.setTopLevelOutputs(topLevelOutputsBuilder.build());
+    }
+  }
 
-      for (AspectValue aspect : aspects) {
-        topLevelOutputsBuilder.addAll(
-            TopLevelArtifactHelper.getAllArtifactsToBuild(
-                    aspect, request.getTopLevelArtifactContext())
-                .getImportantArtifacts());
+
+  /**
+   * Returns a list of build or test outputs produced by the configured target.
+   */
+  private ImmutableList<ActionInput> getTopLevelTargetOutputs(ConfiguredTarget configuredTarget,
+      TopLevelArtifactContext topLevelArtifactContext, String commandName) {
+    if (commandName.equals("test") && isTestRule(configuredTarget)) {
+      TestProvider testProvider = configuredTarget.getProvider(TestProvider.class);
+      if (testProvider == null) {
+        return ImmutableList.of();
       }
+      return testProvider.getTestParams().getOutputs();
+    } else {
+      return ImmutableList.copyOf(TopLevelArtifactHelper.getAllArtifactsToBuild(configuredTarget,
+          topLevelArtifactContext).getImportantArtifacts());
+    }
+  }
 
-      actionContextProvider.setTopLevelOutputs(topLevelOutputsBuilder.build());
+  private static boolean isTestRule(ConfiguredTarget configuredTarget) {
+    if (configuredTarget instanceof RuleConfiguredTarget) {
+      RuleConfiguredTarget ruleConfiguredTarget = (RuleConfiguredTarget) configuredTarget;
+      return TargetUtils.isTestRuleName(ruleConfiguredTarget.getRuleClassString());
     }
+    return false;
   }
 
   private static void cleanAndCreateRemoteLogsDir(Path logDir) throws AbruptExitException {

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

@@ -90,7 +90,7 @@ final class RemoteSpawnCache implements SpawnCache {
    * <p>This set is empty unless {@link RemoteOutputsMode#TOPLEVEL} is specified. If so, this set is
    * used to decide whether to download an output.
    */
-  private final ImmutableSet<Artifact> topLevelOutputs;
+  private final ImmutableSet<ActionInput> topLevelOutputs;
 
   RemoteSpawnCache(
       Path execRoot,
@@ -100,7 +100,7 @@ final class RemoteSpawnCache implements SpawnCache {
       String commandId,
       @Nullable Reporter cmdlineReporter,
       DigestUtil digestUtil,
-      ImmutableSet<Artifact> topLevelOutputs) {
+      ImmutableSet<ActionInput> topLevelOutputs) {
     this.execRoot = execRoot;
     this.options = options;
     this.remoteCache = remoteCache;

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

@@ -118,7 +118,7 @@ public class RemoteSpawnRunner implements SpawnRunner {
    * <p>This set is empty unless {@link RemoteOutputsMode#TOPLEVEL} is specified. If so, this set is
    * used to decide whether to download an output.
    */
-  private final ImmutableSet<Artifact> topLevelOutputs;
+  private final ImmutableSet<ActionInput> topLevelOutputs;
 
   // Used to ensure that a warning is reported only once.
   private final AtomicBoolean warningReported = new AtomicBoolean();
@@ -137,7 +137,7 @@ public class RemoteSpawnRunner implements SpawnRunner {
       @Nullable RemoteRetrier retrier,
       DigestUtil digestUtil,
       Path logDir,
-      ImmutableSet<Artifact> topLevelOutputs) {
+      ImmutableSet<ActionInput> topLevelOutputs) {
     this.execRoot = execRoot;
     this.remoteOptions = remoteOptions;
     this.executionOptions = executionOptions;

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

@@ -100,7 +100,7 @@ public static boolean shouldDownloadAllSpawnOutputs(
 
   /** Returns {@code true} if outputs contains one or more top level outputs. */
   public static boolean hasTopLevelOutputs(
-      Collection<? extends ActionInput> outputs, ImmutableSet<Artifact> topLevelOutputs) {
+      Collection<? extends ActionInput> outputs, ImmutableSet<ActionInput> topLevelOutputs) {
     if (topLevelOutputs.isEmpty()) {
       return false;
     }

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

@@ -1058,8 +1058,62 @@ EOF
 
   [[ -f bazel-bin/a/foobar.txt ]] \
   || fail "Expected toplevel output bazel-bin/a/foobar.txt to be re-downloaded"
+}
+
+function test_download_toplevel_test_rule() {
+  # Test that when using --experimental_remote_download_outputs=toplevel with bazel test only
+  # the test.log and test.xml file are downloaded but not the test binary. However when building
+  # a test then the test binary should be downloaded.
+
+  if [[ "$PLATFORM" == "darwin" ]]; then
+    # TODO(b/37355380): This test is disabled due to RemoteWorker not supporting
+    # setting SDKROOT and DEVELOPER_DIR appropriately, as is required of
+    # action executors in order to select the appropriate Xcode toolchain.
+    return 0
+  fi
 
+  mkdir -p a
+  cat > a/BUILD <<EOF
+package(default_visibility = ["//visibility:public"])
+cc_test(
+  name = 'test',
+  srcs = [ 'test.cc' ],
+)
+EOF
+  cat > a/test.cc <<EOF
+#include <iostream>
+int main() { std::cout << "Hello test!" << std::endl; return 0; }
+EOF
+
+  # When invoking bazel test only test.log and test.xml should be downloaded.
+  bazel test \
+    --remote_executor=grpc://localhost:${worker_port} \
+    --experimental_inmemory_jdeps_files \
+    --experimental_inmemory_dotd_files \
+    --experimental_remote_download_outputs=toplevel \
+    //a:test >& $TEST_log || fail "Failed to test //a:test with remote execution"
+
+  (! [[ -f bazel-bin/a/test ]]) \
+  || fail "Expected test binary bazel-bin/a/test to not be downloaded"
+
+  [[ -f bazel-testlogs/a/test/test.log ]] \
+  || fail "Expected toplevel output bazel-testlogs/a/test/test.log to be downloaded"
+
+  [[ -f bazel-testlogs/a/test/test.xml ]] \
+  || fail "Expected toplevel output bazel-testlogs/a/test/test.log to be downloaded"
+
+  bazel clean
+
+  # When invoking bazel build the test binary should be downloaded.
+  bazel build \
+    --remote_executor=grpc://localhost:${worker_port} \
+    --experimental_inmemory_jdeps_files \
+    --experimental_inmemory_dotd_files \
+    --experimental_remote_download_outputs=toplevel \
+    //a:test >& $TEST_log || fail "Failed to build //a:test with remote execution"
 
+  ([[ -f bazel-bin/a/test ]]) \
+  || fail "Expected test binary bazel-bin/a/test to be downloaded"
 }
 
 function test_downloads_minimal_bep() {