From 091e79ec5f18eda6b3876c0f4e3a36158da394c6 Mon Sep 17 00:00:00 2001 From: Jeff Watts <186512915+lEWFkRAD@users.noreply.github.com> Date: Thu, 2 Jul 2026 11:10:13 -0400 Subject: [PATCH] fix(tests): Windows-aware path-list split and UTF-8 progress output in parallel runner Two Windows bugs in scripts/run_tests_parallel.py: - --files/--paths/HERMES_TEST_PATHS were split on ':', which shreds absolute Windows paths at the drive letter ('C:\repo\tests' -> ['C', '\repo\tests']): the drive letter became a phantom discovery root and the rooted remainder only resolved by WindowsPath re-anchoring it onto repo_root's drive. New _split_pathspec() keeps drive-letter colons glued to their path and accepts ';' (os.pathsep) on Windows, while ':'-joined lists (CI generate job) keep working. - With piped stdout (CI, subprocess capture) Windows encodes the runner's output as the ANSI code page, so printing the per-file progress glyphs raised UnicodeEncodeError inside the executor done-callback and every progress line was silently lost -- which is also why test_bare_value_flag_keeps_its_value failed on win32 (no '1[check]' line, and the summary says '1 tests passed', which does not contain '1 passed'). The runner now reconfigures its own stdout/stderr to UTF-8 on Windows, and the tests decode the captured output as UTF-8. Adds regression tests: os.pathsep-joined absolute roots (all platforms) and no-phantom-drive-root (win32). Fixes #57149 --- scripts/run_tests_parallel.py | 68 ++++++++++++++++++++++++++++---- tests/test_run_tests_parallel.py | 53 ++++++++++++++++++++++++- 2 files changed, 112 insertions(+), 9 deletions(-) diff --git a/scripts/run_tests_parallel.py b/scripts/run_tests_parallel.py index 68c9423db6738..58111b4d8b36c 100755 --- a/scripts/run_tests_parallel.py +++ b/scripts/run_tests_parallel.py @@ -32,7 +32,9 @@ Environment: HERMES_TEST_WORKERS Override worker count (default: os.cpu_count()) - HERMES_TEST_PATHS Override discovery roots (colon-sep, default: 'tests') + HERMES_TEST_PATHS Override discovery roots (colon-sep; on Windows + ';' also works and drive letters are handled; + default: 'tests') Exit code: 0 if every file's pytest exited 0; 1 otherwise. """ @@ -91,6 +93,41 @@ _DURATIONS_FILE = "test_durations.json" +def _split_pathspec(value: str) -> List[str]: + """Split a separator-joined path list (``--paths``/``--files``/ + ``HERMES_TEST_PATHS``) into individual paths. + + POSIX: ``:``-separated, as documented. + + Windows: ``;`` (``os.pathsep``) and ``:`` are both accepted as + separators, but a ``:`` that forms a drive letter (``C:\\...`` or + ``C:/...``) stays glued to its path — a naive ``split(":")`` turns + ``C:\\repo\\tests`` into ``['C', '\\repo\\tests']``, where the bogus + ``C`` becomes a phantom discovery root and the rooted remainder only + resolves by accident of ``Path.__truediv__`` re-anchoring it onto + ``repo_root``'s drive. + """ + if sys.platform != "win32": + return [p for p in value.split(":") if p.strip()] + parts: List[str] = [] + for chunk in value.split(";"): + raw = chunk.split(":") + i = 0 + while i < len(raw): + part = raw[i] + if ( + len(part) == 1 + and part.isalpha() + and i + 1 < len(raw) + and raw[i + 1][:1] in ("\\", "/") + ): + part = f"{part}:{raw[i + 1]}" + i += 1 + parts.append(part) + i += 1 + return [p for p in parts if p.strip()] + + def _approximately_count_tests( files: List[Path], repo_root: Path ) -> dict[Path, int]: @@ -592,6 +629,18 @@ def _slice_files( def main() -> int: + if sys.platform == "win32": + # When stdout/stderr are pipes (CI, subprocess capture, redirection), + # Windows defaults them to the legacy ANSI code page (e.g. cp1252), + # which cannot encode the ✓/✗ progress glyphs. The resulting + # UnicodeEncodeError fires inside the executor's done-callback, so + # per-file progress lines are silently swallowed. Force UTF-8. + for _stream in (sys.stdout, sys.stderr): + try: + _stream.reconfigure(encoding="utf-8", errors="replace") + except (AttributeError, OSError): + pass + parser = argparse.ArgumentParser( description=__doc__, formatter_class=argparse.RawDescriptionHelpFormatter, @@ -606,7 +655,11 @@ def main() -> int: parser.add_argument( "--paths", default=os.environ.get("HERMES_TEST_PATHS", ":".join(_DEFAULT_ROOTS)), - help="Colon-separated discovery roots (default: 'tests')", + help=( + "Colon-separated discovery roots (default: 'tests'). On " + "Windows, ';' also separates and drive letters (C:\\...) are " + "kept intact." + ), ) parser.add_argument( "--include-integration", @@ -652,9 +705,10 @@ def main() -> int: "--files", metavar="LIST", help=( - "Explicit colon-separated list of test files to run. Bypasses " - "discovery entirely — used by CI matrix jobs that receive their " - "file list from the generate job." + "Explicit colon-separated list of test files to run (on " + "Windows, ';' also separates and drive letters are kept " + "intact). Bypasses discovery entirely — used by CI matrix " + "jobs that receive their file list from the generate job." ), ) parser.add_argument( @@ -749,7 +803,7 @@ def _is_our_flag(tok: str) -> bool: # --files: explicit file list from the CI generate job — skip discovery. if args.files: - files = [repo_root / f for f in args.files.split(":") if f.strip()] + files = [repo_root / f for f in _split_pathspec(args.files)] roots = [] else: # Resolve discovery roots: positional path args override --paths if any @@ -757,7 +811,7 @@ def _is_our_flag(tok: str) -> bool: if args.paths_positional: roots = [repo_root / p for p in args.paths_positional] else: - roots = [repo_root / p for p in args.paths.split(":") if p] + roots = [repo_root / p for p in _split_pathspec(args.paths)] if args.include_integration: # Caller takes responsibility — typically used via explicit -k filter. diff --git a/tests/test_run_tests_parallel.py b/tests/test_run_tests_parallel.py index 3cba46fab00da..3604f801672bf 100644 --- a/tests/test_run_tests_parallel.py +++ b/tests/test_run_tests_parallel.py @@ -220,7 +220,11 @@ def _run_runner(probe_dir: Path, *extra: str) -> subprocess.CompletedProcess: cwd=repo_root, stdout=subprocess.PIPE, stderr=subprocess.STDOUT, - text=True, + # The runner emits UTF-8 (it reconfigures its own piped stdout on + # Windows, which would otherwise default to the ANSI code page and + # turn the ✓/✗ glyphs into mojibake on decode here). + encoding="utf-8", + errors="replace", timeout=60, ) @@ -271,9 +275,54 @@ def test_positional_path_not_treated_as_flag(tmp_path: Path) -> None: [sys.executable, str(runner), str(probe_dir), "-j", "1", "--file-timeout", "30", "-q"], cwd=repo_root, stdout=subprocess.PIPE, stderr=subprocess.STDOUT, - text=True, timeout=60, + encoding="utf-8", errors="replace", timeout=60, ) assert proc.returncode == 0, proc.stdout # Discovery found the probe file (2 tests), proving the positional path # was consumed as a root, not forwarded to pytest as a bad flag. assert "test_flagprobe.py" in proc.stdout, proc.stdout + + +def test_multiple_absolute_paths_split_on_pathsep(tmp_path: Path) -> None: + """``--paths`` accepts ``os.pathsep``-joined absolute paths. + + On Windows the absolute paths contain drive-letter colons, so a naive + ``split(":")`` shreds them into phantom roots and only one (or neither) + of the two probe dirs would be discovered. + """ + dir_a = _make_probe_dir(tmp_path) + dir_b = tmp_path / "probe_b" + dir_b.mkdir() + (dir_b / "test_flagprobe_b.py").write_text( + "def test_gamma():\n assert True\n" + ) + repo_root = Path(__file__).resolve().parent.parent + runner = repo_root / "scripts" / "run_tests_parallel.py" + proc = subprocess.run( + [sys.executable, str(runner), + "--paths", os.pathsep.join([str(dir_a), str(dir_b)]), + "-j", "1", "--file-timeout", "30", "-q"], + cwd=repo_root, stdout=subprocess.PIPE, stderr=subprocess.STDOUT, + encoding="utf-8", errors="replace", timeout=60, + ) + assert proc.returncode == 0, proc.stdout + assert "Discovered 2 test files" in proc.stdout, proc.stdout + + +@pytest.mark.skipif(sys.platform != "win32", reason="drive-letter paths") +def test_drive_letter_colon_is_not_a_path_separator(tmp_path: Path) -> None: + """An absolute ``--paths`` value stays one root on Windows. + + The naive split used to produce a phantom relative root ``'C'`` (the + drive letter) alongside the real path; discovery only worked by the + accident of ``repo_root / '\\rooted\\rest'`` re-anchoring onto the + repo's drive. + """ + probe_dir = _make_probe_dir(tmp_path) + proc = _run_runner(probe_dir, "-q") + assert proc.returncode == 0, proc.stdout + drive = str(probe_dir)[0] + assert f"['{drive}', " not in proc.stdout, ( + f"drive letter split off as a phantom root:\n{proc.stdout}" + ) + assert "Discovered 1 test files" in proc.stdout, proc.stdout