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)); } } }