fix(windows-tests): address parallel runner encoding and symlink privilege errors - #39513
fix(windows-tests): address parallel runner encoding and symlink privilege errors#39513aniruddhaadak80 wants to merge 1 commit into
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 |
e19d9f7 to
f0f23d1
Compare
alt-glitch
added
type/test
austinpickett
left a comment
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.
aniruddhaadak80
changed the title
f0f23d1 to
19bfd61
Compare
…ilege errors (Fixes NousResearch#39480)
19bfd61 to
a81b49c
Compare
teknium1
left a comment
teknium1
left a comment
There was a problem hiding this comment.
Thanks for preserving the focused encoding and symlink-compatibility work. The encoding premise remains on current main: scripts/run_tests_parallel.py:390-467 prints Unicode glyphs and does not configure its streams before main() at :594.
Problems
- The PR says
Fixes #39480, but current issue #39480 concerns profile-scoped dotenv loading during cron MCP initialization, not Windows test encoding or symlinks. Please remove that unrelated closing reference. tests/cli/test_worktree_security.py:111skips the directory include test when symlinks are unavailable. That hides the deliberate Windows fallback incli.py:1542-1556, which catches symlink errors and usescopytree; the test should validate that fallback instead.- Direct symlink creation remains ungated at
tests/cli/test_worktree.py:540andtests/cli/test_cli_file_drop.py:246.
Suggested changes
- Keep the directory include test runnable and accept either a symlink or copied directory while checking its contents.
- Gate or locally skip the remaining direct-symlink tests that require privilege.
Automated hermes-sweeper review.
| @@ -110,6 +111,7 @@ def test_allows_valid_file_include(self, git_repo): | |||
| finally: | |||
There was a problem hiding this comment.
This test exercises _setup_worktree's Windows copytree fallback (cli.py:1542-1556) when symlink creation is unavailable. Skipping it removes coverage of that intentional path; keep it runnable and assert the included directory is usable whether it is a symlink or a copied directory.
teknium1
added
sweeper:risk-automation
…and stabilize Windows test suite
|
Half merged in #81965: your |
This PR addresses Windows compatibility test failures (Fixes #39480). It keeps the parallel-runner encoding fix and the symlink privilege handling, but drops the obsolete timeout edit and cosmetic churn as requested by reviewers.