TestRunnerAction now produces a TestResult when test lifecycle fa…

…ils at runtime.

Test execution entails running the target's test binary, and even if the binary
has been successfully built, the build tool may encounter failures when spawning
the test executable process. Previously this scenario did not produce a
TestResult or TestSummary BEP event; this is now fixed. Similarly fixed is the
scenario where execution fails after spawning the test process successfully.

The exception-handling logic that needed fixing is entirely within
`TestRunnerAction` which contains the general-purpose logic for the lifecycle of a
test. In order to report actual `INCOMPLETE` test results in these circumstances we
do need to extend existing `TestStrategy` implementations to create concrete
implementations of `TestAttemptResult`.

RELNOTES: Tests that fail to create or complete their `TestAttemptContinuation` by
throwing an `ExecException` will report an `INCOMPLETE` status. Previously, Bazel
would fail to report any status for the test attempt.
PiperOrigin-RevId: 426455101

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

@@ -22,6 +22,7 @@
 import com.google.devtools.build.lib.actions.ExecException;
 import com.google.devtools.build.lib.actions.SpawnResult;
 import com.google.devtools.build.lib.actions.SpawnResult.Status;
+import com.google.devtools.build.lib.buildeventstream.BuildEventStreamProtos;
 import com.google.devtools.build.lib.util.DetailedExitCode;
 import com.google.devtools.build.lib.vfs.Path;
 import com.google.devtools.build.lib.view.test.TestStatus.TestResultData;
