Simplify TestRunnerAction a bit
- Remove one continuation wrapper from beginExecution; this is not
actually necessary anymore since https://github.com/bazelbuild/bazel/commit/415165a55ca0f1656d1254618758d2a30d8c5030
- Remove duplicate catch clauses
- Remove manual printing of the stack trace; the
EnvironmentalExecException now always comes with a stack trace as of
https://github.com/bazelbuild/bazel/commit/572bc144e77864fb0458155b432547af9cb260a0
- Remove a micro-optimization
PiperOrigin-RevId: 274591280
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/test/TestActionContext.java b/src/main/java/com/google/devtools/build/lib/analysis/test/TestActionContext.java
index d46462d..67a8503 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/test/TestActionContext.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/test/TestActionContext.java
@@ -127,8 +127,7 @@
* return a completed result, or it may return a continuation with a non-null future
* representing asynchronous execution.
*/
- TestAttemptContinuation beginExecution()
- throws InterruptedException, IOException, ExecException;
+ TestAttemptContinuation beginExecution() throws InterruptedException, IOException;
/**
* After the first attempt has run, this method is called to determine the maximum number of
@@ -151,7 +150,7 @@
* attempts are exhausted and then run with another strategy for another set of attempts. This
* is rarely used, and should ideally be removed.
*/
- default TestRunnerSpawn getFallbackRunner() throws ExecException, InterruptedException {
+ default TestRunnerSpawn getFallbackRunner() throws ExecException {
return null;
}
}
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/test/TestRunnerAction.java b/src/main/java/com/google/devtools/build/lib/analysis/test/TestRunnerAction.java
index 978a4c3..2b7b7bc 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/test/TestRunnerAction.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/test/TestRunnerAction.java
@@ -16,7 +16,6 @@
import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Preconditions;
-import com.google.common.base.Throwables;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;
@@ -50,7 +49,6 @@
import com.google.devtools.build.lib.buildeventstream.TestFileNameConstants;
import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.collect.ImmutableIterable;
-import com.google.devtools.build.lib.events.Event;
import com.google.devtools.build.lib.util.Fingerprint;
import com.google.devtools.build.lib.util.LoggingUtil;
import com.google.devtools.build.lib.util.Pair;
@@ -748,26 +746,15 @@
try {
TestRunnerSpawn testRunnerSpawn =
testActionContext.createTestRunnerSpawn(this, actionExecutionContext);
- TestAttemptContinuation beginContinuation =
- new TestAttemptContinuation() {
- @Nullable
- @Override
- public ListenableFuture<?> getFuture() {
- return null;
- }
-
- @Override
- public TestAttemptContinuation execute()
- throws InterruptedException, IOException, ExecException {
- return testRunnerSpawn.beginExecution();
- }
- };
+ TestAttemptContinuation beginContinuation = testRunnerSpawn.beginExecution();
RunAttemptsContinuation continuation =
new RunAttemptsContinuation(
testRunnerSpawn, beginContinuation, testActionContext.isTestKeepGoing());
- return continuation.execute();
+ return continuation;
} catch (ExecException e) {
throw e.toActionExecutionException(this);
+ } catch (IOException e) {
+ throw new EnvironmentalExecException(e).toActionExecutionException(this);
}
}
@@ -945,7 +932,6 @@
int maxAttempts,
List<SpawnResult> spawnResults,
List<FailedAttemptResult> failedAttempts) {
- Preconditions.checkArgument(!testContinuation.isDone());
this.testRunnerSpawn = testRunnerSpawn;
this.testContinuation = testContinuation;
this.keepGoing = keepGoing;
@@ -995,59 +981,41 @@
}
private ActionContinuationOrResult process(TestAttemptResult result, int actualMaxAttempts)
- throws ActionExecutionException, InterruptedException {
- try {
- spawnResults.addAll(result.spawnResults());
- if (!result.hasPassed()) {
- boolean runAnotherAttempt = failedAttempts.size() + 1 < actualMaxAttempts;
- TestRunnerSpawn nextRunner;
- if (runAnotherAttempt) {
- nextRunner = testRunnerSpawn;
- } else {
- nextRunner = testRunnerSpawn.getFallbackRunner();
- if (nextRunner != null) {
- // We only support one level of fallback, in which case this gets doubled once. We
- // don't support a different number of max attempts for the fallback strategy.
- actualMaxAttempts = 2 * actualMaxAttempts;
- }
- }
+ throws ExecException, IOException, InterruptedException {
+ spawnResults.addAll(result.spawnResults());
+ if (!result.hasPassed()) {
+ boolean runAnotherAttempt = failedAttempts.size() + 1 < actualMaxAttempts;
+ TestRunnerSpawn nextRunner;
+ if (runAnotherAttempt) {
+ nextRunner = testRunnerSpawn;
+ } else {
+ nextRunner = testRunnerSpawn.getFallbackRunner();
if (nextRunner != null) {
- failedAttempts.add(
- testRunnerSpawn.finalizeFailedTestAttempt(result, failedAttempts.size() + 1));
-
- TestAttemptContinuation nextContinuation = nextRunner.beginExecution();
- if (nextContinuation.isDone()) {
- // This avoids unnecessary creation of RunAttemptsContinuation objects, but does a
- // recursive call in exchange. We don't allow more than 10 flaky attempts, so that
- // should be fine.
- return process(nextContinuation.get(), actualMaxAttempts);
- }
- return new RunAttemptsContinuation(
- nextRunner,
- nextContinuation,
- keepGoing,
- actualMaxAttempts,
- spawnResults,
- failedAttempts);
+ // We only support one level of fallback, in which case this gets doubled once. We
+ // don't support a different number of max attempts for the fallback strategy.
+ actualMaxAttempts = 2 * actualMaxAttempts;
}
}
- testRunnerSpawn.finalizeTest(result, failedAttempts);
+ if (nextRunner != null) {
+ failedAttempts.add(
+ testRunnerSpawn.finalizeFailedTestAttempt(result, failedAttempts.size() + 1));
- if (!keepGoing && !result.hasPassed()) {
- throw new TestExecException("Test failed: aborting");
+ TestAttemptContinuation nextContinuation = nextRunner.beginExecution();
+ return new RunAttemptsContinuation(
+ nextRunner,
+ nextContinuation,
+ keepGoing,
+ actualMaxAttempts,
+ spawnResults,
+ failedAttempts);
}
- return ActionContinuationOrResult.of(ActionResult.create(spawnResults));
- } catch (ExecException e) {
- throw e.toActionExecutionException(TestRunnerAction.this);
- } catch (IOException e) {
- // Print the stack trace, otherwise the unexpected I/O error is hard to diagnose.
- // A stack trace could help with bugs like https://github.com/bazelbuild/bazel/issues/4924
- testRunnerSpawn
- .getActionExecutionContext()
- .getEventHandler()
- .handle(Event.error(Throwables.getStackTraceAsString(e)));
- throw new EnvironmentalExecException(e).toActionExecutionException(TestRunnerAction.this);
}
+ testRunnerSpawn.finalizeTest(result, failedAttempts);
+
+ if (!keepGoing && !result.hasPassed()) {
+ throw new TestExecException("Test failed: aborting");
+ }
+ return ActionContinuationOrResult.of(ActionResult.create(spawnResults));
}
}
}
