Skip to content

Commit

Permalink
Switch TestRunnerAction to use continuations
Browse files Browse the repository at this point in the history 
This rewrites execute() in terms of beginExecution and the resulting
continuation(s), and removes the old implementation. It implicitly
switches all the existing tests to the new code paths.

There's a small risk that this introduces a subtle behavioral
difference that isn't caught by the existing tests, but I'd say we
have pretty good test coverage.

Progress on #6394.

PiperOrigin-RevId: 240097352
  • Loading branch information
@ulfjack @copybara-github
ulfjack authored and copybara-github committed Mar 25, 2019
1 parent 4a4595e commit 895c43d
Show file tree
Hide file tree
Showing 4 changed files with 30 additions and 212 deletions.
9 changes: 2 additions & 7 deletions src/main/java/com/google/devtools/build/lib/analysis/test/TestActionContext.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -121,18 +121,13 @@ public TestAttemptResult get() {
interface TestRunnerSpawn {
ActionExecutionContext getActionExecutionContext();

/** Execute the test, and handle the test runner protocol. */
TestAttemptResult execute() throws InterruptedException, IOException, ExecException;

/**
* Begin the test attempt execution. This may block until the test attempt is complete and
* return a completed result, or it may return a continuation with a non-null future
* representing asynchronous execution.
*/
default TestAttemptContinuation beginExecution()
throws InterruptedException, IOException, ExecException {
return TestAttemptContinuation.of(execute());
}
TestAttemptContinuation beginExecution()
throws InterruptedException, IOException, ExecException;

/**
* After the first attempt has run, this method is called to determine the maximum number of
Expand Down
81 changes: 13 additions & 68 deletions src/main/java/com/google/devtools/build/lib/analysis/test/TestRunnerAction.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -754,6 +754,13 @@ public ActionContinuationOrResult beginExecution(ActionExecutionContext actionEx
throws InterruptedException, ActionExecutionException {
TestActionContext testActionContext =
actionExecutionContext.getContext(TestActionContext.class);
return beginExecution(actionExecutionContext, testActionContext);
}

@VisibleForTesting
public ActionContinuationOrResult beginExecution(
ActionExecutionContext actionExecutionContext, TestActionContext testActionContext)
throws InterruptedException, ActionExecutionException {
try {
TestRunnerSpawn testRunnerSpawn =
testActionContext.createTestRunnerSpawn(this, actionExecutionContext);
Expand Down Expand Up @@ -792,79 +799,17 @@ public ActionResult execute(
ActionExecutionContext actionExecutionContext, TestActionContext testActionContext)
throws ActionExecutionException, InterruptedException {
try {
TestRunnerSpawn testRunnerSpawn =
testActionContext.createTestRunnerSpawn(this, actionExecutionContext);
return ActionResult.create(
runAttempts(
testRunnerSpawn, actionExecutionContext, testActionContext.isTestKeepGoing()));
} catch (ExecException e) {
throw e.toActionExecutionException(this);
ActionContinuationOrResult continuation =
beginExecution(actionExecutionContext, testActionContext);
while (!continuation.isDone()) {
continuation = continuation.execute();
}
return continuation.get();
} finally {
unconditionalExecution = null;
}
}

private List<SpawnResult> runAttempts(
TestRunnerSpawn testRunnerSpawn,
ActionExecutionContext actionExecutionContext,
boolean keepGoing)
throws ExecException, InterruptedException {
List<SpawnResult> spawnResults = new ArrayList<>();
runAttempts(
testRunnerSpawn, actionExecutionContext, keepGoing, spawnResults, new ArrayList<>());
return spawnResults;
}

private void runAttempts(
TestRunnerSpawn testRunnerSpawn,
ActionExecutionContext actionExecutionContext,
boolean keepGoing,
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, keepGoing, spawnResults, failedAttempts);
} else {
testRunnerSpawn.finalizeTest(testProcessResult, failedAttempts);

if (!keepGoing && !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);
}
}

@Override
public String getMnemonic() {
return MNEMONIC;
Expand Down
127 changes: 0 additions & 127 deletions src/main/java/com/google/devtools/build/lib/exec/StandaloneTestStrategy.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -272,127 +272,6 @@ public SpawnContinuation execute() throws ExecException, InterruptedException {
.execute();
}

private StandaloneTestResult executeTestAttempt(
TestRunnerAction action,
Spawn spawn,
ActionExecutionContext actionExecutionContext,
Path execRoot)
throws ExecException, IOException, InterruptedException {
Closeable streamed = null;
// 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();

SpawnActionContext spawnActionContext =
actionExecutionContext.getContext(SpawnActionContext.class);
List<SpawnResult> spawnResults = new ArrayList<>();

Path out = actionExecutionContext.getInputPath(action.getTestLog());
Path err = action.resolve(execRoot).getTestStderr();
long startTime = actionExecutionContext.getClock().currentTimeMillis();
try (FileOutErr testOutErr = new FileOutErr(out, err)) {
if (executionOptions.testOutput.equals(TestOutputFormat.STREAMED)) {
streamed =
new StreamedTestOutput(
Reporter.outErrForReporter(actionExecutionContext.getEventHandler()), out);
}
try {
spawnResults.addAll(
spawnActionContext.exec(spawn, actionExecutionContext.withFileOutErr(testOutErr)));
builder
.setTestPassed(true)
.setStatus(BlazeTestStatus.PASSED)
.setPassedLog(out.getPathString());
} catch (SpawnExecException e) {
// If this method returns normally, then the higher level will rerun the test (up to
// --flaky_test_attempts times).
if (e.isCatastrophic()) {
// Rethrow as the error was catastrophic and thus the build has to be halted.
throw e;
}
if (!e.getSpawnResult().setupSuccess()) {
// Rethrow as the test could not be run and thus there's no point in retrying.
throw e;
}
builder
.setTestPassed(false)
.setStatus(e.hasTimedOut() ? BlazeTestStatus.TIMEOUT : BlazeTestStatus.FAILED)
.addFailedLogs(out.getPathString());
spawnResults.add(e.getSpawnResult());
}
if (!testOutErr.hasRecordedOutput()) {
// Make sure that the test.log exists.
FileSystemUtils.touchFile(out);
}
// Append any error output to the test.log. This is very rare.
appendStderr(testOutErr);
}

long endTime = actionExecutionContext.getClock().currentTimeMillis();
long duration = endTime - startTime;
// SpawnActionContext guarantees the first entry to correspond to the spawn passed in (there may
// be additional entries due to tree artifact handling).
SpawnResult primaryResult = spawnResults.get(0);

// The SpawnResult of a remotely cached or remotely executed action may not have walltime
// set. We fall back to the time measured here for backwards compatibility.
duration = primaryResult.getWallTime().orElse(Duration.ofMillis(duration)).toMillis();
BuildEventStreamProtos.TestResult.ExecutionInfo.Builder executionInfo =
extractExecutionInfo(primaryResult, builder);

builder.setStartTimeMillisEpoch(startTime);
builder.addTestTimes(duration);
builder.addTestProcessTimes(duration);
builder.setRunDurationMillis(duration);
if (streamed != null) {
streamed.close();
}

// If the test did not create a test.xml, and --experimental_split_xml_generation is enabled,
// then we run a separate action to create a test.xml from test.log. We do this as a spawn
// rather than doing it locally in-process, as the test.log file may only exist remotely (when
// remote execution is enabled), and we do not want to have to download it.
Path xmlOutputPath = action.resolve(actionExecutionContext.getExecRoot()).getXmlOutputPath();
if (executionOptions.splitXmlGeneration
&& action.getTestLog().getPath().exists()
&& !xmlOutputPath.exists()) {
Spawn xmlGeneratingSpawn = createXmlGeneratingSpawn(action, primaryResult);
// We treat all failures to generate the test.xml here as catastrophic, and won't rerun
// the test if this fails. We redirect the output to a temporary file.
try (FileOutErr xmlSpawnOutErr = actionExecutionContext.getFileOutErr().childOutErr()) {
spawnResults.addAll(
spawnActionContext.exec(
xmlGeneratingSpawn, actionExecutionContext.withFileOutErr(xmlSpawnOutErr)));
}
}

TestCase details = parseTestResult(xmlOutputPath);
if (details != null) {
builder.setTestCase(details);
}

if (action.isCoverageMode()) {
builder.setHasCoverage(true);
}

return StandaloneTestResult.builder()
.setSpawnResults(spawnResults)
// 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();
}

/** In rare cases, we might write something to stderr. Append it to the real test.log. */
private static void appendStderr(FileOutErr outErr) throws IOException {
Path stdErr = outErr.getErrorPath();
Expand Down Expand Up @@ -617,12 +496,6 @@ public TestAttemptContinuation beginExecution()
return beginTestAttempt(testAction, spawn, actionExecutionContext, execRoot);
}

@Override
public TestAttemptResult execute() throws InterruptedException, IOException, ExecException {
prepareFileSystem(testAction, tmpDir, coverageDir, workingDirectory);
return executeTestAttempt(testAction, spawn, actionExecutionContext, execRoot);
}

@Override
public int getMaxAttempts(TestAttemptResult firstTestAttemptResult) {
return getTestAttempts(testAction);
Expand Down
25 changes: 15 additions & 10 deletions src/test/java/com/google/devtools/build/lib/exec/StandaloneTestStrategyTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
import com.google.devtools.build.lib.actions.Artifact;
import com.google.devtools.build.lib.actions.Spawn;
import com.google.devtools.build.lib.actions.SpawnActionContext;
import com.google.devtools.build.lib.actions.SpawnContinuation;
import com.google.devtools.build.lib.actions.SpawnResult;
import com.google.devtools.build.lib.actions.SpawnResult.Status;
import com.google.devtools.build.lib.analysis.ConfiguredTarget;
Expand Down Expand Up @@ -188,7 +189,8 @@ public void testRunTestOnce() throws Exception {
.setWallTime(Duration.ofMillis(10))
.setRunnerName("test")
.build();
when(spawnActionContext.exec(any(), any())).thenReturn(ImmutableList.of(expectedSpawnResult));
when(spawnActionContext.beginExecution(any(), any()))
.thenReturn(SpawnContinuation.immediate(expectedSpawnResult));

ActionExecutionContext actionExecutionContext =
new FakeActionExecutionContext(createTempOutErr(tmpDirRoot), spawnActionContext);
Expand Down Expand Up @@ -254,9 +256,9 @@ public void testRunFlakyTest() throws Exception {
.setWallTime(Duration.ofMillis(15))
.setRunnerName("test")
.build();
when(spawnActionContext.exec(any(), any()))
when(spawnActionContext.beginExecution(any(), any()))
.thenThrow(new SpawnExecException("test failed", failSpawnResult, false))
.thenReturn(ImmutableList.of(passSpawnResult));
.thenReturn(SpawnContinuation.immediate(passSpawnResult));

ActionExecutionContext actionExecutionContext =
new FakeActionExecutionContext(createTempOutErr(tmpDirRoot), spawnActionContext);
Expand Down Expand Up @@ -322,7 +324,8 @@ public void testRunTestRemotely() throws Exception {
.setRunnerName("remote")
.setExecutorHostname("a-remote-host")
.build();
when(spawnActionContext.exec(any(), any())).thenReturn(ImmutableList.of(expectedSpawnResult));
when(spawnActionContext.beginExecution(any(), any()))
.thenReturn(SpawnContinuation.immediate(expectedSpawnResult));

ActionExecutionContext actionExecutionContext =
new FakeActionExecutionContext(createTempOutErr(tmpDirRoot), spawnActionContext);
Expand Down Expand Up @@ -380,7 +383,8 @@ public void testRunRemotelyCachedTest() throws Exception {
.setWallTime(Duration.ofMillis(10))
.setRunnerName("remote cache")
.build();
when(spawnActionContext.exec(any(), any())).thenReturn(ImmutableList.of(expectedSpawnResult));
when(spawnActionContext.beginExecution(any(), any()))
.thenReturn(SpawnContinuation.immediate(expectedSpawnResult));

ActionExecutionContext actionExecutionContext =
new FakeActionExecutionContext(createTempOutErr(tmpDirRoot), spawnActionContext);
Expand Down Expand Up @@ -438,7 +442,7 @@ public void testThatTestLogAndOutputAreReturned() throws Exception {
.setExitCode(1)
.setRunnerName("test")
.build();
when(spawnActionContext.exec(any(), any()))
when(spawnActionContext.beginExecution(any(), any()))
.thenAnswer(
(invocation) -> {
Spawn spawn = invocation.getArgument(0);
Expand All @@ -456,7 +460,7 @@ public void testThatTestLogAndOutputAreReturned() throws Exception {
/*forciblyRunRemotely=*/ false,
/*catastrophe=*/ false);
} else {
return ImmutableList.of(
return SpawnContinuation.immediate(
new SpawnResult.Builder()
.setStatus(Status.SUCCESS)
.setRunnerName("test")
Expand Down Expand Up @@ -535,7 +539,7 @@ public void testThatTestLogAndOutputAreReturnedWithSplitXmlGeneration() throws E
.setRunnerName("test")
.build();
List<FileOutErr> called = new ArrayList<>();
when(spawnActionContext.exec(any(), any()))
when(spawnActionContext.beginExecution(any(), any()))
.thenAnswer(
(invocation) -> {
Spawn spawn = invocation.getArgument(0);
Expand All @@ -561,7 +565,7 @@ public void testThatTestLogAndOutputAreReturnedWithSplitXmlGeneration() throws E
? "standalone/failing_test.exe"
: "standalone/failing_test";
assertThat(spawn.getEnvironment()).containsEntry("TEST_BINARY", testName);
return ImmutableList.of(xmlGeneratorSpawnResult);
return SpawnContinuation.immediate(xmlGeneratorSpawnResult);
}
});

Expand Down Expand Up @@ -618,7 +622,8 @@ public void testEmptyOutputCreatesEmptyLogFile() throws Exception {

SpawnResult expectedSpawnResult =
new SpawnResult.Builder().setStatus(Status.SUCCESS).setRunnerName("test").build();
when(spawnActionContext.exec(any(), any())).thenReturn(ImmutableList.of(expectedSpawnResult));
when(spawnActionContext.beginExecution(any(), any()))
.thenReturn(SpawnContinuation.immediate(expectedSpawnResult));

FileOutErr outErr = createTempOutErr(tmpDirRoot);
ActionExecutionContext actionExecutionContext =
Expand Down

0 comments on commit 895c43d

Please sign in to comment.