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
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 37ddad8..a8ef2ac 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 @@
}
/**
+ * 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 @@
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 @@
* @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 c1248a6..91d8297 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 @@
actionExecutionContext
.getEventHandler()
.post(
- new TestAttempt(
+ TestAttempt.forExecutedTestResult(
action,
standaloneTestResult.executionInfo(),
attempt,
@@ -242,7 +242,7 @@
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 a7a4db6..f32336c 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.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 @@
* @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 @@
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 @@
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 8b021c5..9e851aa 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 @@
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 @@
// 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.
