Raised while reviewing #114, which fixes a Windows-only test-isolation bug that no CI job can observe.
Reason. All four jobs in .github/workflows/ci.yml run on ubuntu-latest (:8, :18, :28, :39). Windows is a shipped target: .goreleaser.yaml:13 builds windows for amd64 and arm64, and the README documents the Windows install.
The gap is not theoretical. os.UserHomeDir reads HOME everywhere except Windows, where it reads USERPROFILE, so every test fixture that isolated itself by setting HOME alone was reading the developer's real profile on Windows - TestLoad_MissingFileIsNotConfigured failed because a real wallet existed, and TestLoad_ReadsTheWallet asserted a fixture subOrgId and got the developer's own. #114 fixes all 13 sites, and the pipeline would have been green either way.
The same holds going forward: a new test written the old way compiles, passes unused, passes the ubuntu test job, and merges - reintroducing the class immediately.
Scope. .github/workflows/ci.yml, the test job. Does not touch lint, docs-check or integration-test, and needs no change to any test.
Out of scope, and worth naming so it is not conflated: pkg/cmdutil/host.go:24 returns KH_HOST ahead of cfg.DefaultHost, so an ambient KH_HOST redirects the doctor suite away from its injected test server on every platform. That is a separate isolation gap and a separate fix.
Plan. Add windows-latest to the test job, most cheaply as a runs-on matrix so the ubuntu run is unchanged. Whether the matrix should also cover macOS, and whether a Windows failure should block or be advisory at first, is worth a moment's thought rather than assuming - a first Windows run on a codebase that has never had one may surface unrelated path and line-ending issues, and discovering that in a blocking job on someone else's PR is the bad version of finding out.
If a full matrix turns out to be more than it is worth, the cheap alternative that closes the specific regression is a grep guard in the existing job rejecting a bare Setenv("HOME" outside the setHome helper in those two packages. That is strictly worse - it pins one symptom rather than running the tests - so it is the fallback, not the proposal.
Raised while reviewing #114, which fixes a Windows-only test-isolation bug that no CI job can observe.
Reason. All four jobs in
.github/workflows/ci.ymlrun onubuntu-latest(:8,:18,:28,:39). Windows is a shipped target:.goreleaser.yaml:13buildswindowsfor amd64 and arm64, and the README documents the Windows install.The gap is not theoretical.
os.UserHomeDirreadsHOMEeverywhere except Windows, where it readsUSERPROFILE, so every test fixture that isolated itself by settingHOMEalone was reading the developer's real profile on Windows -TestLoad_MissingFileIsNotConfiguredfailed because a real wallet existed, andTestLoad_ReadsTheWalletasserted a fixture subOrgId and got the developer's own. #114 fixes all 13 sites, and the pipeline would have been green either way.The same holds going forward: a new test written the old way compiles, passes
unused, passes the ubuntutestjob, and merges - reintroducing the class immediately.Scope.
.github/workflows/ci.yml, thetestjob. Does not touchlint,docs-checkorintegration-test, and needs no change to any test.Out of scope, and worth naming so it is not conflated:
pkg/cmdutil/host.go:24returnsKH_HOSTahead ofcfg.DefaultHost, so an ambientKH_HOSTredirects the doctor suite away from its injected test server on every platform. That is a separate isolation gap and a separate fix.Plan. Add
windows-latestto thetestjob, most cheaply as aruns-onmatrix so the ubuntu run is unchanged. Whether the matrix should also cover macOS, and whether a Windows failure should block or be advisory at first, is worth a moment's thought rather than assuming - a first Windows run on a codebase that has never had one may surface unrelated path and line-ending issues, and discovering that in a blocking job on someone else's PR is the bad version of finding out.If a full matrix turns out to be more than it is worth, the cheap alternative that closes the specific regression is a grep guard in the existing job rejecting a bare
Setenv("HOME"outside thesetHomehelper in those two packages. That is strictly worse - it pins one symptom rather than running the tests - so it is the fallback, not the proposal.