From f7d795a3846645bc7d73c30587fe2acc0494a112 Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Mon, 8 May 2023 03:43:30 -0700 Subject: [PATCH] Actually check `TEST_SHARD_STATUS_FILE` has been touched Adds the missing check that a test runner running a sharded test actually declares support for sharding by generating the `TEST_SHARD_STATUS_FILE`. Previously, if it didn't, each shard would silently run all tests. This new behavior is guarded by the new `--incompatible_check_sharding_support` flag. Closes #18236. PiperOrigin-RevId: 530259354 Change-Id: If3b01b2c796e66ad7a77e542145efe3ab412eae9 --- site/en/reference/test-encyclopedia.md | 5 +-- .../lib/analysis/test/TestConfiguration.java | 17 +++++++++ .../lib/analysis/test/TestRunnerAction.java | 4 +++ .../lib/exec/StandaloneTestStrategy.java | 19 ++++++++++ src/test/py/bazel/bazel_windows_test.py | 35 +++++++++++++++++++ src/test/shell/bazel/bazel_test_test.sh | 22 ++++++++++++ .../bazel/remote/remote_execution_test.sh | 26 ++++++++++++++ 7 files changed, 126 insertions(+), 2 deletions(-) diff --git a/site/en/reference/test-encyclopedia.md b/site/en/reference/test-encyclopedia.md index 267d11dd77b08f..88aa868e8e12e1 100644 --- a/site/en/reference/test-encyclopedia.md +++ b/site/en/reference/test-encyclopedia.md @@ -225,8 +225,9 @@ shard index, beginning at 0. Runners use this information to select which tests to run - for example, using a round-robin strategy. Not all test runners support sharding. If a runner supports sharding, it must create or update the last modified date of the file specified by -[`TEST_SHARD_STATUS_FILE`](#initial-conditions). Otherwise, Bazel assumes it -does not support sharding and will not launch additional runners. +[`TEST_SHARD_STATUS_FILE`](#initial-conditions). Otherwise, if +[`--incompatible_check_sharding_support`](/reference/command-line-reference#flag--incompatible_check_sharding_support) +is enabled, Bazel will fail the test if it is sharded. ## Initial conditions {:#initial-conditions} diff --git a/src/main/java/com/google/devtools/build/lib/analysis/test/TestConfiguration.java b/src/main/java/com/google/devtools/build/lib/analysis/test/TestConfiguration.java index ff9258019b9284..333dccce4389ee 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/test/TestConfiguration.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/test/TestConfiguration.java @@ -291,6 +291,19 @@ public static class TestOptions extends FragmentOptions { + "the test exec group.") public boolean useTargetPlatformForTests; + @Option( + name = "incompatible_check_sharding_support", + defaultValue = "false", + documentationCategory = OptionDocumentationCategory.UNCATEGORIZED, + metadataTags = {OptionMetadataTag.INCOMPATIBLE_CHANGE}, + effectTags = {OptionEffectTag.UNKNOWN}, + help = + "If true, Bazel will fail a sharded test if the test runner does not indicate that it " + + "supports sharding by touching the file at the path in TEST_SHARD_STATUS_FILE. " + + "If false, a test runner that does not support sharding will lead to all tests " + + "running in each shard.") + public boolean checkShardingSupport; + @Override public FragmentOptions getExec() { // Options here are either: @@ -395,6 +408,10 @@ public boolean useTargetPlatformForTests() { return options.useTargetPlatformForTests; } + public boolean checkShardingSupport() { + return options.checkShardingSupport; + } + /** * Option converter that han handle two styles of value for "--runs_per_test": * diff --git a/src/main/java/com/google/devtools/build/lib/analysis/test/TestRunnerAction.java b/src/main/java/com/google/devtools/build/lib/analysis/test/TestRunnerAction.java index 8ead7e0ff9434a..512caef2bc9ad9 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/test/TestRunnerAction.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/test/TestRunnerAction.java @@ -332,6 +332,10 @@ public boolean showsOutputUnconditionally() { return true; } + public boolean checkShardingSupport() { + return testConfiguration.checkShardingSupport(); + } + public List getSpawnOutputs() { final List outputs = new ArrayList<>(); outputs.add(ActionInputHelper.fromPath(getXmlOutputPath())); 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 2afb45f1d6d675..30bd2170088953 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 @@ -684,6 +684,25 @@ private TestAttemptResult runTestAttempt( } long endTimeMillis = actionExecutionContext.getClock().currentTimeMillis(); + if (testAction.isSharded()) { + if (testAction.checkShardingSupport() + && !actionExecutionContext + .getPathResolver() + .convertPath(resolvedPaths.getTestShard()) + .exists()) { + TestExecException e = + createTestExecException( + TestAction.Code.LOCAL_TEST_PREREQ_UNMET, + "Sharding requested, but the test runner did not advertise support for it by " + + "touching TEST_SHARD_STATUS_FILE. Either remove the 'shard_count' attribute, " + + "use a test runner that supports sharding or temporarily disable this check " + + "via --noincompatible_check_sharding_support."); + closeSuppressed(e, streamed); + closeSuppressed(e, fileOutErr); + throw e; + } + } + // SpawnActionContext guarantees the first entry to correspond to the spawn passed in (there // may be additional entries due to tree artifact handling). SpawnResult primaryResult = spawnResults.get(0); diff --git a/src/test/py/bazel/bazel_windows_test.py b/src/test/py/bazel/bazel_windows_test.py index 9b176f29865d3c..416b23fed15fc5 100644 --- a/src/test/py/bazel/bazel_windows_test.py +++ b/src/test/py/bazel/bazel_windows_test.py @@ -439,6 +439,41 @@ def testZipUndeclaredTestOutputs(self): self.assertTrue(os.path.exists(output_file)) self.assertFalse(os.path.exists(output_zip)) + def testTestShardStatusFile(self): + self.CreateWorkspaceWithDefaultRepos('WORKSPACE') + self.ScratchFile( + 'BUILD', + [ + 'sh_test(', + ' name = "foo_test",', + ' srcs = ["foo.sh"],', + ' shard_count = 2,', + ')', + ], + ) + self.ScratchFile('foo.sh') + + exit_code, stdout, stderr = self.RunBazel( + ['test', '--incompatible_check_sharding_support', '//:foo_test'], + allow_failure=True, + ) + # Check for "tests failed" exit code + self.AssertExitCode(exit_code, 3, stderr, stdout) + self.assertTrue( + any( + 'Sharding requested, but the test runner did not advertise support' + ' for it by touching TEST_SHARD_STATUS_FILE.' + in line + for line in stderr + ) + ) + + self.ScratchFile('foo.sh', ['touch "$TEST_SHARD_STATUS_FILE"']) + + self.RunBazel( + ['test', '--incompatible_check_sharding_support', '//:foo_test'] + ) + if __name__ == '__main__': unittest.main() diff --git a/src/test/shell/bazel/bazel_test_test.sh b/src/test/shell/bazel/bazel_test_test.sh index 709530a3ae0a84..4bee9e4a4178c4 100755 --- a/src/test/shell/bazel/bazel_test_test.sh +++ b/src/test/shell/bazel/bazel_test_test.sh @@ -997,4 +997,26 @@ EOF expect_log " BUILD +sh_test( + name = 'x', + srcs = ['x.sh'], + shard_count = 2, +) +EOF + touch x.sh + chmod +x x.sh + + bazel test \ + --incompatible_check_sharding_support \ + //:x &> $TEST_log && fail "expected failure" + expect_log "Sharding requested, but the test runner did not advertise support for it by touching TEST_SHARD_STATUS_FILE." + + echo 'touch "$TEST_SHARD_STATUS_FILE"' > x.sh + bazel test \ + --incompatible_check_sharding_support \ + //:x &> $TEST_log || fail "expected success" +} + run_suite "bazel test tests" diff --git a/src/test/shell/bazel/remote/remote_execution_test.sh b/src/test/shell/bazel/remote/remote_execution_test.sh index cf3ec2c8b69763..c13cc6c800eeb8 100755 --- a/src/test/shell/bazel/remote/remote_execution_test.sh +++ b/src/test/shell/bazel/remote/remote_execution_test.sh @@ -3027,4 +3027,30 @@ function test_external_cc_binary_tool_with_dynamic_deps_sibling_repository_layou @other_repo//pkg:rule >& $TEST_log || fail "Build should succeed" } +function test_shard_status_file_checked_remote_download_minimal() { + cat <<'EOF' > BUILD +sh_test( + name = 'x', + srcs = ['x.sh'], + shard_count = 2, +) +EOF + touch x.sh + chmod +x x.sh + + bazel test \ + --remote_executor=grpc://localhost:${worker_port} \ + --incompatible_check_sharding_support \ + --remote_download_minimal \ + //:x &> $TEST_log && fail "expected failure" + expect_log "Sharding requested, but the test runner did not advertise support for it by touching TEST_SHARD_STATUS_FILE." + + echo 'touch "$TEST_SHARD_STATUS_FILE"' > x.sh + bazel test \ + --remote_executor=grpc://localhost:${worker_port} \ + --incompatible_check_sharding_support \ + --remote_download_minimal \ + //:x &> $TEST_log || fail "expected success" +} + run_suite "Remote execution and remote cache tests"