diff --git a/scripts/coverage/README.md b/scripts/coverage/README.md new file mode 100644 index 00000000..91c2f2c1 --- /dev/null +++ b/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.`, 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. +``` + +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. diff --git a/scripts/coverage/cmd/covratchet/main.go b/scripts/coverage/cmd/covratchet/main.go index 568e1784..14c570db 100644 --- a/scripts/coverage/cmd/covratchet/main.go +++ b/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,18 +109,18 @@ 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 @@ -123,22 +128,23 @@ func parse(r io.Reader, what, mod string) (map[fn]entry, error) { 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 "/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 diff --git a/scripts/coverage/cmd/covratchet/main_test.go b/scripts/coverage/cmd/covratchet/main_test.go index f726be28..41ffbe62 100644 --- a/scripts/coverage/cmd/covratchet/main_test.go +++ b/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) }