fix(windows-tests): address timeout crash, parallel runner encoding, and symlink privilege errors#39513
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Introduces a new per-file parallel pytest runner and updates the test infrastructure to rely on per-file process isolation, alongside adding symlink-gated tests and bumping a core dependency to address a crash.
Changes:
- Add
scripts/run_tests_parallel.pyto runpytestin parallel with one subprocess per test file (with optional slicing and per-file timeout). - Update
tests/conftest.pyto remove cross-test state-reset fixtures and add arequire_symlinksmarker with an environment capability check. - Add/adjust tests to use
@pytest.mark.require_symlinksand add coverage forsecure_parent_dir()behavior.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_hermes_constants.py | Adds tests for secure_parent_dir() to prevent chmod on unsafe root/top-level directories. |
| tests/test_atomic_replace_symlinks.py | Marks symlink-dependent tests with require_symlinks for environment portability. |
| tests/conftest.py | Removes per-test state reset/timeouts; adds require_symlinks marker and skip logic. |
| tests/cli/test_worktree_security.py | Marks symlink-dependent worktree security tests with require_symlinks. |
| scripts/run_tests_parallel.py | New per-file parallel test runner with discovery, slicing, timeouts, and summaries. |
| pyproject.toml | Bumps pydantic; removes xdist/split from dev extras; adjusts pytest addopts for new runner model. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def _check_symlink_support() -> bool: | ||
| global _symlink_supported_cache | ||
| if _symlink_supported_cache is not None: | ||
| return _symlink_supported_cache | ||
|
|
||
| import tempfile | ||
| try: | ||
| with tempfile.TemporaryDirectory() as d: | ||
| src = Path(d) / "src" | ||
| src.touch() | ||
| lnk = Path(d) / "lnk" | ||
| lnk.symlink_to(src) | ||
| _symlink_supported_cache = True | ||
| return True | ||
| except OSError: | ||
| _symlink_supported_cache = False | ||
| return False |
| # ── Per-test timeout — handled by the isolation plugin ───────────────────── | ||
| # | ||
| # The subprocess-per-test plugin enforces the configured ``isolate_timeout`` | ||
| # ini key by terminating the child if it overruns. The old SIGALRM-based | ||
| # fixture (POSIX-only, didn't work on Windows) is gone. |
| unless explicitly requested), then runs one ``python -m pytest <file>`` | ||
| subprocess per file, with bounded parallelism (default: ``os.cpu_count()``). |
| if sys.platform == "win32": | ||
| try: | ||
|
|
||
| subprocess.run( | ||
| ["taskkill", "/F", "/T", "/PID", str(proc.pid)], | ||
| stdout=subprocess.DEVNULL, | ||
| stderr=subprocess.DEVNULL, | ||
| timeout=10, | ||
| ) # windows-footgun: ok | ||
| except (subprocess.TimeoutExpired, FileNotFoundError, OSError): | ||
| pass |
| # Happy path: pytest exited on its own. The child process already | ||
| # cleaned up its grandchildren if it's well-behaved, but | ||
| # well-behaved is not universal — kill the group anyway. Already- | ||
| # dead processes are a no-op. | ||
| _kill_tree(proc, pgid=pgid) |
| for fut in futures: | ||
| fut.result() if fut.exception() is None else None |
…and symlink privilege errors
e19d9f7 to
f0f23d1
Compare
austinpickett
left a comment
There was a problem hiding this comment.
Mixed bag — one part is obsolete and would regress main, but two parts are genuinely useful. Recommend salvaging the good pieces into a fresh PR off current main.
1. Timeout fix — 🔴 obsolete, would regress
This PR changes addopts to drop the timeout method (--timeout=30 --timeout-method=signal → just --timeout=30). But current origin/main already carries the canonical cross-platform fix:
addopts = "-m 'not integration' --timeout=30 --timeout-method=thread"
Merging this would downgrade main from an explicit thread method to no method at all. The SIGALRM-on-Windows problem is already solved on main (and #43182 was the superseding work). Drop this hunk.
2. Parallel-runner encoding fix — ✅ valid, net-new
sys.stdout/stderr.reconfigure(encoding='utf-8') guarded by hasattr + try/except. No-op on POSIX (already UTF-8), fixes UnicodeEncodeError on Windows cp1252 consoles, degrades safely. Not on main. Keep this.
3. Symlink privilege handling — ✅ reasonable, but coordinate
require_symlinks marker + runtime _check_symlink_support() probe + pytest_runtest_setup skip hook. Sound and POSIX-safe. But it overlaps conceptually with the still-open #40891 ("harden Windows portability tests and paths") — reconcile to avoid duplication.
Mechanics
mergeable: CONFLICTING / DIRTY — branch is ~678 commits behind main, the pyproject.toml timeout line and tests/conftest.py will conflict directly. Also minor noise: a stray blank line splitting import os/import sys (isort regression) and a cosmetic # windows-footgun: ok comment on an unchanged killpg guard.
Best path: cut a fresh branch from current main with just the encoding reconfigure (and the symlink marker, coordinated with #40891), dropping the obsolete timeout edit and the cosmetic churn.
…and stabilize Windows test suite
Addresses timeout crash, parallel runner encoding, and symlink privilege errors on Windows.