diff options
author | 2017-11-28 01:14:34 -0800 | |
---|---|---|
committer | 2017-11-28 01:16:40 -0800 | |
commit | 32e7a1c55289ff286d5a6f0dea41d76fdf48582b (patch) | |
tree | 6189098c9f8d40967e6f78d0bfa015b0b820af66 /src/test/java/com/google | |
parent | 25eab014b36753d3b959f4f5f883d0c6ea2ccaad (diff) |
Simplify SpawnRunner interface
It turns out that the SUCCESS status is often misunderstood to mean "zero exit",
even though this is clearly documented. I've decided to add another status for
non-zero exit, and use success only for zero exit to avoid this pitfall.
Also, many of the status codes are set, but never used. I decided to reduce the
number of status codes to only those that are actually relevant, which
simplifies further processing. Instead, we should add a string message for the
error case when we need one - we're not using it right now, so I decided not to
add that yet.
PiperOrigin-RevId: 177129441
Diffstat (limited to 'src/test/java/com/google')
4 files changed, 9 insertions, 7 deletions
diff --git a/src/test/java/com/google/devtools/build/lib/exec/AbstractSpawnStrategyTest.java b/src/test/java/com/google/devtools/build/lib/exec/AbstractSpawnStrategyTest.java index e6c7ee9683..3547e31ac6 100644 --- a/src/test/java/com/google/devtools/build/lib/exec/AbstractSpawnStrategyTest.java +++ b/src/test/java/com/google/devtools/build/lib/exec/AbstractSpawnStrategyTest.java @@ -85,7 +85,8 @@ public class AbstractSpawnStrategyTest { public void testNonZeroExit() throws Exception { when(actionExecutionContext.getContext(eq(SpawnCache.class))).thenReturn(SpawnCache.NO_CACHE); when(actionExecutionContext.getExecRoot()).thenReturn(fs.getPath("/execroot")); - SpawnResult result = new SpawnResult.Builder().setStatus(Status.SUCCESS).setExitCode(1).build(); + SpawnResult result = + new SpawnResult.Builder().setStatus(Status.NON_ZERO_EXIT).setExitCode(1).build(); when(spawnRunner.exec(any(Spawn.class), any(SpawnExecutionPolicy.class))) .thenReturn(result); @@ -151,7 +152,8 @@ public class AbstractSpawnStrategyTest { when(actionExecutionContext.getContext(eq(SpawnCache.class))).thenReturn(cache); when(actionExecutionContext.getExecRoot()).thenReturn(fs.getPath("/execroot")); - SpawnResult result = new SpawnResult.Builder().setStatus(Status.SUCCESS).setExitCode(1).build(); + SpawnResult result = + new SpawnResult.Builder().setStatus(Status.NON_ZERO_EXIT).setExitCode(1).build(); when(spawnRunner.exec(any(Spawn.class), any(SpawnExecutionPolicy.class))).thenReturn(result); try { diff --git a/src/test/java/com/google/devtools/build/lib/exec/local/LocalSpawnRunnerTest.java b/src/test/java/com/google/devtools/build/lib/exec/local/LocalSpawnRunnerTest.java index c33c33cc2f..f7aa1d9ff3 100644 --- a/src/test/java/com/google/devtools/build/lib/exec/local/LocalSpawnRunnerTest.java +++ b/src/test/java/com/google/devtools/build/lib/exec/local/LocalSpawnRunnerTest.java @@ -343,7 +343,7 @@ public class LocalSpawnRunnerTest { assertThat(fs.getPath("/execroot").createDirectory()).isTrue(); SpawnResult result = runner.exec(SIMPLE_SPAWN, policy); verify(factory).create(any(SubprocessBuilder.class)); - assertThat(result.status()).isEqualTo(SpawnResult.Status.SUCCESS); + assertThat(result.status()).isEqualTo(SpawnResult.Status.NON_ZERO_EXIT); assertThat(result.exitCode()).isEqualTo(3); assertThat(result.setupSuccess()).isTrue(); assertThat(result.getExecutorHostName()).isEqualTo(NetUtil.getCachedShortHostName()); @@ -406,7 +406,7 @@ public class LocalSpawnRunnerTest { outErr = new FileOutErr(); assertThat(fs.getPath("/execroot").createDirectory()).isTrue(); SpawnResult reply = runner.exec(SIMPLE_SPAWN, policy); - assertThat(reply.status()).isEqualTo(SpawnResult.Status.LOCAL_ACTION_NOT_ALLOWED); + assertThat(reply.status()).isEqualTo(SpawnResult.Status.EXECUTION_DENIED); assertThat(reply.exitCode()).isEqualTo(-1); assertThat(reply.setupSuccess()).isFalse(); assertThat(reply.getWallTime()).isEmpty(); diff --git a/src/test/java/com/google/devtools/build/lib/remote/GrpcRemoteExecutionClientTest.java b/src/test/java/com/google/devtools/build/lib/remote/GrpcRemoteExecutionClientTest.java index 45b52af924..35bd1950e3 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/GrpcRemoteExecutionClientTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/GrpcRemoteExecutionClientTest.java @@ -699,7 +699,7 @@ public class GrpcRemoteExecutionClientTest { fail("Expected an exception"); } catch (SpawnExecException expected) { assertThat(expected.getSpawnResult().status()) - .isEqualTo(SpawnResult.Status.CONNECTION_FAILED); + .isEqualTo(SpawnResult.Status.EXECUTION_FAILED_CATASTROPHICALLY); // Ensure we also got back the stack trace. assertThat(expected).hasMessageThat() .contains("GrpcRemoteExecutionClientTest.passUnavailableErrorWithStackTrace"); diff --git a/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerTest.java b/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerTest.java index b5534f9665..5fec618cc8 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerTest.java @@ -494,7 +494,7 @@ public class RemoteSpawnRunnerTest { SpawnExecutionPolicy policy = new FakeSpawnExecutionPolicy(spawn); SpawnResult res = runner.exec(spawn, policy); - assertThat(res.status()).isEqualTo(Status.SUCCESS); + assertThat(res.status()).isEqualTo(Status.NON_ZERO_EXIT); assertThat(res.exitCode()).isEqualTo(31); verify(executor).executeRemotely(any(ExecuteRequest.class)); @@ -598,7 +598,7 @@ public class RemoteSpawnRunnerTest { SpawnExecutionPolicy policy = new FakeSpawnExecutionPolicy(spawn); SpawnResult res = runner.exec(spawn, policy); - assertThat(res.status()).isEqualTo(Status.SUCCESS); // Even though the command failed. + assertThat(res.status()).isEqualTo(Status.NON_ZERO_EXIT); assertThat(res.exitCode()).isEqualTo(33); verify(executor).executeRemotely(any(ExecuteRequest.class)); |