Refactor TestAttempt event posting code

For flaky tests, Bazel may have cached information about multiple test attempts. In that case, we might want to post all of them on a subsequent cache hit, rather than posting only the passing attempt.

We currently subclass TestResult inside Google, which overrides the new getCachedTestAttempts method.

PiperOrigin-RevId: 196491575

4 files changed, 73 insertions, 58 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/analysis/test/TestResult.java b/src/main/java/com/google/devtools/build/lib/analysis/test/TestResult.java
index 37ddad8b82..a8ef2acb40 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/test/TestResult.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/test/TestResult.java
@@ -15,16 +15,19 @@
 package com.google.devtools.build.lib.analysis.test;
 
 import com.google.common.base.Preconditions;
+import com.google.common.collect.ImmutableList;
 import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.buildeventstream.BuildEventStreamProtos;
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable;
 import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe;
+import com.google.devtools.build.lib.exec.TestAttempt;
 import com.google.devtools.build.lib.util.Pair;
 import com.google.devtools.build.lib.vfs.Path;
 import com.google.devtools.build.lib.view.test.TestStatus.BlazeTestStatus;
 import com.google.devtools.build.lib.view.test.TestStatus.TestResultData;
 import java.util.Collection;
+import java.util.List;
 import javax.annotation.Nullable;
 
 /**
@@ -89,6 +92,22 @@ public class TestResult {
   }
 
   /**
+   * Returns the list of locally cached test attempts. This method must only be called if {@link
+   * #isCached} returns <code>true</code>.
+   */
+  public List<TestAttempt> getCachedTestAttempts() {
+    Preconditions.checkState(isCached());
+    return ImmutableList.of(
+        TestAttempt.fromCachedTestResult(
+            testAction,
+            data,
+            1,
+            getFiles(),
+            BuildEventStreamProtos.TestResult.ExecutionInfo.getDefaultInstance(),
+            /*lastAttempt=*/ true));
+  }
+
+  /**
    * @return Coverage data artifact, if available and null otherwise.
    */
   public Path getCoverageData() {
@@ -106,10 +125,6 @@ public class TestResult {
     return testAction.getCacheStatusArtifact();
   }
 
-  public BuildEventStreamProtos.TestResult.ExecutionInfo getExecutionInfo() {
-    return BuildEventStreamProtos.TestResult.ExecutionInfo.getDefaultInstance();
-  }
-
   /**
    * Gets the test name in a user-friendly format.
    * Will generally include the target name and shard number, if applicable.
@@ -150,7 +165,8 @@ public class TestResult {
    * @return Collection of files created by the test, tagged by their name indicating usage (e.g.,
    *     "test.log").
    */
-  public Collection<Pair<String, Path>> getFiles() {
+  private Collection<Pair<String, Path>> getFiles() {
+    // TODO(ulfjack): Cache the set of generated files in the TestResultData.
     return testAction.getTestOutputsMapping(execRoot);
   }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/exec/StandaloneTestStrategy.java b/src/main/java/com/google/devtools/build/lib/exec/StandaloneTestStrategy.java
index c1248a6e4b..91d829712a 100644
--- a/src/main/java/com/google/devtools/build/lib/exec/StandaloneTestStrategy.java
+++ b/src/main/java/com/google/devtools/build/lib/exec/StandaloneTestStrategy.java
@@ -165,7 +165,7 @@ public class StandaloneTestStrategy extends TestStrategy {
       actionExecutionContext
           .getEventHandler()
           .post(
-              new TestAttempt(
+              TestAttempt.forExecutedTestResult(
                   action,
                   standaloneTestResult.executionInfo(),
                   attempt,
@@ -242,7 +242,7 @@ public class StandaloneTestStrategy extends TestStrategy {
     actionExecutionContext
         .getEventHandler()
         .post(
-            new TestAttempt(
+            TestAttempt.forExecutedTestResult(
                 action,
                 result.executionInfo(),
                 attempt,
diff --git a/src/main/java/com/google/devtools/build/lib/exec/TestAttempt.java b/src/main/java/com/google/devtools/build/lib/exec/TestAttempt.java
index a7a4db679e..f32336c6b6 100644
--- a/src/main/java/com/google/devtools/build/lib/exec/TestAttempt.java
+++ b/src/main/java/com/google/devtools/build/lib/exec/TestAttempt.java
@@ -15,9 +15,9 @@
 package com.google.devtools.build.lib.exec;
 
 import com.google.common.annotations.VisibleForTesting;
+import com.google.common.base.Preconditions;
 import com.google.common.collect.ImmutableList;
 import com.google.devtools.build.lib.actions.Artifact;
-import com.google.devtools.build.lib.analysis.test.TestResult;
 import com.google.devtools.build.lib.analysis.test.TestRunnerAction;
 import com.google.devtools.build.lib.buildeventstream.BuildEventConverters;
 import com.google.devtools.build.lib.buildeventstream.BuildEventId;
@@ -34,6 +34,9 @@ import java.util.Collection;
 import java.util.List;
 
 /** This event is raised whenever an individual test attempt is completed. */
+// TODO(ulfjack): This class should be in the same package as the TestResult class, and TestSummary
+// should live there, too. It's depended upon by TestRunnerAction / TestActionContext, which
+// suggests that it should live in the analysis.test package.
 public class TestAttempt implements BuildEventWithOrderConstraint {
 
   private final TestRunnerAction testAction;
@@ -54,11 +57,11 @@ public class TestAttempt implements BuildEventWithOrderConstraint {
    * @param testAction The test that was run.
    * @param attempt The number of the attempt for this action.
    */
-  public TestAttempt(
+  private TestAttempt(
       boolean cachedLocally,
       TestRunnerAction testAction,
       BuildEventStreamProtos.TestResult.ExecutionInfo executionInfo,
-      Integer attempt,
+      int attempt,
       BlazeTestStatus status,
       long startTimeMillis,
       long durationMillis,
@@ -66,72 +69,62 @@ public class TestAttempt implements BuildEventWithOrderConstraint {
       List<String> testWarnings,
       boolean lastAttempt) {
     this.testAction = testAction;
-    this.executionInfo = executionInfo;
+    this.executionInfo = Preconditions.checkNotNull(executionInfo);
     this.attempt = attempt;
-    this.status = status;
+    this.status = Preconditions.checkNotNull(status);
     this.cachedLocally = cachedLocally;
     this.startTimeMillis = startTimeMillis;
     this.durationMillis = durationMillis;
-    this.files = files;
-    this.testWarnings = testWarnings;
+    this.files = Preconditions.checkNotNull(files);
+    this.testWarnings = Preconditions.checkNotNull(testWarnings);
     this.lastAttempt = lastAttempt;
   }
 
-  public TestAttempt(
-      boolean cachedLocally,
-      TestRunnerAction testAction,
-      Integer attempt,
-      BlazeTestStatus status,
-      long startTimeMillis,
-      long durationMillis,
-      Collection<Pair<String, Path>> files,
-      List<String> testWarnings,
-      boolean lastAttempt) {
-    this(cachedLocally, testAction,
-        BuildEventStreamProtos.TestResult.ExecutionInfo.getDefaultInstance(), attempt, status,
-        startTimeMillis, durationMillis, files, testWarnings, lastAttempt);
-  }
-
-  public TestAttempt(
+  /**
+   * Creates a test attempt result instance for a test that was not locally cached; it may have been
+   * locally executed, remotely executed, or remotely cached.
+   */
+  public static TestAttempt forExecutedTestResult(
       TestRunnerAction testAction,
       BuildEventStreamProtos.TestResult.ExecutionInfo executionInfo,
-      Integer attempt,
+      int attempt,
       BlazeTestStatus status,
       long startTimeMillis,
       long durationMillis,
       Collection<Pair<String, Path>> files,
       List<String> testWarnings,
       boolean lastAttempt) {
-    this(false, testAction, executionInfo, attempt, status, startTimeMillis, durationMillis, files,
-        testWarnings, lastAttempt);
+    return new TestAttempt(
+        false,
+        testAction,
+        executionInfo,
+        attempt,
+        status,
+        startTimeMillis,
+        durationMillis,
+        files,
+        testWarnings,
+        lastAttempt);
   }
 
-  public TestAttempt(
+  public static TestAttempt fromCachedTestResult(
       TestRunnerAction testAction,
-      Integer attempt,
-      BlazeTestStatus status,
-      long startTimeMillis,
-      long durationMillis,
+      TestResultData attemptData,
+      int attempt,
       Collection<Pair<String, Path>> files,
-      List<String> testWarnings,
+      BuildEventStreamProtos.TestResult.ExecutionInfo executionInfo,
       boolean lastAttempt) {
-    this(testAction,  BuildEventStreamProtos.TestResult.ExecutionInfo.getDefaultInstance(), attempt,
-        status, startTimeMillis, durationMillis, files, testWarnings, lastAttempt);
-  }
-
-  public static TestAttempt fromCachedTestResult(TestResult result) {
-    TestResultData data = result.getData();
     return new TestAttempt(
         true,
-        result.getTestAction(),
-        result.getExecutionInfo(),
-        1,
-        data.getStatus(),
-        data.getStartTimeMillisEpoch(),
-        data.getRunDurationMillis(),
-        result.getFiles(),
-        result.getData().getWarningList(),
-        true);
+        testAction,
+        executionInfo,
+        attempt,
+        attemptData.getStatus(),
+        attemptData.getStartTimeMillisEpoch(),
+        attemptData.getRunDurationMillis(),
+        files,
+        attemptData.getWarningList(),
+        lastAttempt);
   }
 
   @VisibleForTesting
@@ -154,6 +147,11 @@ public class TestAttempt implements BuildEventWithOrderConstraint {
     return status;
   }
 
+  @VisibleForTesting
+  public boolean isCachedLocally() {
+    return cachedLocally;
+  }
+
   @Override
   public BuildEventId getEventId() {
     return BuildEventId.testResult(
diff --git a/src/main/java/com/google/devtools/build/lib/runtime/AggregatingTestListener.java b/src/main/java/com/google/devtools/build/lib/runtime/AggregatingTestListener.java
index 8b021c522e..9e851aa509 100644
--- a/src/main/java/com/google/devtools/build/lib/runtime/AggregatingTestListener.java
+++ b/src/main/java/com/google/devtools/build/lib/runtime/AggregatingTestListener.java
@@ -127,10 +127,11 @@ public class AggregatingTestListener {
     ConfiguredTargetKey targetLabel =
         ConfiguredTargetKey.of(testOwner.getLabel(), result.getTestAction().getConfiguration());
 
-    // If a test result was cached, then no attempts for that test were actually
-    // executed. Hence report that fact as a cached attempt.
+    // If a test result was cached, then post the cached attempts to the event bus.
     if (result.isCached()) {
-      eventBus.post(TestAttempt.fromCachedTestResult(result));
+      for (TestAttempt attempt : result.getCachedTestAttempts()) {
+        eventBus.post(attempt);
+      }
     }
 
     TestSummary finalTestSummary = null;
@@ -142,7 +143,7 @@ public class AggregatingTestListener {
         // This situation is likely to happen if --notest_keep_going is set with multiple targets.
         return;
       }
-     
+
       summary = analyzer.incrementalAnalyze(summary, result);
 
       // If all runs are processed, the target is finished and ready to report.