Return exit codes describing test action system failures instead of 3
Previously, the numerical exit code returned from "test" and "coverage"
invocations which experienced a system failure during a test action
execution was 3, the same as what is returned given a user-attributable
test failure.
Now, the numerical exit code will reflect the test action failure when
it's attributable to the system. For example, if a test action fails
because of a remote execution error, the invocation will return 34
(REMOTE_ERROR).
Tests that failed because of a system error get rerun when subsequently
requested, regardless of the value specified for "--cache_test_results".
This cache avoiding behavior is implemented by resurrecting the use of
the TestResultData.cachable field, which was previously used nowhere,
since an ancient change to the codebase took it out of use.
TestResultData.cachable is now set to false when a test action fails due
to a system error. The logic that determines whether to use cached test
results based on the properties of that result and the value of
"--cache_test_results" now always ignores those results if they have
cachable=false.
RELNOTES: The "test" and "coverage" commands no longer return 3 when a
test action fails because of a system error. Instead, the exit code
reflects the type of system error.
PiperOrigin-RevId: 349337510
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 cdc4b8d..96b4e88 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
@@ -49,6 +49,7 @@
import com.google.devtools.build.lib.collect.nestedset.Order;
import com.google.devtools.build.lib.events.Reporter;
import com.google.devtools.build.lib.server.FailureDetails.Execution.Code;
+import com.google.devtools.build.lib.server.FailureDetails.FailureDetail;
import com.google.devtools.build.lib.server.FailureDetails.TestAction;
import com.google.devtools.build.lib.util.Pair;
import com.google.devtools.build.lib.util.io.FileOutErr;
@@ -99,9 +100,14 @@
TestRunnerAction action, ActionExecutionContext actionExecutionContext)
throws ExecException, InterruptedException {
if (action.getExecutionSettings().getInputManifest() == null) {
+ String errorMessage = "cannot run local tests with --nobuild_runfile_manifests";
throw new TestExecException(
- "cannot run local tests with --nobuild_runfile_manifests",
- TestAction.Code.LOCAL_TEST_PREREQ_UNMET);
+ errorMessage,
+ FailureDetail.newBuilder()
+ .setTestAction(
+ TestAction.newBuilder().setCode(TestAction.Code.LOCAL_TEST_PREREQ_UNMET))
+ .setMessage(errorMessage)
+ .build());
}
Map<String, String> testEnvironment =
createEnvironment(
@@ -228,7 +234,8 @@
dataBuilder.setStatus(BlazeTestStatus.FLAKY);
}
TestResultData data = dataBuilder.build();
- TestResult result = new TestResult(action, data, false);
+ TestResult result =
+ new TestResult(action, data, false, standaloneTestResult.primarySystemFailure());
postTestResult(actionExecutionContext, result);
}
@@ -489,7 +496,7 @@
@Override
public TestResult newCachedTestResult(
Path execRoot, TestRunnerAction action, TestResultData data) {
- return new TestResult(action, data, /*cached*/ true, execRoot);
+ return new TestResult(action, data, /*cached*/ true, execRoot, /*systemFailure=*/ null);
}
@VisibleForTesting
@@ -562,7 +569,10 @@
@Override
public void finalizeCancelledTest(List<FailedAttemptResult> failedAttempts) throws IOException {
TestResultData.Builder builder =
- TestResultData.newBuilder().setTestPassed(false).setStatus(BlazeTestStatus.INCOMPLETE);
+ TestResultData.newBuilder()
+ .setCachable(false)
+ .setTestPassed(false)
+ .setStatus(BlazeTestStatus.INCOMPLETE);
StandaloneTestResult standaloneTestResult =
StandaloneTestResult.builder()
.setSpawnResults(ImmutableList.of())
@@ -628,8 +638,7 @@
// 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.
+ // that is created right here, or one that is read back from disk.
TestResultData.Builder builder = null;
ImmutableList<SpawnResult> spawnResults;
try {
@@ -649,7 +658,7 @@
}
spawnResults = nextContinuation.get();
builder = TestResultData.newBuilder();
- builder.setTestPassed(true).setStatus(BlazeTestStatus.PASSED);
+ builder.setCachable(true).setTestPassed(true).setStatus(BlazeTestStatus.PASSED);
} catch (SpawnExecException e) {
if (e.isCatastrophic()) {
closeSuppressed(e, streamed);
@@ -665,6 +674,7 @@
spawnResults = ImmutableList.of(e.getSpawnResult());
builder = TestResultData.newBuilder();
builder
+ .setCachable(e.getSpawnResult().status().isConsideredUserError())
.setTestPassed(false)
.setStatus(e.hasTimedOut() ? BlazeTestStatus.TIMEOUT : BlazeTestStatus.FAILED);
} catch (InterruptedException e) {
@@ -947,6 +957,7 @@
throw e;
}
testResultDataBuilder
+ .setCachable(e.getSpawnResult().status().isConsideredUserError())
.setTestPassed(false)
.setStatus(e.hasTimedOut() ? BlazeTestStatus.TIMEOUT : BlazeTestStatus.FAILED);
} catch (ExecException | InterruptedException e) {
