test(win32): use a portable no-op verify command (#292)#301
Conversation
WalkthroughThe env-fault escalation test now creates ChangesTest platform compatibility
Estimated code review effort: 1 (Trivial) | ~3 minutes Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…rg#292 review) `Path.write_text` uses `newline=None`, so `\n` is already translated to CRLF on Windows — the explicit `\r` put `\r\r\n` on disk. `cmd` tolerates it for a one-line launcher, but it is a parse hazard the moment the helper grows labels or `goto`, and it is the same defect the per-OS script in the previous commit was written to avoid.
The env-fault escalation test wrote a POSIX check.sh into its VerifyPolicy tuple. Verify commands run shell=True, so on Windows cmd hands that path to ShellExecute: with no .sh association it pops the interactive file picker mid-run, and with one it launches the associated app and returns success. Neither is visible to the test — the fault it asserts comes from an injected SessionResult(env_fault=True) — so CI stayed green. The script was inert scaffolding: written once as `exit 0`, never mutated, referenced by no assertion. It is copy-paste residue from the sibling test_fix_phase_env_fault_escalates_instead_of_looping, which chmods it to 644 mid-run to produce rc=126 — there it is the point. Here the only verify signal is the marker command. So drop the file rather than branch on sys.platform for it, and use conftest's `_OK` for the second command: the host-shell stub block is the sanctioned single platform-detection spot, and no script file means no shell to mis-handle one. No behavior change on any platform — `exit 0` either way.
2b6b5ad to
8d06aa5
Compare
|
Diagnosis is right and the Windows verification is appreciated — I reproduced the reasoning end to end: I pushed an amendment that changes the approach, so flagging it here rather than merging silently. The script in that test is inert. It's written once as And So the amendment drops the file instead of branching for it: - script = project.project / "check.sh"
- script.write_text("#!/bin/sh\nexit 0\n", encoding="utf-8")
- script.chmod(0o755)
...
- verify=VerifyPolicy(commands=(_file_exists_cmd(marker), f'"{script}"')),
+ verify=VerifyPolicy(commands=(_file_exists_cmd(marker), _OK)),Net +5/−5, no Your conftest commit is kept as-is, authorship intact. Its stated rationale (not symptom-patching the shared helper) no longer applies once the test stops hand-rolling a launcher, but the Verification on this end:
One thing left to you: the PR description now documents the superseded per-OS approach — feel free to update it, I'd rather not edit your body. #302 stays open and untouched; the reasoning there for not riding it on a test-only fix is right. |
Greptile SummaryTest-only fix for a Windows incompatibility where
Confidence Score: 5/5Safe to merge; both changes are test-only and confined to fixing Windows-specific file-writing and shell-execution quirks. The two changes are straightforward: swapping Files Needing Attention: No files require special attention.
|
| Filename | Overview |
|---|---|
| tests/conftest.py | Fixed double-CR bug in win32 .cmd launcher: explicit \r before \n was producing \r\r\n on disk because write_text text mode already translates \n to \r\n on Windows. |
| tests/test_engine.py | Replaced an inline check.sh verify command with the cross-platform _OK constant ("exit 0"), preventing Windows from routing the .sh path through ShellExecute; also adds _OK to the conftest import list. |
Reviews (1): Last reviewed commit: "test(win32): use a portable no-op verify..." | Re-trigger Greptile
pbean
changed the title
2 participants
Closes #292
Problem
test_fix_phase_session_env_fault_escalatesput a POSIXcheck.shinto aVerifyPolicycommand tuple. Verify commands run through the host shell (run_verify_commandsisshell=True,verify.py:1379), so on Windowscmdhands that path to ShellExecute — two failure modes, neither of which the test can see:.shassociation (stock Windows): the interactive "How do you want to open this file?" picker pops mid-run and steals focus..shassociated (e.g. Git for Windows): the associated app launches and the command returns success without the script ever running — a silent false pass.Either way the test passes, because the fault it asserts comes from an injected
SessionResult(env_fault=True), so CI stayed green and only local full-suite runs saw the dialog.Fix
Two commits, both test-only.
1.
tests/test_engine.py— drop the script entirely and use conftest's_OKas the second verify command.The script was inert: written once as
exit 0, never mutated, referenced by no assertion.run_verify_commandsdoes not short-circuit, so its whole contribution was rc 0, twice. It is copy-paste residue from the siblingtest_fix_phase_env_fault_escalates_instead_of_looping(tests/test_engine.py:5314), whichchmod(0o644)s it mid-run to produce rc=126 — there it is the point. Here the only verify signal is the marker command, and the escalation is the session's.tests/conftest.py:79-82is the sanctioned spot for this — the block headed "host-shell verify/lifecycle stub commands (single platform-detection spot)", whose comment reads "These build them per-OS in one place instead of each test file re-deriving the win32 branch." It exports_OK = "exit 0", already imported bytests/test_engine_worktree.py:14.Net +5/−5, no
sys.platformbranch in the test. A win32 skip was also rejected: the subject (#194 — a fix session that lost its API connection escalates instead of burning dev budget) is not POSIX-specific, unlike the three sibling tests, which skip on win32 because they depend onshrc 126/127.2.
tests/conftest.py—write_script_launcher's win32 launcher wrote an explicit\r\n.Path.write_textusesnewline=Noneand already translates to CRLF on Windows, so the explicit\rlanded on disk as\r\r\n(reproduced). Harmless for a one-line launcher, and unrelated to #292 once the test stops hand-rolling a launcher — kept as an acknowledged drive-by because the defect is real and the fix is correct.Verification
pytest tests/test_engine.py→ 194 passed, 3 skipped, no dialog;pytest tests/test_engine_plugin.py tests/test_opencode_http.py(thewrite_script_launchercallers) → 112 passed, 4 skippedpytest tests/test_engine.py→ 198 passed;-k env_fault→ 6 passed, 0 skipped (unchanged from baseline)trunk checkcleanTwo ablations, since nothing in the suite catches this class on its own — the test's assertions never depended on the script:
env_fault=True→Falsefails the test (the escalation path is still load-bearing)_OK→exit 3fails the test (the second command is genuinely executed, so_OKoccupies the same slot the script did)What
_OKbuys over a per-OS branch is that the failure mode becomes structurally impossible rather than handled: no script file exists for any shell to mis-handle.Scope
The three other inline
.shverify sites (_self_disarming_scriptandtest_fix_phase_env_fault_escalates_instead_of_looping) are already@pytest.mark.skipif(sys.platform == "win32"), so this was the only unguarded one.test_install.py's POSIX-script sites are likewise win32-skipped, and the git-hook sites run under git's ownsh.No CHANGELOG entry: test-only.
Follow-up: #302
One finding from this review is deliberately out of scope and filed as its own issue:
ENV_FAULT_RCS = frozenset({126, 127})never matches on Windows —cmdreports a missing or non-executable verify tool as 9009/1, so the charged-attempt behavior #126 exists to prevent still happens on win32, and the three skips above hide it. Reclassifying rc 1 carries a real false-positive risk (undercmdit is also the ordinary "tests failed" code), so it needs its own decision rather than riding a test-only fix.