Repository navigation
fix: keep the theme suite honest on a Windows runner - #141
Merged
Merged
Conversation
Two tests failed on the Windows runner from the moment internal/themes landed, so main has been red since. Neither is a Windows bug in the feature; both are the suite making a Linux assumption. TestListReturnsDirectoryError expected List to fail when a regular file sits where the themes directory belongs. On Windows, ReadDir on that file reports not-exist, which the "nothing imported yet" branch swallowed into an empty list — so the same state was an error on one OS and silence on another. This changes the product rather than the test: customThemes stats the path first and says outright that it is not a directory, which reads the same everywhere. TestDocumentedExampleIsImportable located the documented JSON by a fence followed by a bare \n. The repo pins no .gitattributes, so a Windows checkout carries CRLF and the pattern matched nothing. The pattern now takes either ending, and a test feeds it both so the assumption cannot come back unnoticed. Neither test ran on either pull request that introduced them: os-tests only runs on a push to main or under the ci:os label, and cross-compiling — which both changes did pass — only proves the code builds.
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.
Summary
mainhas been red on the Windows runner since #131 merged — three consecutive failing runs (0980607, aae21c2, 789f8f1). It surfaced when thev0.24.0tag ranrelease.yml: thewindowsjob failed, sorelease,aurandhomebrewwere skipped and nothing was published.Two tests fail, both from #131, and neither is a Windows bug in the feature. Both are the suite assuming Linux.
TestListReturnsDirectoryErrorThe test puts a regular file where the themes directory belongs and expects
Listto fail. On Windows,ReadDiron that file reports not-exist, andcustomThemeshad aos.IsNotExistbranch meaning "nothing imported yet" — so identical state read as an error on Linux and as silence on Windows.Fixed in the product rather than the test, since the divergence was real:
customThemesnow stats the path first and says outright that it is not a directory. Same answer on every OS, and the "nothing imported yet" branch still means only that.TestDocumentedExampleIsImportableMine, from the review follow-up. It finds the documented JSON with
```jsonfollowed by a bare\n. The repo pins no.gitattributes, so a Windows checkout carries CRLF and the pattern matched nothing — the test reported "docs/themes.md has no JSON example" on a file that has one.The pattern takes either ending now, and
TestDocJSONBlockReadsBothLineEndingsfeeds it both, so the assumption fails on Linux if anyone hardcodes\nagain.Why neither PR caught it
os-testsruns on a push tomain, or on a pull request carryingci:os. Neither #131 nor #140 had the label. The local cross-compile loop passed on both — but as CLAUDE.md says, that only proves the code builds; it never runs the suite. #131 touched_test.goand added Windows-specific validation, so it was exactly the case the label exists for.This PR carries
ci:os, so the real Windows and macOS runners have to go green before it merges.Testing
gofmt -l .,go vet ./...,go test -race ./...— 22 packageslinux,darwin,windowscd frontend && ./node_modules/.bin/vite buildNo
CHANGELOG.mdentry:v0.24.0never published, so no released version ever carried this, and the behaviour change it does contain — an error instead of an empty list when a file occupies the themes directory path — is not a state a user reaches.