covratchet: ask the coverage question before the push, not 47 minutes later

scripts/coverage/README.md

@@ -0,0 +1,101 @@
+# The coverage ratchet
+
+CI said **`N function(s) dropped below 100% -- named above`** and you landed
+here. This page says what that gate is, why it is not a percentage, and how to
+answer it without a round trip through CI.
+
+## What is gated
+
+Not the total. The **set of functions below 100%**, recorded per lane in
+`BELOW100.<lane>`, and that set may not grow.
+
+A gate on the rounded total could not be calibrated. On `main` both POSIX lanes
+report `coverage: 100.0%` and pass *while printing* 19 (ubuntu) and 16 (macOS)
+functions below 100%, the lowest at 75.0%. The tolerance is whatever `%.1f`
+absorbs at the current repository size: nobody chose it, nothing states it, and
+it **shrinks as the codebase grows**. A function falling from 100% to 75% is
+free until it is suddenly fatal, and the commit that trips the gate is not the
+one that created the debt.
+
+It was also arbitrary which lane you believed. On #727 the same commit gave
+99.9% on ubuntu (FAIL) and 100.0% on macOS (pass): ubuntu carries three
+filesystem-dependent functions macOS does not, so one further dead function
+tipped only one lane over the edge.
+
+Percentages are still *reported*. They are not gated, because coverage is
+measured under `-race`, where a percentage jitters and a set does not — and a
+gate that flakes is a gate people learn to rerun.
+
+## Answering it without waiting 47 minutes for CI
+
+The lane that keeps the record runs the whole suite. Taking a full profile
+locally costs about the same, so the useful local question is narrower:
+
+> the functions this commit **claims** to have covered — are they actually at
+> 100%?
+
+```sh
+go test ./internal/vm/ -run 'FTP' -count=1 -coverprofile=/tmp/ftp.cov
+go tool cover -func=/tmp/ftp.cov > /tmp/ftp.func
+go run ./scripts/coverage/cmd/covratchet -profile /tmp/ftp.func \
+    -require ftpApplyOpts,ftpTimeoutErr
+```
+
+```
+required 2 function(s), matched 2 record(s) in a profile of 7347
+
+all required functions at 100%
+```
+
+A selective `-run` is fine here: a function reaching 100% under *some* tests is
+at 100% under all of them. `-require` takes bare names, or `file:name` when a
+name occurs in more than one file.
+
+### ⛔ Why this is a command and not a `grep`
+
+**A name that matches nothing fails.** With `grep`, a typo, a renamed function
+or a profile taken over the wrong package all print nothing — and nothing is
+exactly what success looks like:
+
+```sh
+$ ... -require ftpApplyOpts,ftpTimeoutErrr      # one extra 'r'
+required 2 function(s), matched 1 record(s) in a profile of 7347
+
+-- NOT IN THIS PROFILE -- the name matched nothing (1) --
+   ftpTimeoutErrr
+
+::error::1 name(s) matched nothing; this profile cannot answer for them
+```
+
+This is the same failure mode as `go test -run` exiting `ok` on a pattern that
+selected nothing. Every verdict therefore prints its **denominator** — how many
+records matched, out of how big a profile — so a run that read nothing cannot be
+mistaken for a clean one.
+
+It exists because the ratchet, correct as it is, only refuses **after** the
+spend. Twice in one day a commit written to answer it did not move the
+measurement at all and came back red on the *same two functions*. A gate that
+refuses after the spend teaches the lesson; one that refuses before it prevents
+the mistake.
+
+## When a function legitimately reaches 100%
+
+The record is not updated by hand. On a green run of that lane:
+
+```sh
+UPDATE_COVERAGE=1 ...   # rewrites BELOW100.<lane>
+```
+
+Leaving a fixed function in the record is not harmless: a stale allowance is
+where the next regression hides.
+
+## Exit status
+
+| code | meaning |
+| --- | --- |
+| 0 | no function newly below 100% (or, with `-require`, all of them at 100%) |
+| 1 | a regression, or a required name that matched nothing |
+| 2 | **refused to decide** — the profile or the record could not be read |
+
+A profile with no `total:` line is refused rather than reported clean: a parser
+that swallows what it does not understand turns a format change into a pass.

scripts/coverage/cmd/covratchet/main.go

@@ -88,11 +88,16 @@ type entry struct {
 }
 
 // parse reads `go tool cover -func` output and returns every function below
