test(windows): make the suite runnable on Windows - #771
Draft
JamBalaya56562 wants to merge 1 commit into
Draft
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Central YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughShell tests now validate fixture execution, use configurable shell executables, normalize paths across platforms, and expand shell override coverage. The Python SDK test now passes its temporary path as a command-line argument. ChangesCross-platform shell tests
SDK test portability
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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 |
Contributor
Greptile SummaryThis PR makes the shell-oriented test suite portable to Windows without changing production code.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains; the configured-shell probe and every guarded shell invocation now resolve through the same override-aware helper, including the zsh timeout path. Important Files Changed
Reviews (2): Last reviewed commit: "test(windows): make the suite runnable o..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
JamBalaya56562
force-pushed
the
windows-test-suite
branch
from
August 2, 2026 04:36
6787fac to
8818c3b
Compare
JamBalaya56562
marked this pull request as draft
August 2, 2026 04:45
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
cli/tests/shell_override.rs (1)
79-98: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMake the PATH override observable.
USAGE_SHELL_BASH="bash"is indistinguishable from the fallback. This test passes if the override is ignored. It also passes if PATH resolution is broken but the default shell has the same result.Prepend the directory that contains
substitute_program()toPATH. SetUSAGE_SHELL_BASHto that program’s file name. Then assert that its stderr containsSCRIPTand that the fixture output is absent.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cli/tests/shell_override.rs` around lines 79 - 98, Update the_override_may_name_a_program_on_path to prepend the directory containing substitute_program() to PATH and set USAGE_SHELL_BASH to that program’s filename instead of bash. Assert the run succeeds with stderr containing SCRIPT, and verify the normal fixture output is absent so the PATH-resolved override is observable.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cli/tests/shell_completions_integration.rs`:
- Around line 103-129: Update shell_can_run_a_script so the pwsh branch invokes
the shell with -NoProfile, -NonInteractive, and -File before the script path,
matching test_powershell_completion_integration. Keep the existing invocation
for other shells and preserve the current stdout validation.
---
Nitpick comments:
In `@cli/tests/shell_override.rs`:
- Around line 79-98: Update the_override_may_name_a_program_on_path to prepend
the directory containing substitute_program() to PATH and set USAGE_SHELL_BASH
to that program’s filename instead of bash. Assert the run succeeds with stderr
containing SCRIPT, and verify the normal fixture output is absent so the
PATH-resolved override is observable.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: fa2773fa-e178-4806-87f9-7c7bb26ced9a
📒 Files selected for processing (4)
cli/tests/complete_word.rscli/tests/examples.rscli/tests/shell_completions_integration.rscli/tests/shell_override.rs
JamBalaya56562
force-pushed
the
windows-test-suite
branch
from
August 2, 2026 05:16
8818c3b to
bb9c130
Compare
greptile-apps
Bot
dismissed
their stale review
August 2, 2026 05:16
Dismissed because a newer commit was pushed; Greptile will re-review the current head.
This was referenced Aug 2, 2026
JamBalaya56562
force-pushed
the
windows-test-suite
branch
from
August 2, 2026 05:57
bb9c130 to
d1a3d02
Compare
`cargo test --all --all-features` on a Windows machine with WSL gives 15
failures, and `cli/tests/shell_override.rs` never runs at all. None of it is a
bug in usage; the tests are.
`bash` there is the WSL launcher. The Win32 executable search order puts the
system directory ahead of PATH and installing WSL puts `bash.exe` in it, so a
bare `bash` cannot open the Windows paths these tests write. GitHub's
windows-latest is the same: `where bash` lists Git Bash first, but
`Command::new("bash")` still reaches System32, and without an override it exits
1 with "Windows Subsystem for Linux has no installed distributions".
Both skip guards missed this by probing a proxy rather than the precondition:
`--version` spawns fine under WSL bash, and in `complete_word.rs` the probe
asked about `sh` while the mount fixtures need `bash` — on Windows the two need
not be the same family. Both now run a script, and a real fixture, and require
it to exit cleanly, so an unusable shell skips instead of failing. The fixture
is named from `CARGO_MANIFEST_DIR` rather than `fs::canonicalize`, whose `\?\`
prefix usage cannot open: probing with one made the guard fail on the shape of
the path rather than on the shell, skipping every mount test on a machine where
they run.
Shell resolution goes through `USAGE_SHELL_<SHELL>`, the variable
`shell_program_override` already reads, so one setting covers the whole suite.
Every invocation is built by one `script_command`, so the guard and the tests it
guards cannot drift apart — not in which program they run, nor in how. pwsh is
where they would have: it needs `-File` to take a script path, and `-NoProfile
-NonInteractive` so a developer's profile cannot add output to a run whose
stdout is compared against a single token. The zsh pty test's `timeout` wrapper
goes through the shell too, since spawning it directly walks into the same trap
as `bash` — Windows keeps an unrelated `timeout.exe`, a sleep rather than a
watchdog, in the system directory.
Three places hand a Windows path to something that parses it, and each needed a
different answer. Paths interpolated into shell script bodies are normalized to
`/`, because `\` is an escape character there and `C:\Users\Jam` was arriving as
`C:UsersJam`. `$PATH` entries go through `cygpath` instead: `$PATH` is
colon-separated, so `C:/Users/...` splits into `C` and `/Users/...` and neither
resolves, and only the shell in use knows whether the answer is `/c/...` or
`/cygdrive/c/...`. And `sdk_compile.rs` put the SDK directory inside a Python
string literal, where `C:\Users\RUNNER~1\...` fails to parse at all because
`\U` opens a unicode escape — that one now travels as argv, clear of any
escaping rules.
`test_empty_defaults_example` now runs through `usage bash` like the nine other
tests in its file. It was the only one relying on the shebang, and Windows
cannot execute a `.sh` at all — os error 193, before the shebang is read.
`shell_override.rs` loses `#![cfg(unix)]`. Its substitute program is `$CARGO`
rather than `/bin/echo`, which has no Windows counterpart: cargo sets the
variable for the processes it spawns, and handed a path it does not recognize it
repeats it back, which is what makes the substitution observable. The three
fallback tests compare against a baseline run with nothing overridden instead of
asserting the script's output, since whether the default shell works is a
property of the machine, while "same as unset" is what those tests are about.
Tests only; no library or CLI source changes. Measured on a windows-latest
runner: with `USAGE_SHELL_BASH` naming a real bash and `core.autocrlf` off, the
whole suite passes with nothing skipped. Without the override the
shell-dependent tests skip rather than fail. Linux is unchanged.
JamBalaya56562
force-pushed
the
windows-test-suite
branch
from
August 2, 2026 08:33
d1a3d02 to
4fe17a5
Compare
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
1 participant
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.
Groundwork for running the test suite on Windows. Today it gives 15 failures and one file that never runs, and every one of them is the test's own doing rather than a bug in
usage.Measured on a
windows-latestrunner after these changes: 538 passed, 0 failed, 0 skipped.What was wrong
cli/tests/complete_word.rssh, but the fixtures needbashcli/tests/shell_completions_integration.rsbashis the WSL launcher, and Windows paths get eatencli/tests/examples.rs.shdirectlylib/tests/sdk_compile.rscli/tests/shell_override.rs#![cfg(unix)], so 5 tests never run at allThe last one is the sharpest: those are the tests for
USAGE_SHELL_<SHELL>from #767, a feature that exists for Windows, and they only ever ran on Linux.bashon Windows is not a POSIX bashThe Win32 executable search order puts the system directory ahead of
PATH, and installing WSL putsbash.exethere. So on such a machinebashis the WSL launcher no matter what else is installed, and it cannot open a Windows path:Two problems in one line. WSL cannot read
C:\…, and the backslashes were already gone before it looked — the path was interpolated into a script body where\is an escape character, soC:\Users\Jamarrived asC:UsersJam.This is not exotic; a stock GitHub runner does it too. On
windows-latest:Git Bash is listed first, and yet
Command::new("bash")reaches System32 — becausewheresearchesPATHwhileCreateProcesssearches the system directory before it. Unset,usage bashthere exits 1 withWindows Subsystem for Linux has no installed distributions. The two disagreeing is exactly whatUSAGE_SHELL_<SHELL>exists to settle.The skip guards were checking the wrong thing
skip_if_shell_missingasked whether<shell> --versionspawns. WSL's bash answers that perfectly well and then fails at everything the tests need, which is why five bash tests failed where zsh and fish correctly skipped — those are simply absent on Windows.skip_if_posix_shell_missingincomplete_word.rshad the same shape of gap for a subtler reason: it probessh, but the mount fixtures are#!/usr/bin/env -S usage bashscripts. On Windows the two need not be the same family —shresolves to Git Bash while a barebashis the WSL launcher — soshstarting proves nothing about whether the shebang will work.What changed
Tests only. No library or CLI source is touched.
Guards now probe the precondition, not a proxy.
skip_if_shell_missingruns a script out of the temp directory and requires it to exit cleanly;skip_if_posix_shell_missingruns a real mount fixture throughusage bashat the absolute pathshwould hand its shebang. Both keep the existing rule of panicking underCIrather than skipping. On Linux nothing changes; on Windows an unusable shell now skips instead of failing.The fixture path is built from
CARGO_MANIFEST_DIRrather thanfs::canonicalize, whose\\?\prefixusagecannot open — probing with one made the guard fail on the shape of the path rather than on the shell, and quietly skipped all eight mount tests on a machine where they run.Shell resolution goes through
USAGE_SHELL_<SHELL>— the same variableshell_program_overridereads incli/src/env.rs, so a developer configures one thing and the whole suite follows. Unset, which is every platform but Windows, means the bare name as before.One
script_commandbuilds every invocation, so the guard and the tests it guards cannot drift apart — not in which program they run, nor in how. pwsh is where they would have: it needs-Fileto take a script path, and-NoProfile -NonInteractiveso a developer's profile cannot add output to a run whose stdout is compared against a single token.Three places hand a Windows path to something that parses it, and each needed a different answer:
/, because\is an escape character there.$PATHentries — throughcygpath, because$PATHis colon-separated, soC:/Users/…splits intoCand/Users/…and neither resolves. Only the shell in use knows whether the answer is/c/…,/cygdrive/c/…, or a mount point from fstab, so the conversion is asked of it rather than of acygpaththat may not be on Windows' ownPATH. Applies to bash and zsh; fish'sset -gx PATH a bis space-separated and PowerShell's separator is;.sdk_compile.rsput the SDK directory inside one, whereC:\Users\RUNNER~1\…fails to parse at all:\Uopens a unicode escape. That one now travels as argv, clear of any escaping rules rather than escaped around.The zsh pty test's
timeoutwrapper also goes through the shell now. Spawning it directly walks into the same trap asbash: Windows keeps an unrelatedtimeout.exe— a sleep, not a watchdog — in the system directory.test_empty_defaults_exampleruns throughusage bash, like the nine other tests in its file. It was the only one relying on the shebang, and Windows cannot execute a.shat all — the spawn fails with os error 193 before the shebang is read.shell_override.rsloses#![cfg(unix)]. Two changes make it portable:$CARGOinstead of/bin/echo, which has no Windows counterpart. Cargo sets the variable for the processes it spawns, so it is the one executable a test can name without knowing the platform, and handed a path it does not recognize it repeats it back —no such subcommand \…`— which is what makes the substitution observable. (Not theusage` binary itself: its top level has a positional argument, so a path lands there and never reaches the error message.)bashcan run the script is a property of the machine, and on the platform this feature exists for it may well not — but "same as unset" is the property those tests are actually about, and it holds either way. The positive case is covered on every platform bythe_override_replaces_the_program.Verified
On a
windows-latestrunner, withUSAGE_SHELL_BASHnaming Git Bash andcore.autocrlfoff: 538 passed, 0 failed, 0 skipped — every shell-dependent test genuinely runs rather than skipping past.Locally on Windows, with and without the override:
complete_wordshell_completions_integrationexamplesshell_overrideLinux:
cargo test --all --all-featuresandcargo clippy --all --all-features --all-targets -- -D warningsunchanged.Follow-up: the Windows CI job
Deliberately not in this PR, but no longer guesswork — I ran it on the fork to find out what it needs, and all three of these are measured rather than assumed:
core.autocrlf false, set before checkout. git defaults it totrueon Windows and the repo has no.gitattributes, so fixtures arrive with CRLF and seven help tests plus two markdown tests fail on an ending nobody can see — the printed diff showed identical text.USAGE_SHELL_BASHpointed at Git Bash, for thewhereversusCreateProcessreason above.CIpanic inskip_if_shell_missingrelaxed on Windows. The rule exists because the Linux job installs zsh and fish on purpose; nothing installs them on Windows, so their absence is expected rather than a configuration bug. mise draws the same line — its Windows jobs install no POSIX shells at all. That is a one-line change I left out of this PR, since it is about CI policy rather than about the tests.Happy to open that as a second PR whenever you want it.
This pull request was generated by Claude Code.
Summary by CodeRabbit