Skip to content

fix(tests): use shutil.which and skip POSIX-only tests on Windows - #42872

Open
suhas-svg wants to merge 1 commit into
NousResearch:mainfrom
suhas-svg:fix/windows-test-search-hidden-dirs-which
Open

fix(tests): use shutil.which and skip POSIX-only tests on Windows#42872
suhas-svg wants to merge 1 commit into
NousResearch:mainfrom
suhas-svg:fix/windows-test-search-hidden-dirs-which

Conversation

@suhas-svg

@suhas-svg suhas-svg commented Jun 9, 2026

Copy link
Copy Markdown

What does this PR do?

test_search_hidden_dirs.py crashes at collection time on Windows because subprocess.run(["which", "rg"]) calls the Unix which command, which does not exist on Windows (FileNotFoundError: [WinError 2]). This prevents the entire test suite from running.

This PR replaces the Unix-only which with stdlib shutil.which() (as recommended by CONTRIBUTING.md rule #2) and adds platform skip markers for find/grep test classes that use other POSIX-only commands.

Related Issue

No existing issue — discovered while setting up the dev environment on Windows.

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)

Changes Made

  • tests/tools/test_search_hidden_dirs.py:
    • Added import shutil and import sys
    • Replaced subprocess.run(["which", "rg"], capture_output=True).returncode != 0 with shutil.which("rg") is None (lines 100, 113)
    • Added @pytest.mark.skipif(sys.platform == "win32", reason="find is not available on Windows") to TestFindExcludesHiddenDirs
    • Added @pytest.mark.skipif(sys.platform == "win32", reason="grep is not available on Windows") to TestGrepExcludesHiddenDirs

How to Test

  1. On a Windows machine, checkout this branch
  2. Run pytest tests/tools/test_search_hidden_dirs.py -v --timeout-method=thread
  3. Verify: 4 passed, 5 skipped, 0 errors

Before this fix:

ERROR collecting tests/tools/test_search_hidden_dirs.py FileNotFoundError: [WinError 2] The system cannot find the file specified

After this fix:

4 passed, 5 skipped in 0.79s

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(scope):, feat(scope):, etc.)
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix/feature (no unrelated commits)
  • I've run pytest tests/ -q and all tests pass
  • I've added tests for my changes (required for bug fixes, strongly encouraged for features)
  • I've tested on my platform: Windows 10, Python 3.12.5

Documentation & Housekeeping

  • I've considered cross-platform impact (Windows, macOS) per the compatibility guide — or N/A
  • I've updated relevant documentation (README, docs/, docstrings) — or N/A (no docs changes needed)
  • I've updated cli-config.yaml.example if I added/changed config keys — or N/A
  • I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — or N/A
  • I've updated tool descriptions/schemas if I changed tool behavior — or N/A

Screenshots / Logs

============================= test session starts ============================= platform win32 -- Python 3.12.5, pytest-9.0.2 tests/tools/test_search_hidden_dirs.py::TestFindExcludesHiddenDirs::test_find_skips_hub_cache_files SKIPPED tests/tools/test_search_hidden_dirs.py::TestFindExcludesHiddenDirs::test_find_skips_git_internals SKIPPED tests/tools/test_search_hidden_dirs.py::TestFindExcludesHiddenDirs::test_find_still_returns_visible_files SKIPPED tests/tools/test_search_hidden_dirs.py::TestGrepExcludesHiddenDirs::test_grep_skips_hub_cache SKIPPED tests/tools/test_search_hidden_dirs.py::TestGrepExcludesHiddenDirs::test_grep_still_finds_visible_content SKIPPED tests/tools/test_search_hidden_dirs.py::TestRipgrepAlreadyExcludesHidden::test_rg_skips_hub_by_default PASSED tests/tools/test_search_hidden_dirs.py::TestRipgrepAlreadyExcludesHidden::test_rg_finds_visible_content PASSED tests/tools/test_search_hidden_dirs.py::TestIgnoreFileWritten::test_write_index_cache_creates_ignore_file PASSED tests/tools/test_search_hidden_dirs.py::TestIgnoreFileWritten::test_write_index_cache_does_not_overwrite_existing_ignore PASSED ======================== 4 passed, 5 skipped in 0.79s =========================

