Refactor StandaloneTestStrategy with new interfaces
This change adds a set of generic interfaces to TestStrategy, in terms
of which all existing strategies can be implemented. It also refactors
StandaloneTestStrategy to do so.
This increases sharing at the test strategy level, which is in
preparation for supporting async test execution.
Note that this fixes the collection of SpawnResult instances as a side
effect - previously we only kept the last SpawnResult, now we collect
all of them.
PiperOrigin-RevId: 235914420
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 885f42b..f36b4e0 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
@@ -41,4 +41,57 @@
*/
TestResult newCachedTestResult(Path execRoot, TestRunnerAction action, TestResultData cached)
throws IOException;
+
+ /**
+ * An object representing an individual test attempt result. Note that {@link TestRunnerSpawn} is
+ * generic in a subtype of this type; this interface only provide a tiny amount of generic
+ * top-level functionality necessary to share code between the different {@link TestActionContext}
+ * implementations.
+ */
+ interface TestAttemptResult {
+ /** Returns {@code true} if the test attempt passed successfully. */
+ boolean hasPassed();
+
+ /** Returns a list of spawn results for this test attempt. */
+ List<SpawnResult> spawnResults();
+ }
+
+ /**
+ * An object representing a failed non-final attempt. This is only used for tests that are run
+ * multiple times. At this time, Bazel retries tests until the first passed attempt, or until the
+ * number of retries is exhausted - whichever comes first. This interface represents the result
+ * from a previous attempt, but never the final attempt, even if unsuccessful.
+ */
+ interface FailedAttemptResult {}
+
+ /** A delegate to run a test. This may include running multiple spawns, renaming outputs, etc. */
+ interface TestRunnerSpawn {
+ /** Execute the test, and handle the test runner protocol. */
+ TestAttemptResult execute() throws InterruptedException, IOException, ExecException;
+
+ /**
+ * After the first attempt has run, this method is called to determine the maximum number of
+ * attempts for this test.
+ */
+ int getMaxAttempts(TestAttemptResult firstTestAttemptResult);
+
+ /** Rename the output files if the test attempt failed, and post the test attempt result. */
+ FailedAttemptResult finalizeFailedTestAttempt(TestAttemptResult testAttemptResult, int attempt)
+ throws IOException;
+
+ /** Post the final test result based on the last attempt and the list of failed attempts. */
+ void finalizeTest(
+ TestAttemptResult lastTestAttemptResult, List<FailedAttemptResult> failedAttempts)
+ throws IOException, ExecException;
+
+ /**
+ * Return a {@link TestRunnerSpawn} object if test fallback is enabled, or {@code null}
+ * otherwise. Test fallback is a feature to allow a test to run with one strategy until the max
+ * 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 {
+ return null;
+ }
+ }
}
diff --git a/src/main/java/com/google/devtools/build/lib/exec/StandaloneTestResult.java b/src/main/java/com/google/devtools/build/lib/exec/StandaloneTestResult.java
index 047f3de..6f311b7 100644
--- a/src/main/java/com/google/devtools/build/lib/exec/StandaloneTestResult.java
+++ b/src/main/java/com/google/devtools/build/lib/exec/StandaloneTestResult.java
@@ -24,12 +24,14 @@
/** Contains information about the results of test execution. */
@AutoValue
-public abstract class StandaloneTestResult {
+public abstract class StandaloneTestResult implements TestStrategy.TestAttemptResult {
+ @Override
public boolean hasPassed() {
return testResultDataBuilder().getStatus() == BlazeTestStatus.PASSED;
}
/** Returns the SpawnResults created by the test, if any. */
+ @Override
public abstract List<SpawnResult> spawnResults();
/** Returns the TestResultData for the test. */
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 21cd23a..6885355 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
@@ -22,7 +22,6 @@
import com.google.devtools.build.lib.actions.ActionExecutionContext;
import com.google.devtools.build.lib.actions.ActionInputHelper;
import com.google.devtools.build.lib.actions.Artifact;
-import com.google.devtools.build.lib.actions.EnvironmentalExecException;
import com.google.devtools.build.lib.actions.ExecException;
import com.google.devtools.build.lib.actions.ExecutionRequirements;
import com.google.devtools.build.lib.actions.ExecutionStrategy;
@@ -95,40 +94,11 @@
public List<SpawnResult> exec(
TestRunnerAction action, ActionExecutionContext actionExecutionContext)
throws ExecException, InterruptedException {
- StandaloneTestRunnerSpawn testRunnerSpawn =
- createStandaloneTestRunnerSpawn(action, actionExecutionContext);
-
- List<TestResultData> failedAttempts = new ArrayList<>();
- try {
- int maxAttempts = getTestAttempts(action);
- StandaloneTestResult standaloneTestResult = testRunnerSpawn.execute();
- int attempt;
- for (attempt = 1; !standaloneTestResult.hasPassed() && attempt < maxAttempts; attempt++) {
- failedAttempts.add(
- processFailedTestAttempt(
- attempt, actionExecutionContext, action, standaloneTestResult));
- standaloneTestResult = testRunnerSpawn.execute();
- }
-
- finalizeTest(action, actionExecutionContext, standaloneTestResult, failedAttempts, attempt);
-
- // TODO(b/62588075): Should we accumulate SpawnResults across test attempts instead of only
- // returning the last list?
- return standaloneTestResult.spawnResults();
- } 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
- StringBuilder sb = new StringBuilder();
- sb.append("Caught I/O exception: ").append(e.getMessage());
- for (Object s : e.getStackTrace()) {
- sb.append("\n\t").append(s);
- }
- actionExecutionContext.getEventHandler().handle(Event.error(sb.toString()));
- throw new EnvironmentalExecException("unexpected I/O exception", e);
- }
+ TestRunnerSpawn testRunnerSpawn = createTestRunnerSpawn(action, actionExecutionContext);
+ return runAttempts(testRunnerSpawn, actionExecutionContext);
}
- private StandaloneTestRunnerSpawn createStandaloneTestRunnerSpawn(
+ protected TestRunnerSpawn createTestRunnerSpawn(
TestRunnerAction action, ActionExecutionContext actionExecutionContext)
throws ExecException, InterruptedException {
Path execRoot = actionExecutionContext.getExecRoot();
@@ -178,7 +148,7 @@
action, actionExecutionContext, spawn, tmpDir, coverageDir, workingDirectory, execRoot);
}
- private TestResultData processFailedTestAttempt(
+ private StandaloneFailedAttemptResult processFailedTestAttempt(
int attempt,
ActionExecutionContext actionExecutionContext,
TestRunnerAction action,
@@ -245,7 +215,7 @@
result.executionInfo(),
false));
processTestOutput(actionExecutionContext, new TestResult(action, data, false), testLog);
- return data;
+ return new StandaloneFailedAttemptResult(data);
}
private Map<String, String> setupEnvironment(
@@ -518,19 +488,21 @@
TestRunnerAction action,
ActionExecutionContext actionExecutionContext,
StandaloneTestResult standaloneTestResult,
- List<TestResultData> failedAttempts,
- int attempt)
+ List<FailedAttemptResult> failedAttempts)
throws IOException, ExecException {
TestResultData.Builder dataBuilder = standaloneTestResult.testResultDataBuilder();
- for (TestResultData failedAttempt : failedAttempts) {
- dataBuilder.addAllFailedLogs(failedAttempt.getFailedLogsList());
- dataBuilder.addTestTimes(failedAttempt.getTestTimes(0));
- dataBuilder.addAllTestProcessTimes(failedAttempt.getTestProcessTimesList());
+ for (FailedAttemptResult failedAttempt : failedAttempts) {
+ TestResultData failedAttemptData =
+ ((StandaloneFailedAttemptResult) failedAttempt).testResultData;
+ dataBuilder.addAllFailedLogs(failedAttemptData.getFailedLogsList());
+ dataBuilder.addTestTimes(failedAttemptData.getTestTimes(0));
+ dataBuilder.addAllTestProcessTimes(failedAttemptData.getTestProcessTimesList());
}
ImmutableList<Pair<String, Path>> testOutputs =
action.getTestOutputsMapping(
actionExecutionContext.getPathResolver(), actionExecutionContext.getExecRoot());
TestResultData data = dataBuilder.build();
+ int attempt = failedAttempts.size() + 1;
actionExecutionContext
.getEventHandler()
.post(
@@ -563,7 +535,15 @@
return new TestResult(action, data, /*cached*/ true, execRoot);
}
- private final class StandaloneTestRunnerSpawn {
+ private final class StandaloneFailedAttemptResult implements FailedAttemptResult {
+ private final TestResultData testResultData;
+
+ StandaloneFailedAttemptResult(TestResultData testResultData) {
+ this.testResultData = testResultData;
+ }
+ }
+
+ private final class StandaloneTestRunnerSpawn implements TestRunnerSpawn {
private final TestRunnerAction testAction;
private final ActionExecutionContext actionExecutionContext;
private final Spawn spawn;
@@ -589,7 +569,7 @@
this.execRoot = execRoot;
}
- StandaloneTestResult execute() throws InterruptedException, IOException, ExecException {
+ public StandaloneTestResult execute() throws InterruptedException, IOException, ExecException {
return executeTestAttempt(
testAction,
spawn,
@@ -599,5 +579,25 @@
tmpDir,
workingDirectory);
}
+
+ @Override
+ public int getMaxAttempts(TestAttemptResult firstTestAttemptResult) {
+ return getTestAttempts(testAction);
+ }
+
+ @Override
+ public FailedAttemptResult finalizeFailedTestAttempt(
+ TestAttemptResult testAttemptResult, int attempt) throws IOException {
+ return processFailedTestAttempt(
+ attempt, actionExecutionContext, testAction, (StandaloneTestResult) testAttemptResult);
+ }
+
+ @Override
+ public void finalizeTest(
+ TestAttemptResult finalResult, List<FailedAttemptResult> failedAttempts)
+ throws IOException, ExecException {
+ StandaloneTestStrategy.this.finalizeTest(
+ testAction, actionExecutionContext, (StandaloneTestResult) finalResult, failedAttempts);
+ }
}
}
diff --git a/src/main/java/com/google/devtools/build/lib/exec/TestStrategy.java b/src/main/java/com/google/devtools/build/lib/exec/TestStrategy.java
index 737d7ca..efb31ea 100644
--- a/src/main/java/com/google/devtools/build/lib/exec/TestStrategy.java
+++ b/src/main/java/com/google/devtools/build/lib/exec/TestStrategy.java
@@ -23,6 +23,7 @@
import com.google.devtools.build.lib.actions.ActionExecutionContext;
import com.google.devtools.build.lib.actions.Artifact;
import com.google.devtools.build.lib.actions.CommandLineExpansionException;
+import com.google.devtools.build.lib.actions.EnvironmentalExecException;
import com.google.devtools.build.lib.actions.ExecException;
import com.google.devtools.build.lib.actions.SpawnResult;
import com.google.devtools.build.lib.actions.TestExecException;
@@ -51,6 +52,7 @@
import java.io.IOException;
import java.io.InputStream;
import java.time.Duration;
+import java.util.ArrayList;
import java.util.Arrays;
import java.util.HashMap;
import java.util.List;
@@ -130,6 +132,62 @@
TestRunnerAction action, ActionExecutionContext actionExecutionContext)
throws ExecException, InterruptedException;
+ protected List<SpawnResult> runAttempts(
+ TestRunnerSpawn testRunnerSpawn, ActionExecutionContext actionExecutionContext)
+ throws ExecException, InterruptedException {
+ List<SpawnResult> spawnResults = new ArrayList<>();
+ runAttempts(testRunnerSpawn, actionExecutionContext, spawnResults, new ArrayList<>());
+ return spawnResults;
+ }
+
+ private void runAttempts(
+ TestRunnerSpawn testRunnerSpawn,
+ ActionExecutionContext actionExecutionContext,
+ List<SpawnResult> spawnResults,
+ List<FailedAttemptResult> failedAttempts)
+ throws ExecException, InterruptedException {
+ try {
+ TestAttemptResult testProcessResult = testRunnerSpawn.execute();
+ spawnResults.addAll(testProcessResult.spawnResults());
+ int maxAttempts = testRunnerSpawn.getMaxAttempts(testProcessResult);
+ // Failed test retry loop.
+ for (int attempt = 1; attempt < maxAttempts && !testProcessResult.hasPassed(); attempt++) {
+ failedAttempts.add(
+ testRunnerSpawn.finalizeFailedTestAttempt(
+ testProcessResult, failedAttempts.size() + 1));
+
+ testProcessResult = testRunnerSpawn.execute();
+ spawnResults.addAll(testProcessResult.spawnResults());
+ }
+
+ TestRunnerSpawn fallbackRunner;
+ if (!testProcessResult.hasPassed()
+ && (fallbackRunner = testRunnerSpawn.getFallbackRunner()) != null) {
+ // All attempts failed and fallback is enabled.
+ failedAttempts.add(
+ testRunnerSpawn.finalizeFailedTestAttempt(
+ testProcessResult, failedAttempts.size() + 1));
+ runAttempts(fallbackRunner, actionExecutionContext, spawnResults, failedAttempts);
+ } else {
+ testRunnerSpawn.finalizeTest(testProcessResult, failedAttempts);
+
+ if (!executionOptions.testKeepGoing && !testProcessResult.hasPassed()) {
+ throw new TestExecException("Test failed: aborting");
+ }
+ }
+ } 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
+ StringBuilder sb = new StringBuilder();
+ sb.append("Caught I/O exception: ").append(e.getMessage());
+ for (Object s : e.getStackTrace()) {
+ sb.append("\n\t").append(s);
+ }
+ actionExecutionContext.getEventHandler().handle(Event.error(sb.toString()));
+ throw new EnvironmentalExecException("unexpected I/O exception", e);
+ }
+ }
+
/**
* Generates a command line to run for the test action, taking into account coverage and {@code
* --run_under} settings.
diff --git a/src/test/java/com/google/devtools/build/lib/exec/StandaloneTestStrategyTest.java b/src/test/java/com/google/devtools/build/lib/exec/StandaloneTestStrategyTest.java
index b499f26..5ea50c6 100644
--- a/src/test/java/com/google/devtools/build/lib/exec/StandaloneTestStrategyTest.java
+++ b/src/test/java/com/google/devtools/build/lib/exec/StandaloneTestStrategyTest.java
@@ -255,7 +255,7 @@
List<SpawnResult> spawnResults =
standaloneTestStrategy.exec(testRunnerAction, actionExecutionContext);
- assertThat(spawnResults).containsExactly(passSpawnResult);
+ assertThat(spawnResults).containsExactly(failSpawnResult, passSpawnResult).inOrder();
TestResult result = standaloneTestStrategy.postedResult;
assertThat(result).isNotNull();