@@ -68,6 +69,15 @@ TestResult newCachedTestResult(Path execRoot, TestRunnerAction action, TestResul
   @Nullable
   ListenableFuture<Void> getTestCancelFuture(ActionOwner owner, int shardNum);
 
+  /**
+   * Post the final test result when a test execution is incomplete, perhaps due to environmental
+   * failures.
+   */
+  void finalizeIncompleteTest(
+      TestRunnerAction action,
+      ActionExecutionContext actionExecutionContext,
+      List<FailedAttemptResult> failedAttempts);
+
   /** An individual test attempt result. */
   interface TestAttemptResult {
     /** Test attempt result classification, splitting failures into permanent vs retriable. */
@@ -90,6 +100,12 @@ boolean canRetry() {
     /** Returns a list of spawn results for this test attempt. */
     ImmutableList<SpawnResult> spawnResults();
 
+    /** Returns the TestResultData for the test. */
+    TestResultData.Builder testResultDataBuilder();
+
+    /** Return the ExecutionInfo data for posting to Build Event Protocol. */
+    BuildEventStreamProtos.TestResult.ExecutionInfo executionInfo();
+
     /**
      * Returns a description of the system failure associated with the primary spawn result, if any.
      */
@@ -192,6 +208,12 @@ interface TestRunnerSpawn {
     FailedAttemptResult finalizeFailedTestAttempt(TestAttemptResult testAttemptResult, int attempt)
         throws IOException;
 
+    /**
+     * Post the final test result when a test execution is incomplete, perhaps due to environmental
+     * failures.
+     */
+    void finalizeIncompleteTest(TestRunnerAction action, List<FailedAttemptResult> failedAttempts);
+
     /** Post the final test result based on the last attempt and the list of failed attempts. */
     void finalizeTest(
         TestAttemptResult lastTestAttemptResult, List<FailedAttemptResult> failedAttempts)

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

@@ -935,33 +935,36 @@ public ActionContinuationOrResult beginExecution(ActionExecutionContext actionEx
   public ActionContinuationOrResult beginExecution(
       ActionExecutionContext actionExecutionContext, TestActionContext testActionContext)
       throws InterruptedException, ActionExecutionException {
+    TestRunnerSpawn testRunnerSpawn;
     try {
-      TestRunnerSpawn testRunnerSpawn =
-          testActionContext.createTestRunnerSpawn(this, actionExecutionContext);
-      ListenableFuture<Void> cancelFuture = null;
-      if (cancelConcurrentTestsOnSuccess) {
-        cancelFuture = testActionContext.getTestCancelFuture(getOwner(), shardNum);
-      }
-      TestAttemptContinuation testAttemptContinuation =
-          beginIfNotCancelled(testRunnerSpawn, cancelFuture);
+      testRunnerSpawn = testActionContext.createTestRunnerSpawn(this, actionExecutionContext);
+    } catch (ExecException e) {
+      testActionContext.finalizeIncompleteTest(this, actionExecutionContext, ImmutableList.of());
+      throw ActionExecutionException.fromExecException(e, this);
+    }
+    ListenableFuture<Void> cancelFuture = null;
+    TestAttemptContinuation testAttemptContinuation;
+    if (cancelConcurrentTestsOnSuccess) {
+      cancelFuture = testActionContext.getTestCancelFuture(getOwner(), shardNum);
+    }
+    try {
+      testAttemptContinuation = beginIfNotCancelled(testRunnerSpawn, cancelFuture);
       if (testAttemptContinuation == null) {
         testRunnerSpawn.finalizeCancelledTest(ImmutableList.of());
         // We need to create the mandatory output files even if we're not going to run anything.
         createEmptyOutputs(actionExecutionContext);
         return ActionContinuationOrResult.of(ActionResult.create(ImmutableList.of()));
       }
-      return new RunAttemptsContinuation(
-          this,
-          testRunnerSpawn,
-          testAttemptContinuation,
-          testActionContext.isTestKeepGoing(),
-          cancelFuture);
-    } catch (ExecException e) {
-      throw ActionExecutionException.fromExecException(e, this);
     } catch (IOException e) {
       throw ActionExecutionException.fromExecException(
           new EnvironmentalExecException(e, Code.TEST_RUNNER_IO_EXCEPTION), this);
     }
+    return new RunAttemptsContinuation(
+        this,
+        testRunnerSpawn,
+        testAttemptContinuation,
+        testActionContext.isTestKeepGoing(),
+        cancelFuture);
   }
 
   @Nullable
@@ -1225,6 +1228,8 @@ public ListenableFuture<?> getFuture() {
     @Override
     public ActionContinuationOrResult execute()
         throws ActionExecutionException, InterruptedException {
+      ActionContinuationOrResult nextAttemptOrResult;
+      TestAttemptResult result;
       try {
         TestAttemptContinuation nextContinuation;
         try {
@@ -1251,80 +1256,93 @@ public ActionContinuationOrResult execute()
               cancelFuture);
         }
 
-        TestAttemptResult result = nextContinuation.get();
+        result = nextContinuation.get();
         int actualMaxAttempts =
             failedAttempts.isEmpty() ? testRunnerSpawn.getMaxAttempts(result) : maxAttempts;
         Preconditions.checkState(actualMaxAttempts != 0);
-        return process(result, actualMaxAttempts);
+        TestRunnerSpawnAndMaxAttempts nextRunnerAndAttempts = null;
+        if (result.result() != TestAttemptResult.Result.PASSED) {
+          nextRunnerAndAttempts =
+              computeNextRunnerAndMaxAttempts(
+                  result.result(), testRunnerSpawn, failedAttempts.size() + 1, actualMaxAttempts);
+        }
+        // If we made it this far, we finalize and post the test result in the #process() method.
+        // Otherwise we finalize in the exception handlers.
+        nextAttemptOrResult = process(result, nextRunnerAndAttempts);
       } catch (ExecException e) {
-        throw ActionExecutionException.fromExecException(e, this.testRunnerAction);
+        testRunnerSpawn.finalizeIncompleteTest(testRunnerAction, failedAttempts);
+        throw ActionExecutionException.fromExecException(e, testRunnerAction);
       } catch (IOException e) {
         throw ActionExecutionException.fromExecException(
             new EnvironmentalExecException(e, Code.TEST_RUNNER_IO_EXCEPTION),
             this.testRunnerAction);
       }
+      // At this point, we know that the test attempt was finalized.
+      TestAttemptResult.Result testResult = result.result();
+      if (!keepGoing && testResult != TestAttemptResult.Result.PASSED) {
+        DetailedExitCode systemFailure = result.primarySystemFailure();
+        if (systemFailure != null) {
+          throw ActionExecutionException.fromExecException(
+              new TestExecException(
+                  "Test failed (system error), aborting: "
+                      + systemFailure.getFailureDetail().getMessage(),
+                  systemFailure.getFailureDetail()),
+              testRunnerAction);
+        }
+        String errorMessage = "Test failed, aborting";
+        throw ActionExecutionException.fromExecException(
+            new TestExecException(
+                errorMessage,
+                FailureDetail.newBuilder()
+                    .setTestAction(
+                        TestAction.newBuilder().setCode(TestAction.Code.NO_KEEP_GOING_TEST_FAILURE))
+                    .setMessage(errorMessage)
+                    .build()),
+            testRunnerAction);
+      }
+      return nextAttemptOrResult;
     }
 
-    private ActionContinuationOrResult process(TestAttemptResult result, int actualMaxAttempts)
-        throws ExecException, IOException, InterruptedException {
+    private ActionContinuationOrResult process(
+        TestAttemptResult result, @Nullable TestRunnerSpawnAndMaxAttempts nextRunnerAndAttempts)
+        throws IOException, InterruptedException {
       spawnResults.addAll(result.spawnResults());
       TestAttemptResult.Result testResult = result.result();
       if (testResult == TestAttemptResult.Result.PASSED) {
         if (cancelFuture != null) {
           cancelFuture.cancel(true);
         }
+        testRunnerSpawn.finalizeTest(result, failedAttempts);
+      } else if (nextRunnerAndAttempts == null) {
+        testRunnerSpawn.finalizeTest(result, failedAttempts);
       } else {
-        TestRunnerSpawnAndMaxAttempts nextRunnerAndAttempts =
-            computeNextRunnerAndMaxAttempts(
-                testResult, testRunnerSpawn, failedAttempts.size() + 1, actualMaxAttempts);
-        if (nextRunnerAndAttempts != null) {
-          failedAttempts.add(
-              testRunnerSpawn.finalizeFailedTestAttempt(result, failedAttempts.size() + 1));
-
-          TestAttemptContinuation nextContinuation =
-              beginIfNotCancelled(nextRunnerAndAttempts.getSpawn(), cancelFuture);
-          if (nextContinuation == null) {
-            testRunnerSpawn.finalizeCancelledTest(failedAttempts);
-            // We need to create the mandatory output files even if we're not going to run anything.
-            testRunnerAction.createEmptyOutputs(testRunnerSpawn.getActionExecutionContext());
-            return ActionContinuationOrResult.of(ActionResult.create(spawnResults));
-          }
-
-          // Change the phase here because we are executing a rerun of the failed attempt.
-          this.testRunnerSpawn
-              .getActionExecutionContext()
-              .getEventHandler()
-              .post(new SpawnExecutedEvent.ChangePhase(this.testRunnerAction));
-
-          return new RunAttemptsContinuation(
-              testRunnerAction,
-              nextRunnerAndAttempts.getSpawn(),
-              nextContinuation,
-              keepGoing,
-              nextRunnerAndAttempts.getMaxAttempts(),
-              spawnResults,
-              failedAttempts,
-              cancelFuture);
+        failedAttempts.add(
+            testRunnerSpawn.finalizeFailedTestAttempt(result, failedAttempts.size() + 1));
+
+        TestAttemptContinuation nextContinuation =
+            beginIfNotCancelled(nextRunnerAndAttempts.getSpawn(), cancelFuture);
+        if (nextContinuation == null) {
+          testRunnerSpawn.finalizeCancelledTest(failedAttempts);
+          // We need to create the mandatory output files even if we're not going to run anything.
+          testRunnerAction.createEmptyOutputs(testRunnerSpawn.getActionExecutionContext());
+          return ActionContinuationOrResult.of(ActionResult.create(spawnResults));
         }
-      }
-      testRunnerSpawn.finalizeTest(result, failedAttempts);
 
-      if (!keepGoing && testResult != TestAttemptResult.Result.PASSED) {
-        DetailedExitCode systemFailure = result.primarySystemFailure();
-        if (systemFailure != null) {
-          throw new TestExecException(
-              "Test failed (system error), aborting: "
-                  + systemFailure.getFailureDetail().getMessage(),
-              systemFailure.getFailureDetail());
-        }
-        String errorMessage = "Test failed, aborting";
-        throw new TestExecException(
-            errorMessage,
-            FailureDetail.newBuilder()
-                .setTestAction(
-                    TestAction.newBuilder().setCode(TestAction.Code.NO_KEEP_GOING_TEST_FAILURE))
-                .setMessage(errorMessage)
-                .build());
+        // Change the phase here because we are executing a rerun of the failed attempt.
+        this.testRunnerSpawn
+            .getActionExecutionContext()
+            .getEventHandler()
+            .post(new SpawnExecutedEvent.ChangePhase(this.testRunnerAction));
+
+        return new RunAttemptsContinuation(
+            testRunnerAction,
+            nextRunnerAndAttempts.getSpawn(),
+            nextContinuation,
+            keepGoing,
+            nextRunnerAndAttempts.getMaxAttempts(),
+            spawnResults,
+            failedAttempts,
+            cancelFuture);
       }
       return ActionContinuationOrResult.of(ActionResult.create(spawnResults));
     }

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

@@ -31,6 +31,7 @@
 import com.google.devtools.build.lib.analysis.config.BuildConfigurationValue;
 import com.google.devtools.build.lib.analysis.config.PerLabelOptions;
 import com.google.devtools.build.lib.analysis.test.TestRunnerAction.ResolvedPaths;
+import com.google.devtools.build.lib.buildeventstream.BuildEventStreamProtos;
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.events.Event;
 import com.google.devtools.build.lib.events.EventKind;
@@ -67,6 +68,16 @@ public abstract class TestStrategy implements TestActionContext {
   private final ConcurrentHashMap<ShardKey, ListenableFuture<Void>> futures =
       new ConcurrentHashMap<>();
 
+  /** Base implementation of TestRunnerSpawn with common functionality across implementations. */
+  public abstract class AbstractTestRunnerSpawn implements TestRunnerSpawn {
+
+    @Override
+    public final void finalizeIncompleteTest(
+        TestRunnerAction action, List<FailedAttemptResult> failedAttempts) {
+      TestStrategy.this.finalizeIncompleteTest(action, getActionExecutionContext(), failedAttempts);
+    }
+  }
+
   /**
    * Ensures that all directories used to run test are in the correct state and their content will
    * not result in stale files.
@@ -218,7 +229,6 @@ private static void addRunUnderArgs(
    * <p>For rules with "flaky = 1" attribute, this method will return 3 unless --flaky_test_attempts
    * option is given and specifies another value.
    */
-  @VisibleForTesting /* protected */
   public int getTestAttempts(TestRunnerAction action) {
     return action.getTestProperties().isFlaky()
         ? getTestAttemptsForFlakyTest(action)
@@ -262,12 +272,54 @@ protected static final Duration getTimeout(TestRunnerAction testAction) {
         .get(testAction.getTestProperties().getTimeout());
   }
 
-  /*
-   * Finalize test run: persist the result, and post on the event bus.
+  /** Converts the test results to a TestAttemptResult. */
+  public abstract TestActionContext.TestAttemptResult makeIncompleteTestResult();
+
+  /**
+   * Post the final test result when a test execution is incomplete, perhaps due to environmental
+   * failures.
    */
-  protected void postTestResult(ActionExecutionContext actionExecutionContext, TestResult result)
-      throws IOException {
+  @Override
+  public void finalizeIncompleteTest(
+      TestRunnerAction action,
+      ActionExecutionContext actionExecutionContext,
+      List<FailedAttemptResult> failedAttempts) {
+    TestAttemptResult testResult = makeIncompleteTestResult();
+    TestResultData.Builder data = testResult.testResultDataBuilder();
+    BuildEventStreamProtos.TestResult.ExecutionInfo executionInfo = testResult.executionInfo();
+    int attemptId = failedAttempts.size() + 1;
+
+    TestAttempt testAttempt =
+        TestAttempt.forExecutedTestResult(
+            action,
+            data.build(),
+            attemptId,
+            /*files=*/ ImmutableList.of(),
+            executionInfo,
+            /*lastAttempt=*/ true);
+    TestResult result =
+        new TestResult(action, data.build(), false, testResult.primarySystemFailure());
+    actionExecutionContext.getEventHandler().post(testAttempt);
+    postTestResultNeverCached(actionExecutionContext, result);
+  }
+
+  /** Report test run: post on the event bus, and write the result to disk cache. */
+  public final void postTestResultCached(
+      ActionExecutionContext actionExecutionContext, TestResult result) throws IOException {
     result.getTestAction().saveCacheStatus(actionExecutionContext, result.getData());
+    postTestResultNeverCached(actionExecutionContext, result);
+  }
+
+  /**
+   * Report a test run to the event bus.
+   *
+   * <p>Called by {@link #postTestResultCached(ActionExecutionContext, TestResult)}. May be called
+   * directly for results unsuitable for caching (eg. environmental failure to run the test
+   * executable).
+   */
+  // Not final to support testing.
+  protected void postTestResultNeverCached(
+      ActionExecutionContext actionExecutionContext, TestResult result) {
     actionExecutionContext.getEventHandler().post(result);
   }
 

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

@@ -38,8 +38,10 @@ public TestActionContext.TestAttemptResult.Result result() {
   public abstract ImmutableList<SpawnResult> spawnResults();
 
   /** Returns the TestResultData for the test. */
+  @Override
   public abstract TestResultData.Builder testResultDataBuilder();
 
+  @Override
   public abstract BuildEventStreamProtos.TestResult.ExecutionInfo executionInfo();
 
   /** Returns a builder that can be used to construct a {@link StandaloneTestResult} object. */

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

@@ -234,7 +234,16 @@ private StandaloneFailedAttemptResult processFailedTestAttempt(
         attemptId, /*isLastAttempt=*/ false, actionExecutionContext, action, result);
   }
 
-  private void finalizeTest(
+  @Override
+  public TestAttemptResult makeIncompleteTestResult() {
+    return StandaloneTestResult.builder()
+        .setSpawnResults(ImmutableList.of())
+        .setExecutionInfo(ExecutionInfo.getDefaultInstance())
+        .setTestResultDataBuilder(TestResultData.newBuilder().setStatus(BlazeTestStatus.INCOMPLETE))
+        .build();
+  }
+
+  public void finalizeTest(
       TestRunnerAction action,
       ActionExecutionContext actionExecutionContext,
       StandaloneTestResult standaloneTestResult,
@@ -261,7 +270,7 @@ private void finalizeTest(
     TestResultData data = dataBuilder.build();
     TestResult result =
         new TestResult(action, data, false, standaloneTestResult.primarySystemFailure());
-    postTestResult(actionExecutionContext, result);
+    postTestResultCached(actionExecutionContext, result);
   }
 
   private StandaloneFailedAttemptResult processTestAttempt(
@@ -639,7 +648,7 @@ TestResultData testResultData() {
     }
   }
 
-  private final class StandaloneTestRunnerSpawn implements TestRunnerSpawn {
+  private final class StandaloneTestRunnerSpawn extends AbstractTestRunnerSpawn {
     private final TestRunnerAction testAction;
     private final ActionExecutionContext actionExecutionContext;
     private final Spawn spawn;