test: gate OS-specific tests by real host, add macOS + Windows CI lanes - #77992
Conversation
૮ >ﻌ< ა ci reviewran on a906748
|
alt-glitch
added
type/test
bedcaa1 to
2c1cfaf
Compare
2c1cfaf to
53b0520
Compare
OutThisLife
left a comment
OutThisLife
left a comment
There was a problem hiding this comment.
This is excellent work — the OS-gating design is sound and the motivating bug is real: the breakaway-bit assertions in TestGatewayRunRestartWatcherOuterPopenFallback sat behind if IS_WINDOWS: while the class ran on Linux under a sys.platform patch, so _subprocess_compat had cached IS_WINDOWS=False at import and those assertions never executed on any host. Moving the class to windows_only and asserting the breakaway bit unconditionally is a genuine dead-coverage fix. The conftest hook, the three markers, the list_os_marked_tests.py file-narrowing helper (with its zero-list guard tested), the exit-code-5 guard, and the CRLF-safe byte output are all correctly reasoned. Both new lanes are green and non-empty.
One thing before I approve: please split out the rider. scripts/run_tests_parallel.py's -j/--jobs default changes from cpu*2 to cpu here — you flagged it yourself. It's an unrelated behavior change to test-runner parallelism riding inside a test: PR, and whether cpu or cpu*2 is the right default is an unmeasured question that deserves its own PR with a rationale, not a silent default flip under an OS-gating change.
Drop the -j/--jobs default + help-string change from this branch and open it separately. Everything else is approved as-is — re-request review after the push and I'll approve.
53b0520 to
9d0e1e1
Compare
done. |
kshitijk4poor
left a comment
kshitijk4poor
left a comment
There was a problem hiding this comment.
Reviewed end-to-end: full diff read, coverage-regression audit of all 14 deleted/renamed test names, lane-selection parity check, and live runs from a macOS host (macos_only lane executes for real — 20/20 pass; linux_only/windows_only skip with correct reasons; tests/ci/ 86/86).
No regressions found. Every deleted test name traces to a split, host-invariant rewrite, or move into a marked class — nothing lost, nothing weakened (the BOM-gating rewrite still fails on the Linux lane if the bug returns; the prompt-builder assertion is strictly stronger; the verify-core-deps pair is now bidirectional). grep↔list_os_marked_tests.py selection is an exact set match for all three markers. No production code touched. The rider is confirmed gone from head.
Minor non-blocking nits, fine as follow-ups:
- The exit-5 diagnostic in tests-os.yml is dead code.
shell: bashmakes GitHub run the step with injected-e;set -uo pipefaildoesn't clear it, so a non-zero pytest exit kills the script beforestatus=$?— the-eq 5branch and its::errormessage are unreachable. Safety holds (the job still fails red), but the promised diagnostic never prints. Fix:status=0; uv run … pytest … || status=$?. TestConfirmDestructiveSlashdocstring overclaims ("including the Windows CI job") — the class is unmarked, so-m windows_onlydeselects it on that lane; it runs on Linux only. The real Windows arm is the marked deadlock tests, so coverage is fine — just trim the clause.- A test carrying two OS markers would be silently skipped on every host (the exact failure class this PR eliminates). A cheap conftest assert (≤1 OS mark per item) would close it; relatedly the AGENTS.md example shows the three decorators stacked in one block, which can read as valid stacking.
- A handful of host fakes remain outside this PR's sweep (test_linux_desktop_entry's
parametrize("darwin","win32"), test_gui_uninstall, test_graphical_browser_detection, test_auth_nous_provider, tts_macos_output/voice_mode) — all inert on the lane that runs them today, but the PR body's "every host-OS fake converted or deleted" slightly overclaims. Follow-up material. - PR body still mentions the (now-removed) rider commit — stale after the force-push.
The failure-mode engineering here is unusually thorough — zero-selection guarded at three layers, the skipif-alias trap explicitly fixed, the previously dead breakaway-bit assertions now executing for the first time. Nice work.
|
This was generated by AI during triage. Summary: Problems:
Solution: Checked against |
many tests patched sys.platform or a module's _IS_WINDOWS flag, then ran on linux ci. the patch selects the branch under test, but the host does not have the behavior the branch exists for. the test proves the patch, not the platform. some gated assertions never ran on any host. this commit adds three markers: linux_only, macos_only, windows_only. a conftest hook skips a marked test on the other hosts, with a clear reason. no test fakes a host now. two documented fakes remain (android/termux, freebsd) because no ci runner exists for them. each fake site got one of four treatments: - gate it: the real host supplies the platform; mocks cover real dependencies only, never host identity - patch the module's own probe when the subject is the probe's consumer - assert against the real host when the fake stood in for any non-x host - delete the patch when it set the value the host already has bare skipif(sys.platform != ...) guards became markers too. the lane model skips these on linux and never imports them on windows, so they ran on no host. platform parametrize tables are now one marked test per os. running on real hosts found real errors: a chrome-sandbox failure in test_gui_command that main hides, and two windows failures fixed here. the agents.md testing section now documents the policy.
the markers from the previous commit skip off-host. without a host to run them on, every marked test is a silent skip. this commit adds the hosts. - tests-os.yml runs -m macos_only on macos-latest and -m windows_only on windows-latest. ci.yml requires both lanes in all-checks-pass. - a lane fails on pytest exit code 5 (zero tests selected). a renamed marker cannot produce a green job that ran nothing. - each lane repeats 'not integration' because a command-line -m replaces the addopts filter. - scripts/ci/list_os_marked_tests.py selects which files each lane imports. -m filters after collection, and collection imports every module. without this helper, one unrelated ImportError on the foreign host fails a job whose own tests passed. the helper exits non-zero when a marker matches no file, and writes bytes with explicit lf so windows crlf translation cannot corrupt the bash file list. it has its own tests in tests/ci/. - the local runner now reports the skipped count and prints a note: macos_only/windows_only tests were skipped on this host, and this ci lane runs them. a green local run on linux no longer reads as coverage of the other hosts. - the runner default job count is now #cpu, not #cpu*2.
`shell: bash` runs the step with -e injected, and `set -uo pipefail` does not clear it. A non-zero pytest exit killed the script before `status=$?`, so the -eq 5 branch and its ::error message never ran. The job still failed red, but the diagnostic that names the cause never printed.
Six test files still selected an OS branch with a faked host. Each one now carries the marker for the host that owns the branch, or derives the expectation from the real host: - test_clipboard: macos_only on the has_clipboard_image dispatch. The fake picked the branch, but _macos_has_image needs osascript. - test_claw: windows_only on the tasklist/powershell scan, with return_value in place of a side_effect list that pinned the call count. - test_linux_desktop_entry: the parametrize over "darwin"/"win32" becomes one marked test per host. A fake left POSIX paths and a POSIX XDG layout. - test_graphical_browser_detection: linux_only on the display-server arm. The $BROWSER check runs before the platform branch, so its test stays unmarked. - test_auth_nous_provider: the fixture pinned linux so the macOS certifi fallback could not change the result. The assertion now reads the host, so the macOS lane covers the fallback too. - test_tts_macos_output and test_voice_mode: the afplay policy exists because CoreAudio init raises a TCC prompt, which no Linux runner reproduces. tests/conftest.py refuses collection when one test carries two OS markers. Each marker skips on all but one host, so two of them make a test that runs nowhere while every lane reports green. tests/test_os_marker_gating.py pins that behavior. The docstring on TestConfirmDestructiveSlash said the Windows job runs it. The class has no marker, so -m windows_only deselects it.
9d0e1e1 to
a906748
Compare
5 participants
What does this PR do?
This PR stops OS-specific tests from faking the host OS. It gives them real hosts to run on instead.
Before this change, tests patched
sys.platform = "win32"or set a module's_IS_WINDOWSflag, then ran on Linux CI. The patch selects the branch under test on a machine that does not have the behavior the branch exists for.msvcrtdoes not import.taskkillis not onPATH. Paths are POSIX.signal.SIGKILLexists. The test proves the patch, not the platform.Some tests were worse. The breakaway-bit assertions in the gateway restart-watcher test sat behind
if IS_WINDOWS:. They never ran on any host, and the test reported green.The fix has two parts:
linux_only,macos_only,windows_only) that skip a test on the other hosts. No test fakes a host now.Every host-OS fake in
tests/is now converted or deleted. This includes two follow-up sweeps of files outside the first conversion. Two documented fakes remain: anandroid/Termux gate and a FreeBSD refusal test. No CI runner exists for these platforms. Both fakes are safe because the faked check has no OS facility under it.Related Issue
Fixes #
Type of Change
Changes Made
Marker infrastructure
tests/conftest.py— the_OS_MARKScollection hook. A marked test skips off-host, with a clear reason. The block comment holds the policy: what you must gate, what stays unmarked.pyproject.toml— registers the three markers.CI lanes
.github/workflows/tests-os.yml— runs-m macos_onlyonmacos-latestand-m windows_onlyonwindows-latest.ci.ymlgates the lanes with thepythonlane and requires them inall-checks-pass. A lane fails on pytest exit code 5 (zero tests selected). Each lane repeatsnot integrationbecause a command-line-mreplaces theaddoptsfilter.scripts/ci/list_os_marked_tests.py(with its own tests intests/ci/) — selects which files each lane imports.-mfilters after collection, and collection imports every module. Without this helper, one unrelatedImportErroron the foreign host fails a job whose own tests passed. The helper exits non-zero when a marker matches no file. It writes bytes with explicit LF, so Windows CRLF translation cannot corrupt the bash file list.scripts/run_tests_parallel.py— the local runner's summary now shows the skipped count. It also prints a note:macos_only/windows_onlytests were SKIPPED on this host, and this CI lane runs them. This is not a behavior change. The conftest hook already prevented off-host execution. The gap was that the skips were silent, and a green local run looked like full coverage.Test conversions (~65 files) — one decision per site, in this order:
subprocess.runthat shells out totaskkill,launchctl, orsecurity), never host identity._is_wsl,_is_macos_arm64) when the test subject is the probe's consumer. This keeps the coverage on the Linux lane.expected = sys.platform == "darwin") when the fake only stood in for "some non-X host".Other conversions:
skipif(sys.platform != "win32")guards became markers. The lane model skipped these tests on Linux and never imported them on Windows. They ran on no host.parametrizetables, are now one marked test per OS.list2cmdlineregression test inhermes_cli/managed_uvnow runs on a real Windows host for the first time.macos_only. They exercise the realplatform.system()gate. The Keychain itself stays mocked.Second residue sweep (this round)
test_clipboard,test_claw,test_linux_desktop_entry,test_graphical_browser_detection,test_auth_nous_provider,test_tts_macos_output, andtest_voice_modestill selected an OS branch from a faked host. Each is now gated on the host that owns the branch, or reads the expectation from the real host (test_auth_nous_provider's CA-bundle fallback, which the oldlinuxpin hid from the macOS lane). Theparametrize("darwin", "win32")table intest_linux_desktop_entryis one marked test per OS.Zero-selection diagnostic —
shell: bashinjects-e, whichset -uo pipefaildoes not clear, so a non-zero pytest exit killed the lane script beforestatus=$?and the exit-5::errormessage could never print.status=0; pytest … || status=$?restores it. Rehearsed locally on all three arms (pass, exit 5, other failure).Real errors found on real hosts
fix: two real windows failures the new lane surfaced(406bc26).test_gui_command— the darwin-by-default fake hid achrome-sandboxerror onmain. This PR corrects it.test_verify_core_dependencies— now host-invariant and bidirectional. It makes sure that exactly one dep of a marker-gated pair is filtered on each host. The old test never covered the positive-marker direction.Double-marker guard
A test that carried two OS markers would be skipped on every host: the exact failure class this PR removes, wearing a different hat.
tests/conftest.pynow refuses collection when one item carries more than one marker from_OS_MARKS, and names the offenders.tests/test_os_marker_gating.pypins the behavior.How to Test
scripts/run_tests.sh. The summary ends with the OS-gating notes that name the lanes.pytest <converted file> -q -rs. Each skip shows the hook's reason (native Windows-only test (marked windows_only); host is linux). This proves that the marker fired, not an oldskipif.python scripts/ci/list_os_marked_tests.py windows_only | xargs pytest -m "windows_only and not integration" --collect-only -q. This shows what the lane selects.linux_only(runs on the main Linux lane)macos_only(macos-latest)windows_only(windows-latest)Full CI is green on this branch: the Linux suite, both new OS lanes, and
all-checks-pass. On the Linux suite, this branch also fixed 6 tests, which includes thetest_gui_commanderror above.Checklist
Code
scripts/run_tests.sh(the canonical wrapper — numbers above)tests/ci/test_list_os_marked_tests.py)Documentation & Housekeeping
_OS_MARKSpolicy block intests/conftest.py, the workflow header comments, and the AGENTS.md testing section ("Don't fake the host OS")cli-config.yaml.example— N/A, no config keysCONTRIBUTING.mdorAGENTS.md— yes, AGENTS.md (see above)