From 270ad7a71b075b429bccab42dd59bddd7e2ac87f Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Thu, 10 Sep 2026 18:21:12 -0400 Subject: [PATCH] fix(ci): tests do not ship, so they must not re-version an artifact MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Completes the pathspec. 17212e9e excluded CI, docs and tooling but left tests in the shipped set, so its own commit re-versioned the image on the strength of a _test.go file. `go build` drops *_test.go outright and the Vite build never imports a .test.ts — neither reaches an image or an APK. Globs over files rather than a directory exclusion, because this repo has no tests/ tree to exclude: Go tests sit inline beside the code they cover (158 files) and the web suite beside its modules (115). Patterns match what exists and nothing speculative — there are no .spec.* files, no __tests__/ directories and no androidTest/ tree. If any appear they re-version until named, which is the harmless direction and the point of a denylist. The guard that matters is not "a test-only commit is inert" — it is that a commit touching a test AND its source still moves the version. `':!internal'` would satisfy every inertness assertion while silently excluding the entire server, which is the stale-version-on-changed-artifact failure this whole derivation exists to prevent. Falsified: drop the Go exclusion and a _test.go commit moves the version; drop the web one and a .test.ts does; replace the globs with `':!internal'` and the source-alongside-test case breaks. That last check failed first time, on a bug in the FIXTURE rather than the derivation, and it is worth recording because it makes a test pass for the wrong reason. Both commit helpers wrote the constant "x\n", so re-writing a file with identical bytes recorded NOTHING — the "source and test together" commit actually contained only the test, and the assertion was quietly checking the case it was meant to contrast against. Content is now derived from the commit's epoch, and the test asserts HEAD really contains both paths before drawing any conclusion from it. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01SQ31KQpYbStyK5y58UmPLH --- ci/version.sh | 22 +++++ internal/server/release_version_test.go | 119 +++++++++++++++++++++++- 2 files changed, 140 insertions(+), 1 deletion(-) diff --git a/ci/version.sh b/ci/version.sh index fb8774ef..b12a13d0 100755 --- a/ci/version.sh +++ b/ci/version.sh @@ -76,6 +76,28 @@ readonly SHIPPED=( ':!.dockerignore' ':!renovate.json' ':!.golangci.yml' + + # TESTS DO NOT SHIP, so they must not re-version an artifact. + # + # Named as globs rather than a directory because this repo has no tests/ + # tree to exclude: Go tests sit inline beside the code they cover, and the + # web suite sits beside its modules. `go build` drops *_test.go outright and + # the Vite build never imports a .test.ts, so neither reaches an artifact. + # + # A commit touching a test AND its source still moves the version — the + # source path matches on its own. Only a test-ONLY commit is inert, which is + # the whole intent. + # + # Patterns match what exists today and nothing speculative: there are no + # .spec.* files, no __tests__/ directories and no androidTest/ tree. If any + # appear they will re-version until named here, which is the harmless + # direction and the reason this list is a denylist. + ':!*_test.go' # 158 files, inline beside the code + ':!*.test.ts' # 114 files + ':!*.test.js' + ':!android/app/src/test' # JVM unit tests; no androidTest tree exists + ':!web/vitest.config.ts' # test-harness config, not build config + ':!web/vitest.setup.ts' ) commit_epoch="${MINSTREL_COMMIT_EPOCH:-}" diff --git a/internal/server/release_version_test.go b/internal/server/release_version_test.go index 1846adca..fa508d71 100644 --- a/internal/server/release_version_test.go +++ b/internal/server/release_version_test.go @@ -496,7 +496,11 @@ func commitFile(t *testing.T, dir, path, epoch string) { if err := os.MkdirAll(filepath.Dir(full), 0o755); err != nil { t.Fatal(err) } - if err := os.WriteFile(full, []byte("x\n"), 0o644); err != nil { + // Content must DIFFER from any earlier write to the same path, or git + // records nothing and the commit silently covers fewer files than the + // test believes. That is not hypothetical: it made the + // source-alongside-test case pass vacuously. + if err := os.WriteFile(full, []byte("content @"+epoch+"\n"), 0o644); err != nil { t.Fatal(err) } for _, args := range [][]string{{"add", "-A"}, {"commit", "-q", "-m", path}} { @@ -599,3 +603,116 @@ func TestVersionScript_RefusesWhenNothingShippedIsInRange(t *testing.T) { t.Errorf("script emitted a version name while refusing — that value could still be consumed\n%s", out) } } + +// commitFiles is commitFile for more than one path in a single commit. +func commitFiles(t *testing.T, dir, epoch string, paths ...string) { + t.Helper() + for _, rel := range paths { + full := filepath.Join(dir, rel) + if err := os.MkdirAll(filepath.Dir(full), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(full, []byte("content @"+epoch+"\n"), 0o644); err != nil { + t.Fatal(err) + } + } + for _, args := range [][]string{{"add", "-A"}, {"commit", "-q", "-m", strings.Join(paths, " ")}} { + cmd := exec.Command("git", args...) + cmd.Dir = dir + cmd.Env = append(os.Environ(), + "GIT_CONFIG_GLOBAL=/dev/null", "GIT_CONFIG_SYSTEM=/dev/null", + "GIT_AUTHOR_DATE=@"+epoch+" +0000", + "GIT_COMMITTER_DATE=@"+epoch+" +0000", + ) + if out, err := cmd.CombinedOutput(); err != nil { + t.Fatalf("git %v: %v\n%s", args, err, out) + } + } +} + +// gitOutput runs a git command in dir and returns its combined output. +func gitOutput(t *testing.T, dir string, args ...string) string { + t.Helper() + cmd := exec.Command("git", args...) + cmd.Dir = dir + cmd.Env = append(os.Environ(), "GIT_CONFIG_GLOBAL=/dev/null", "GIT_CONFIG_SYSTEM=/dev/null") + out, err := cmd.CombinedOutput() + if err != nil { + t.Fatalf("git %v: %v\n%s", args, err, out) + } + return string(out) +} + +// Tests do not ship, so a test-only commit must not re-version an artifact. +// +// `go build` drops *_test.go outright and the Vite build never imports a +// .test.ts, so neither reaches an image or an APK. This repo has no tests/ +// tree — Go tests sit inline beside the code — so the exclusions are globs, +// and a glob is easy to get subtly wrong in a way that still looks right. +func TestVersionName_TestsDoNotReVersionTheArtifact(t *testing.T) { + const ( + shipped = "1757443736" // 2025.09.09.1848 + later = "1789000920" // 2026.09.10.0042 + ) + for _, tc := range []struct{ name, path string }{ + {"go test beside its source", "internal/server/thing_test.go"}, + {"web unit test", "web/src/lib/api/admin.test.ts"}, + {"web script test", "web/scripts/tokens-to-css.test.js"}, + {"android JVM unit test", "android/app/src/test/java/A.kt"}, + {"vitest harness config", "web/vitest.config.ts"}, + } { + t.Run(tc.name, func(t *testing.T) { + dir := gitRepo(t) + commitFile(t, dir, "internal/server/thing.go", shipped) + commitFile(t, dir, tc.path, later) + + out, err := versionIn(t, dir) + if err != nil { + t.Fatalf("version.sh failed: %v\n%s", err, out) + } + if !strings.Contains(out, "name=2025.09.09.1848") { + t.Errorf("a commit touching only %s moved the version; tests do not ship\n%s", tc.path, out) + } + }) + } +} + +// The direction that actually costs something, and the reason the exclusions +// above are globs over FILES rather than over their directories. +// +// `':!internal'` would satisfy every assertion in the test above while +// silently excluding the entire server. This pins the opposite: a commit that +// changes a test AND the source under it must still move the version, because +// the source path matches on its own. Without this, a too-broad exclusion +// reads as a passing test suite and ships an artifact under a stale version. +func TestVersionName_ATestAlongsideItsSourceStillCounts(t *testing.T) { + const ( + shipped = "1757443736" + later = "1789000920" + ) + dir := gitRepo(t) + commitFile(t, dir, "internal/server/thing.go", shipped) + commitFiles(t, dir, later, + "internal/server/thing.go", + "internal/server/thing_test.go", + ) + + // The commit must actually contain BOTH paths. Rewriting a file with + // identical bytes records nothing, and this assertion would then be + // checking a test-only commit while appearing to check a mixed one. + touched := gitOutput(t, dir, "show", "--name-only", "--format=", "HEAD") + for _, want := range []string{"internal/server/thing.go", "internal/server/thing_test.go"} { + if !strings.Contains(touched, want) { + t.Fatalf("fixture is wrong: HEAD does not contain %s\n%s", want, touched) + } + } + + out, err := versionIn(t, dir) + if err != nil { + t.Fatalf("version.sh failed: %v\n%s", err, out) + } + if !strings.Contains(out, "name=2026.09.10.0042") { + t.Errorf("a source change accompanied by a test change did NOT move the version — "+ + "the exclusions are matching directories rather than test files\n%s", out) + } +}