ci: run full test suite on Windows - #652
Conversation
|
I have the enumeration this draft is waiting for — I ran the full suite on Windows locally while working on #641/#644/#645/#646/#647, so here it is up front rather than after a red shard. windows/amd64, go1.26.6,
|
| cluster | n |
|---|---|
TestLoadCompileCommands_* (clangd compile DB) |
5 |
TestGDScript* |
5 |
TestTSConfigPathAlias_* |
3 |
symlink confinement (*SymlinkOutOfRepo*) |
4 |
| startup-barrier / marker permission fail-closed | 2 |
| unclustered | 13 |
internal/mcp — 27
| cluster | n | note |
|---|---|---|
| symlink / file-mode | 5 | SeCreateSymbolicLinkPrivilege, and Windows has no POSIX mode bits |
| review rulepack join | 4 | 3 addressed by #646, 1 is a different cause |
| localization ranking (Swift ×3, Rust, C#, PHP) | 6 | not diagnosed |
| fidelity glob | 1 | #647 |
| POSIX-spelled assertions | ~4 | e.g. TestPathToFileURI_Absolute wants file:///work/main.go, Windows resolves file:///D:/work/main.go |
| unclustered | ~7 |
Two things that will bite the shard, and one is the same class as the failure you already hit
1. Some local Windows failures are the developer's environment, not the platform. Your note about .claude/worktrees/* and TestNewIsFencedToIndexerStaging is exactly this class. I hit my own: TestMapGitDiffRepoPrefixJoin and TestMapGitDiffFileChangeKinds fail here on TempDir RemoveAll cleanup: …\.git: The directory is not empty — because the git on my PATH is a non-stock fork that writes .git/ai/logs into every git init. Reproduced with a bare git init in an empty directory outside any repo. A clean runner with stock git will not show those two, so my analysis count of 4 should read as 2 for CI purposes.
Worth stating in the workflow or the contributing notes that the Windows shard is authoritative and a local red is suspect until reproduced there — the inverse of the usual advice, and non-obvious.
2. fail-fast: false plus a full suite will surface order-dependent flakes, and there is at least one. TestNotesManager_SaveQueryDelete passed in one full-package run and failed 3 of 3 when run in isolation on the same commit. I nearly credited a fix for it in #647 on the strength of a before/after count. Anyone reading the first red shard should diff failing test names against a baseline rather than counts, or a flake will read as a regression — and vice versa.
On removing the build-windows job
No objection from me, and the reasoning in #647 is fair — a selector list is a guess and mine did drift. One consequence worth deciding deliberately rather than inheriting: those steps also carried three non-test guarantees that the full-suite matrix does not reproduce — the CGO/tree-sitter build smoke test (go build ./... on native Windows), and the gortex.exe CLI build. If the full shard only runs go test, a Windows build break lands without a red until someone runs the suite. Cheap to keep as two steps in the new shard if you want that signal.
Happy to take any of the clusters above once the shard has enumerated them for real — the .NET/GDScript/TSConfig ones in particular look like one root cause each rather than per-test work.
Addresses the review on zzet#646. reviewChangedGraphPaths inferred "this path is already graph-keyed" from strings.HasPrefix(f, repoPrefix+"/"). The two domains overlap, so that is not recoverable by inspection: in a repo whose tree carries a top-level directory named like the repo prefix, `repo-a/pkg/widget.go` is a valid git-relative path AND a valid graph key for a different file. The inference skips the real key `repo-a/repo-a/pkg/widget.go`, so the changed file is never scanned - and when a same-named `pkg/widget.go` exists, that unchanged shadow is scanned in its place. Make the domain explicit and travel with the data: changedPathsRepoRelative git's spelling, relative to the working tree changedPathsGraphKeyed the graph's "<prefix>/<rel>" node key All four production callers pass DiffResult.ChangedFiles, which MapGitDiff documents as keeping "the diff-relative paths (callers re-join them with git pathspecs)", so they pass changedPathsRepoRelative and the prefix is now applied unconditionally. Only the test covering a caller that already holds node keys passes changedPathsGraphKeyed. The Norm-based join and output normalization from the first revision are unchanged; this only replaces the inference. Regression test: a fixture carrying both pkg/widget.go and repo-a/pkg/widget.go, only the nested path marked changed, asserting every match reports the changed target. Both files carry the detector fixture, so a wrong-target scan still returns matches and only the reported path separates the outcomes - restoring the HasPrefix inference fails it with "pkg/widget.go" is the unchanged shadow. The ci.yml selector hunk is dropped: zzet#652 removes that job outright.
|
Full classification, as promised. Measured on 173 failing tests across 28 packages — but 2 of those are my machine, not the repo, so read the number as 171.
Buckets
Buckets 1–6 are individually enumerated (I have the test names). Bucket 7 is the residual, classified by first error line, so a handful may move as each is opened. Bucket 7 is the same family as #646 and it reaches production code, not just fixtures. Confirmed samples:
Three things I need from you1. File modes (bucket 3) — a real decision, and one is security-shaped.
2. The launchd / XDG tests. 3. Sequencing. The Windows leg cannot go green in one step — 171 failures across 26 packages is several PRs of work, and one PR carrying all of it would not be reviewable. Two ways to land it:
I would take (a). It reports real numbers from day one, it does not ask anyone to maintain a selector list you have already said can miss the regression it is meant to protect, and the flip to blocking becomes a one-line PR at the end. I have not measured bucket 2 on your runner. GitHub's windows image may well hold #655 is the first tranche and is independent of whichever you pick. |
2 participants
Summary
windows-latestto the existing full test matrix used by Linux and macOSfail-fast: falseso every OS shard completesbuild-windowsjobWhy
PR #647 showed that focused Windows test-name selectors can miss the exact regression they are meant to protect. Running
go test -race -timeout=30m -coverprofile=coverage.out ./...on every supported OS makes the platform contract explicit and lets CI reveal portability failures directly.macOS was already running the same full command as Linux; this change brings Windows into that matrix.
Validation
actionlint .github/workflows/ci.ymlgo build -o <temp>/gortex ./cmd/gortex/The repository-wide local test was attempted in the shared checkout. Its only observed failure was
TestNewIsFencedToIndexerStaging, caused by unrelated.claude/worktrees/*directories being scanned. GitHub clean runners are the authoritative result.Draft status
The previous workflow deliberately avoided the full Windows suite, and recent Windows runs documented existing platform-assumption failures. This PR stays draft while the new shard enumerates those failures so they can be classified from concrete CI output rather than another guessed selector list.