Set exit_code unconditionally on TestResult in BEP

Fixes #25165

I hope I did this right. I am able to see the output in my repro on rules_apple (which I can share if required by code review). Let me know if there's anything I've overlooked!

It would be great to get this into 8.1.0 if possible.

Closes #25203.

PiperOrigin-RevId: 725718124
Change-Id: I9c845ce1dc18307f5932da972f4ad31058a42f89

Author: aaronsky

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

@@ -367,6 +367,12 @@ private static BuildEventStreamProtos.TestResult.ExecutionInfo extractExecutionI
     BuildEventStreamProtos.TestResult.ExecutionInfo.Builder executionInfo =
         BuildEventStreamProtos.TestResult.ExecutionInfo.newBuilder();
 
+    // The return of `SpawnResult#exitCode()` is noted to only be meaningful if the subprocess
+    // actually executed. In this position, `spawnResult.exitCode()` is always meaningful,
+    // because the code only runs if `spawnResult.setupSuccess()` is previously verified to
+    // be `true`.
+    executionInfo.setExitCode(spawnResult.exitCode());
+
     if (spawnResult.isCacheHit()) {
       result.setRemotelyCached(true);
       executionInfo.setCachedRemotely(true);

src/test/java/com/google/devtools/build/lib/exec/StandaloneTestStrategyTest.java

@@ -331,6 +331,7 @@ public void testRunTestOnce() throws Exception {
     assertThat(result.isCached()).isFalse();
     assertThat(result.getTestAction()).isSameInstanceAs(testRunnerAction);
     assertThat(result.getData().getTestPassed()).isTrue();
+    assertThat(result.getData().getExitCode()).isEqualTo(0);
     assertThat(result.getData().getRemotelyCached()).isFalse();
     assertThat(result.getData().getIsRemoteStrategy()).isFalse();
     assertThat(result.getData().getRunDurationMillis()).isEqualTo(10);
@@ -411,6 +412,7 @@ public void testRunFlakyTest() throws Exception {
     assertThat(result.getTestAction()).isSameInstanceAs(testRunnerAction);
     assertThat(result.getData().getStatus()).isEqualTo(BlazeTestStatus.FLAKY);
     assertThat(result.getData().getTestPassed()).isTrue();
+    assertThat(result.getData().getExitCode()).isEqualTo(0);
     assertThat(result.getData().getRemotelyCached()).isFalse();
     assertThat(result.getData().getIsRemoteStrategy()).isFalse();
     assertThat(result.getData().getRunDurationMillis()).isEqualTo(15L);
@@ -425,9 +427,11 @@ public void testRunFlakyTest() throws Exception {
     assertThat(failedAttempt.getExecutionInfo().getStrategy()).isEqualTo("test");
     assertThat(failedAttempt.getExecutionInfo().getHostname()).isEqualTo("");
     assertThat(failedAttempt.getStatus()).isEqualTo(TestStatus.FAILED);
+    assertThat(failedAttempt.getExecutionInfo().getExitCode()).isEqualTo(1);
     assertThat(failedAttempt.getExecutionInfo().getCachedRemotely()).isFalse();
     TestAttempt okAttempt = attempts.get(1);
     assertThat(okAttempt.getStatus()).isEqualTo(TestStatus.PASSED);
+    assertThat(okAttempt.getExecutionInfo().getExitCode()).isEqualTo(0);
     assertThat(okAttempt.getExecutionInfo().getStrategy()).isEqualTo("test");
     assertThat(okAttempt.getExecutionInfo().getHostname()).isEqualTo("");
   }
@@ -483,6 +487,7 @@ public void testRunTestRemotely() throws Exception {
     assertThat(result.isCached()).isFalse();
     assertThat(result.getTestAction()).isSameInstanceAs(testRunnerAction);
     assertThat(result.getData().getTestPassed()).isTrue();
+    assertThat(result.getData().getExitCode()).isEqualTo(0);
     assertThat(result.getData().getRemotelyCached()).isFalse();
     assertThat(result.getData().getIsRemoteStrategy()).isTrue();
     assertThat(result.getData().getRunDurationMillis()).isEqualTo(10);
@@ -495,6 +500,7 @@ public void testRunTestRemotely() throws Exception {
             .map(TestAttempt.class::cast)
             .collect(MoreCollectors.onlyElement());
     assertThat(attempt.getStatus()).isEqualTo(TestStatus.PASSED);
+    assertThat(attempt.getExecutionInfo().getExitCode()).isEqualTo(0);
     assertThat(attempt.getExecutionInfo().getStrategy()).isEqualTo("remote");
     assertThat(attempt.getExecutionInfo().getHostname()).isEqualTo("a-remote-host");
   }
@@ -551,6 +557,7 @@ public void testRunRemotelyCachedTest() throws Exception {
     assertThat(result.isCached()).isFalse();
     assertThat(result.getTestAction()).isSameInstanceAs(testRunnerAction);
     assertThat(result.getData().getTestPassed()).isTrue();
+    assertThat(result.getData().getExitCode()).isEqualTo(0);
     assertThat(result.getData().getRemotelyCached()).isTrue();
     assertThat(result.getData().getIsRemoteStrategy()).isFalse();
     assertThat(result.getData().getRunDurationMillis()).isEqualTo(10);
@@ -911,6 +918,7 @@ public void testExperimentalCancelConcurrentTests() throws Exception {
     assertThat(standaloneTestStrategy.postedResult).isNotNull();
     assertThat(standaloneTestStrategy.postedResult.getData().getStatus())
         .isEqualTo(BlazeTestStatus.PASSED);
+    assertThat(standaloneTestStrategy.postedResult.getData().getExitCode()).isEqualTo(0);
     assertThat(storedEvents.getEvents())
         .contains(Event.of(EventKind.PASS, null, "//standalone:empty_test (run 1 of 2)"));
     // Reset postedResult.
@@ -1010,6 +1018,7 @@ public void testExperimentalCancelConcurrentTestsDoesNotTriggerOnFailedRun() thr
     assertThat(standaloneTestStrategy.postedResult).isNotNull();
     assertThat(standaloneTestStrategy.postedResult.getData().getStatus())
         .isEqualTo(BlazeTestStatus.FAILED);
+    assertThat(standaloneTestStrategy.postedResult.getData().getExitCode()).isEqualTo(1);
     assertContainsPrefixedEvent(
         storedEvents.getEvents(),
         Event.of(EventKind.FAIL, null, "//standalone:empty_test (run 1 of 2)"));
@@ -1030,6 +1039,7 @@ public void testExperimentalCancelConcurrentTestsDoesNotTriggerOnFailedRun() thr
     assertThat(standaloneTestStrategy.postedResult).isNotNull();
     assertThat(standaloneTestStrategy.postedResult.getData().getStatus())
         .isEqualTo(BlazeTestStatus.PASSED);
+    assertThat(standaloneTestStrategy.postedResult.getData().getExitCode()).isEqualTo(0);
     assertThat(storedEvents.getEvents())
         .contains(Event.of(EventKind.PASS, null, "//standalone:empty_test (run 2 of 2)"));
   }
@@ -1116,6 +1126,7 @@ public void testExperimentalCancelConcurrentTestsAllFailed() throws Exception {
     assertThat(standaloneTestStrategy.postedResult).isNotNull();
     assertThat(standaloneTestStrategy.postedResult.getData().getStatus())
         .isEqualTo(BlazeTestStatus.FAILED);
+    assertThat(standaloneTestStrategy.postedResult.getData().getExitCode()).isEqualTo(1);
     assertContainsPrefixedEvent(
         storedEvents.getEvents(),
         Event.of(EventKind.FAIL, null, "//standalone:empty_test (run 1 of 2)"));
@@ -1179,5 +1190,6 @@ public void missingTestLogSpawnTestResultIsIncomplete() throws Exception {
     assertThat(failedResult).isInstanceOf(StandaloneProcessedAttemptResult.class);
     TestResultData data = ((StandaloneProcessedAttemptResult) failedResult).testResultData();
     assertThat(data.getStatus()).isEqualTo(BlazeTestStatus.INCOMPLETE);
+    assertThat(data.getExitCode()).isEqualTo(0);
   }
 }