test_search_hidden_dirs.py crashes at collection time on Windows because
subprocess.run(["which", "rg"]) calls the Unix `which` command, which
does not exist on Windows (FileNotFoundError: [WinError 2]).

Changes:
- Replace subprocess.run(["which", "rg"]) with shutil.which("rg")
  for cross-platform ripgrep detection (CONTRIBUTING.md rule NousResearch#2)
- Add pytest.mark.skipif(sys.platform == "win32") to find/grep test
  classes since those commands are not available on Windows

All 9 tests now pass on Windows (4 passed, 5 correctly skipped).
@alt-glitch alt-glitch added type/test Test coverage or test infrastructure P3 Low — cosmetic, nice to have comp/tools Tool registry, model_tools, toolsets labels Jun 9, 2026

@tonydwb tonydwb left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review Summary

Verdict: Approved

Changes Overview

  • Windows compatibility fix for test_search_hidden_dirs.py
  • Uses shutil.which instead of subprocess.run(["which", ...]) for command detection
  • Skips POSIX-only tests (find, grep) on Windows

Analysis

Correctness

  • Clean, focused fix for Windows compatibility
  • shutil.which is the idiomatic Python way to check for command availability

Code Quality

  • Minimal changes, single-purpose fix

Testing

  • Appropriate use of @pytest.mark.skipif for platform-specific tests

Reviewed by Hermes Agent

@teknium1

Copy link
Copy Markdown
Contributor

Thanks for the focused Windows test-compatibility fix. Current main still evaluates the POSIX-only which command in collection-time decorators at tests/tools/test_search_hidden_dirs.py:100 and :113. The replacement with shutil.which("rg") directly removes that failure mode, while the class-level Windows guards match the shell find/grep test implementations at :50-92.

No substantive problems identified. The patch applies to unchanged surrounding test code and is mechanically salvageable.

Automated hermes-sweeper review.

@hyqqx

hyqqx commented Aug 3, 2026

Copy link
Copy Markdown

Native Windows 11 verification

Verified on a real Windows 11 Home host, build 26200, Python 3.11.6, from PowerShell. This still applies cleanly and still fixes a live failure on today's main (fe4cf36c2).

Before — current main, collection aborts:

$ python -m pytest tests/tools/test_search_hidden_dirs.py --collect-only -q
E   FileNotFoundError: [WinError 2] The system cannot find the file specified
ERROR tests/tools/test_search_hidden_dirs.py
Interrupted: 1 error during collection

Cherry-pick of ecfd1e86e onto fe4cf36c2 — clean, no conflicts.

After:

$ python -m pytest tests/tools/test_search_hidden_dirs.py --collect-only -q
8 tests collected in 0.29s

$ python -m pytest tests/tools/test_search_hidden_dirs.py -q
2 passed, 6 skipped in 0.96s

Why this matters more than one file

pytest aborts the entire run on a collection error, so any one of these blocks all ~50k tests on Windows, not just its own module. There were three such blockers:

With this one merged, all three are cleared and pytest tests/ collects on native Windows.

One note for anyone re-testing: run it from PowerShell or cmd, not Git Bash. MSYS puts its own POSIX which on PATH, so the failure this PR fixes is invisible there — which is likely part of why it sat unverified.

LGTM from native Win11. (Not a formal approve — I'm not a collaborator on this repo.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/tools Tool registry, model_tools, toolsets P3 Low — cosmetic, nice to have sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users sweeper:risk-platform-windows Sweeper risk: may break or behave differently on native Windows type/test Test coverage or test infrastructure

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants