diff --git a/.github/workflows/ci-guards.yml b/.github/workflows/ci-guards.yml index 9ca07a3f911..4394b04f5f3 100644 --- a/.github/workflows/ci-guards.yml +++ b/.github/workflows/ci-guards.yml @@ -618,6 +618,7 @@ jobs: python3 tests/test_ci_focused_test_selectors.py python3 tests/test_ci_ui_tests_dispatch.py python3 tests/test_ci_pr_media.py + python3 tests/test_ci_guard_tests_emit_no_workflow_commands.py - name: Validate manual macOS package cache recovery if: ${{ matrix.group == 'app-host-execution' }} diff --git a/tests/test-execution.toml b/tests/test-execution.toml index 69dcb0e8028..03da13d385a 100644 --- a/tests/test-execution.toml +++ b/tests/test-execution.toml @@ -1429,3 +1429,7 @@ lane = "linux-guard" [[test]] path = "tests/test_ci_pr_media.py" lane = "linux-guard" + +[[test]] +path = "tests/test_ci_guard_tests_emit_no_workflow_commands.py" +lane = "linux-guard" diff --git a/tests/test_ci_guard_tests_emit_no_workflow_commands.py b/tests/test_ci_guard_tests_emit_no_workflow_commands.py new file mode 100755 index 00000000000..9b9c038bd9f --- /dev/null +++ b/tests/test_ci_guard_tests_emit_no_workflow_commands.py @@ -0,0 +1,83 @@ +#!/usr/bin/env python3 +"""Passing guard tests print no GitHub workflow commands. + +The runner reads every line of a step's output that starts with `::` as a +workflow command, so a fixture that makes a script under test print +`::error::...` becomes a red annotation on the pull request's run page even +though the test passed. PR 15160's run 36420353579 showed "No ci-ui-tests.yml +run titled 'UI tests for CI run 100 attempt 1' appeared" and "https://x/900 +ended success without running the UI tests" from test_ci_ui_tests_dispatch.py, +which read as real CI failures. + +Each module below exercises code that annotates on purpose. It runs here the +way the guard job runs it, with GITHUB_ACTIONS set, and its output must hold +no line the runner would parse as a command. A module that fails is left to +its own step to report: a failing test's captured output may annotate. +""" + +from __future__ import annotations + +import os +import re +import subprocess +import sys +import tempfile +import unittest +from concurrent.futures import ThreadPoolExecutor +from pathlib import Path + +ROOT = Path(__file__).resolve().parents[1] + +# Guard tests whose scripts under test print annotations. +ANNOTATING_MODULES = ( + "tests/test_ci_helper_prebuild_lifecycle.py", + "tests/test_ci_late_placement.py", + "tests/test_ci_main_regression_bisect.py", + "tests/test_ci_parallel_artifact_transport.py", + "tests/test_ci_pr_media.py", + "tests/test_ci_prune_pr_media.py", + "tests/test_ci_ui_tests_dispatch.py", + "tests/test_ios_upload_batching.py", +) + +# unittest's progress characters share stderr with the script's output, so a +# command may follow them on one line of the combined log; the runner reads +# stdout and stderr as separate streams, where it starts the line. +COMMAND = re.compile(r"^\s*[.EFsxu]*::(?:error|warning|notice|debug|group|endgroup|add-mask|stop-commands|echo" + r"|set-output|save-state|set-env|add-path|add-matcher|remove-matcher)\b") + + +def run_module(relative: str) -> tuple[int, str]: + """Exit code and combined output. Files, not pipes: a descendant a test + leaves behind would hold a pipe open and hang the read.""" + env = {**os.environ, "GITHUB_ACTIONS": "true"} + env.pop("GITHUB_OUTPUT", None) + env.pop("GITHUB_STEP_SUMMARY", None) + with tempfile.TemporaryFile("w+") as output: + code = subprocess.run([sys.executable, str(ROOT / relative)], cwd=ROOT, env=env, stdin=subprocess.DEVNULL, + stdout=output, stderr=output, timeout=600, check=False).returncode + output.seek(0) + return code, output.read() + + +class GuardTestsEmitNoWorkflowCommandsTests(unittest.TestCase): + def test_passing_guard_tests_print_no_workflow_commands(self) -> None: + with ThreadPoolExecutor(max_workers=len(ANNOTATING_MODULES)) as pool: + results = dict(zip(ANNOTATING_MODULES, pool.map(run_module, ANNOTATING_MODULES))) + checked = 0 + for relative, result in results.items(): + code, output = result + with self.subTest(module=relative): + if code != 0: + # Its own step reports the failure; say which module this check skipped. + self.skipTest(f"{relative} failed here (exit {code}), so its output was not checked") + checked += 1 + leaked = [line for line in output.splitlines() if COMMAND.match(line)] + # repr() quotes each line, so this report is never read as a command itself. + self.assertFalse(leaked, f"{relative} printed workflow commands:\n" + + "\n".join(repr(line) for line in leaked[:10])) + self.assertGreater(checked, 0, "no annotating guard test passed, so nothing was checked") + + +if __name__ == "__main__": + unittest.main(buffer=True) diff --git a/tests/test_ci_helper_prebuild_lifecycle.py b/tests/test_ci_helper_prebuild_lifecycle.py index 460796b336c..107f66fe372 100644 --- a/tests/test_ci_helper_prebuild_lifecycle.py +++ b/tests/test_ci_helper_prebuild_lifecycle.py @@ -101,4 +101,4 @@ def test_cancellation_kills_helper_descendants(self): if __name__ == "__main__": - unittest.main() + unittest.main(buffer=True) diff --git a/tests/test_ci_late_placement.py b/tests/test_ci_late_placement.py index 8f5b84c248d..ac96cd3c5bf 100644 --- a/tests/test_ci_late_placement.py +++ b/tests/test_ci_late_placement.py @@ -439,4 +439,4 @@ def test_moved_jobs_leave_the_marker_the_rescue_watch_looks_for(self): if __name__ == "__main__": - unittest.main() + unittest.main(buffer=True) diff --git a/tests/test_ci_main_regression_bisect.py b/tests/test_ci_main_regression_bisect.py index 988d99a3200..75459b505cf 100644 --- a/tests/test_ci_main_regression_bisect.py +++ b/tests/test_ci_main_regression_bisect.py @@ -479,4 +479,4 @@ def test_advances_on_a_schedule_and_after_each_report(self): if __name__ == "__main__": - unittest.main() + unittest.main(buffer=True) diff --git a/tests/test_ci_parallel_artifact_transport.py b/tests/test_ci_parallel_artifact_transport.py index 23d33c9b0d9..942439c757d 100644 --- a/tests/test_ci_parallel_artifact_transport.py +++ b/tests/test_ci_parallel_artifact_transport.py @@ -458,4 +458,4 @@ def test_unknown_kind_cannot_choose_a_destination(self): if __name__ == "__main__": - unittest.main(verbosity=2) + unittest.main(verbosity=2, buffer=True) diff --git a/tests/test_ci_pr_media.py b/tests/test_ci_pr_media.py index ee5284f8119..e051cc11667 100644 --- a/tests/test_ci_pr_media.py +++ b/tests/test_ci_pr_media.py @@ -722,4 +722,4 @@ def test_adopts_on_matches_the_product_family(self) -> None: if __name__ == "__main__": - unittest.main() + unittest.main(buffer=True) diff --git a/tests/test_ci_prune_pr_media.py b/tests/test_ci_prune_pr_media.py index 8a657e16651..55265df0d99 100644 --- a/tests/test_ci_prune_pr_media.py +++ b/tests/test_ci_prune_pr_media.py @@ -154,4 +154,4 @@ def test_only_main_prunes_and_a_dry_run_is_the_default(self) -> None: if __name__ == "__main__": - unittest.main() + unittest.main(buffer=True) diff --git a/tests/test_ci_ui_tests_dispatch.py b/tests/test_ci_ui_tests_dispatch.py index cb135071428..424a8f371e9 100644 --- a/tests/test_ci_ui_tests_dispatch.py +++ b/tests/test_ci_ui_tests_dispatch.py @@ -458,4 +458,4 @@ def test_the_dispatching_workflow_runs_from_the_default_branch(self) -> None: if __name__ == "__main__": - unittest.main() + unittest.main(buffer=True) diff --git a/tests/test_ios_upload_batching.py b/tests/test_ios_upload_batching.py index a9d9d5b0bbb..8bfea6e439e 100644 --- a/tests/test_ios_upload_batching.py +++ b/tests/test_ios_upload_batching.py @@ -408,4 +408,4 @@ def test_batching_steps_continue_on_error(self): if __name__ == "__main__": - unittest.main() + unittest.main(buffer=True)