test(cron): skip POSIX mode assertions on Windows - #71428
Conversation
Native Win11 verification (monerostar)Host: Windows 11 build Compared current Results
Failure shape on main (expected)Mode assertions compare
Failing classes: Notes
Verdict: correct, minimal, green on live Win11. Evidence only — not a maintainer approve. |
monerostar
left a comment
There was a problem hiding this comment.
monerostar native Windows live-verify
Environment
- Windows 11 native (Build 26200) · Python 3.11 hermes venv
- Control: install
tests/cron/test_file_permissions.py(no nt skip) - PR HEAD:
1d3ba1837— worktreepr-71428 - Note: no CI checks reported on this branch head (stale/behind?); local proof below
Why main fails here
Windows chmod only toggles the read-only bit. stat.S_IMODE never reports POSIX 0o700 / 0o600. Live on main:
pytest tests/cron/test_file_permissions.py -q -o addopts=
→ 6 failed, 2 passed
Failures are pure mode equality (511 != 448 = 0o777 vs 0o700, 438 != 384 = 0o666 vs 0o600). TestSecureHelpers (2 tests) still pass.
PR
Class-level @unittest.skipIf(os.name == "nt", "POSIX mode bits not enforced on Windows") on the two mode-asserting classes — same idiom used elsewhere in the suite.
pytest tests/cron/test_file_permissions.py -q -o addopts= # PR tree
→ 2 passed, 6 skipped
Assessment
- Correct test-portability fix; does not weaken production permission helpers on POSIX
- +2/−0, focused
- Optional follow-up (not blocking): Win-specific assertions on readonly bit if you want coverage rather than skip-only
Verdict: LGTM for landing so Windows contributors get a green local suite on this file.
Formal Approve: blocked for external collaborator — comment review only.
|
Thanks for the focused Windows test-portability fix. Current This matches an existing suite convention at Automated hermes-sweeper review. |
|
@monerostar thanks for the native Win11 run here too, and for listing the actual assertion values rather than just a pass/fail count. Independent hardware evidence on these Windows PRs is exactly what they were missing. On the nit: I kept No changes needed on this one after the review, and it still reports no conflict against current main. |
26cb308 to
591ff47
Compare
Related: #60888 covers the same Windows cron permission-test family more broadly. This is the narrower class-level assertion gate, so retain both for maintainer comparison. |
591ff47 to
0e4e99b
Compare
Windows chmod() only toggles the read-only bit, so os.stat() never reports 0o700 or 0o600 there and all six permission tests fail on a Windows checkout. Both classes assert nothing but modes, so gate them the same way the rest of the suite already gates POSIX-only checks. Failures fixed on Windows (6, now skipped): tests/cron/test_file_permissions.py::TestCronFilePermissions tests/cron/test_file_permissions.py::TestConfigFilePermissions The helpers in TestSecureHelpers still run everywhere.
0e4e99b to
ae79f56
Compare
tests/cron/test_file_permissions.pyasserts exact POSIX modes (0o700/0o600). Windowschmod()only toggles the read-only bit, soos.stat()never reports those values and all six tests in the two permission classes fail on a Windows checkout.Both classes assert nothing but modes, so they are gated at class level with the same idiom the suite already uses elsewhere:
os.name == "nt"with reasonPOSIX mode bits not enforced on Windows.TestSecureHelpersis untouched and still runs everywhere.Verified on Windows 11, Python 3.13, cherry-picked onto current main:
Green ubuntu run for this head: https://github.com/cryptoyasenka/hermes-agent/actions/runs/31056703155
Test only, no source changes.
ruff checkclean.