fix(tests): Windows-aware path-list split and UTF-8 progress output in parallel runner - #57152
fix(tests): Windows-aware path-list split and UTF-8 progress output in parallel runner#57152lEWFkRAD wants to merge 1 commit into
Conversation
…n 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 NousResearch#57149
Fix PR for #57149 (issue↔PR pair). This is a superset of the open #51896, which fixes only the legacy-encoding (UnicodeEncodeError) half of the same file — this PR fixes both that and the drive-letter path-list split. Related to the broader Windows test-runner compat work in #42775. Flagging the #57152 / #51896 overlap so a maintainer picks one. |
|
Thanks for the focused Windows compatibility fix. The current-main implementation still has the two reported failure mechanisms: Problems
Suggested changes
This is an automated hermes-sweeper review. |
|
Half merged in #81965: your |
What does this PR do?
Fixes two pre-existing Windows bugs in the parallel test runner (
scripts/run_tests_parallel.py), both verified onmain@ 30e947e on Windows 11:Drive-letter-safe path-list splitting.
--files,--paths, andHERMES_TEST_PATHSwere split with a naive.split(":"), which shreds absolute Windows paths at the drive letter:--paths C:\repo\testsbecame['C', '\repo\tests']. The bogusCturned into a phantom discovery root (visible in the discovery banner), and the rooted remainder only resolved by accident ofWindowsPath.__truediv__re-anchoring it ontorepo_root's drive — point it at another drive and discovery silently finds nothing. Two roots joined withos.pathsep(;) were shredded even worse. A new_split_pathspec()helper keeps:as the separator on POSIX and, on Windows, accepts both;(os.pathsep) and:while keeping drive-letter colons glued to their paths — so the:-joined repo-relative lists emitted by the CI generate job keep working unchanged on every platform.Per-file progress lines no longer vanish on piped Windows output (this is what made
tests/test_run_tests_parallel.py::test_bare_value_flag_keeps_its_valuefail on win32). With stdout attached to a pipe (CI, subprocess capture, redirection), Windows encodes it with the legacy ANSI code page (cp1252), which cannot encode the✓/✗glyphs — every_print_progress()call raisedUnicodeEncodeErrorinside theThreadPoolExecutordone-callback, so all per-file progress lines were silently swallowed (with traceback spam on stderr). The failing test was a symptom: it asserts"1✓" in stdout or "1 passed" in stdout, but the1✓line never printed and the ASCII summary reads1 tests passed, which doesn't contain the substring1 passed. The runner now reconfigures its own stdout/stderr to UTF-8 (errors="replace") on Windows at the top ofmain(), and the test helpers decode the captured output as UTF-8 explicitly so the glyphs round-trip instead of turning into mojibake.The runner fix (not an assertion relaxation) was chosen for bug 2 because losing every per-file progress line under CI/redirection is a real product defect, not just a test-expectation mismatch.
Related Issue
Fixes #57149
Type of Change
Changes Made
scripts/run_tests_parallel.py_split_pathspec()— drive-letter-aware splitting for--files/--paths/HERMES_TEST_PATHS; both call sites switched to it; help text and module docstring updated.main()reconfiguressys.stdout/sys.stderrto UTF-8 witherrors="replace"on win32 (guarded, no-op elsewhere).tests/test_run_tests_parallel.py_run_runner()and the inlinesubprocess.runcapture the runner's output withencoding="utf-8", errors="replace"(previously locale-dependenttext=True).test_multiple_absolute_paths_split_on_pathsep(all platforms): two absolute roots joined withos.pathsepare both discovered.test_drive_letter_colon_is_not_a_path_separator(win32-only): no phantom drive-letter root in the discovery banner.How to Test
main@ 30e947e:python -m pytest tests/test_run_tests_parallel.py::test_bare_value_flag_keeps_its_value -x→ fails; the captured runner output shows the phantom['C', 'C:\\...']discovery root and aUnicodeEncodeErrortraceback from the progress callback.python -m pytest tests/test_run_tests_parallel.py -q→6 passed, 1 skipped(the skip is the POSIX-only zombie-cleanup verifier).python scripts/run_tests_parallel.py --paths C:\abs\path\to\tests -j 1 -q | more→ discovery banner shows a single intact root and the per-file✓progress line survives the pipe.:-joined lists (including CI's generate-job output) split exactly as before.Checklist
Code
fix(scope):,feat(scope):, etc.)pytest tests/ -qand all tests pass (fulltests/test_run_tests_parallel.pysuite; runner also exercised end-to-end)Documentation & Housekeeping
docs/, docstrings) — runner docstring +--paths/--fileshelp textcli-config.yaml.exampleif I added/changed config keys — N/ACONTRIBUTING.mdorAGENTS.mdif I changed architecture or workflows — N/AScreenshots / Logs
Before (win32, piped stdout):
After (win32, piped stdout):