-// 100%, keyed by identity. The total line is recognised and skipped rather than
-// silently mis-parsed: a parser that swallows what it does not understand turns
-// a format change into a clean result.
-func parse(r io.Reader, what, mod string) (map[fn]entry, error) {
+// 100%, keyed by identity, plus EVERY function it read. The total line is
+// recognised and skipped rather than silently mis-parsed: a parser that swallows
+// what it does not understand turns a format change into a clean result.
+//
+// The second map exists for -require, which has to tell "this function is at
+// 100%" apart from "this profile has never heard of that function". The first
+// map cannot: both are simply absent from it.
+func parse(r io.Reader, what, mod string) (map[fn]entry, map[fn]entry, error) {
 	out := make(map[fn]entry)
+	all := make(map[fn]entry)
 	seen := make(map[string]int) // (file, name) -> how many seen so far, in file order
 	sc := bufio.NewScanner(r)
 	sc.Buffer(make([]byte, 0, 64*1024), 4*1024*1024)
@@ -104,41 +109,42 @@ func parse(r io.Reader, what, mod string) (map[fn]entry, error) {
 		}
 		f := strings.Fields(line)
 		if len(f) < 2 || !strings.HasSuffix(f[len(f)-1], "%") {
-			return nil, fmt.Errorf("%s line %d: not a cover -func record: %q", what, n, line)
+			return nil, nil, fmt.Errorf("%s line %d: not a cover -func record: %q", what, n, line)
 		}
 		pct, err := strconv.ParseFloat(strings.TrimSuffix(f[len(f)-1], "%"), 64)
 		if err != nil {
-			return nil, fmt.Errorf("%s line %d: %q is not a percentage", what, n, f[len(f)-1])
+			return nil, nil, fmt.Errorf("%s line %d: %q is not a percentage", what, n, f[len(f)-1])
 		}
 		if f[0] == "total:" {
 			seenTotal = true
 			continue
 		}
 		if len(f) < 3 {
-			return nil, fmt.Errorf("%s line %d: expected file:line: name pct, got %q", what, n, line)
+			return nil, nil, fmt.Errorf("%s line %d: expected file:line: name pct, got %q", what, n, line)
 		}
 		file, ln := splitPos(f[0], mod)
 		// The ordinal is assigned BEFORE the 100% filter, so it does not shift
 		// when a sibling of the same name crosses the line.
 		nameKey := file + "\x00" + f[1]
 		ord := seen[nameKey]
 		seen[nameKey] = ord + 1
+		k := fn{file: file, name: f[1], ord: ord}
+		all[k] = entry{fn: k, pct: pct, line: ln}
 		if pct >= 100 {
 			continue
 		}
-		k := fn{file: file, name: f[1], ord: ord}
 		if prev, dup := out[k]; dup {
-			return nil, fmt.Errorf("%s line %d: %s appears twice (was %.1f%%)", what, n, k, prev.pct)
+			return nil, nil, fmt.Errorf("%s line %d: %s appears twice (was %.1f%%)", what, n, k, prev.pct)
 		}
 		out[k] = entry{fn: k, pct: pct, line: ln}
 	}
 	if err := sc.Err(); err != nil {
-		return nil, fmt.Errorf("reading %s: %w", what, err)
+		return nil, nil, fmt.Errorf("reading %s: %w", what, err)
 	}
 	if !seenTotal {
-		return nil, fmt.Errorf("%s has no total: line -- a profile this tool could not read must not be reported as clean", what)
+		return nil, nil, fmt.Errorf("%s has no total: line -- a profile this tool could not read must not be reported as clean", what)
 	}
-	return out, nil
+	return out, all, nil
 }
 
 // splitPos splits "<module>/internal/vm/vm.go:1473:" into its repo-relative path
