fix(tests): make test_doctor_journal_modes collectable and green on Windows - #81926
Open
hyqqx wants to merge 1 commit into
Open
fix(tests): make test_doctor_journal_modes collectable and green on Windows#81926hyqqx wants to merge 1 commit into
hyqqx wants to merge 1 commit into
Conversation
…indows This file is new in v0.20.0 (6583297) and aborts collection on native Windows, which takes the whole run down with it -- pytest stops on a collection error, so no test in the invocation gets to run. ERROR collecting tests/hermes_cli/test_doctor_journal_modes.py E AttributeError: module 'os' has no attribute 'geteuid' The Windows guard is already there and already correct: @pytest.mark.skipif(os.name == "nt", reason="chmod is a no-op on Windows") @pytest.mark.skipif(os.geteuid() == 0, reason="root ignores file permissions") but the second decorator evaluates `os.geteuid()` at import time, before pytest looks at either marker -- so the first one never gets a chance to apply. `os.geteuid` does not exist on Windows. Short-circuiting on `hasattr` fixes it and is the form already used for the same pair in tests/test_hermes_state_readonly_preflight.py:31. POSIX behaviour is byte-identical: `hasattr(os, "geteuid")` is True there, so the euid check runs exactly as before. With collection unblocked, one more Windows failure becomes reachable in the same file: test_lists_every_managed_database asserts a POSIX path. doctor names nested databases with `str(board_db.relative_to(hermes_home))` (hermes_cli/doctor.py:114), which uses the platform separator, so the expectation is built the same way instead of hardcoding forward slashes. No production code is touched -- doctor's output is already correct on both platforms. Verified on native Windows 11 (build 26200), Python 3.11.6, from PowerShell: before: ERROR at collection, 0 tests runnable after: 25 passed, 2 skipped (exit 0) The two skips are the author's own `chmod is a no-op on Windows` reason, now actually firing. Across the 230 test files added since v2026.7.30 this was the only collection error, and the bare `skipif(os.geteuid()...)` pattern appears nowhere else in tests/. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
monerostar
reviewed
Aug 12, 2026
monerostar
left a comment
Contributor
There was a problem hiding this comment.
Native Win11 verification (monerostar)
Host: Windows 11 (build 10.0.26200), Python 3.11.15, pytest 9.1.1
SHA reviewed: c8c3ae8cd
Baseline (upstream/main @ 222465d84)
python -m pytest tests/hermes_cli/test_doctor_journal_modes.py -o addopts= --collect-only
# ERROR collecting tests/hermes_cli/test_doctor_journal_modes.py
# E AttributeError: module 'os' has no attribute 'geteuid'
# (decorator line: @pytest.mark.skipif(os.geteuid() == 0, ...))
Bare os.geteuid() is evaluated at import/class-body time, so the existing os.name == "nt" skip never gets a chance to run. Confirmed live: hasattr(os, "geteuid") is False on this host.
This PR
python -m pytest tests/hermes_cli/test_doctor_journal_modes.py -o addopts=
# 27 collected → 25 passed, 2 skipped in ~2.3s
Both chmod-gated tests correctly skip on nt. test_lists_every_managed_database passes with the os.path.join(...) expectation (backslash path on Windows).
Verdict
Matches the stated bug class and the existing hasattr(os, "geteuid") pattern elsewhere. Test-only, focused, green on native Win11. LGTM from a Windows contributor host.
Contributor
19 tasks
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
tests/hermes_cli/test_doctor_journal_modes.pyis new in v0.20.0 (658329708) and aborts collection on native Windows. pytest stops the entire run on a collection error, so no test in the invocation gets to execute:The Windows guard is already there — it just never runs
The first decorator is correct and does exactly the right thing. The second one evaluates
os.geteuid()at import time, before pytest looks at either marker — andos.geteuiddoes not exist on Windows. The author's own guard is annulled by the line directly beneath it.Short-circuiting on
hasattrfixes it, and it is the form already used for this same pair intests/test_hermes_state_readonly_preflight.py:31:POSIX behaviour is byte-identical —
hasattr(os, "geteuid")isTruethere, so the euid check runs exactly as before.One more failure becomes reachable once collection is unblocked
test_lists_every_managed_databaseasserts a POSIX path:doctornames nested databases withstr(board_db.relative_to(hermes_home))(hermes_cli/doctor.py:114), which uses the platform separator — so on Windows the output carries backslashes. The product is correct on both platforms; only the expectation was POSIX-only. It is now built withos.path.joinso it follows whatever separator the host uses.No production code is touched.
How to test
On native Windows, from PowerShell:
Before:
After:
The two skips carry the author's own reason — now actually firing.
On Linux/macOS nothing changes: both markers evaluate as before and all 27 tests run.
Scope
I collected all 230 test files added since
v2026.7.30on this host. This was the only collection error among them, and the bareskipif(os.geteuid()...)pattern appears nowhere else undertests/.Why CI never caught this
All CI jobs run on
ubuntu-latest(the only other runner in the tree isubuntu-24.04-armindocker.yml). There is no Windows runner, soos.geteuidalways exists in CI and the import-time evaluation never raises.Platforms tested
Native Windows 11 Home, build 26200 · Python 3.11.6 · PowerShell.
Related
658329708—feat(doctor): report per-database journal mode with WAL-reset exposure, which added the file two days ago.