From b22c7ee8ae8f7403a0e2b7e8f9a7ade4d5979429 Mon Sep 17 00:00:00 2001 From: ulfjack Date: Mon, 14 May 2018 05:32:03 -0700 Subject: 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 --- .../build/lib/analysis/test/TestResult.java | 26 ++++-- .../build/lib/exec/StandaloneTestStrategy.java | 4 +- .../devtools/build/lib/exec/TestAttempt.java | 92 +++++++++++----------- .../build/lib/runtime/AggregatingTestListener.java | 9 ++- 4 files changed, 73 insertions(+), 58 deletions(-) (limited to 'src/main/java/com/google/devtools') 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; /** @@ -88,6 +91,22 @@ public class TestResult { return cached; } + /** + * Returns the list of locally cached test attempts. This method must only be called if {@link + * #isCached} returns true. + */ + public List 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. */ @@ -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> getFiles() { + private Collection> 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 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> files, - List 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> files, List 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> files, - List 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. -- cgit v1.2.3