Make test.xml a regular test action output

This ensures that BwoB correctly invalidates the action if `test.xml` is newly requested to be downloaded.

Fixes #25762

Closes #26048.

PiperOrigin-RevId: 799116042
Change-Id: I5d0b4182ea43e1447cd8b3d10cc0c378f5ab9e5f

Author: fmeum

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

@@ -22,6 +22,7 @@
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.Lists;
 import com.google.devtools.build.lib.actions.ActionInput;
+import com.google.devtools.build.lib.actions.ActionInputHelper;
 import com.google.devtools.build.lib.actions.ActionOwner;
 import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.actions.ArtifactRoot;
@@ -54,6 +55,13 @@
 
 /** Helper class to create test actions. */
 public final class TestActionBuilder {
+
+  // Whether the test.xml is a declared output of this action rather than just an output of the test
+  // spawn. True for Bazel so that it behaves properly with Build without the Bytes (i.e.,
+  // --remote_download_regex), but false for Blaze because not all test rules generate a test.xml.
+  // DO NOT inline this constant, as it's rewritten by Copybara on import/export.
+  private static final boolean TEST_XML_IS_ACTION_OUTPUT = true;
+
   private static final String CC_CODE_COVERAGE_SCRIPT = "CC_CODE_COVERAGE_SCRIPT";
   private static final String LCOV_MERGER = "LCOV_MERGER";
   // The coverage tool Bazel uses to generate a code coverage report for C++.
@@ -353,6 +361,11 @@ private TestParams createTestAction()
 
         Artifact.DerivedArtifact testLog =
             ruleContext.getPackageRelativeArtifact(dir.getRelative("test.log"), root);
+        ActionInput testXml =
+            TEST_XML_IS_ACTION_OUTPUT
+                ? ruleContext.getPackageRelativeArtifact(dir.getRelative("test.xml"), root)
+                : ActionInputHelper.fromPath(
+                    testLog.getExecPath().getParentDirectory().getRelative("test.xml"));
         Artifact.DerivedArtifact cacheStatus =
             ruleContext.getPackageRelativeArtifact(dir.getRelative("test.cache_status"), root);
 
@@ -386,6 +399,7 @@ private TestParams createTestAction()
                 testXmlGeneratorExecutable,
                 collectCoverageScript,
                 testLog,
+                testXml,
                 cacheStatus,
                 coverageArtifact,
                 coverageDirectory,

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

@@ -119,6 +119,7 @@ public class TestRunnerAction extends AbstractAction
   private final BuildConfigurationValue configuration;
   private final TestConfiguration testConfiguration;
   private final Artifact testLog;
+  private final ActionInput testXml;
   private final Artifact cacheStatus;
   private final PathFragment testWarningsPath;
   private final PathFragment unusedRunfilesLogPath;
@@ -129,7 +130,6 @@ public class TestRunnerAction extends AbstractAction
   private final PathFragment undeclaredOutputsManifestPath;
   private final PathFragment undeclaredOutputsAnnotationsPath;
   private final PathFragment undeclaredOutputsAnnotationsPbPath;
-  private final PathFragment xmlOutputPath;
   @Nullable private final PathFragment testShard;
   private final PathFragment testExitSafe;
   private final PathFragment testStderr;
@@ -199,6 +199,7 @@ private static ImmutableSet<Artifact> nonNullAsSet(Artifact... artifacts) {
       @Nullable
           FilesToRunProvider collectCoverageScript, // filesToRun must be in input, if not null
       Artifact testLog,
+      ActionInput testXml,
       Artifact cacheStatus,
       Artifact coverageArtifact,
       @Nullable Artifact coverageDirectory,
@@ -219,7 +220,13 @@ private static ImmutableSet<Artifact> nonNullAsSet(Artifact... artifacts) {
         owner,
         inputs,
         nonNullAsSet(
-            testLog, cacheStatus, coverageArtifact, coverageDirectory, undeclaredOutputsDir));
+            testLog,
+            // See TestActionBuilder.TEST_XML_IS_ACTION_OUTPUT for details.
+            testXml instanceof Artifact testXmlArtifact ? testXmlArtifact : null,
+            cacheStatus,
+            coverageArtifact,
+            coverageDirectory,
+            undeclaredOutputsDir));
     Preconditions.checkState((collectCoverageScript == null) == (coverageArtifact == null));
     this.runfilesTree = runfilesTree;
     this.testSetupScript = testSetupScript;
