diff --git a/src/test/java/com/google/devtools/build/lib/exec/local/BUILD b/src/test/java/com/google/devtools/build/lib/exec/local/BUILD index 4a633ce34959e6..845d2786a4e1c0 100644 --- a/src/test/java/com/google/devtools/build/lib/exec/local/BUILD +++ b/src/test/java/com/google/devtools/build/lib/exec/local/BUILD @@ -24,13 +24,11 @@ java_test( deps = [ "//src/main/java/com/google/devtools/build/lib:runtime", "//src/main/java/com/google/devtools/build/lib/actions", - "//src/main/java/com/google/devtools/build/lib/actions:artifacts", "//src/main/java/com/google/devtools/build/lib/actions:execution_requirements", "//src/main/java/com/google/devtools/build/lib/actions:localhost_capacity", "//src/main/java/com/google/devtools/build/lib/actions:resource_manager", "//src/main/java/com/google/devtools/build/lib/exec:bin_tools", "//src/main/java/com/google/devtools/build/lib/exec:runfiles_tree_updater", - "//src/main/java/com/google/devtools/build/lib/exec:spawn_input_expander", "//src/main/java/com/google/devtools/build/lib/exec:spawn_runner", "//src/main/java/com/google/devtools/build/lib/exec/local", "//src/main/java/com/google/devtools/build/lib/exec/local:options", @@ -46,11 +44,11 @@ java_test( "//src/main/java/com/google/devtools/common/options", "//src/test/java/com/google/devtools/build/lib/actions/util", "//src/test/java/com/google/devtools/build/lib/exec/util", + "//src/test/java/com/google/devtools/build/lib/sandbox:testutil", "//src/test/java/com/google/devtools/build/lib/testutil", "//src/test/java/com/google/devtools/build/lib/testutil:TestConstants", "//src/test/java/com/google/devtools/build/lib/testutil:TestUtils", "//third_party:guava", - "//third_party:jsr305", "//third_party:junit4", "//third_party:mockito", "//third_party:truth", 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 ba467b2f47b50c..57b1ec42f1c38d 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 @@ -15,7 +15,6 @@ package com.google.devtools.build.lib.exec.local; import static com.google.common.truth.Truth.assertThat; -import static com.google.common.util.concurrent.Futures.immediateVoidFuture; import static com.google.devtools.build.lib.testing.common.DirectoryListingHelper.file; import static java.nio.charset.StandardCharsets.UTF_8; import static org.junit.Assert.assertThrows; @@ -30,13 +29,8 @@ import com.google.common.collect.ImmutableMap; import com.google.common.io.ByteStreams; import com.google.common.io.Files; -import com.google.common.util.concurrent.ListenableFuture; -import com.google.devtools.build.lib.actions.ActionContext; -import com.google.devtools.build.lib.actions.ActionInput; -import com.google.devtools.build.lib.actions.Artifact.ArtifactExpander; import com.google.devtools.build.lib.actions.CommandLines.ParamFileActionInput; import com.google.devtools.build.lib.actions.ExecutionRequirements; -import com.google.devtools.build.lib.actions.InputMetadataProvider; import com.google.devtools.build.lib.actions.LocalHostCapacity; import com.google.devtools.build.lib.actions.ParameterFile.ParameterFileType; import com.google.devtools.build.lib.actions.ResourceManager; @@ -49,12 +43,10 @@ import com.google.devtools.build.lib.exec.BinTools; import com.google.devtools.build.lib.exec.RunfilesTreeUpdater; import com.google.devtools.build.lib.exec.SpawnExecutingEvent; -import com.google.devtools.build.lib.exec.SpawnInputExpander; -import com.google.devtools.build.lib.exec.SpawnRunner.ProgressStatus; -import com.google.devtools.build.lib.exec.SpawnRunner.SpawnExecutionContext; import com.google.devtools.build.lib.exec.SpawnSchedulingEvent; import com.google.devtools.build.lib.exec.util.SpawnBuilder; import com.google.devtools.build.lib.runtime.ProcessWrapper; +import com.google.devtools.build.lib.sandbox.SpawnRunnerTestUtil.SpawnExecutionContextForTesting; import com.google.devtools.build.lib.shell.JavaSubprocessFactory; import com.google.devtools.build.lib.shell.Subprocess; import com.google.devtools.build.lib.shell.SubprocessBuilder; @@ -84,11 +76,7 @@ import java.io.OutputStream; import java.nio.charset.StandardCharsets; import java.time.Duration; -import java.util.ArrayList; -import java.util.List; import java.util.Map; -import java.util.SortedMap; -import java.util.TreeMap; import java.util.concurrent.ExecutorService; import java.util.concurrent.Executors; import java.util.concurrent.Future; @@ -97,7 +85,6 @@ import java.util.logging.Filter; import java.util.logging.LogRecord; import java.util.logging.Logger; -import javax.annotation.Nullable; import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; @@ -226,98 +213,6 @@ public Subprocess create(SubprocessBuilder params) throws IOException { } } - private final class SpawnExecutionContextForTesting implements SpawnExecutionContext { - private final List reportedStatus = new ArrayList<>(); - private final TreeMap inputMapping = new TreeMap<>(); - - private long timeoutMillis; - private boolean prefetchCalled; - private boolean lockOutputFilesCalled; - private FileOutErr fileOutErr; - - public SpawnExecutionContextForTesting(FileOutErr fileOutErr) { - this.fileOutErr = fileOutErr; - } - - @Override - public int getId() { - return 0; - } - - @Override - public ListenableFuture prefetchInputs() { - prefetchCalled = true; - return immediateVoidFuture(); - } - - @Override - public void lockOutputFiles(int exitCode, String errorMessage, FileOutErr outErr) - throws InterruptedException { - lockOutputFilesCalled = true; - } - - @Override - public boolean speculating() { - return false; - } - - @Override - public InputMetadataProvider getInputMetadataProvider() { - return mockFileCache; - } - - @Override - public ArtifactExpander getArtifactExpander() { - throw new UnsupportedOperationException(); - } - - @Override - public SpawnInputExpander getSpawnInputExpander() { - throw new UnsupportedOperationException(); - } - - @Override - public Duration getTimeout() { - return Duration.ofMillis(timeoutMillis); - } - - @Override - public FileOutErr getFileOutErr() { - return fileOutErr; - } - - @Override - public SortedMap getInputMapping( - PathFragment baseDirectory, boolean willAccessRepeatedly) { - return inputMapping; - } - - @Override - public void report(ProgressStatus progress) { - reportedStatus.add(progress); - } - - @Override - public boolean isRewindingEnabled() { - return false; - } - - @Override - public void checkForLostInputs() {} - - @Override - public T getContext(Class identifyingType) { - throw new UnsupportedOperationException(); - } - - @Nullable - @Override - public FileSystem getActionFileSystem() { - return null; - } - } - - private final InputMetadataProvider mockFileCache = mock(InputMetadataProvider.class); private final ResourceManager resourceManager = ResourceManager.instanceForTestingOnly(); private Logger logger; @@ -391,10 +286,10 @@ public void vanillaZeroExit() throws Exception { LocalSpawnRunner runner = testedRunner; FileOutErr fileOutErr = new FileOutErr(fs.getPath("/out/stdout"), fs.getPath("/out/stderr")); - SpawnExecutionContextForTesting policy = new SpawnExecutionContextForTesting(fileOutErr); - policy.timeoutMillis = 123 * 1000L; + SpawnExecutionContextForTesting context = + new SpawnExecutionContextForTesting(SIMPLE_SPAWN, fileOutErr, Duration.ofSeconds(123)); assertThat(fs.getPath("/execroot").createDirectory()).isTrue(); - SpawnResult result = runner.exec(SIMPLE_SPAWN, policy); + SpawnResult result = runner.exec(SIMPLE_SPAWN, context); verify(factory).create(any(SubprocessBuilder.class)); assertThat(result.status()).isEqualTo(SpawnResult.Status.SUCCESS); assertThat(result.exitCode()).isEqualTo(0); @@ -417,8 +312,8 @@ public void vanillaZeroExit() throws Exception { assertThat(captor.getValue().getStderr()).isEqualTo(StreamAction.REDIRECT); assertThat(captor.getValue().getStderrFile()).isEqualTo(new File("/out/stderr")); - assertThat(policy.lockOutputFilesCalled).isTrue(); - assertThat(policy.reportedStatus) + assertThat(context.lockOutputFilesCalled).isTrue(); + assertThat(context.reportedStatus) .containsExactly(SpawnSchedulingEvent.create("local"), SpawnExecutingEvent.create("local")) .inOrder(); } @@ -458,10 +353,10 @@ public void testParamFiles() throws Exception { .withEnvironment("VARIABLE", "value") .build(); FileOutErr fileOutErr = new FileOutErr(fs.getPath("/out/stdout"), fs.getPath("/out/stderr")); - SpawnExecutionContextForTesting policy = new SpawnExecutionContextForTesting(fileOutErr); - policy.timeoutMillis = 123 * 1000L; + SpawnExecutionContextForTesting context = + new SpawnExecutionContextForTesting(spawn, fileOutErr, Duration.ofSeconds(123)); assertThat(fs.getPath("/execroot").createDirectory()).isTrue(); - SpawnResult result = runner.exec(spawn, policy); + SpawnResult result = runner.exec(spawn, context); assertThat(result.status()).isEqualTo(SpawnResult.Status.SUCCESS); assertThat(result.exitCode()).isEqualTo(0); assertThat(result.setupSuccess()).isTrue(); @@ -493,9 +388,10 @@ public void exec_materializesVirtualInputAsExecutable() throws Exception { VirtualActionInput virtualInput = ActionsTestUtil.createVirtualActionInput("input1", "hello"); Spawn spawn = new SpawnBuilder("/bin/true").withInput(virtualInput).build(); FileOutErr fileOutErr = new FileOutErr(fs.getPath("/out/stdout"), fs.getPath("/out/stderr")); - SpawnExecutionContext spawnExecutionContext = new SpawnExecutionContextForTesting(fileOutErr); + SpawnExecutionContextForTesting context = + new SpawnExecutionContextForTesting(spawn, fileOutErr, Duration.ZERO); - SpawnResult result = runner.exec(spawn, spawnExecutionContext); + SpawnResult result = runner.exec(spawn, context); assertThat(result.status()).isEqualTo(Status.SUCCESS); assertThat(DirectoryListingHelper.leafDirectoryEntries(execRoot)) @@ -530,10 +426,10 @@ public void noProcessWrapper() throws Exception { LocalSpawnRunnerTest::keepLocalEnvUnchanged); FileOutErr fileOutErr = new FileOutErr(fs.getPath("/out/stdout"), fs.getPath("/out/stderr")); - SpawnExecutionContextForTesting policy = new SpawnExecutionContextForTesting(fileOutErr); - policy.timeoutMillis = 123 * 1000L; + SpawnExecutionContextForTesting context = + new SpawnExecutionContextForTesting(SIMPLE_SPAWN, fileOutErr, Duration.ofSeconds(123)); assertThat(fs.getPath("/execroot").createDirectory()).isTrue(); - SpawnResult result = runner.exec(SIMPLE_SPAWN, policy); + SpawnResult result = runner.exec(SIMPLE_SPAWN, context); verify(factory).create(any()); assertThat(result.status()).isEqualTo(SpawnResult.Status.SUCCESS); assertThat(result.exitCode()).isEqualTo(0); @@ -544,9 +440,9 @@ public void noProcessWrapper() throws Exception { .containsExactlyElementsIn(ImmutableList.of("/bin/echo", "Hi!")); assertThat(captor.getValue().getEnv()).containsExactly("VARIABLE", "value"); // Without the process wrapper, we use the Command API to enforce the timeout. - assertThat(captor.getValue().getTimeoutMillis()).isEqualTo(policy.timeoutMillis); + assertThat(captor.getValue().getTimeoutMillis()).isEqualTo(123 * 1000L); - assertThat(policy.lockOutputFilesCalled).isTrue(); + assertThat(context.lockOutputFilesCalled).isTrue(); } @Test @@ -575,8 +471,9 @@ public void nonZeroExit() throws Exception { assertThat(fs.getPath("/execroot").createDirectory()).isTrue(); FileOutErr fileOutErr = new FileOutErr(fs.getPath("/out/stdout"), fs.getPath("/out/stderr")); - SpawnExecutionContextForTesting policy = new SpawnExecutionContextForTesting(fileOutErr); - SpawnResult result = runner.exec(SIMPLE_SPAWN, policy); + SpawnExecutionContextForTesting context = + new SpawnExecutionContextForTesting(SIMPLE_SPAWN, fileOutErr, Duration.ZERO); + SpawnResult result = runner.exec(SIMPLE_SPAWN, context); verify(factory).create(any(SubprocessBuilder.class)); assertThat(result.status()).isEqualTo(SpawnResult.Status.NON_ZERO_EXIT); assertThat(result.exitCode()).isEqualTo(3); @@ -598,7 +495,7 @@ public void nonZeroExit() throws Exception { assertThat(captor.getValue().getStderr()).isEqualTo(StreamAction.REDIRECT); assertThat(captor.getValue().getStderrFile()).isEqualTo(new File("/out/stderr")); - assertThat(policy.lockOutputFilesCalled).isTrue(); + assertThat(context.lockOutputFilesCalled).isTrue(); } @Test @@ -622,8 +519,9 @@ public void processStartupThrows() throws Exception { assertThat(fs.getPath("/out").createDirectory()).isTrue(); assertThat(fs.getPath("/execroot").createDirectory()).isTrue(); FileOutErr fileOutErr = new FileOutErr(fs.getPath("/out/stdout"), fs.getPath("/out/stderr")); - SpawnExecutionContextForTesting policy = new SpawnExecutionContextForTesting(fileOutErr); - SpawnResult result = runner.exec(SIMPLE_SPAWN, policy); + SpawnExecutionContextForTesting context = + new SpawnExecutionContextForTesting(SIMPLE_SPAWN, fileOutErr, Duration.ZERO); + SpawnResult result = runner.exec(SIMPLE_SPAWN, context); verify(factory).create(any(SubprocessBuilder.class)); assertThat(result.status()).isEqualTo(SpawnResult.Status.EXECUTION_FAILED); assertThat(result.exitCode()).isEqualTo(-1); @@ -636,7 +534,7 @@ public void processStartupThrows() throws Exception { assertThat(FileSystemUtils.readContent(fs.getPath("/out/stderr"), UTF_8)) .isEqualTo("Action failed to execute: java.io.IOException: I'm sorry, Dave\n"); - assertThat(policy.lockOutputFilesCalled).isTrue(); + assertThat(context.lockOutputFilesCalled).isTrue(); } @Test @@ -655,8 +553,9 @@ public void disallowLocalExecution() throws Exception { assertThat(fs.getPath("/execroot").createDirectory()).isTrue(); FileOutErr fileOutErr = new FileOutErr(); - SpawnExecutionContextForTesting policy = new SpawnExecutionContextForTesting(fileOutErr); - SpawnResult reply = runner.exec(SIMPLE_SPAWN, policy); + SpawnExecutionContextForTesting context = + new SpawnExecutionContextForTesting(SIMPLE_SPAWN, fileOutErr, Duration.ZERO); + SpawnResult reply = runner.exec(SIMPLE_SPAWN, context); assertThat(reply.status()).isEqualTo(SpawnResult.Status.EXECUTION_DENIED); assertThat(reply.exitCode()).isEqualTo(-1); assertThat(reply.setupSuccess()).isFalse(); @@ -666,7 +565,7 @@ public void disallowLocalExecution() throws Exception { assertThat(reply.getExecutorHostName()).isEqualTo(NetUtil.getCachedShortHostName()); // TODO(ulfjack): Maybe we should only lock after checking? - assertThat(policy.lockOutputFilesCalled).isTrue(); + assertThat(context.lockOutputFilesCalled).isTrue(); } @Test @@ -705,11 +604,12 @@ public void waitFor() throws InterruptedException { LocalSpawnRunnerTest::keepLocalEnvUnchanged); FileOutErr fileOutErr = new FileOutErr(fs.getPath("/out/stdout"), fs.getPath("/out/stderr")); - SpawnExecutionContextForTesting policy = new SpawnExecutionContextForTesting(fileOutErr); + SpawnExecutionContextForTesting context = + new SpawnExecutionContextForTesting(SIMPLE_SPAWN, fileOutErr, Duration.ZERO); assertThat(fs.getPath("/execroot").createDirectory()).isTrue(); - assertThrows(InterruptedException.class, () -> runner.exec(SIMPLE_SPAWN, policy)); + assertThrows(InterruptedException.class, () -> runner.exec(SIMPLE_SPAWN, context)); Thread.interrupted(); - assertThat(policy.lockOutputFilesCalled).isTrue(); + assertThat(context.lockOutputFilesCalled).isTrue(); } @Test @@ -729,7 +629,6 @@ public void interruptWaitsForProcessExit() throws Exception { Mockito.mock(RunfilesTreeUpdater.class)); FileOutErr fileOutErr = new FileOutErr(tempDir.getRelative("stdout"), tempDir.getRelative("stderr")); - SpawnExecutionContextForTesting policy = new SpawnExecutionContextForTesting(fileOutErr); // This test to exercise a race condition by attempting an operation multiple times. We can get // false positives (the test passing without us catching a problem), so try a few times. When @@ -757,6 +656,9 @@ public void interruptWaitsForProcessExit() throws Exception { + "done"; Spawn spawn = new SpawnBuilder("/bin/sh", "-c", script).build(); + SpawnExecutionContextForTesting context = + new SpawnExecutionContextForTesting(spawn, fileOutErr, Duration.ZERO); + ExecutorService executor = Executors.newSingleThreadExecutor(); try { for (int i = 0; i < tries; i++) { @@ -767,7 +669,7 @@ public void interruptWaitsForProcessExit() throws Exception { executor.submit( () -> { try { - runner.exec(spawn, policy); + runner.exec(spawn, context); } catch (InterruptedException e) { interruptCaught.release(); } catch (Throwable t) { @@ -812,11 +714,11 @@ public void checkPrefetchCalled() throws Exception { LocalSpawnRunnerTest::keepLocalEnvUnchanged); FileOutErr fileOutErr = new FileOutErr(fs.getPath("/out/stdout"), fs.getPath("/out/stderr")); - SpawnExecutionContextForTesting policy = new SpawnExecutionContextForTesting(fileOutErr); - policy.timeoutMillis = 123 * 1000L; + SpawnExecutionContextForTesting context = + new SpawnExecutionContextForTesting(SIMPLE_SPAWN, fileOutErr, Duration.ofSeconds(123)); assertThat(fs.getPath("/execroot").createDirectory()).isTrue(); - runner.exec(SIMPLE_SPAWN, policy); - assertThat(policy.prefetchCalled).isTrue(); + runner.exec(SIMPLE_SPAWN, context); + assertThat(context.prefetchCalled).isTrue(); } @Test @@ -837,16 +739,18 @@ public void checkNoPrefetchCalled() throws Exception { LocalSpawnRunnerTest::keepLocalEnvUnchanged); FileOutErr fileOutErr = new FileOutErr(fs.getPath("/out/stdout"), fs.getPath("/out/stderr")); - SpawnExecutionContextForTesting policy = new SpawnExecutionContextForTesting(fileOutErr); - policy.timeoutMillis = 123 * 1000L; Spawn spawn = new SpawnBuilder("/bin/echo", "Hi!") .withExecutionInfo(ExecutionRequirements.DISABLE_LOCAL_PREFETCH, "") .build(); + + SpawnExecutionContextForTesting context = + new SpawnExecutionContextForTesting(spawn, fileOutErr, Duration.ofSeconds(123)); + assertThat(fs.getPath("/execroot").createDirectory()).isTrue(); - runner.exec(spawn, policy); - assertThat(policy.prefetchCalled).isFalse(); + runner.exec(spawn, context); + assertThat(context.prefetchCalled).isFalse(); } @Test @@ -868,11 +772,11 @@ public void checkLocalEnvProviderCalled() throws Exception { localEnvProvider); FileOutErr fileOutErr = new FileOutErr(fs.getPath("/out/stdout"), fs.getPath("/out/stderr")); - SpawnExecutionContextForTesting policy = new SpawnExecutionContextForTesting(fileOutErr); - policy.timeoutMillis = 123 * 1000L; + SpawnExecutionContextForTesting context = + new SpawnExecutionContextForTesting(SIMPLE_SPAWN, fileOutErr, Duration.ofSeconds(123)); assertThat(fs.getPath("/execroot").createDirectory()).isTrue(); - runner.exec(SIMPLE_SPAWN, policy); + runner.exec(SIMPLE_SPAWN, context); verify(localEnvProvider) .rewriteLocalEnv(any(), any(), matches("^/execroot/tmp[0-9a-fA-F]+_[0-9a-fA-F]+/work$")); } @@ -995,9 +899,10 @@ public void hasExecutionStatistics() throws Exception { .build(); FileOutErr fileOutErr = new FileOutErr(fs.getPath("/dev/null"), fs.getPath("/dev/null")); - SpawnExecutionContextForTesting policy = new SpawnExecutionContextForTesting(fileOutErr); + SpawnExecutionContextForTesting context = + new SpawnExecutionContextForTesting(spawn, fileOutErr, Duration.ZERO); - SpawnResult spawnResult = runner.exec(spawn, policy); + SpawnResult spawnResult = runner.exec(spawn, context); assertThat(spawnResult.status()).isEqualTo(SpawnResult.Status.SUCCESS); assertThat(spawnResult.exitCode()).isEqualTo(0); @@ -1043,9 +948,11 @@ public void relativePath() throws Exception { LocalSpawnRunnerTest::keepLocalEnvUnchanged); FileOutErr fileOutErr = new FileOutErr(fs.getPath("/out/stdout"), fs.getPath("/out/stderr")); - SpawnExecutionContextForTesting policy = new SpawnExecutionContextForTesting(fileOutErr); + Spawn spawn = new SpawnBuilder("foo/bar", "Hi!").build(); + SpawnExecutionContextForTesting context = + new SpawnExecutionContextForTesting(spawn, fileOutErr, Duration.ofSeconds(123)); assertThat(fs.getPath("/execroot").createDirectory()).isTrue(); - runner.exec(new SpawnBuilder("foo/bar", "Hi!").build(), policy); + runner.exec(spawn, context); verify(factory).create(any(SubprocessBuilder.class)); assertThat(captor.getValue().getArgv()).containsExactly("/execroot/foo/bar", "Hi!"); diff --git a/src/test/java/com/google/devtools/build/lib/sandbox/SpawnRunnerTestUtil.java b/src/test/java/com/google/devtools/build/lib/sandbox/SpawnRunnerTestUtil.java index 03e64980237adc..888970a87b06a0 100644 --- a/src/test/java/com/google/devtools/build/lib/sandbox/SpawnRunnerTestUtil.java +++ b/src/test/java/com/google/devtools/build/lib/sandbox/SpawnRunnerTestUtil.java @@ -56,7 +56,9 @@ private SpawnRunnerTestUtil() {} /** A rigged spawn execution policy that can be used for testing purposes. */ public static final class SpawnExecutionContextForTesting implements SpawnExecutionContext { - private final List reportedStatus = new ArrayList<>(); + public final List reportedStatus = new ArrayList<>(); + public boolean prefetchCalled; + public boolean lockOutputFilesCalled; private final Spawn spawn; private final Duration timeout; @@ -92,12 +94,15 @@ public int getId() { @Override public ListenableFuture prefetchInputs() { + prefetchCalled = true; return immediateVoidFuture(); } @Override public void lockOutputFiles(int exitCode, String errorMessage, FileOutErr outErr) - throws InterruptedException {} + throws InterruptedException { + lockOutputFilesCalled = true; + } @Override public boolean speculating() {