diff --git a/internal/guard/changelogshape_test.go b/internal/guard/changelogshape_test.go index 129e9c1..d8c05ef 100644 --- a/internal/guard/changelogshape_test.go +++ b/internal/guard/changelogshape_test.go @@ -3,6 +3,7 @@ package guard import ( "os" "path/filepath" + "slices" "strings" "testing" ) @@ -99,3 +100,115 @@ func TestTheChangelogHasOneHeadingPerKindOfChange(t *testing.T) { t.Errorf("the first release heading is %q and docs/GIT.md puts [Unreleased] at the top", order[0]) } } + +// Every version in the changelog is linkable, and Unreleased compares from the +// newest one. +// +// What this defends against happened on 2026-09-03, closing the release of +// 0.3.0-rc1. A "## [0.3.0-rc1]" heading went in and the link definitions at the +// bottom were left as they were: the new version had none, so the heading +// rendered as literal square brackets, and [Unreleased] still compared from +// v0.2.0 - which means it showed the release's own changes as if they were +// still coming. Nothing went red. It was found by reading, and the reading +// happened to be looking for something else. +// +// Keep a Changelog is declared at the top of the file and in docs/GIT.md ยง3, so +// this is a convention the project has written down and nothing was holding it. +// The same class as O178, and closed on the same day. +// +// The oldest version points at its release tag rather than at a comparison, +// because there is nothing before it to compare from. That is why only the +// presence of a definition is required here, and only Unreleased has its target +// checked. +func TestEveryChangelogVersionHasItsLinkDefinition(t *testing.T) { + path := filepath.Join(repoRoot(t), "CHANGELOG.md") + raw, err := os.ReadFile(path) + if err != nil { + t.Fatalf("reading the changelog: %v", err) + } + + var headings []string + defined := map[string]string{} + inFence := false + for _, line := range strings.Split(string(raw), "\n") { + if strings.HasPrefix(line, "```") { + inFence = !inFence + continue + } + if inFence { + continue + } + if strings.HasPrefix(line, "## [") { + if name, ok := bracketed(line[len("## "):]); ok { + headings = append(headings, name) + } + continue + } + // A link definition is a bracketed name, a colon and a target, at the + // left margin. Anywhere else on a line it is a link being used. + if strings.HasPrefix(line, "[") { + if name, ok := bracketed(line); ok { + rest := strings.TrimPrefix(line[len(name)+2:], ":") + if strings.HasPrefix(line[len(name)+2:], ":") { + defined[name] = strings.TrimSpace(rest) + } + } + } + } + + if len(headings) == 0 { + t.Fatal("the changelog has no version headings at all, so this guard is reading the wrong file") + } + + for _, name := range headings { + if _, ok := defined[name]; !ok { + t.Errorf("the %q section has no link definition at the bottom of the changelog.\n"+ + "Reason: Keep a Changelog makes every version linkable, and this file declares that\n"+ + "format in its own header. Without the definition the heading renders as literal\n"+ + "square brackets and the reader has no way to the diff.\n"+ + "What to do: add a line like [%s]: beside the others at the bottom.", + name, name) + } + } + for name := range defined { + if !slices.Contains(headings, name) { + t.Errorf("the bottom of the changelog defines a link for %q and there is no such section.\n"+ + "Reason: a definition left behind after a section is renamed or removed points at a\n"+ + "comparison nobody can reach from the document.", name) + } + } + + // Unreleased compares from the newest released version. Getting this wrong + // is worse than leaving it out, because the link works: it just shows a + // released version's own changes as if they were still coming. + if len(headings) < 2 { + return + } + newest := headings[1] + target, ok := defined["Unreleased"] + if !ok { + return // already reported above + } + want := "compare/v" + newest + "...HEAD" + if !strings.HasSuffix(target, want) { + t.Errorf("[Unreleased] compares from something other than the newest version.\n"+ + "Reason: the newest section is %q, so the link should end in %q, and it is %q.\n"+ + "This went wrong on 0.3.0-rc1: it still compared from v0.2.0, so everything the\n"+ + "release shipped showed up under Unreleased as if it were still coming.\n"+ + "What to do: point it at the tag for %s.", + newest, want, target, newest) + } +} + +// bracketed reads the name out of a line that opens with a square bracket, and +// says whether there was one to read. +func bracketed(line string) (string, bool) { + if !strings.HasPrefix(line, "[") { + return "", false + } + end := strings.Index(line, "]") + if end < 2 { + return "", false + } + return line[1:end], true +} diff --git a/internal/guard/screenpixels_test.go b/internal/guard/screenpixels_test.go index e3f07c7..19936fc 100644 --- a/internal/guard/screenpixels_test.go +++ b/internal/guard/screenpixels_test.go @@ -4,6 +4,7 @@ import ( "bytes" "fmt" "image" + "image/color" "image/draw" "image/png" "os" @@ -538,9 +539,24 @@ func TestEveryScreenStillDrawsItsStoredPicture(t *testing.T) { picture := filepath.Join("testdata", "screens", sc.name+".png") tree := filepath.Join("testdata", "screens", sc.name+".xml") if writing { - writeImage(t, picture, got) - writeText(t, tree, markup) - t.Logf("wrote %s and %s", picture, tree) + // Each of the two files on its own, and only where what is + // stored no longer stands. Regenerating one screen used to + // rewrite all fifty files - see needsWriting for what that + // cost and how it was measured. + var wrote []string + if needsWriting(judgePicture(picture, got)) { + writeImage(t, picture, got) + wrote = append(wrote, picture) + } + if !storedTreeStands(tree, markup) { + writeText(t, tree, markup) + wrote = append(wrote, tree) + } + if len(wrote) == 0 { + t.Logf("left alone, both still stand: %s and %s", picture, tree) + } else { + t.Logf("wrote %s", strings.Join(wrote, " and ")) + } return } // The tree first, and that order is the whole reason it is kept. @@ -559,6 +575,98 @@ func TestEveryScreenStillDrawsItsStoredPicture(t *testing.T) { } } +// Regenerating the stored screens touches the ones that changed and no others. +// +// What this defends against happened, and it nearly shipped. Closing the +// release of 0.3.0-rc1 needed one screen rewritten, the About screen, because +// it draws the version. Regenerating rewrote all twenty five, and the other +// twenty four grew by between 12.6% and 28.9% while their pixels stayed byte +// identical - Go 1.27 encodes PNG less tightly than 1.26.7, which wrote them. +// Twenty four pointless binary files went into the release commit, and what +// took them back out was somebody reading the diff, because `git diff --stat` +// says "Bin" and nothing goes red. +// +// The three cases are the whole of the decision, so this is the whole guard. +func TestRegeneratingAScreenLeavesTheUnchangedOnesAlone(t *testing.T) { + dir := t.TempDir() + drawn := solidImage(8, 8, color.NRGBA{R: 10, G: 20, B: 30, A: 255}) + + missing := filepath.Join(dir, "never-stored.png") + if !needsWriting(judgePicture(missing, drawn)) { + t.Errorf("a screen with no stored picture was reported as not needing one.\n" + + "Reason: a new screen has nothing to be compared against, and skipping it would\n" + + "leave the guard with no reference on the next run.") + } + + // The case that cost 402 kB. The stored file holds the same picture through + // a different encoder, which is what a compiler update leaves behind, and + // the guard accepts it - so regenerating has nothing to write. + reencoded := filepath.Join(dir, "same-picture-other-encoder.png") + writeBytes(t, reencoded, encodeAtCompression(t, drawn, png.NoCompression)) + if same := encodePNG(t, drawn); bytes.Equal(same, mustRead(t, reencoded)) { + t.Fatal("this case needs a stored file whose BYTES differ from a fresh encode, " + + "and the two encoders produced the same bytes - the case is not being exercised") + } + if needsWriting(judgePicture(reencoded, drawn)) { + t.Errorf("a stored picture the guard still accepts was reported as needing rewriting.\n" + + "Reason: measured on 2026-09-03, this rewrote twenty four of twenty five screens with\n" + + "byte identical pixels after the move to Go 1.27, and 402 kB of that went into the\n" + + "0.3.0-rc1 release commit. Asking whether the bytes match instead of whether the guard\n" + + "accepts them brings all of it back on the first regeneration after a compiler update.") + } + + moved := filepath.Join(dir, "moved-on.png") + writeBytes(t, moved, encodePNG(t, solidImage(8, 8, color.NRGBA{R: 200, G: 20, B: 30, A: 255}))) + if !needsWriting(judgePicture(moved, drawn)) { + t.Errorf("a stored picture showing something else was reported as not needing writing.\n" + + "Reason: then regenerating would not regenerate, and the guard would go on comparing\n" + + "against a picture nobody can refresh.") + } + + // The tree is stored beside the picture and asked separately, because a + // screen can be rebuilt into the same pixels out of different widgets. + storedXML := filepath.Join(dir, "tree.xml") + writeText(t, storedXML, "one") + if !storedTreeStands(storedXML, "one") { + t.Errorf("a stored tree holding exactly this markup was reported as no longer standing") + } + if storedTreeStands(storedXML, "two") { + t.Errorf("a stored tree holding different markup was reported as still standing") + } + if storedTreeStands(filepath.Join(dir, "no-tree.xml"), "one") { + t.Errorf("a tree that was never stored was reported as still standing.\n" + + "Reason: a new screen would then never get one written.") + } +} + +// solidImage is a picture with nothing in it but one colour, which is all these +// cases need - the decision under test is about encoders and equality, not +// about what a screen looks like. +func solidImage(w, h int, c color.NRGBA) *image.NRGBA { + img := image.NewNRGBA(image.Rect(0, 0, w, h)) + draw.Draw(img, img.Bounds(), &image.Uniform{C: c}, image.Point{}, draw.Src) + return img +} + +func encodeAtCompression(t *testing.T, img image.Image, level png.CompressionLevel) []byte { + t.Helper() + var buf bytes.Buffer + enc := png.Encoder{CompressionLevel: level} + if err := enc.Encode(&buf, img); err != nil { + t.Fatalf("%v", err) + } + return buf.Bytes() +} + +func mustRead(t *testing.T, path string) []byte { + t.Helper() + raw, err := os.ReadFile(path) + if err != nil { + t.Fatalf("%v", err) + } + return raw +} + // compareMarkup holds the widget tree against the stored one. // // Exact, on every system, with no tolerance anywhere - measured on 2026-08-18 @@ -567,14 +675,13 @@ func TestEveryScreenStillDrawsItsStoredPicture(t *testing.T) { func compareMarkup(t *testing.T, name, reference, got string) { t.Helper() - stored, err := os.ReadFile(reference) + want, err := storedTree(reference) if err != nil { t.Fatalf("there is no stored tree for the %s screen at %s.\n"+ "Reason: this guard compares the widget tree as well as the picture.\n"+ "What to do: run once with TFG_WRITE_SCREEN_REFERENCE=1 and read what it wrote before committing it.\n"+ "Underlying error: %v", name, reference, err) } - want := strings.ReplaceAll(string(stored), "\r\n", "\n") if want == got { return } @@ -833,44 +940,35 @@ func explanationBeside(t *testing.T, o fyne.CanvasObject, label string) *parts.D func compareAgainstReference(t *testing.T, name, reference string, got image.Image) { t.Helper() - stored, err := os.ReadFile(reference) - if err != nil { - t.Fatalf("there is no stored picture for the %s screen at %s.\n"+ + v := judgePicture(reference, got) + if v.unusable { + t.Fatalf("there is no readable stored picture for the %s screen at %s.\n"+ "Reason: this guard compares against images kept in the repository.\n"+ "What to do: run it once with TFG_WRITE_SCREEN_REFERENCE=1 and look at what it wrote before committing it.\n"+ - "Underlying error: %v", name, reference, err) + "Underlying error: %v", name, reference, v.err) } - want, err := png.Decode(bytes.NewReader(stored)) - if err != nil { - t.Fatalf("the stored picture %s could not be read as a PNG: %v", reference, err) - } - - wantPix, gotPix := toNRGBA(want), toNRGBA(got) - if wantPix.Rect != gotPix.Rect { + if v.resized { t.Fatalf("the %s screen rendered %v and the stored picture is %v.\n"+ "Reason: the window is being laid out at a different size than the reference was taken at.\n"+ "What to do: this guard renders at %dx%d, so a deliberate change of that constant needs the pictures regenerated.", - name, gotPix.Rect.Size(), wantPix.Rect.Size(), referenceWidth, referenceHeight) + name, v.got.Rect.Size(), v.want.Rect.Size(), referenceWidth, referenceHeight) } - if bytes.Equal(wantPix.Pix, gotPix.Pix) { - return - } - - differing, worst, box := pixelDifference(wantPix, gotPix) - if worst <= pixelTolerance() { - // Said out loud rather than passed in silence. If this number ever - // starts climbing, the tolerance is covering something it was not - // measured to cover, and nobody would see that from a green run. - t.Logf("%d pixels differ by at most %d of 255, which is inside the %d allowed on %s - treated as edge blending", - differing, worst, pixelTolerance(), runtime.GOARCH) + if v.accepts { + if v.differing > 0 { + // Said out loud rather than passed in silence. If this number ever + // starts climbing, the tolerance is covering something it was not + // measured to cover, and nobody would see that from a green run. + t.Logf("%d pixels differ by at most %d of 255, which is inside the %d allowed on %s - treated as edge blending", + v.differing, v.worst, pixelTolerance(), runtime.GOARCH) + } return } - dir := saveEvidence(t, name, wantPix, gotPix) + dir := saveEvidence(t, name, v.want, v.got) t.Errorf("the %s screen no longer draws its stored picture.\n"+ "Reason: %d of %d pixels differ, by at most %d of 255 in one channel, inside %v.\n"+ "What to look at: %s holds the stored picture, what was rendered now, and the two subtracted.\n"+ "What to do: if the change was meant, regenerate with TFG_WRITE_SCREEN_REFERENCE=1 and commit the new pictures.", - name, differing, wantPix.Rect.Dx()*wantPix.Rect.Dy(), worst, box, dir) + name, v.differing, v.want.Rect.Dx()*v.want.Rect.Dy(), v.worst, v.box, dir) } // pixelDifference counts the pixels that differ, the largest distance any one @@ -965,18 +1063,115 @@ func toNRGBA(img image.Image) *image.NRGBA { func writeImage(t *testing.T, path string, img image.Image) { t.Helper() - if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil { - t.Fatalf("%v", err) - } + writeBytes(t, path, encodePNG(t, img)) +} + +// encodePNG is the one place a picture becomes bytes, so a reference and the +// bytes it is compared against cannot be produced by two different calls. +func encodePNG(t *testing.T, img image.Image) []byte { + t.Helper() var buf bytes.Buffer if err := png.Encode(&buf, img); err != nil { t.Fatalf("%v", err) } - if err := os.WriteFile(path, buf.Bytes(), 0o644); err != nil { + return buf.Bytes() +} + +func writeBytes(t *testing.T, path string, content []byte) { + t.Helper() + if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil { + t.Fatalf("%v", err) + } + if err := os.WriteFile(path, content, 0o644); err != nil { t.Fatalf("%v", err) } } +// pictureVerdict is what a stored picture says about what was just rendered. +// +// It is one value rather than two calls because the comparison and the writing +// have to answer out of the same judgement. A writer with its own idea of +// "changed" drifts away from the guard, and then regenerating either rewrites +// files the guard was happy with or leaves ones it was not. +type pictureVerdict struct { + accepts bool // the stored picture still stands for what was rendered + unusable bool // missing, or not readable as a PNG + err error + resized bool + differing int + worst int + box image.Rectangle + want *image.NRGBA + got *image.NRGBA +} + +// judgePicture is the single place that decides whether a stored picture still +// stands. Both the comparison and TFG_WRITE_SCREEN_REFERENCE read it. +func judgePicture(path string, got image.Image) pictureVerdict { + v := pictureVerdict{got: toNRGBA(got)} + + stored, err := os.ReadFile(path) + if err != nil { + v.unusable, v.err = true, err + return v + } + want, err := png.Decode(bytes.NewReader(stored)) + if err != nil { + v.unusable, v.err = true, err + return v + } + v.want = toNRGBA(want) + + if v.want.Rect != v.got.Rect { + v.resized = true + return v + } + if bytes.Equal(v.want.Pix, v.got.Pix) { + v.accepts = true + return v + } + v.differing, v.worst, v.box = pixelDifference(v.want, v.got) + v.accepts = v.worst <= pixelTolerance() + return v +} + +// needsWriting says whether regenerating has anything to write for one file. +// +// It exists because regenerating used to rewrite all twenty five screens +// whether or not any of them had changed, and that is not a tidiness problem. +// Measured on 2026-09-03, while closing the release of 0.3.0-rc1: one screen +// legitimately changed, twenty four came back with byte identical pixels and +// files between 12.6% and 28.9% larger, because Go 1.27 encodes PNG less +// tightly than 1.26.7 did, which is what wrote them. Nobody reads a binary +// diff and `git diff --stat` says only "Bin", so 402 kB of pure encoder churn +// across twenty four files went into the release commit, and what took it back +// out was somebody reading rather than anything red. See O180. +// +// The question is the guard's own, not "are these bytes identical". Byte +// equality was written here first and measured wrong on the spot: it rewrites +// every picture once on the first regeneration after a compiler update, which +// is the exact churn this is here to stop. A stored picture the guard still +// accepts is a stored picture that does not need replacing. +func needsWriting(v pictureVerdict) bool { return !v.accepts } + +// storedTree reads a stored widget tree with the line endings a checkout on +// another system may have given it. One place does this normalising, because +// the comparison and the regenerating both have to mean the same by "the same +// tree". +func storedTree(path string) (string, error) { + stored, err := os.ReadFile(path) + if err != nil { + return "", err + } + return strings.ReplaceAll(string(stored), "\r\n", "\n"), nil +} + +// storedTreeStands says whether the stored tree still says what was just built. +func storedTreeStands(path, markup string) bool { + want, err := storedTree(path) + return err == nil && want == markup +} + // writeText stores the widget tree with newlines the way the toolkit produced // them, so a checkout on another system does not read as a change on every // line. The comparison normalises the same way.