Skip to content

fix(tests): don't assert host-OS state in the IS_WINDOWS=False no-op test - #74220

Closed
hyqqx wants to merge 1 commit into
NousResearch:mainfrom
hyqqx:fix/win32-ver-host-assert
Closed

fix(tests): don't assert host-OS state in the IS_WINDOWS=False no-op test#74220
hyqqx wants to merge 1 commit into
NousResearch:mainfrom
hyqqx:fix/win32-ver-host-assert

Conversation

@hyqqx

@hyqqx hyqqx commented Jul 29, 2026

Copy link
Copy Markdown

What does this PR do?

test_suppress_platform_ver_console_posix_noop fails on a native Windows host — not
because the code under test is wrong, but because the test asserts a fact about the
machine it runs on.

tests\test_windows_subprocess_no_window_flags.py:1074:
    assert platform.win32_ver() == ("", "", "", "")
E   AssertionError: assert ('10', '10.0.26200', 'SP0', 'Multiprocessor Free')
                       == ('', '', '', '')

The test monkeypatches _subprocess_compat.IS_WINDOWS to False and verifies that
suppress_platform_ver_console() then leaves platform._syscmd_ver untouched. That part
is sound and platform-independent, and it is the actual contract being tested.

The final line is different in kind: platform.win32_ver() reads the real OS.
Monkeypatching IS_WINDOWS does not change what it reports, so on Windows the assertion
compares the host's genuine version tuple against the blanks you get on Linux.

This is precisely the pitfall CONTRIBUTING's Testing cross-platform section warns
about:

If you monkeypatch sys.platform for cross-platform tests, also patch
platform.system() / platform.release() / platform.mac_ver() — each re-reads the
real OS independently, so half-patched tests still route through the wrong branch on a
Windows runner.

The change

Keep the behavioural assertion on every platform — win32_ver() must still be
callable and still return a 4-tuple, which is what actually proves the helper did not
damage it — and gate only the emptiness check, which holds off Windows and cannot hold on
it.

    version = platform.win32_ver()
    assert isinstance(version, tuple) and len(version) == 4
    if sys.platform != "win32":
        assert version == ("", "", "", "")

POSIX behaviour is unchanged: the emptiness assertion still runs there exactly as before.
No production code is touched.

How to test

On native Windows:

python -m pytest tests/test_windows_subprocess_no_window_flags.py -q

Before: 39 passed, 1 failed
After: 40 passed

On Linux/macOS the file is unaffected — the added branch is simply taken.

Why CI never caught this

All CI jobs run on ubuntu-latest (the only matrix.runner value in the tree is
ubuntu-24.04-arm, in docker.yml). There is no Windows runner, so a test that asserts
"we are not on Windows" always passes in CI and only fails for contributors actually
developing on the platform.

Platforms tested

Native Windows 11 Home, build 26200 · Python 3.11.6.

Related

Same family, different files, already covered elsewhere — deliberately not duplicated
here:

To my knowledge no open PR touches tests/test_windows_subprocess_no_window_flags.py.

…test

test_suppress_platform_ver_console_posix_noop monkeypatches
_subprocess_compat.IS_WINDOWS to False and checks that
suppress_platform_ver_console() then does nothing. That part is sound and
platform-independent.

Its last line, however, asserts platform.win32_ver() == ("", "", "", ""),
which reads the real host OS. monkeypatching IS_WINDOWS does not change
what platform.win32_ver() reports, so on a native Windows host the test
fails on a fact about the machine rather than about the helper:

  E  AssertionError: assert ('10', '10.0.26200', 'SP0', 'Multiprocessor Free')
                       == ('', '', '', '')

This is the pitfall CONTRIBUTING's "Testing cross-platform" section calls
out: patching one platform signal while another re-reads the real OS
independently leaves the test routing through the wrong branch on a
Windows runner.

Keep the behavioural assertion on every platform -- win32_ver() must still
be callable and still return a 4-tuple, which is what proves the helper did
not damage it -- and gate only the emptiness check, which is true off
Windows and false on it.

Verified on native Windows 11 (build 26200), Python 3.11.6:
before, this file reported 39 passed / 1 failed; after, 40 passed.
The behaviour on POSIX is unchanged: the emptiness assertion still runs
there exactly as before.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@alt-glitch alt-glitch added type/test Test coverage or test infrastructure P3 Low — cosmetic, nice to have platform/windows Native Windows-specific behavior or breakage labels Jul 29, 2026
@monerostar

Copy link
Copy Markdown
Contributor

Native Windows 11 status (monerostar)

Quick live check on Windows 11 Pro Build 26200 / Python 3.11.15.

Finding: PR target test no longer exists on main

Wave-1 suite prune landed on main as 6b81590c5 (test: prune low-value tests suite-wide…):

-def test_suppress_platform_ver_console_posix_noop(monkeypatch):
-    ...
-    assert platform.win32_ver() == ("", "", "", "")

On current main, tests/test_windows_subprocess_no_window_flags.py only has 9 tests left; posix_noop is gone. Full-file run on this host:

9 passed in 2.57s

So the failure mode this PR fixes is already gone — not because the assertion was softened, but because the test was deleted.

PR branch still correct in isolation

On PR head (9561c80d2), the fixed test still exists and passes here:

tests/test_windows_subprocess_no_window_flags.py::test_suppress_platform_ver_console_posix_noop PASSED
40 passed in 3.03s   # full file on PR tip

Suggestion

Safe to close as superseded / implemented_on_main (via prune) unless someone wants to re-introduce a host-OS-safe no-op test. No merge needed for the original Win11 red; main no longer carries the bad assertion.

(Not a reject of the approach — gating emptiness on sys.platform != "win32" was the right fix for that test while it lived.)

@teknium1

Copy link
Copy Markdown
Contributor

Thanks for identifying the native-Windows test failure. The proposed assertion change was sound while this test existed.

Automated hermes-sweeper review found that the PR is already superseded on current main:

  • 6b81590c55bcc9c1001a33b51b528784f96c6a07 removed test_suppress_platform_ver_console_posix_noop from tests/test_windows_subprocess_no_window_flags.py, including the host-OS-dependent platform.win32_ver() assertion.
  • Current main no longer contains that test; the remaining related coverage starts at tests/test_windows_subprocess_no_window_flags.py:355.
  • This matches the Windows verification reported in the follow-up discussion: no merge is needed to eliminate the reported red test.

@teknium1 teknium1 closed this Jul 30, 2026
@teknium1 teknium1 added the sweeper:implemented-on-main Sweeper: behavior already present on current main label Jul 30, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P3 Low — cosmetic, nice to have platform/windows Native Windows-specific behavior or breakage sweeper:implemented-on-main Sweeper: behavior already present on current main type/test Test coverage or test infrastructure

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants