Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Actually check TEST_SHARD_STATUS_FILE has been touched #18236

Closed
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions site/en/reference/test-encyclopedia.md
Original file line number Diff line number Diff line change
Expand Up @@ -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}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down Expand Up @@ -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":
*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -332,6 +332,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 @@ -684,6 +684,22 @@ 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);
Expand Down
28 changes: 28 additions & 0 deletions src/test/py/bazel/bazel_windows_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -449,6 +449,34 @@ 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'])
# 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"'])

exit_code, stdout, stderr = self.RunBazel(
['test', '--incompatible_check_sharding_support', '//:foo_test'])
self.AssertExitCode(exit_code, 0, stderr, stdout)


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 @@ -3012,4 +3012,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"