Skip to content

fix(tests): make Windows pytest collection portable - #84073

Open
omerbek wants to merge 1 commit into
NousResearch:mainfrom
omerbek:fix/windows-pytest-collection-83935
Open

fix(tests): make Windows pytest collection portable#84073
omerbek wants to merge 1 commit into
NousResearch:mainfrom
omerbek:fix/windows-pytest-collection-83935

Conversation

@omerbek

@omerbek omerbek commented Aug 11, 2026

Copy link
Copy Markdown

Summary

  • avoid calling POSIX-only os.geteuid() while pytest is importing the doctor journal-mode tests on Windows
  • use tempfile.gettempdir() for the parallel-runner handoff directory instead of assuming /tmp
  • make the nested database path assertion use the platform path separator

Fixes #83935.

Testing

  • .venv\Scripts\python.exe -m pytest --collect-only -q tests\hermes_cli\test_doctor_journal_modes.py tests\test_run_tests_parallel.py
  • .venv\Scripts\python.exe -m pytest -q tests\hermes_cli\test_doctor_journal_modes.py

Note: running tests\test_run_tests_parallel.py fully on this Windows checkout still times out inside its subprocess runner smoke test after collection succeeds; this PR targets the collection-time abort described in the issue.

@alt-glitch alt-glitch added type/test Test coverage or test infrastructure comp/cli CLI entry point, hermes_cli/, setup wizard platform/windows Native Windows-specific behavior or breakage P3 Low — cosmetic, nice to have sweeper:risk-platform-windows Sweeper risk: may break or behave differently on native Windows labels Aug 11, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Related: #81926 fixes the same doctor-test collection seam. This PR also adds the separate platform-temp-directory repair for the parallel-runner test.

monerostar

This comment was marked as outdated.

@monerostar monerostar left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Native Win11 verification (monerostar)

Host: Windows 11 (10.0.26200), Python 3.11.15, pytest 9.1.1
SHA: 23b13dd78

Baseline (upstream/main @ 222465d84)

python -m pytest tests/hermes_cli/test_doctor_journal_modes.py --collect-only -q -o addopts=
# ERROR collecting ... AttributeError: module 'os' has no attribute 'geteuid'

test_run_tests_parallel.py still has:

_HANDOFF_DIR = Path(os.environ.get("TMPDIR", "/tmp")) / "hermes-isolation-probe"

On this host collect still succeeds for that file because C:\tmp already exists, so the /tmp fallback does not raise. That masks the issue #83935 class on machines that happen to have \tmp. A missing-parent TMPDIR still fails mkdir with WinError 3 (checked separately).

This PR

# TMPDIR cleared in PowerShell
python -m pytest tests/hermes_cli/test_doctor_journal_modes.py tests/test_run_tests_parallel.py --collect-only -q -o addopts=
# 37 tests collected

python -m pytest tests/hermes_cli/test_doctor_journal_modes.py -q -rs -o addopts=
# 25 passed, 2 skipped  (chmod no-op on Windows)

Both collection holes this PR targets are fixed here. Sibling map for maintainers: #81926 is doctor-journal-only (also green on this box earlier); #74083 is handoff-only and much further behind main. This tip is the complete two-file path.

LGTM from a Windows contributor host.

@monerostar

Copy link
Copy Markdown
Contributor

Native Win11 verification (monerostar overnight, not a maintainer merge signal)

Host: Windows 11 Pro 10.0.26200, CPython 3.11.15, pytest 9.1.1. Compared upstream/main against this PR's two test files.

Collection on current main still dies here. pytest --collect-only of tests/hermes_cli/test_doctor_journal_modes.py from upstream/main:

E   AttributeError: module 'os' has no attribute 'geteuid'

at the class-body @pytest.mark.skipif(os.geteuid() == 0, ...). getattr(os, "geteuid", None) is None on this interpreter, so the decorator is evaluated at collection and the whole module is uncollectable.

This PR: same file collects 27 tests and _running_as_root() returns False. The nested-path assert also holds on this host (test_lists_every_managed_database passed) because it now uses Path("kanban") / "boards" / ... instead of a hardcoded /.

The isolation-probe handoff change (tempfile.gettempdir() instead of $TMPDIR//tmp) is the right Windows default. Note that this git-bash session already exports TMPDIR=C:\Users\Admin\AppData\Local\Temp, so the /tmp fallback is not what I hit tonight — the geteuid collection crash is the live breakage.

Sibling overlap: #74083 also retargets that isolation-probe handoff at the platform temp dir. The geteuid + nested Path pieces here are independent and still needed on main.

Small, correct, I would take it.

@Enough1122

Copy link
Copy Markdown
Contributor

AI code review — automated review for reference, author can ignore or act on any point.

fix(tests): make Windows pytest collection portable — small, correct, no blocking issues. Observations:

  1. tests/hermes_cli/test_doctor_journal_modes.py_running_as_root() fixes the collection-time AttributeError cleanly via getattr; the os.name == "nt" skipifs on those tests become redundant-but-harmless (the helper already returns False on Windows). Could drop the duplicated guard, but leaving it is fine.
  2. nested_db = str(Path("kanban") / "boards" / "myboard" / "kanban.db") yields native separators, so the assertion f"{nested_db} is in WAL mode" in out depends on the doctor report using the same native separator. Verified on native Win11 per the PR, but the assertion stays brittle if the doctor ever normalizes separators (e.g. forward slashes) in its output — consider asserting on a Path-relative form instead of the string.
  3. tests/test_run_tests_parallel.pytempfile.gettempdir() is the right Windows default. Note gettempdir() can resolve differently across processes on Windows (case/8.3 aliasing), but the unique-per-run names plus exist_ok=True make collisions harmless.
  4. The p.read_text(encoding="utf-8") additions elsewhere in the same area are an unrelated-but-welcome portability hardening — just flagging they're outside the PR's stated scope.

kshitijk4poor pushed a commit that referenced this pull request Aug 17, 2026
…rows

read_header_bytes_preopen answers None for a live connection, a missing
file and an unreadable file alike, so the error string doctor prints is
now chosen rather than inherited from the OSError. These cases pin that
choice: the missing file keeps its errno text, and the chmod-000 file is
still reported as a permission problem rather than collapsing into the
generic message — the behaviour the raw open() gave before.

test_reason_does_not_open_the_file is the load-bearing one. It patches
builtins.open to raise and asserts _unreadable_reason still answers,
which fixes the constraint that makes the helper safe to call on a
database path at all: stat() and access() read metadata and take no file
descriptor, so no close() of ours can cancel the file's advisory locks. A
future edit that reached for open() here to get a better message would
reintroduce the original bug on the error path, and this test fails
loudly if it does.

The root check is written as hasattr(os, "geteuid") and os.geteuid() == 0
rather than the bare call the surrounding tests use. skipif conditions are
evaluated at collection time and os.geteuid is POSIX-only, so the bare
form raises AttributeError and takes the whole module down on Windows.
The pre-existing occurrences are left alone — #81926 and #84073 are
already open against exactly those lines, and this only avoids adding a
third instance of the same defect.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/cli CLI entry point, hermes_cli/, setup wizard P3 Low — cosmetic, nice to have platform/windows Native Windows-specific behavior or breakage 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.

[Bug]: Windows: plain pytest aborts during collection, so the whole suite cannot run

4 participants