Refactor StandaloneTestStrategy
This makes it more similar to other non-oss test strategies. The goal is
to share more of the code in order to then implement async test
execution.
PiperOrigin-RevId: 235713015
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 58d0575..047f3de 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
@@ -18,18 +18,22 @@
import com.google.common.collect.ImmutableList;
import com.google.devtools.build.lib.actions.SpawnResult;
import com.google.devtools.build.lib.buildeventstream.BuildEventStreamProtos;
+import com.google.devtools.build.lib.view.test.TestStatus.BlazeTestStatus;
import com.google.devtools.build.lib.view.test.TestStatus.TestResultData;
import java.util.List;
/** Contains information about the results of test execution. */
@AutoValue
public abstract class StandaloneTestResult {
+ public boolean hasPassed() {
+ return testResultDataBuilder().getStatus() == BlazeTestStatus.PASSED;
+ }
/** Returns the SpawnResults created by the test, if any. */
public abstract List<SpawnResult> spawnResults();
/** Returns the TestResultData for the test. */
- public abstract TestResultData testResultData();
+ public abstract TestResultData.Builder testResultDataBuilder();
public abstract BuildEventStreamProtos.TestResult.ExecutionInfo executionInfo();
@@ -49,7 +53,7 @@
public abstract Builder setSpawnResults(List<SpawnResult> spawnResults);
/** Sets the TestResultData for the test. */
- public abstract Builder setTestResultData(TestResultData testResultData);
+ public abstract Builder setTestResultDataBuilder(TestResultData.Builder testResultDataBuilder);
public abstract Builder setExecutionInfo(
BuildEventStreamProtos.TestResult.ExecutionInfo executionInfo);
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 07bb569..21cd23a 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
@@ -95,6 +95,42 @@
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);
+ }
+ }
+
+ private StandaloneTestRunnerSpawn createStandaloneTestRunnerSpawn(
+ TestRunnerAction action, ActionExecutionContext actionExecutionContext)
+ throws ExecException, InterruptedException {
Path execRoot = actionExecutionContext.getExecRoot();
Path coverageDir = execRoot.getRelative(action.getCoverageDirectory());
Path runfilesDir =
@@ -138,76 +174,16 @@
/*tools=*/ ImmutableList.<Artifact>of(),
ImmutableList.copyOf(action.getSpawnOutputs()),
localResourceUsage);
-
- TestResultData.Builder dataBuilder = TestResultData.newBuilder();
-
- try {
- int maxAttempts = getTestAttempts(action);
- StandaloneTestResult standaloneTestResult =
- executeTestAttempt(
- action,
- spawn,
- actionExecutionContext,
- execRoot,
- coverageDir,
- tmpDir,
- workingDirectory);
- int attempt;
- for (attempt = 1;
- standaloneTestResult.testResultData().getStatus() != BlazeTestStatus.PASSED
- && attempt < maxAttempts;
- attempt++) {
- processFailedTestAttempt(
- attempt, actionExecutionContext, action, dataBuilder, standaloneTestResult);
- standaloneTestResult =
- executeTestAttempt(
- action,
- spawn,
- actionExecutionContext,
- execRoot,
- coverageDir,
- tmpDir,
- workingDirectory);
- }
- processLastTestAttempt(attempt, dataBuilder, standaloneTestResult.testResultData());
- ImmutableList<Pair<String, Path>> testOutputs =
- action.getTestOutputsMapping(actionExecutionContext.getPathResolver(), execRoot);
- actionExecutionContext
- .getEventHandler()
- .post(
- TestAttempt.forExecutedTestResult(
- action,
- standaloneTestResult.testResultData(),
- attempt,
- testOutputs,
- standaloneTestResult.executionInfo(),
- true));
- finalizeTest(actionExecutionContext, action, dataBuilder.build());
-
- // 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);
- }
+ return new StandaloneTestRunnerSpawn(
+ action, actionExecutionContext, spawn, tmpDir, coverageDir, workingDirectory, execRoot);
}
- private void processFailedTestAttempt(
+ private TestResultData processFailedTestAttempt(
int attempt,
ActionExecutionContext actionExecutionContext,
TestRunnerAction action,
- TestResultData.Builder dataBuilder,
StandaloneTestResult result)
throws IOException {
- ImmutableList.Builder<Pair<String, Path>> testOutputsBuilder = new ImmutableList.Builder<>();
// Rename outputs
String namePrefix =
FileSystemUtils.removeExtension(action.getTestLog().getExecPath().getBaseName());
@@ -219,6 +195,7 @@
// Get the normal test output paths, and then update them to use "attempt_N" names, and
// attemptDir, before adding them to the outputs.
+ ImmutableList.Builder<Pair<String, Path>> testOutputsBuilder = new ImmutableList.Builder<>();
ImmutableList<Pair<String, Path>> testOutputs =
action.getTestOutputsMapping(actionExecutionContext.getPathResolver(),
actionExecutionContext.getExecRoot());
@@ -249,11 +226,14 @@
testOutputsBuilder.add(Pair.of(testOutput.getFirst(), destinationPath));
}
- // Add the test log to the output
- TestResultData data = result.testResultData();
+ TestResultData.Builder dataBuilder = result.testResultDataBuilder();
+
+ // We add the test log as a failed log here - we know this attempt failed, and we need to keep
+ // this information around for computing the test summary.
dataBuilder.addFailedLogs(testLog.toString());
- dataBuilder.addTestTimes(data.getTestTimes(0));
- dataBuilder.addAllTestProcessTimes(data.getTestProcessTimesList());
+
+ // Add the test log to the output
+ TestResultData data = dataBuilder.build();
actionExecutionContext
.getEventHandler()
.post(
@@ -265,31 +245,7 @@
result.executionInfo(),
false));
processTestOutput(actionExecutionContext, new TestResult(action, data, false), testLog);
- }
-
- private void processLastTestAttempt(
- int attempt, TestResultData.Builder dataBuilder, TestResultData data) {
- dataBuilder.setHasCoverage(data.getHasCoverage());
- dataBuilder.setRemotelyCached(data.getRemotelyCached());
- dataBuilder.setIsRemoteStrategy(data.getIsRemoteStrategy());
- dataBuilder.setStatus(
- data.getStatus() == BlazeTestStatus.PASSED && attempt > 1
- ? BlazeTestStatus.FLAKY
- : data.getStatus());
- dataBuilder.setTestPassed(data.getTestPassed());
- for (int i = 0; i < data.getFailedLogsCount(); i++) {
- dataBuilder.addFailedLogs(data.getFailedLogs(i));
- }
- if (data.getTestPassed()) {
- dataBuilder.setPassedLog(data.getPassedLog());
- }
- dataBuilder.addTestTimes(data.getTestTimes(0));
- dataBuilder.addAllTestProcessTimes(data.getTestProcessTimesList());
- dataBuilder.setStartTimeMillisEpoch(data.getStartTimeMillisEpoch());
- dataBuilder.setRunDurationMillis(data.getRunDurationMillis());
- if (data.hasTestCase()) {
- dataBuilder.setTestCase(data.getTestCase());
- }
+ return data;
}
private Map<String, String> setupEnvironment(
@@ -322,6 +278,16 @@
Closeable streamed = null;
Path testLogPath = actionExecutionContext.getInputPath(action.getTestLog());
+ // We have two protos to represent test attempts:
+ // 1. com.google.devtools.build.lib.view.test.TestStatus.TestResultData represents both failed
+ // attempts and finished tests. Bazel stores this to disk to persist cached test result
+ // information across server restarts.
+ // 2. com.google.devtools.build.lib.buildeventstream.BuildEventStreamProtos.TestResult
+ // represents only individual attempts (failed or not). Bazel reports this as an event to the
+ // Build Event Protocol, but never saves it to disk.
+ //
+ // The TestResult proto is always constructed from a TestResultData instance, either one that is
+ // created right here, or one that is read back from disk.
TestResultData.Builder builder = TestResultData.newBuilder();
long startTime = actionExecutionContext.getClock().currentTimeMillis();
@@ -420,7 +386,10 @@
return StandaloneTestResult.builder()
.setSpawnResults(spawnResults)
- .setTestResultData(builder.build())
+ // We return the TestResultData.Builder rather than the finished TestResultData instance,
+ // as we may have to rename the output files in case the test needs to be rerun (if it
+ // failed here _and_ is marked flaky _and_ the number of flaky attempts is larger than 1).
+ .setTestResultDataBuilder(builder)
.setExecutionInfo(executionInfo.build())
.build();
} catch (IOException e) {
@@ -546,8 +515,32 @@
}
private final void finalizeTest(
- ActionExecutionContext actionExecutionContext, TestRunnerAction action, TestResultData data)
+ TestRunnerAction action,
+ ActionExecutionContext actionExecutionContext,
+ StandaloneTestResult standaloneTestResult,
+ List<TestResultData> failedAttempts,
+ int attempt)
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());
+ }
+ ImmutableList<Pair<String, Path>> testOutputs =
+ action.getTestOutputsMapping(
+ actionExecutionContext.getPathResolver(), actionExecutionContext.getExecRoot());
+ TestResultData data = dataBuilder.build();
+ actionExecutionContext
+ .getEventHandler()
+ .post(
+ TestAttempt.forExecutedTestResult(
+ action, data, attempt, testOutputs, standaloneTestResult.executionInfo(), true));
+
+ if (dataBuilder.getStatus() == BlazeTestStatus.PASSED && !failedAttempts.isEmpty()) {
+ dataBuilder.setStatus(BlazeTestStatus.FLAKY);
+ }
+ data = dataBuilder.build();
TestResult result = new TestResult(action, data, false);
postTestResult(actionExecutionContext, result);
@@ -569,4 +562,42 @@
Path execRoot, TestRunnerAction action, TestResultData data) {
return new TestResult(action, data, /*cached*/ true, execRoot);
}
+
+ private final class StandaloneTestRunnerSpawn {
+ private final TestRunnerAction testAction;
+ private final ActionExecutionContext actionExecutionContext;
+ private final Spawn spawn;
+ private final Path tmpDir;
+ private final Path coverageDir;
+ private final Path workingDirectory;
+ private final Path execRoot;
+
+ StandaloneTestRunnerSpawn(
+ TestRunnerAction testAction,
+ ActionExecutionContext actionExecutionContext,
+ Spawn spawn,
+ Path tmpDir,
+ Path coverageDir,
+ Path workingDirectory,
+ Path execRoot) {
+ this.testAction = testAction;
+ this.actionExecutionContext = actionExecutionContext;
+ this.spawn = spawn;
+ this.tmpDir = tmpDir;
+ this.coverageDir = coverageDir;
+ this.workingDirectory = workingDirectory;
+ this.execRoot = execRoot;
+ }
+
+ StandaloneTestResult execute() throws InterruptedException, IOException, ExecException {
+ return executeTestAttempt(
+ testAction,
+ spawn,
+ actionExecutionContext,
+ execRoot,
+ coverageDir,
+ tmpDir,
+ workingDirectory);
+ }
+ }
}
