From f439f44c782a5d987e28c54fe86f9345deeb956b Mon Sep 17 00:00:00 2001 From: Googler Date: Tue, 9 Aug 2022 08:45:24 -0700 Subject: [PATCH] Migrate test `test_download_outputs_invalidation` to java. PiperOrigin-RevId: 466370782 Change-Id: I21af8102e209a7fee700b65157999103a3aa498f --- .../google/devtools/build/lib/remote/BUILD | 2 + .../BuildWithoutTheBytesIntegrationTest.java | 109 +++++++++++++++++- .../remote/build_without_the_bytes_test.sh | 32 ----- 3 files changed, 110 insertions(+), 33 deletions(-) diff --git a/src/test/java/com/google/devtools/build/lib/remote/BUILD b/src/test/java/com/google/devtools/build/lib/remote/BUILD index afac08af681c5b..efa7c2e0c0d4e3 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/BUILD +++ b/src/test/java/com/google/devtools/build/lib/remote/BUILD @@ -129,6 +129,8 @@ java_test( "//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/remote", + "//src/main/java/com/google/devtools/build/lib/sandbox", + "//src/main/java/com/google/devtools/build/lib/util:os", "//src/main/java/com/google/devtools/build/lib/vfs", "//src/test/java/com/google/devtools/build/lib/buildtool/util", "//src/test/java/com/google/devtools/build/lib/remote/util:integration_test_utils", diff --git a/src/test/java/com/google/devtools/build/lib/remote/BuildWithoutTheBytesIntegrationTest.java b/src/test/java/com/google/devtools/build/lib/remote/BuildWithoutTheBytesIntegrationTest.java index 2ee0010212b563..917c60e7b40f46 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/BuildWithoutTheBytesIntegrationTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/BuildWithoutTheBytesIntegrationTest.java @@ -21,13 +21,19 @@ import static org.junit.Assert.assertThrows; import com.google.common.collect.ImmutableList; +import com.google.common.eventbus.Subscribe; +import com.google.devtools.build.lib.actions.ActionExecutedEvent; import com.google.devtools.build.lib.actions.Artifact; import com.google.devtools.build.lib.actions.BuildFailedException; +import com.google.devtools.build.lib.actions.CachedActionEvent; import com.google.devtools.build.lib.buildtool.util.BuildIntegrationTestCase; import com.google.devtools.build.lib.remote.util.IntegrationTestUtils.WorkerInstance; import com.google.devtools.build.lib.runtime.BlazeModule; import com.google.devtools.build.lib.runtime.BlazeRuntime; +import com.google.devtools.build.lib.runtime.BlockWaitingModule; import com.google.devtools.build.lib.runtime.BuildSummaryStatsModule; +import com.google.devtools.build.lib.sandbox.SandboxModule; +import com.google.devtools.build.lib.util.OS; import java.io.IOException; import java.util.ArrayList; import java.util.List; @@ -79,13 +85,16 @@ public BuildWithoutTheBytesIntegrationTest(RemoteMode remoteMode, OutputMode out @Override protected BlazeRuntime.Builder getRuntimeBuilder() throws Exception { - return super.getRuntimeBuilder().addBlazeModule(new BuildSummaryStatsModule()); + return super.getRuntimeBuilder() + .addBlazeModule(new BuildSummaryStatsModule()) + .addBlazeModule(new BlockWaitingModule()); } @Override protected ImmutableList getSpawnModules() { return ImmutableList.builder() .addAll(super.getSpawnModules()) + .add(new SandboxModule()) .add(new RemoteModule()) .build(); } @@ -123,6 +132,22 @@ private void addOutputModeOptions() { } } + private String getSpawnStrategy() { + if (remoteMode.executeRemotely()) { + return "remote"; + } else { + OS currentOS = OS.getCurrent(); + switch (currentOS) { + case LINUX: + return "linux-sandbox"; + case DARWIN: + return "processwrapper-sandbox"; + default: + return "local"; + } + } + } + @Test public void executeRemotely_unnecessaryOutputsAreNotDownloaded() throws Exception { if (!remoteMode.executeRemotely()) { @@ -245,4 +270,86 @@ public void executeRemotely_actionFails_outputsAreAvailableLocallyForDebuggingPu assertThat(output.getFilename()).isEqualTo("substitute-buchgr.txt"); assertThat(readContent(output.getPath(), UTF_8)).isEqualTo("Hello buchgr!"); } + + @Test + public void changeOutputMode_invalidateActions() throws Exception { + addRemoteModeOptions(); + addOutputModeOptions(); + write( + "a/BUILD", + "genrule(", + " name = 'foo',", + " srcs = [],", + " outs = ['foo.txt'],", + " cmd = 'echo foo > $@',", + ")", + "", + "genrule(", + " name = 'foobar',", + " srcs = [':foo'],", + " outs = ['foobar.txt'],", + " cmd = 'cat $(location :foo) > $@ && echo bar > $@',", + ")"); + ActionEventCollector actionEventCollector = new ActionEventCollector(); + runtimeWrapper.registerSubscriber(actionEventCollector); + buildTarget("//a:foobar"); + // 3 = workspace status action + //:foo + //:foobar + assertThat(actionEventCollector.getNumActionNodesEvaluated()).isEqualTo(3); + actionEventCollector.clear(); + events.assertContainsInfo("3 processes: 1 internal, 2 " + getSpawnStrategy()); + events.clear(); + + addOptions("--remote_download_outputs=all"); + buildTarget("//a:foobar"); + + // Changing output mode should invalidate SkyFrame's in-memory caching and make it re-evaluate + // the action nodes. + assertThat(actionEventCollector.getNumActionNodesEvaluated()).isEqualTo(3); + if (remoteMode.executeRemotely()) { + // The dummy workspace status action does not re-executed, so wouldn't be displayed here + if (outputMode.minimal()) { + events.assertContainsInfo("2 processes: 2 remote cache hit"); + } else { + // output of toplevel target are on the local disk, so the action can hit the action cache, + // hence not shown here + events.assertContainsInfo("1 process: 1 remote cache hit"); + } + } else { + // all actions can hit action cache since all outputs are on the local disk due to they were + // executed locally. + events.assertContainsInfo("0 processes"); + } + } + + private static class ActionEventCollector { + private final List actionExecutedEvents = new ArrayList<>(); + private final List cachedActionEvents = new ArrayList<>(); + + @Subscribe + public void onActionExecuted(ActionExecutedEvent event) { + actionExecutedEvents.add(event); + } + + @Subscribe + public void onCachedAction(CachedActionEvent event) { + cachedActionEvents.add(event); + } + + public int getNumActionNodesEvaluated() { + return getActionExecutedEvents().size() + getCachedActionEvents().size(); + } + + public void clear() { + this.actionExecutedEvents.clear(); + this.cachedActionEvents.clear(); + } + + public List getActionExecutedEvents() { + return actionExecutedEvents; + } + + public List getCachedActionEvents() { + return cachedActionEvents; + } + } } diff --git a/src/test/shell/bazel/remote/build_without_the_bytes_test.sh b/src/test/shell/bazel/remote/build_without_the_bytes_test.sh index 92dc607eee18ca..f6aa0acd8c16ad 100755 --- a/src/test/shell/bazel/remote/build_without_the_bytes_test.sh +++ b/src/test/shell/bazel/remote/build_without_the_bytes_test.sh @@ -177,38 +177,6 @@ EOF || fail "Expected bazel-bin/a/remote.txt to be downloaded" } -function test_download_outputs_invalidation() { - # Test that when changing values of --remote_download_minimal all actions are - # invalidated. - mkdir -p a - cat > a/BUILD <<'EOF' -genrule( - name = "remote", - srcs = [], - outs = ["remote.txt"], - cmd = "echo -n \"remote\" > \"$@\"", -) -EOF - - bazel build \ - --genrule_strategy=remote \ - --remote_executor=grpc://localhost:${worker_port} \ - --remote_download_minimal \ - //a:remote >& $TEST_log || fail "Failed to build //a:remote" - - expect_log "2 processes: 1 internal, 1 remote" - - bazel build \ - --genrule_strategy=remote \ - --remote_executor=grpc://localhost:${worker_port} \ - --remote_download_outputs=all \ - //a:remote >& $TEST_log || fail "Failed to build //a:remote" - - # Changing --remote_download_outputs to "all" should invalidate SkyFrames in-memory - # caching and make it re-run the action. - expect_log "2 processes: 1 remote cache hit, 1 internal" -} - function test_downloads_minimal_hit_action_cache() { # Test that remote metadata is saved and action cache is hit across server restarts when using # --remote_download_minimal