@@ -235,10 +241,11 @@ func run(args []string, stdin io.Reader, stdout io.Writer) error {
 	lane := fs.String("lane", "", "lane name, for the record's header")
 	mod := fs.String("module", "github.com/go-embedded-ruby/ruby/", "module path prefix to strip from each file path")
 	update := fs.Bool("update", false, "rewrite the record from this run and exit 0")
+	require := fs.String("require", "", "comma-separated functions (`name` or `file:name`) that must be at 100% in this profile")
 	if err := fs.Parse(args); err != nil {
 		return err
 	}
-	if *record == "" {
+	if *record == "" && *require == "" {
 		return fmt.Errorf("-record is required")
 	}
 
@@ -251,11 +258,15 @@ func run(args []string, stdin io.Reader, stdout io.Writer) error {
 		defer f.Close()
 		src = f
 	}
-	got, err := parse(src, "profile", *mod)
+	got, all, err := parse(src, "profile", *mod)
 	if err != nil {
 		return err
 	}
 
+	if *require != "" {
+		return requireFull(stdout, all, *require)
+	}
+
 	if *update {
 		f, err := os.Create(*record)
 		if err != nil {
@@ -318,6 +329,90 @@ func run(args []string, stdin io.Reader, stdout io.Writer) error {
 	return nil
 }
 
+// requireFull answers one question, before the push rather than after the CI
+// run: are the functions this commit CLAIMS to have covered actually at 100% in
+// a profile taken from this working tree?
+//
+// ⛔ WHY IT EXISTS. The ratchet already catches this, but it catches it in CI,
+// 47 minutes later, on a lane that has to run the whole suite. Twice in one day
+// a commit written to answer the ratchet did not move the measurement at all
+// and came back red on the SAME two functions. A gate that refuses after the
+// spend teaches the lesson; a gate that refuses before it prevents the mistake.
+//
+// ⛔ A SPEC THAT MATCHES NOTHING IS A FAILURE, NOT A PASS. This is the whole
+// reason the tool takes the full profile and not just the sub-100 set: with the
+// sub-100 set alone, "ftpTimeoutErr is at 100%" and "there is no function here
+// by that name" are the same observation -- absence. A typo, a renamed
+// function, a profile taken over the wrong package would all read as success,
+// and the check would be worse than nothing because it would be believed. The
+// same shape as `go test -run` exiting 0 on a pattern that selected nothing.
+func requireFull(w io.Writer, all map[fn]entry, spec string) error {
+	type want struct{ file, name string }
+	var wants []want
+	for _, s := range strings.Split(spec, ",") {
+		s = strings.TrimSpace(s)
+		if s == "" {
+			continue
+		}
+		if i := strings.LastIndexByte(s, ':'); i >= 0 {
+			wants = append(wants, want{file: s[:i], name: s[i+1:]})
+			continue
+		}
+		wants = append(wants, want{name: s})
+	}
+	if len(wants) == 0 {
+		return fmt.Errorf("-require named no function")
+	}
+
+	var missing, below []string
+	matched := 0
+	for _, w2 := range wants {
+		var hits []entry
+		for k, e := range all {
+			if k.name != w2.name {
+				continue
+			}
+			// A bare name matches in any file; file:name matches that path or
+			// any suffix of it, so a repo-relative path and a bare basename
+			// both work.
+			if w2.file != "" && k.file != w2.file && !strings.HasSuffix(k.file, "/"+w2.file) {
+				continue
+			}
+			hits = append(hits, e)
+		}
+		if len(hits) == 0 {
+			missing = append(missing, w2.name)
+			continue
+		}
+		matched += len(hits)
+		for _, e := range hits {
+			if e.pct < 100 {
+				below = append(below, fmt.Sprintf("%s  %.1f%%", e.fn, e.pct))
+			}
+		}
+	}
+	sort.Strings(missing)
+	sort.Strings(below)
+
+	// Report the denominator beside the verdict: a check that says only "OK"
+	// cannot be told from one that looked at nothing.
+	fmt.Fprintf(w, "required %d function(s), matched %d record(s) in a profile of %d\n",
+		len(wants), matched, len(all))
+	section(w, "NOT IN THIS PROFILE -- the name matched nothing", missing)
+	section(w, "BELOW 100%", below)
+
+	if len(missing) > 0 {
+		fmt.Fprintf(w, "\n::error::%d name(s) matched nothing; this profile cannot answer for them\n", len(missing))
+		return errRegression
+	}
+	if len(below) > 0 {
+		fmt.Fprintf(w, "\n::error::%d required function(s) below 100%% -- named above\n", len(below))
+		return errRegression
+	}
+	fmt.Fprintln(w, "\nall required functions at 100%")
+	return nil
+}
+
 func section(w io.Writer, title string, items []string) {
 	if len(items) == 0 {
 		return

scripts/coverage/cmd/covratchet/main_test.go

@@ -29,6 +29,60 @@ func judge(t *testing.T, profile string, extra ...string) (string, error) {
 	return out.String(), err
 }
 
+// ⛔ -require's whole point is the THIRD case below. A check built on the
+// sub-100 set alone cannot tell "this function is fully covered" from "this
+// profile has never heard of that name" -- both are absence -- so a typo, a
+// rename or a profile taken over the wrong package would read as success. That
+// is the `go test -run` failure mode: exit 0 on a selection that matched
+// nothing.
+func TestRequireTellsCoveredFromAbsent(t *testing.T) {
+	for _, tc := range []struct {
+		name    string
+		require string
+		wantBad bool
+		wantStr string
+	}{
+		{"a function at 100% passes", "baz", false, "all required functions at 100%"},
+		{"a function below 100% fails", "foo", true, "BELOW 100%"},
+		// The control this tool exists for.
+		{"a name that matches nothing FAILS", "nosuchfunc", true, "matched nothing"},
+		// One good name must not cover for one bad one: the verdict is per name.
+		{"a real name beside an absent one still fails", "baz,nosuchfunc", true, "matched nothing"},
+		{"file:name narrows to one file", "internal/vm/c.go:baz", false, "all required functions at 100%"},
+		// Right name, wrong file: this is an absence, not a pass.
+		{"file:name that points at the wrong file fails", "internal/vm/a.go:baz", true, "matched nothing"},
+	} {
+		t.Run(tc.name, func(t *testing.T) {
+			var out strings.Builder
+			err := run([]string{"-require", tc.require}, strings.NewReader(profile2), &out)
+			if bad := errors.Is(err, errRegression); bad != tc.wantBad {
+				t.Errorf("regression=%v, want %v (err=%v)\n%s", bad, tc.wantBad, err, out.String())
+			}
+			if !strings.Contains(out.String(), tc.wantStr) {
+				t.Errorf("output does not mention %q:\n%s", tc.wantStr, out.String())
+			}
+			// ⛔ The denominator is printed beside every verdict, so a run that
+			// read nothing cannot be mistaken for a clean one.
+			if !strings.Contains(out.String(), "in a profile of 3") {
+				t.Errorf("the verdict does not report what it read:\n%s", out.String())
+			}
+		})
+	}
+}
+
+// -require needs no record: it answers from the profile alone, which is what
+// makes it runnable before a push rather than only in the lane that keeps one.
+func TestRequireNeedsNoRecordButStillRefusesAnEmptyList(t *testing.T) {
+	var out strings.Builder
+	if err := run([]string{"-require", " , "}, strings.NewReader(profile2), &out); err == nil || errors.Is(err, errRegression) {
+		t.Errorf("an empty -require must be a usage refusal, got %v", err)
+	}
+	out.Reset()
+	if err := run(nil, strings.NewReader(profile2), &out); err == nil {
+		t.Error("without -record and without -require it must refuse")
+	}
+}
+
 // TestCovRatchetVerdicts: the answer must depend on the input, and in
 // particular a function newly below 100% must fail EVEN WHEN the rounded total
 // still reads 100.0% -- which is the whole defect this replaces. On main both
@@ -158,7 +212,7 @@ func TestSameFunctionTwiceIsLegalNotMalformed(t *testing.T) {
 	const prof = "github.com/go-embedded-ruby/ruby/a.go:1:\tf\t50.0%\n" +
 		"github.com/go-embedded-ruby/ruby/a.go:9:\tf\t60.0%\n" +
 		"total:\t(statements)\t100.0%\n"
-	got, err := parse(strings.NewReader(prof), "profile", "github.com/go-embedded-ruby/ruby/")
+	got, _, err := parse(strings.NewReader(prof), "profile", "github.com/go-embedded-ruby/ruby/")
 	if err != nil {
 		t.Fatalf("two same-named functions must parse, got %v", err)
 	}
@@ -261,7 +315,7 @@ github.com/x/y/internal/vm/csv.go:280:	ToS	100.0%
 github.com/x/y/internal/vm/io.go:10:	displayStr	90.0%
 total:	(statements)	99.9%
 `
-	got, err := parse(strings.NewReader(prof), "profile", "github.com/x/y")
+	got, _, err := parse(strings.NewReader(prof), "profile", "github.com/x/y")
 	if err != nil {
 		t.Fatalf("parse: %v", err)
 	}
@@ -324,11 +378,11 @@ total:	(statements)	99.9%
 github.com/x/y/a.go:120:	ToS	50.0%
 total:	(statements)	99.9%
 `
-	b, err := parse(strings.NewReader(before), "before", "github.com/x/y")
+	b, _, err := parse(strings.NewReader(before), "before", "github.com/x/y")
 	if err != nil {
 		t.Fatalf("before: %v", err)
 	}
-	a, err := parse(strings.NewReader(after), "after", "github.com/x/y")
+	a, _, err := parse(strings.NewReader(after), "after", "github.com/x/y")
 	if err != nil {
 		t.Fatalf("after: %v", err)
 	}