ci: add windows-latest to the CI matrix - #54
Merged
Merged
Conversation
Convert the single ubuntu-latest test job into a matrix over ubuntu-latest and windows-latest with fail-fast disabled, so a failure on one OS does not mask the other's result. GitHub's Windows runner images do not ship GNU make, so the Windows leg runs the equivalent Go commands directly (go build -trimpath -o grant.exe . and go test -race ./... -v) while Linux keeps the make targets. The Windows build drops -ldflags: they only inject version strings that are already exercised on Linux, and reimplementing the Makefile's git rev-parse/date shell-outs in PowerShell would buy nothing. -race is kept on Windows rather than degraded to plain go test: the runner image ships gcc and setup-go leaves CGO_ENABLED=1, so the race detector genuinely runs there. golangci-lint stays Linux-only. internal/config tests asserting POSIX permission bits and HOME-based home-directory resolution are skipped on Windows; neither behaves that way there. MERGE ORDER: fix/ui-test-data-race must merge before this branch. The internal/ui tests swap the package-level IsTerminalFunc while marked t.Parallel(), which is a real data race; go test -race ./... fails on it today on Linux and will fail on both matrix legs until that fix lands. The race fix is deliberately not carried here.
There was a problem hiding this comment.
Pull request overview
Adds Windows coverage to the GitHub Actions CI pipeline so platform-specific behavior (notably Windows executable replacement semantics) is exercised in PRs, while documenting the matrix and adjusting a couple of Unix-only tests for portability.
Changes:
- Updated CI to run a test job matrix over
ubuntu-latestandwindows-latestwithfail-fast: false, usinggo build/go testdirectly on Windows (nomake). - Skipped two config tests on Windows where the underlying OS behaviors don’t match POSIX expectations (chmod-based unreadability and
HOME-driven home resolution). - Documented the CI matrix behavior in
CLAUDE.mdand recorded it in theCHANGELOG.md.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
.github/workflows/ci.yml |
Adds an OS matrix (Ubuntu + Windows), preserves race testing on both, and gates lint to non-Windows. |
internal/config/config_test.go |
Skips two Unix-specific assertions on Windows to keep the test suite portable. |
CLAUDE.md |
Documents the CI matrix structure and the Windows-vs-Linux command differences. |
CHANGELOG.md |
Notes the new Windows CI leg and the Windows test portability adjustments under Unreleased. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
The first windows-latest CI run on this branch failed three packages. Two are portability bugs in the tests themselves; the third (internal/ui) is the pre-existing IsTerminalFunc data race fixed by #52 and is left alone here. cmd: TestConfigureCommand/config_save_error forced a write failure by pointing GRANT_CONFIG at /dev/null/config.yaml, relying on /dev/null being a file so config.Save's MkdirAll fails with ENOTDIR. On Windows that path is an ordinary writable location, MkdirAll happily creates D:\dev\null, the save succeeds and the test sees no error. Replaced with a temp regular file used as the parent component, which fails on both POSIX (ENOTDIR) and Windows (ERROR_DIRECTORY). This keeps the case running everywhere rather than skipping it on Windows. internal/cache: TestSet_FilePermissions asserts the cache file is 0600. Go reports ordinary writable Windows files as 0666 (os/types_windows.go) and os.Chmod there only toggles the read-only attribute, so the assertion is meaningless rather than wrong. Skipped on Windows; the 0600 assertion keeps its full strength on Linux and macOS. No shared skip helper: after this change only two tests skip for permission reasons (this one and TestLoadConfig_PermissionError), and an inline t.Skip with its own reason reads better than an indirection for two callers. A sweep of every _test.go for mode literals, Chmod, and Mode().Perm() turned up no other permission assertions.
# Conflicts: # CHANGELOG.md
Two Windows-only failures surfaced by the new windows-latest CI leg: - unwritable target directory: chmod 0500 does not deny writes to a directory on Windows, so applyWithOptions succeeded where the test expected a failure. Skip the case there. - rollback failure hint: recoveryHint renders paths with %q, which escapes the backslashes in a Windows path, so the raw-path strings.Contains assertion never matched. Compare against strconv.Quote of each path instead.
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
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.
Merge after #52. Until
fix/ui-test-data-racelands,go test -race ./...fails on the pre-existinginternal/uirace, so both matrix legs will be red. That is the existing Linux failure now also visible on Windows, not a Windows-specific problem.Why
The self-update rewrite (#51) replaces the running binary on disk, and the Windows path is materially different — a running executable can't be deleted but can be renamed. Nothing covered it: CI was a single
ubuntu-latestjob.Approach
Matrix over
ubuntu-latestandwindows-latestwithfail-fast: false, so a Windows failure can't cancel the Linux leg and hide which platform actually broke.Windows runs
godirectly instead ofmake.GNU Make isn't in the Windows runner image, and the Makefile shells out todate -uandgit rev-parse, so it needs a POSIX shell.shell: bashwould probably work via Git Bash, but that stacks two unverifiable assumptions on the one job meant to be a safety net. Two explicitgocommands have no unknowns. Cost: the legs must be kept in sync manually — noted in CLAUDE.md.-raceis kept on Windows, not degraded. It needs cgo and a C toolchain; the runner image ships gcc andactions/setup-goleavesCGO_ENABLED=1, so it genuinely runs.-ldflagsdropped on the Windows build. They only inject version/commit/date strings — platform-independent and already exercised on Linux. Reimplementing the Makefile's shell-outs in PowerShell would add risk for no coverage.-trimpathis kept for parity.Lint runs on Linux only, guarded by
if: runner.os != 'Windows'.Test portability fixes
Two tests in
internal/config/config_test.gowould have made the Windows leg red on day one and now skip there with a stated reason:TestLoadConfig_PermissionError(chmod0o000only toggles the read-only bit on Windows, so the read succeeds) andTestConfigDir_Error(clearingHOMEdoesn't failos.UserHomeDiron Windows, which readsUSERPROFILE).Not verified
The Windows leg has never actually executed — the tests are cross-compiled and vetted only, so runtime failures beyond the two above are possible on the first real run.
make test-integrationremains outside CI on both platforms.