Skip to content

Commit

Permalink
Remote: Fixes an issue when --experimental_remote_cache_async encount…
Browse files Browse the repository at this point in the history
…er flaky tests.

When `--experimental_remote_cache_async` is set, the uploads happened in the background -- usually after spawn execution.

When the test is failed and there is another test attempt, the outputs of previous test attempt are moved to other places immediately after spawn execution. This fine when combining `--experimental_remote_cache_async`  because outputs of failed action don't get uploaded.

However, there is an exception that `test.xml` is generated with another spawn before the "move" happens. The result of the spawn used to generate `test.xml` is usually "succeed" which means Bazel will attempt upload `test.xml` even if the test itself is failed.

This PR makes the `test.xml` generation spawn ignores remote cache if the test itself is failed.

Fixes #14008.

Closes #14220.

PiperOrigin-RevId: 408237437
  • Loading branch information
coeuvre authored and copybara-github committed Nov 8, 2021
1 parent bd8f1e6 commit f5cf8b0
Show file tree
Hide file tree
Showing 2 changed files with 44 additions and 1 deletion.
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Maps;
import com.google.common.io.ByteStreams;
import com.google.common.util.concurrent.ListenableFuture;
import com.google.devtools.build.lib.actions.ActionExecutionContext;
Expand Down Expand Up @@ -535,13 +536,22 @@ private static Spawn createXmlGeneratingSpawn(
envBuilder.put("TEST_SHARD_INDEX", "0");
envBuilder.put("TEST_TOTAL_SHARDS", "0");
}
Map<String, String> executionInfo =
Maps.newHashMapWithExpectedSize(action.getExecutionInfo().size() + 1);
executionInfo.putAll(action.getExecutionInfo());
if (result.exitCode() != 0) {
// If the test is failed, the spawn shouldn't use remote cache since the test.xml file is
// renamed immediately after the spawn execution. If there is another test attempt, the async
// upload will fail because it cannot read the file at original position.
executionInfo.put(ExecutionRequirements.NO_REMOTE_CACHE, "");
}
return new SimpleSpawn(
action,
args,
envBuilder.build(),
// Pass the execution info of the action which is identical to the supported tags set on the
// test target. In particular, this does not set the test timeout on the spawn.
ImmutableMap.copyOf(action.getExecutionInfo()),
ImmutableMap.copyOf(executionInfo),
null,
ImmutableMap.of(),
/*inputs=*/ NestedSetBuilder.create(
Expand Down
33 changes: 33 additions & 0 deletions src/test/shell/bazel/remote/remote_execution_test.sh
Original file line number Diff line number Diff line change
Expand Up @@ -3171,4 +3171,37 @@ EOF
expect_log "-r-xr-xr-x"
}

function test_async_upload_works_for_flaky_tests() {
mkdir -p a
cat > a/BUILD <<EOF
sh_test(
name = "test",
srcs = ["test.sh"],
)
genrule(
name = "foo",
outs = ["foo.txt"],
cmd = "echo \"foo bar\" > \$@",
)
EOF
cat > a/test.sh <<EOF
#!/bin/sh
echo "it always fails"
exit 1
EOF
chmod +x a/test.sh

# Check the error message when failed to upload
bazel build --remote_cache=http://nonexistent.example.org //a:foo >& $TEST_log || fail "Failed to build"
expect_log "WARNING: Writing to Remote Cache:"

bazel test \
--remote_cache=grpc://localhost:${worker_port} \
--experimental_remote_cache_async \
--flaky_test_attempts=2 \
//a:test >& $TEST_log && fail "expected failure" || true
expect_not_log "WARNING: Writing to Remote Cache:"
}

run_suite "Remote execution and remote cache tests"

0 comments on commit f5cf8b0

Please sign in to comment.