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 supports sharding. Previously, if it didn't, each shard would
silently run all tests.

RELNOTES: If sharding is requested for a test but the test runner does
not advertise support for test sharding by touching the
`TEST_SHARD_STATUS_FILE`, the tests will fail instead of silently
running all tests in each shard.
  • Loading branch information
fmeum committed Apr 27, 2023
1 parent 31c5a72 commit 3c3d5a0
Show file tree
Hide file tree
Showing 4 changed files with 104 additions and 0 deletions.
25 changes: 25 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,31 @@ 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, _, stderr = self.RunBazel(['test', '//:foo_test'])
# Check for "tests failed" exit code
self.AssertExitCode(exit_code, 3, stderr)
self.assertIn("Sharding requested, but the test runner did not advertise "
"support for it by touching TEST_SHARD_STATUS_FILE.", stderr)

self.ScratchFile('foo.sh', ['touch "$TEST_SHARD_STATUS_FILE"'])

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


if __name__ == '__main__':
unittest.main()
20 changes: 20 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,24 @@ 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 //:x --test_output=errors &> $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 //:x --test_output=errors &> $TEST_log \
|| fail "expected success"
}

run_suite "bazel test tests"
13 changes: 13 additions & 0 deletions tools/test/test-setup.sh
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,9 @@ fi
if [[ -n "$TEST_SHARD_STATUS_FILE" ]]; then
is_absolute "$TEST_SHARD_STATUS_FILE" || TEST_SHARD_STATUS_FILE="$PWD/$TEST_SHARD_STATUS_FILE"
mkdir -p "$(dirname "$TEST_SHARD_STATUS_FILE")"
# Get the modification time in nano second granularity or record that it was missing.
TEST_SHARD_STATUS_FILE_MTIME_AT_START=$(stat -c '%.9Y' "$TEST_SHARD_STATUS_FILE" 2> /dev/null ||
echo "missing")
fi

is_absolute "$RUNFILES_DIR" || RUNFILES_DIR="$PWD/$RUNFILES_DIR"
Expand Down Expand Up @@ -368,6 +371,16 @@ kill_group SIGKILL $childPid
kill_group SIGKILL $cleanupPid &> /dev/null
wait $cleanupPid

# Sharding was requested, verify that the test runner actually supported it.
if [[ -n "$TEST_SHARD_STATUS_FILE" ]]; then
TEST_SHARD_STATUS_FILE_MTIME_AT_END=$(stat -c '%.9Y' "$TEST_SHARD_STATUS_FILE" 2> /dev/null ||
echo "missing")
if [[ "$TEST_SHARD_STATUS_FILE_MTIME_AT_START" = "$TEST_SHARD_STATUS_FILE_MTIME_AT_END" ]]; then
echo >&2 "Sharding requested, but the test runner did not advertise support for it by touching TEST_SHARD_STATUS_FILE. Either remove the 'shard_count' attribute or use a test runner that supports sharding."
exit 1
fi
fi

for signal in $signals; do
trap - ${signal}
done
Expand Down
46 changes: 46 additions & 0 deletions tools/test/windows/tw.cc
Original file line number Diff line number Diff line change
Expand Up @@ -605,6 +605,42 @@ bool ExportShardStatusFile(const Path& cwd) {
CreateDirectories(status_file.Dirname());
}

bool GetLenientShardStatusFileModificationTime(uint64_t* mtime) {
Path status_file;
if (!GetPathEnv(L"TEST_SHARD_STATUS_FILE", &status_file) ||
status_file.Get().empty()) {
return false;
}

bazel::windows::AutoHandle handle(CreateFileW(
AddUncPrefixMaybe(status_file).c_str(), GENERIC_READ,
FILE_SHARE_READ | FILE_SHARE_DELETE, nullptr, OPEN_EXISTING,
FILE_ATTRIBUTE_NORMAL, nullptr));
if (!handle.IsValid()) {
DWORD err = GetLastError();
if (err != ERROR_FILE_NOT_FOUND) {
LogErrorWithArgAndValue(
__LINE__, "Failed to open shard status file", status_file.Get(), err);
}
*mtime = 0;
return true;
}

FILETIME lastWriteTime;
if (!GetFileTime(handle, nullptr, nullptr, &lastWriteTime)) {
DWORD err = GetLastError();
LogErrorWithArgAndValue(
__LINE__, "Failed to get file time of shard status file",
status_file.Get(), err);
*mtime = 0;
return true;
}

*mtime = ((uint64_t) lastWriteTime.dwHighDateTime << 32) |
lastWriteTime.dwLowDateTime;
return true;
}

bool ExportGtestVariables(const Path& test_tmpdir) {
// # Tell googletest about Bazel sharding.
std::wstring total_shards_str;
Expand Down Expand Up @@ -1925,6 +1961,7 @@ int TestWrapperMain(int argc, wchar_t** argv) {
Path test_path, exec_root, srcdir, tmpdir, test_outerr, xml_log;
UndeclaredOutputs undecl;
std::wstring args;
uint64_t shard_status_file_mtime_start, shard_status_file_mtime_end;
if (!AddCurrentDirectoryToPATH() ||
!ParseArgs(argc, argv, &argv0, &test_path_arg, &args) ||
!PrintTestLogStartMarker() || !GetCwd(&exec_root) || !ExportUserName() ||
Expand All @@ -1939,8 +1976,17 @@ int TestWrapperMain(int argc, wchar_t** argv) {
return 1;
}

GetLenientShardStatusFileModificationTime(&shard_status_file_mtime_start);
Duration test_duration;
int result = RunSubprocess(test_path, args, test_outerr, &test_duration);
if (GetLenientShardStatusFileModificationTime(&shard_status_file_mtime_end) &&
shard_status_file_mtime_start == shard_status_file_mtime_end) {
LogError(__LINE__, "Sharding requested, but the test runner did not"
" advertise support for it by touching TEST_SHARD_STATUS_FILE. Either"
" remove the 'shard_count' attribute or use a test runner that supports"
" sharding.");
return 1;
}
if (!CreateXmlLog(xml_log, test_outerr, test_duration, result,
DeleteAfterwards::kEnabled, MainType::kTestWrapperMain) ||
!ArchiveUndeclaredOutputs(undecl) ||
Expand Down

0 comments on commit 3c3d5a0

Please sign in to comment.