@@ -228,6 +235,7 @@ private static ImmutableSet<Artifact> nonNullAsSet(Artifact... artifacts) {
     this.configuration = checkNotNull(configuration);
     this.testConfiguration = checkNotNull(configuration.getFragment(TestConfiguration.class));
     this.testLog = testLog;
+    this.testXml = testXml;
     this.cacheStatus = cacheStatus;
     this.coverageData = coverageArtifact;
     this.coverageDirectory = coverageDirectory;
@@ -246,7 +254,6 @@ private static ImmutableSet<Artifact> nonNullAsSet(Artifact... artifacts) {
     this.testExitSafe = baseDir.getChild("test.exited_prematurely");
     // testShard Path should be set only if sharding is enabled.
     this.testShard = totalShards > 1 ? baseDir.getChild("test.shard") : null;
-    this.xmlOutputPath = baseDir.getChild("test.xml");
     this.testWarningsPath = baseDir.getChild("test.warnings");
     this.unusedRunfilesLogPath = baseDir.getChild("test.unused_runfiles_log");
     this.testStderr = baseDir.getChild("test.err");
@@ -282,7 +289,6 @@ private static ImmutableSet<Artifact> nonNullAsSet(Artifact... artifacts) {
     ImmutableSet.Builder<PathFragment> filesToDeleteBuilder =
         ImmutableSet.<PathFragment>builder()
             .add(
-                xmlOutputPath,
                 testWarningsPath,
                 unusedRunfilesLogPath,
                 testStderr,
@@ -292,6 +298,9 @@ private static ImmutableSet<Artifact> nonNullAsSet(Artifact... artifacts) {
                 // instead.
                 baseDir.getChild("coverage.dat"),
                 baseDir.getChild("test.zip")); // Delete files fetched from remote execution.
+    if (!(testXml instanceof Artifact)) {
+      filesToDeleteBuilder.add(testXml.getExecPath());
+    }
     if (testShard != null) {
       filesToDeleteBuilder.add(testShard);
     }
@@ -360,7 +369,7 @@ public boolean checkShardingSupport() {
 
   public List<ActionInput> getSpawnOutputs() {
     final List<ActionInput> outputs = new ArrayList<>();
-    outputs.add(ActionInputHelper.fromPath(getXmlOutputPath()));
+    outputs.add(testXml);
     outputs.add(ActionInputHelper.fromPath(getExitSafeFile()));
     if (isSharded()) {
       outputs.add(ActionInputHelper.fromPath(getTestShard()));
@@ -767,7 +776,7 @@ public void setupEnvVariables(Map<String, String> env) {
       env.put("TEST_TOTAL_SHARDS", Integer.toString(getExecutionSettings().getTotalShards()));
       env.put("TEST_SHARD_STATUS_FILE", getTestShard().getPathString());
     }
-    env.put("XML_OUTPUT_FILE", getXmlOutputPath().getPathString());
+    env.put("XML_OUTPUT_FILE", testXml.getExecPathString());
 
     if (!configuration.runfilesEnabled()) {
       // If runfiles are disabled, tell remote-runtest.sh/local-runtest.sh about that.
@@ -826,6 +835,10 @@ public Artifact getTestLog() {
     return testLog;
   }
 
+  public ActionInput getTestXml() {
+    return testXml;
+  }
+
   /** Returns all environment variables which must be set in order to run this test. */
   public ActionEnvironment getExtraTestEnv() {
     return extraTestEnv;
@@ -900,11 +913,6 @@ public PathFragment getInfrastructureFailureFile() {
     return testInfrastructureFailure;
   }
 
-  /** Returns path to the optionally created XML output file created by the test. */
-  public PathFragment getXmlOutputPath() {
-    return xmlOutputPath;
-  }
-
   /** Returns coverage data artifact or null if code coverage was not requested. */
   @Nullable
   public Artifact getCoverageData() {
@@ -1184,7 +1192,7 @@ public Path getInfrastructureFailureFile() {
 
     /** Returns path to the optionally created XML output file created by the test. */
     public Path getXmlOutputPath() {
-      return getPath(xmlOutputPath);
+      return getPath(testXml.getExecPath());
     }
 
     public Path getCoverageDirectory() {

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

@@ -26,7 +26,6 @@
 import com.google.common.io.ByteStreams;
 import com.google.devtools.build.lib.actions.ActionExecutionContext;
 import com.google.devtools.build.lib.actions.ActionInput;
-import com.google.devtools.build.lib.actions.ActionInputHelper;
 import com.google.devtools.build.lib.actions.Artifact.SpecialArtifact;
 import com.google.devtools.build.lib.actions.ArtifactPathResolver;
 import com.google.devtools.build.lib.actions.EnvironmentalExecException;
@@ -459,7 +458,7 @@ private static Spawn createXmlGeneratingSpawn(
                 .getExecPath()
                 .getCallablePathStringForOs(action.getExecutionSettings().getExecutionOs()),
             action.getTestLog().getExecPathString(),
-            action.getXmlOutputPath().getPathString(),
+            action.getTestXml().getExecPathString(),
             Integer.toString(result.getWallTimeInMs() / 1000),
             Integer.toString(result.exitCode()));
     ImmutableMap.Builder<String, String> envBuilder = ImmutableMap.builder();
@@ -483,7 +482,7 @@ private static Spawn createXmlGeneratingSpawn(
         /* inputs= */ NestedSetBuilder.create(
             Order.STABLE_ORDER, action.getTestXmlGeneratorScript(), action.getTestLog()),
         /* tools= */ NestedSetBuilder.emptySet(Order.STABLE_ORDER),
-        /* outputs= */ ImmutableSet.of(ActionInputHelper.fromPath(action.getXmlOutputPath())),
+        /* outputs= */ ImmutableSet.of(action.getTestXml()),
         /* mandatoryOutputs= */ null,
         SpawnAction.DEFAULT_RESOURCE_SET);
   }

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

@@ -700,6 +700,63 @@ EOF
   assert_exists bazel-bin/a/test
 }
 
+function test_download_regex_changed_with_action_cache_hit_for_test() {
+  # Regression test for #25762
+  # Test that changes to --remote_download_regex are effective when matching
+  # test.xml even when the test is cached.
+
+  add_rules_shell "MODULE.bazel"
+  mkdir -p a
+
+  cat > a/test.sh <<'EOF'
+#!/bin/sh
+
+cat > "$XML_OUTPUT_FILE" <<EOF2
+<?xml version="1.0" encoding="UTF-8"?>
+<testsuites>
+  <testsuite name="test" tests="1" failures="0" errors="0">
+    <testcase name="test_case" status="run">
+      <system-out>test_case succeeded.</system-out>
+    </testcase>
+  </testsuite>
+</testsuites>
+EOF2
+echo "hi" > "$TEST_UNDECLARED_OUTPUTS_DIR/out.txt"
+EOF
+
+  chmod +x a/test.sh
+
+  cat > a/BUILD <<EOF
+load("@rules_shell//shell:sh_test.bzl", "sh_test")
+sh_test(
+  name = 'test',
+  srcs = [ 'test.sh' ],
+)
+EOF
+
+  bazel test \
+    --remote_executor=grpc://localhost:${worker_port} \
+    --remote_download_minimal \
+    //a:test >& $TEST_log || fail "Expected success"
+
+  rm -rf bazel-out bazel-testlogs
+
+  bazel test \
+    --remote_executor=grpc://localhost:${worker_port} \
+    --remote_download_minimal \
+    //a:test >& $TEST_log || fail "Expected success"
+
+  assert_not_exists bazel-testlogs/a/test/test.xml
+
+  bazel test \
+    --remote_executor=grpc://localhost:${worker_port} \
+    --remote_download_minimal \
+    --remote_download_regex='.*/test.xml' \
+    //a:test >& $TEST_log || fail "Expected success"
+
+  assert_exists bazel-testlogs/a/test/test.xml
+}
+
 function do_test_non_test_toplevel_targets() {
   # Regression test for https://github.com/bazelbuild/bazel/issues/17190.
   #