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.

Author: fmeum

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

@@ -388,6 +388,8 @@ private TestParams createTestAction()
 
         Artifact.DerivedArtifact testLog =
             ruleContext.getPackageRelativeArtifact(dir.getRelative("test.log"), root);
+        Artifact.DerivedArtifact testXml =
+            ruleContext.getPackageRelativeArtifact(dir.getRelative("test.xml"), root);
         Artifact.DerivedArtifact cacheStatus =
             ruleContext.getPackageRelativeArtifact(dir.getRelative("test.cache_status"), root);
 
@@ -421,6 +423,7 @@ private TestParams createTestAction()
                 testXmlGeneratorExecutable,
                 collectCoverageScript,
                 testLog,
+                testXml,
                 cacheStatus,
                 coverageArtifact,
                 coverageDirectory,

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

@@ -118,6 +118,7 @@ public class TestRunnerAction extends AbstractAction
   private final BuildConfigurationValue configuration;
   private final TestConfiguration testConfiguration;
   private final Artifact testLog;
+  private final Artifact testXml;
   private final Artifact cacheStatus;
   private final PathFragment testWarningsPath;
   private final PathFragment unusedRunfilesLogPath;
@@ -128,7 +129,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;
@@ -198,6 +198,7 @@ private static ImmutableSet<Artifact> nonNullAsSet(Artifact... artifacts) {
       Artifact testXmlGeneratorScript, // Must be in inputs
       @Nullable Artifact collectCoverageScript, // Must be in inputs, if not null
       Artifact testLog,
+      Artifact testXml,
       Artifact cacheStatus,
       Artifact coverageArtifact,
       @Nullable Artifact coverageDirectory,
@@ -219,7 +220,12 @@ private static ImmutableSet<Artifact> nonNullAsSet(Artifact... artifacts) {
         owner,
         inputs,
         nonNullAsSet(
-            testLog, cacheStatus, coverageArtifact, coverageDirectory, undeclaredOutputsDir));
+            testLog,
+            testXml,
+            cacheStatus,
+            coverageArtifact,
+            coverageDirectory,
+            undeclaredOutputsDir));
     Preconditions.checkState((collectCoverageScript == null) == (coverageArtifact == null));
     this.runfilesTree = runfilesTree;
     this.testSetupScript = testSetupScript;
@@ -228,6 +234,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 +253,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");
@@ -283,7 +289,6 @@ private static ImmutableSet<Artifact> nonNullAsSet(Artifact... artifacts) {
     ImmutableSet.Builder<PathFragment> filesToDeleteBuilder =
         ImmutableSet.<PathFragment>builder()
             .add(
-                xmlOutputPath,
                 testWarningsPath,
                 unusedRunfilesLogPath,
                 testStderr,
@@ -366,7 +371,6 @@ public boolean checkShardingSupport() {
 
   public List<ActionInput> getSpawnOutputs() {
     final List<ActionInput> outputs = new ArrayList<>();
-    outputs.add(ActionInputHelper.fromPath(getXmlOutputPath()));
     outputs.add(ActionInputHelper.fromPath(getExitSafeFile()));
     if (isSharded()) {
       outputs.add(ActionInputHelper.fromPath(getTestShard()));
@@ -773,7 +777,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.
@@ -832,6 +836,10 @@ public Artifact getTestLog() {
     return testLog;
   }
 
+  public Artifact getTestXml() {
+    return testXml;
+  }
+
   /** Returns all environment variables which must be set in order to run this test. */
   public ActionEnvironment getExtraTestEnv() {
     return extraTestEnv;
@@ -906,11 +914,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() {
@@ -1183,7 +1186,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

@@ -450,7 +450,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();
@@ -474,7 +474,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

@@ -648,6 +648,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.
   #