Post test result for cancelled runs
We create the equivalent of an empty FailedAttemptResult with a status
code of INCOMPLETE, and post that in the case of a cancelled concurrent
test action. We add a new method on TestRunnerSpawn to finalize a
cancelled test. Also add test coverage to check that a TestResult with
the correct status is posted for each action.
Why INCOMPLETE?
We have three options here: 1. use INCOMPLETE, 2. use NO_STATUS, or 3.
add a new status code to BlazeTestStatus. The other existing enum values
are all inappropriate:
PASSED, FLAKY, TIMEOUT, FAILED, REMOTE_FAILURE, FAILED_TO_BUILD, and
BLAZE_HALTED_BEFORE_TESTING
About option 2: NO_STATUS would not allow us to distinguish cancelled
tests from tests that were not run in the first place. INCOMPLETE is
currently unused and undocumented, so it's unclear what the originally
intended semantics are. Before open sourcing Bazel, Google had an
INTERRUPTED status, which got translated to INCOMPLETE in the process.
About option 3: This would require updating all the downstream systems
to support the new status code, including the BEP, which has the exact
same list of status codes as BlazeTestStatus.
CancelFuture handling?
It might be better to push down the cancelFuture handling to the
TestAttemptContinuation - that'd allow us to collect additional data
about the test attempt and simplify this code. Possibly, we could only
push down interrupt handling, but we probably need to know that the
interrupt is due to a cancelled run rather than a user interrupt.
PiperOrigin-RevId: 275246041
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 23a6436..f54da5a 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
@@ -156,6 +156,9 @@
TestAttemptResult lastTestAttemptResult, List<FailedAttemptResult> failedAttempts)
throws IOException;
+ /** Post the final test result based on the last attempt and the list of failed attempts. */
+ void finalizeCancelledTest(List<FailedAttemptResult> failedAttempts) throws IOException;
+
/**
* 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
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 1fe5ff0..acc5ea5 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
@@ -758,6 +758,7 @@
TestAttemptContinuation testAttemptContinuation =
beginIfNotCancelled(testRunnerSpawn, cancelFuture);
if (testAttemptContinuation == null) {
+ testRunnerSpawn.finalizeCancelledTest(ImmutableList.of());
return ActionContinuationOrResult.of(ActionResult.create(ImmutableList.of()));
}
return new RunAttemptsContinuation(
@@ -1020,10 +1021,11 @@
} catch (InterruptedException e) {
if (cancelFuture != null && cancelFuture.isCancelled()) {
// Clear the interrupt bit.
- Thread.currentThread().isInterrupted();
+ Thread.interrupted();
for (Artifact output : TestRunnerAction.this.getMandatoryOutputs()) {
FileSystemUtils.touchFile(output.getPath());
}
+ testRunnerSpawn.finalizeCancelledTest(failedAttempts);
return ActionContinuationOrResult.of(ActionResult.create(spawnResults));
}
throw e;
@@ -1077,6 +1079,7 @@
TestAttemptContinuation nextContinuation = beginIfNotCancelled(nextRunner, cancelFuture);
if (nextContinuation == null) {
+ testRunnerSpawn.finalizeCancelledTest(failedAttempts);
return ActionContinuationOrResult.of(ActionResult.create(spawnResults));
}
return new RunAttemptsContinuation(
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 a6aa223..66c527c 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
@@ -41,6 +41,7 @@
import com.google.devtools.build.lib.analysis.test.TestRunnerAction;
import com.google.devtools.build.lib.analysis.test.TestRunnerAction.ResolvedPaths;
import com.google.devtools.build.lib.buildeventstream.BuildEventStreamProtos;
+import com.google.devtools.build.lib.buildeventstream.BuildEventStreamProtos.TestResult.ExecutionInfo;
import com.google.devtools.build.lib.buildeventstream.TestFileNameConstants;
import com.google.devtools.build.lib.events.Reporter;
import com.google.devtools.build.lib.util.OS;
@@ -239,7 +240,6 @@
testOutputs = renameOutputs(actionExecutionContext, action, testOutputs, attemptId);
}
- TestResultData.Builder dataBuilder = result.testResultDataBuilder();
// Recover the test log path, which may have been renamed, and add it to the data builder.
Path renamedTestLog = null;
for (Pair<String, Path> pair : testOutputs) {
@@ -247,8 +247,13 @@
renamedTestLog = pair.getSecond();
}
}
+
+ TestResultData.Builder dataBuilder = result.testResultDataBuilder();
if (dataBuilder.getStatus() == BlazeTestStatus.PASSED) {
dataBuilder.setPassedLog(renamedTestLog.toString());
+ } else if (dataBuilder.getStatus() == BlazeTestStatus.INCOMPLETE) {
+ // Incomplete (cancelled) test runs don't have a log.
+ Preconditions.checkState(renamedTestLog == null);
} else {
dataBuilder.addFailedLogs(renamedTestLog.toString());
}
@@ -466,6 +471,19 @@
StandaloneTestStrategy.this.finalizeTest(
testAction, actionExecutionContext, (StandaloneTestResult) finalResult, failedAttempts);
}
+
+ @Override
+ public void finalizeCancelledTest(List<FailedAttemptResult> failedAttempts) throws IOException {
+ TestResultData.Builder builder =
+ TestResultData.newBuilder().setTestPassed(false).setStatus(BlazeTestStatus.INCOMPLETE);
+ StandaloneTestResult standaloneTestResult =
+ StandaloneTestResult.builder()
+ .setSpawnResults(ImmutableList.of())
+ .setTestResultDataBuilder(builder)
+ .setExecutionInfo(ExecutionInfo.getDefaultInstance())
+ .build();
+ finalizeTest(standaloneTestResult, failedAttempts);
+ }
}
private final class BazelTestAttemptContinuation extends TestAttemptContinuation {
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 a8ec7d7..86f9c2a 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
@@ -773,10 +773,169 @@
assertThat(cancelFuture.isCancelled()).isTrue();
verify(spawnActionContext).beginExecution(any(), any());
assertThat(resultA).hasSize(1);
+ assertThat(standaloneTestStrategy.postedResult).isNotNull();
+ assertThat(standaloneTestStrategy.postedResult.getData().getStatus())
+ .isEqualTo(BlazeTestStatus.PASSED);
+ // Reset postedResult.
+ standaloneTestStrategy.postedResult = null;
when(spawnActionContext.beginExecution(any(), any()))
.thenThrow(new AssertionError("failure: this should not have been called"));
List<SpawnResult> resultB = execute(actionB, actionExecutionContext, standaloneTestStrategy);
assertThat(resultB).isEmpty();
+ assertThat(standaloneTestStrategy.postedResult).isNotNull();
+ assertThat(standaloneTestStrategy.postedResult.getData().getStatus())
+ .isEqualTo(BlazeTestStatus.INCOMPLETE);
+ }
+
+ @Test
+ public void testExperimentalCancelConcurrentTestsDoesNotTriggerOnFailedRun() throws Exception {
+ useConfiguration(
+ "--runs_per_test=2",
+ "--runs_per_test_detects_flakes",
+ "--experimental_cancel_concurrent_tests");
+ ExecutionOptions executionOptions = Options.getDefaults(ExecutionOptions.class);
+ Path tmpDirRoot = TestStrategy.getTmpRoot(rootDirectory, outputBase, executionOptions);
+ BinTools binTools = BinTools.forUnitTesting(directories, analysisMock.getEmbeddedTools());
+ TestedStandaloneTestStrategy standaloneTestStrategy =
+ new TestedStandaloneTestStrategy(executionOptions, binTools, tmpDirRoot);
+
+ scratch.file("standalone/empty_test.sh", "this does not get executed, it is mocked out");
+ scratch.file(
+ "standalone/BUILD",
+ "sh_test(",
+ " name = \"empty_test\",",
+ " size = \"small\",",
+ " srcs = [\"empty_test.sh\"],",
+ ")");
+ List<TestRunnerAction> testRunnerActions = getTestActions("//standalone:empty_test");
+ assertThat(testRunnerActions).hasSize(2);
+
+ TestRunnerAction actionA = testRunnerActions.get(0);
+ TestRunnerAction actionB = testRunnerActions.get(1);
+ ListenableFuture<Void> cancelFuture =
+ standaloneTestStrategy.getTestCancelFuture(actionA.getOwner(), actionA.getShardNum());
+ assertThat(cancelFuture)
+ .isSameInstanceAs(
+ standaloneTestStrategy.getTestCancelFuture(actionB.getOwner(), actionB.getShardNum()));
+ assertThat(cancelFuture.isCancelled()).isFalse();
+
+ SpawnResult expectedSpawnResultA =
+ new SpawnResult.Builder()
+ .setStatus(Status.NON_ZERO_EXIT)
+ .setExitCode(1)
+ .setRunnerName("test")
+ .build();
+ when(spawnActionContext.beginExecution(any(), any()))
+ .then(
+ (invocation) -> {
+ // Avoid triggering split XML generation by creating an empty XML file.
+ FileSystemUtils.touchFile(actionA.resolve(getExecRoot()).getXmlOutputPath());
+ return SpawnContinuation.failedWithExecException(
+ new SpawnExecException("", expectedSpawnResultA, false));
+ });
+
+ ActionExecutionContext actionExecutionContext =
+ new FakeActionExecutionContext(createTempOutErr(tmpDirRoot), spawnActionContext);
+ List<SpawnResult> resultA = execute(actionA, actionExecutionContext, standaloneTestStrategy);
+ assertThat(cancelFuture.isCancelled()).isFalse();
+ verify(spawnActionContext).beginExecution(any(), any());
+ assertThat(resultA).hasSize(1);
+ assertThat(standaloneTestStrategy.postedResult).isNotNull();
+ assertThat(standaloneTestStrategy.postedResult.getData().getStatus())
+ .isEqualTo(BlazeTestStatus.FAILED);
+ // Reset postedResult.
+ standaloneTestStrategy.postedResult = null;
+
+ SpawnResult expectedSpawnResultB =
+ new SpawnResult.Builder().setStatus(Status.SUCCESS).setRunnerName("test").build();
+ when(spawnActionContext.beginExecution(any(), any()))
+ .then(
+ (invocation) -> {
+ // Avoid triggering split XML generation by creating an empty XML file.
+ FileSystemUtils.touchFile(actionB.resolve(getExecRoot()).getXmlOutputPath());
+ return SpawnContinuation.immediate(expectedSpawnResultB);
+ });
+ List<SpawnResult> resultB = execute(actionB, actionExecutionContext, standaloneTestStrategy);
+ assertThat(cancelFuture.isCancelled()).isTrue();
+ assertThat(resultB).hasSize(1);
+ assertThat(standaloneTestStrategy.postedResult).isNotNull();
+ assertThat(standaloneTestStrategy.postedResult.getData().getStatus())
+ .isEqualTo(BlazeTestStatus.PASSED);
+ }
+
+ @Test
+ public void testExperimentalCancelConcurrentTestsAllFailed() throws Exception {
+ useConfiguration(
+ "--runs_per_test=2",
+ "--runs_per_test_detects_flakes",
+ "--experimental_cancel_concurrent_tests");
+ ExecutionOptions executionOptions = Options.getDefaults(ExecutionOptions.class);
+ Path tmpDirRoot = TestStrategy.getTmpRoot(rootDirectory, outputBase, executionOptions);
+ BinTools binTools = BinTools.forUnitTesting(directories, analysisMock.getEmbeddedTools());
+ TestedStandaloneTestStrategy standaloneTestStrategy =
+ new TestedStandaloneTestStrategy(executionOptions, binTools, tmpDirRoot);
+
+ scratch.file("standalone/empty_test.sh", "this does not get executed, it is mocked out");
+ scratch.file(
+ "standalone/BUILD",
+ "sh_test(",
+ " name = \"empty_test\",",
+ " size = \"small\",",
+ " srcs = [\"empty_test.sh\"],",
+ ")");
+ List<TestRunnerAction> testRunnerActions = getTestActions("//standalone:empty_test");
+ assertThat(testRunnerActions).hasSize(2);
+
+ TestRunnerAction actionA = testRunnerActions.get(0);
+ TestRunnerAction actionB = testRunnerActions.get(1);
+ ListenableFuture<Void> cancelFuture =
+ standaloneTestStrategy.getTestCancelFuture(actionA.getOwner(), actionA.getShardNum());
+ assertThat(cancelFuture)
+ .isSameInstanceAs(
+ standaloneTestStrategy.getTestCancelFuture(actionB.getOwner(), actionB.getShardNum()));
+ assertThat(cancelFuture.isCancelled()).isFalse();
+
+ SpawnResult expectedSpawnResult =
+ new SpawnResult.Builder()
+ .setStatus(Status.NON_ZERO_EXIT)
+ .setExitCode(1)
+ .setRunnerName("test")
+ .build();
+ when(spawnActionContext.beginExecution(any(), any()))
+ .then(
+ (invocation) -> {
+ // Avoid triggering split XML generation by creating an empty XML file.
+ FileSystemUtils.touchFile(actionA.resolve(getExecRoot()).getXmlOutputPath());
+ return SpawnContinuation.failedWithExecException(
+ new SpawnExecException("", expectedSpawnResult, false));
+ });
+
+ ActionExecutionContext actionExecutionContext =
+ new FakeActionExecutionContext(createTempOutErr(tmpDirRoot), spawnActionContext);
+ List<SpawnResult> resultA = execute(actionA, actionExecutionContext, standaloneTestStrategy);
+ assertThat(cancelFuture.isCancelled()).isFalse();
+ verify(spawnActionContext).beginExecution(any(), any());
+ assertThat(resultA).hasSize(1);
+ assertThat(standaloneTestStrategy.postedResult).isNotNull();
+ assertThat(standaloneTestStrategy.postedResult.getData().getStatus())
+ .isEqualTo(BlazeTestStatus.FAILED);
+ // Reset postedResult.
+ standaloneTestStrategy.postedResult = null;
+
+ when(spawnActionContext.beginExecution(any(), any()))
+ .then(
+ (invocation) -> {
+ // Avoid triggering split XML generation by creating an empty XML file.
+ FileSystemUtils.touchFile(actionB.resolve(getExecRoot()).getXmlOutputPath());
+ return SpawnContinuation.failedWithExecException(
+ new SpawnExecException("", expectedSpawnResult, false));
+ });
+ List<SpawnResult> resultB = execute(actionB, actionExecutionContext, standaloneTestStrategy);
+ assertThat(cancelFuture.isCancelled()).isFalse();
+ assertThat(resultB).hasSize(1);
+ assertThat(standaloneTestStrategy.postedResult).isNotNull();
+ assertThat(standaloneTestStrategy.postedResult.getData().getStatus())
+ .isEqualTo(BlazeTestStatus.FAILED);
}
}
