Repository: bazelbuild/bazel
Issue: --remote_download_toplevel --nozip_undeclared_test_outputs does not download any undeclared test outputs for passing tests
Labels: type: bug
P2
team-Remote-Exec

### Description of the bug:


Prior to upgrading to  bazel 6.0.0, on a setup with remote execution, with `bazel test --remote_download_toplevel --nozip_undeclared_test_outputs` I'd see `test.outputs` populated with files. 
On bazel 6.0.0, `test.outputs` is always empty on passing tests.

### What's the simplest, easiest way to reproduce this bug? Please provide a minimal example if possible.


With remote execution, run `bazel test --remote_download_toplevel --nozip_undeclared_test_outputs`. Observe that test.outputs is empty on passing tests.

### Which operating system are you running Bazel on?


Linux

### What is the output of `bazel info release`?


release 6.0.0

### If `bazel info release` returns `development version` or `(@non-git)`, tell us how you built Bazel.


_No response_

### What's the output of `git remote get-url origin; git rev-parse master; git rev-parse HEAD` ?


_No response_

### Have you found anything relevant by searching the web?

_No response_

### Any other information, logs, or outputs that you want to share?

If I remove `--nozip_undeclared_test_outputs`, I see `test.outputs/outputs.zip` on passing tests too
If I remove `--remote_download_toplevel`, I see the desired files in `test.outputs/*` on passing tests too

Adding `--experimental_remote_download_regex=".*"` didn't help.

Issue comments:
Note to future self: the minimal repro for https://github.com/bazelbuild/bazel/issues/17911 is also applicable to this issue.
@tjgq My usual question 😅 is this cherry-pickable?
I don't think it's going to be possible, sorry :( there are way too many other changes that this one depends on.
That is unfortunate, as it was working in 5.x, but broke in 6.x, and will only work again in 7.0 ☹️.
I will try to work out a dedicated fix for 6.3.
@bazel-io fork 6.3.0

Pull request review comments:

Patch that resolved the issue:
diff --git a/src/main/java/com/google/devtools/build/lib/remote/ToplevelArtifactsDownloader.java b/src/main/java/com/google/devtools/build/lib/remote/ToplevelArtifactsDownloader.java
index 75ad886bb3d5f2..dc6481f800cb9d 100644
--- a/src/main/java/com/google/devtools/build/lib/remote/ToplevelArtifactsDownloader.java
+++ b/src/main/java/com/google/devtools/build/lib/remote/ToplevelArtifactsDownloader.java
@@ -47,8 +47,10 @@
 import com.google.devtools.build.lib.skyframe.TreeArtifactValue;
 import com.google.devtools.build.lib.util.Pair;
 import com.google.devtools.build.lib.vfs.Path;
+import com.google.devtools.build.lib.vfs.Symlinks;
 import com.google.devtools.build.skyframe.MemoizingEvaluator;
 import com.google.devtools.build.skyframe.SkyValue;
+import java.io.IOException;
 import java.util.HashMap;
 import javax.annotation.Nullable;
 
@@ -117,6 +119,18 @@ public interface PathToMetadataConverter {
   }
 
   private void downloadTestOutput(Path path) {
+    if (path.isDirectory()) {
+      try {
+        var entries = path.readdir(Symlinks.FOLLOW);
+        for (var entry : entries) {
+          downloadTestOutput(path.getRelative(entry.getName()));
+          return;
+        }
+      } catch (IOException e) {
+        logger.atWarning().withCause(e).log("Failed to read dir %s.", path);
+      }
+    }
+
     // Since the event is fired within action execution, the skyframe doesn't know the outputs of
     // test actions yet, so we can't get their metadata through skyframe. However, the fileSystem
     // of the path is an ActionFileSystem, we use it to get the metadata for this file.


Test patch:
diff --git a/src/test/shell/bazel/remote/build_without_the_bytes_test.sh b/src/test/shell/bazel/remote/build_without_the_bytes_test.sh
index 5cd898753c6f25..5be0b0a05bd238 100755
--- a/src/test/shell/bazel/remote/build_without_the_bytes_test.sh
+++ b/src/test/shell/bazel/remote/build_without_the_bytes_test.sh
@@ -685,6 +685,31 @@ EOF
   expect_log "test.outputs_manifest__MANIFEST"
 }
 
+function test_nozip_undeclared_test_outputs() {
+  mkdir -p a
+  cat > a/test.sh << 'EOF'
+#!/bin/sh
+echo foo > "$TEST_UNDECLARED_OUTPUTS_DIR/text.txt"
+EOF
+  chmod +x a/test.sh
+
+  cat > a/BUILD <<'EOF'
+sh_test(
+  name = "foo",
+  srcs = ["test.sh"],
+)
+EOF
+
+  bazel test \
+    --remote_executor=grpc://localhost:${worker_port} \
+    --remote_download_toplevel \
+    --nozip_undeclared_test_outputs \
+    //a:foo || fail "Failed to test //a:foo"
+
+  [[ -e "bazel-testlogs/a/foo/test.outputs/text.txt" ]] || fail "bazel-testlogs/a/foo/test.outputs/text.txt does not exist"
+  assert_contains "foo" "bazel-testlogs/a/foo/test.outputs/text.txt"
+}
+
 function test_multiple_test_attempts() {
   # Test that test logs of multiple test attempts can be renamed and reported by
   # BEP.