fix(windows): keep the Windows script tests runnable on Windows PowerShell 5.1 - #329
corydouthat-sq wants to merge 2 commits into
Conversation
…Shell 5.1 Every Windows recipe in the justfile runs powershell.exe, WindowsDev.psm1 is written to stay 5.1-compatible, and docs/windows-onboarding.md tells contributors to use normal PowerShell. Yet `just test-windows-dev` failed on that host: Test-WindowsDev.ps1 used the PowerShell 6+ semver type accelerator, which Windows PowerShell 5.1 does not have, and nothing in CI ran the script tests, so the break was invisible upstream. Keep the two ordering assertions where SemanticVersion exists and skip them (visibly) where it does not; they check the type Tauri's updater ordering relies on, not Berd code. Add a scan that fails the suite if any Windows script picks up a 6+ only construct again, run `just test-windows-dev` in the rust-windows CI job under Windows PowerShell, and state the host contract in the onboarding doc. The alternative is to require PowerShell 7 everywhere (install it in the bootstrap, `#Requires -Version 7`, switch the recipes to pwsh); this change keeps the smaller, already-documented contract instead. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
morgmart
left a comment
There was a problem hiding this comment.
🤖 Automated code review
COMMENT — one non-blocking test-honesty finding. The PowerShell 5.1 failure is fixed and the supported host now runs the Windows script suite in CI, but the added compatibility denylist is narrower than its stated guarantee. Supplied GitHub evidence was inspected; several required checks were still in progress at capture time, so CI remains a separate merge-readiness gate.
Deterministic publication result: 0 blocking and 1 non-blocking inline finding(s) publishable; 0 duplicate(s) suppressed; 0 blocking screenshot-evidence requirement(s) in this review body.
|
|
||
| # Windows PowerShell 5.1 is the host every justfile Windows recipe pins, so | ||
| # the Windows scripts must stay clear of PowerShell 6+ only constructs. | ||
| $powerShell7OnlyPattern = '\[semver\]|Start-ThreadJob|ForEach-Object[^\r\n]*-Parallel\b' |
There was a problem hiding this comment.
🤖 P2 · Compatibility check overstates coverage (non-blocking)
The new assertion presents itself as a general PowerShell 5.1 compatibility guard, but its regular expression recognizes only three spellings. Other PowerShell 6+ syntax, commands, aliases, multiline forms, or unavailable APIs can pass this check, so the test can remain green while a Windows script no longer works in the documented host.
User effect: A future Windows script change could pass CI but fail for contributors using the supported Windows PowerShell 5.1 setup, recreating the broken onboarding path this PR is meant to prevent.
Recommended fix: Either narrow the assertion and documentation to the three constructs actually prohibited, or validate every Windows script with a comprehensive PowerShell 5.1-owned compatibility mechanism rather than a small denylist.
Test: Add an adversarial fixture containing a different PowerShell 6+ incompatibility and verify the compatibility guard rejects it; if the guarantee is narrowed instead, test each explicitly prohibited construct and name that limited contract accurately.
There was a problem hiding this comment.
Narrowed rather than broadened, in b846167. The regex now lives in Test-PowerShell7OnlyConstruct, documented as a denylist of the three constructs that have actually bitten these scripts, and the assertion label names them ([semver], Start-ThreadJob, ForEach-Object -Parallel). Added fixtures proving each construct is detected and that look-alikes (ForEach-Object { }, the word semver in a string, Start-Job) are not. The actual Windows PowerShell 5.1 guarantee is the CI step that runs this whole suite under powershell.exe; docs/windows-onboarding.md now says so too.
…an actually checks The assertion in Test-WindowsDev.ps1 presented itself as a general Windows PowerShell 5.1 compatibility guard, but its regex only recognizes three spellings. Narrow the contract instead of overstating it: - Extract the denylist into Test-PowerShell7OnlyConstruct and document that it catches the constructs that have actually bitten these scripts ([semver], Start-ThreadJob, ForEach-Object -Parallel); the real 5.1 guarantee is the CI step that runs this whole suite under powershell.exe. - Add fixture assertions that each denylisted construct is detected and that look-alike 5.1-safe lines (plain ForEach-Object, "semver" in a string, Start-Job) are not. - Rename the scan assertion so it names the limited contract. - Align the sentence in docs/windows-onboarding.md that said the suite checks for PowerShell 6+ constructs in general. The harness stays exempt from the scan because the fixtures spell out the denylisted tokens. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
morgmart
left a comment
There was a problem hiding this comment.
🤖 Automated code review
APPROVE — no new publishable findings. The original PowerShell 5.1 failure is fixed, CI now runs the Windows script suite under the supported host, the known-construct denylist is accurately scoped and directly tested, and all supplied GitHub checks passed. One same-root concern is suppressed because it belongs to an existing unresolved automation thread with a substantive human reply.
Deterministic publication result: 0 blocking and 0 non-blocking inline finding(s) publishable; 1 duplicate(s) suppressed; 0 blocking screenshot-evidence requirement(s) in this review body.
Pending checks: 1 check(s) are not complete.
This approval reflects the completed code review only; merge readiness remains governed by the repository's required checks.
Summary
Every Windows recipe in the
justfilerunspowershell.exe(Windows PowerShell 5.1),WindowsDev.psm1is deliberately written for 5.1, anddocs/windows-onboarding.mdtells contributors to use normal PowerShell. Yetjust test-windows-devfails on that host:Test-WindowsDev.ps1uses the PowerShell 6+[semver]type accelerator at lines 162-165, which 5.1 does not have (Unable to find type [semver]). Nothing in CI ran the script tests, so the break was invisible.This PR keeps the smaller, already-documented contract (5.1 and 7 both work) rather than requiring PowerShell 7:
System.Management.Automation.SemanticVersionvia-as [type]and run only where it exists, with a visibleSKIPotherwise; they check the type Tauri's updater ordering relies on, not Berd code;scripts/windows/*.ps1|*.psm1for PowerShell 6+ only constructs ([semver],Start-ThreadJob,ForEach-Object -Parallel) so the suite fails if one comes back;.github/workflows/ci.ymlrust-windowsrunsjust test-windows-devundershell: powershell(5.1 on GitHub runners) after the Rust checks;docs/windows-onboarding.mdstates the host contract.The alternative, if preferred, is to require PowerShell 7 everywhere: install
Microsoft.PowerShellinBootstrap-Windows.ps1, add#Requires -Version 7to the scripts, and switch thejustfilerecipes andset windows-shelltopwsh. Happy to swap to that if maintainers would rather consolidate on 7.Why now: an internal Windows build lane had to re-launch itself under
pwshpurely to get past this line; with this change that workaround goes away.Related issue
None found.
Testing
scripts/windows/Test-WindowsDev.ps1under Windows PowerShell 5.1.26100 on Windows 11: passes, printsSKIP native updater semver ordering (... has no SemanticVersion type), and the new compatibility scan reports no offenders.ci.ymlparses; therust-windowsjob now has the extra step after "Run Windows-native Rust tests".