Skip to content

Commit

Permalink
Actually check TEST_SHARD_STATUS_FILE has been touched
Browse files Browse the repository at this point in the history
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
  • Loading branch information
fmeum committed May 16, 2023
1 parent 758b44d commit 91fe6fe
Show file tree
Hide file tree
Showing 7 changed files with 126 additions and 2 deletions.
5 changes: 3 additions & 2 deletions site/en/reference/test-encyclopedia.md
Original file line number Diff line number Diff line change
Expand Up @@ -223,8 +223,9 @@ 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}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -280,6 +280,19 @@ public static class TestOptions extends FragmentOptions {
help = "If true, undeclared test outputs will be archived in a zip file.")
public boolean zipUndeclaredTestOutputs;

@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 getHost() {
// Options here are either:
Expand Down Expand Up @@ -380,6 +393,10 @@ public boolean getZipUndeclaredTestOutputs() {
return options.zipUndeclaredTestOutputs;
}

public boolean checkShardingSupport() {
return options.checkShardingSupport;
}

/**
* Option converter that han handle two styles of value for "--runs_per_test":
*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -336,6 +336,10 @@ public boolean showsOutputUnconditionally() {
return true;
}

public boolean checkShardingSupport() {
return testConfiguration.checkShardingSupport();
}

public List<ActionInput> getSpawnOutputs() {
final List<ActionInput> outputs = new ArrayList<>();
outputs.add(ActionInputHelper.fromPath(getXmlOutputPath()));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -747,6 +747,25 @@ public TestAttemptContinuation execute()
}
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);
Expand Down
35 changes: 35 additions & 0 deletions src/test/py/bazel/bazel_windows_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -451,6 +451,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()
22 changes: 22 additions & 0 deletions src/test/shell/bazel/bazel_test_test.sh
Original file line number Diff line number Diff line change
Expand Up @@ -997,4 +997,26 @@ EOF
expect_log "<testcase name=\"x\""
}

function test_shard_status_file_checked() {
cat <<'EOF' > 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"
26 changes: 26 additions & 0 deletions src/test/shell/bazel/remote/remote_execution_test.sh
Original file line number Diff line number Diff line change
Expand Up @@ -3271,4 +3271,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"

0 comments on commit 91fe6fe

Please sign in to comment.