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) + } +}