diff --git a/scripts/conformance/rubyspec/README.md b/scripts/conformance/rubyspec/README.md index e271e206..a09f2067 100644 --- a/scripts/conformance/rubyspec/README.md +++ b/scripts/conformance/rubyspec/README.md @@ -80,6 +80,53 @@ and be recorded as not loading, which bakes the load into the gate. The CI job (`.github/workflows/rubyspec-ratchet.yml`) enforces the baseline on every push and pull request, and raises `TIMEOUT` to 60s for exactly this reason. +### ⛔ A re-record reads the old baseline first, and refuses to bank a loss + +`-update` used to write straight over `BASELINE` **without reading it**, so the +one thing it could never report was whether the sweep replacing the record was +better or worse than it. The comparison simply never happened. + +The hazard was written down twice in this repository and gated in neither — +`cmd/guards`' own header (*"`UPDATE_BASELINE=1` records whatever it sees and +would absorb a loss"*) and the ratchet lane's comment about an Ubuntu image +roll: *"the same run also showed `kernel/require` +1, equally environmental, and +`UPDATE_BASELINE=1` would bank it as progress."* Two notes, no refusal. + +You re-record to lock in a **win**. A file that went backwards is either a real +regression or an environment that moved, and both need a person — not an +overwrite that erases the evidence of itself: + +```console +$ UPDATE_BASELINE=1 scripts/conformance/rubyspec/run.sh +re-record: 2206 files measured, 2206 in the baseline being replaced + +-- REGRESSED (1) -- + core/a_spec.rb: 9 passing, baseline 10 (-1) + +::error::REFUSING to re-record: 1 file(s) went backwards -- named above +A re-record locks in a win. Fix the regression, or pass -absorb to record +it on purpose -- the baseline then carries a line naming every file absorbed. +``` + +It exits **1**, distinct in the source from the plain regression verdict +although they share the status: *"this branch broke something"* and *"I declined +to erase the evidence that something is broken"* send a reader to different +places. + +`-absorb` is the named derogation and it **stamps the artefact**, not just the +log — a console warning scrolls away, and the baseline outlives the run that +wrote it by months: + +```text +# ⛔ ABSORBED 1 file(s) that went backwards, recorded with -absorb on 2026-11-03: +# core/a_spec.rb: 9 passing, baseline 10 (-1) +``` + +A refused re-record leaves `BASELINE` **byte for byte as it was**, which is +asserted: a refusal that truncated the file would destroy the record it declined +to replace. A first recording, with no baseline to be worse than, is not a +refusal at all. + ## `cmd/phantom` — is a pass the behaviour, or the method's absence? A spec that asserts an exception class cannot tell why it got one. diff --git a/scripts/conformance/rubyspec/cmd/ratchet/main.go b/scripts/conformance/rubyspec/cmd/ratchet/main.go index 05350f11..725dc556 100644 --- a/scripts/conformance/rubyspec/cmd/ratchet/main.go +++ b/scripts/conformance/rubyspec/cmd/ratchet/main.go @@ -33,6 +33,7 @@ import ( "sort" "strconv" "strings" + "time" ) // state is what the baseline records for one file, and what a sweep measures. @@ -183,12 +184,21 @@ func section(w io.Writer, title string, items []string, limit int) { // verdict is reachable from a test. var errRegression = errors.New("regression") +// errWouldAbsorb is the re-record refusing to overwrite a better baseline with +// a worse sweep. It is kept DISTINCT from errRegression although both exit 1: +// one says "this branch broke something", the other says "I declined to erase +// the evidence that something is broken", and a reader who confuses them looks +// in the wrong place. +var errWouldAbsorb = errors.New("would absorb a regression") + func main() { err := run(os.Args[1:], os.Stdin, os.Stdout) switch { case err == nil: return - case errors.Is(err, errRegression): + case errors.Is(err, errRegression), errors.Is(err, errWouldAbsorb): + // Both are answers, not malfunctions: exit 1, never the 2 that means + // the tool could not read its input. os.Exit(1) default: fmt.Fprintf(os.Stderr, "ratchet: %v\n", err) @@ -202,6 +212,7 @@ func run(args []string, stdin io.Reader, stdout io.Writer) error { baseline := fs.String("baseline", "", "per-file baseline TSV (required)") results := fs.String("results", "", "sweep results TSV (default: stdin)") update := fs.Bool("update", false, "rewrite the baseline from this sweep and exit 0") + absorb := fs.Bool("absorb", false, "with -update, re-record even though files went backwards; the baseline is stamped with their names") limit := fs.Int("limit", 40, "cap each printed section at N entries (0 = no cap)") if err := fs.Parse(args); err != nil { return err @@ -225,9 +236,67 @@ func run(args []string, stdin io.Reader, stdout io.Writer) error { } if *update { - const header = "# ruby/spec per-file baseline. Regenerate with UPDATE_BASELINE=1 scripts/conformance/rubyspec/run.sh.\n" + + header := "# ruby/spec per-file baseline. Regenerate with UPDATE_BASELINE=1 scripts/conformance/rubyspec/run.sh.\n" + "# One record per spec file: OKpathpassing, or FILEFAILpath.\n" + "# Judged per file, so no margin is needed and every move is attributable.\n" + + // ⛔ A RE-RECORD READS THE OLD BASELINE FIRST. It used to write straight + // over it, so the one thing it could never report is the one thing that + // matters: -update had no idea whether this sweep was better or worse + // than the record it was replacing. The comparison simply never happened. + // + // The hazard was already written down, in this repository, twice — + // cmd/guards' header ("UPDATE_BASELINE=1 records whatever it sees and + // would absorb a loss") and the ratchet lane's own comment about an + // Ubuntu image roll: "the same run also showed kernel/require +1, + // equally environmental, and UPDATE_BASELINE=1 would bank it as + // progress." A note in two places and no refusal in either. + // + // You re-record to lock in a WIN. A file that went backwards is either a + // real regression or an environment that moved, and both need a person, + // not an overwrite that erases the evidence of itself. + var base map[string]state + if bf, err := os.Open(*baseline); err == nil { + base, err = parse(bf, "baseline") + bf.Close() + if err != nil { + return err + } + } else if !os.IsNotExist(err) { + return err + } + // No baseline yet is the first recording, and has nothing to go + // backwards from — distinct from a baseline that exists and is worse. + if base != nil { + v := compare(base, got) + fmt.Fprintf(stdout, "re-record: %d files measured, %d in the baseline being replaced\n", len(got), len(base)) + section(stdout, "REGRESSED", v.regressed, *limit) + section(stdout, "FAILED TO LOAD (the baseline says they load)", v.unloaded, *limit) + section(stdout, "MISSING FROM THE SWEEP ENTIRELY", v.missing, *limit) + section(stdout, "improved", v.improved, *limit) + section(stdout, "now loading", v.nowLoading, *limit) + section(stdout, "new files, not in the baseline", v.added, *limit) + if v.bad() { + n := len(v.regressed) + len(v.unloaded) + len(v.missing) + if !*absorb { + fmt.Fprintf(stdout, "\n::error::REFUSING to re-record: %d file(s) went backwards -- named above\n", n) + fmt.Fprintln(stdout, "A re-record locks in a win. Fix the regression, or pass -absorb to record") + fmt.Fprintln(stdout, "it on purpose -- the baseline then carries a line naming every file absorbed.") + return errWouldAbsorb + } + // ⛔ The derogation STAMPS THE ARTEFACT, not just the log. A + // console warning scrolls away and the baseline outlives it by + // months; the next reader has to be able to see the loss in the + // file that was written, without having the run that wrote it. + header += fmt.Sprintf("# ⛔ ABSORBED %d file(s) that went backwards, recorded with -absorb on %s:\n", + n, time.Now().UTC().Format("2006-01-02")) + for _, s := range append(append(append([]string{}, v.regressed...), v.unloaded...), v.missing...) { + header += "# " + s + "\n" + } + fmt.Fprintf(stdout, "\n⛔ -absorb: recording %d file(s) that went backwards; the baseline names them.\n", n) + } + } + f, err := os.Create(*baseline) if err != nil { return err diff --git a/scripts/conformance/rubyspec/cmd/ratchet/main_test.go b/scripts/conformance/rubyspec/cmd/ratchet/main_test.go index e5e13d86..a04a1173 100644 --- a/scripts/conformance/rubyspec/cmd/ratchet/main_test.go +++ b/scripts/conformance/rubyspec/cmd/ratchet/main_test.go @@ -166,6 +166,126 @@ func TestRatchetUpdateRoundTrips(t *testing.T) { } } +// ⛔ -update used to write straight over the baseline WITHOUT READING IT, so the +// one thing it could never report was whether the sweep replacing the record was +// better or worse than it. The hazard was written down twice in this repository +// — cmd/guards' header, and the ratchet lane's comment about an Ubuntu image +// roll ("UPDATE_BASELINE=1 would bank it as progress") — and gated in neither. +// +// recordOver re-records `sweep` over the three-file baseline and returns the +// tool's output, its error, and what the baseline file holds afterwards. +func recordOver(t *testing.T, sweep string, extra ...string) (string, error, string) { + t.Helper() + dir := t.TempDir() + bp := filepath.Join(dir, "BASELINE") + if err := os.WriteFile(bp, []byte(baseline3), 0o644); err != nil { + t.Fatal(err) + } + var out strings.Builder + err := run(append([]string{"-baseline", bp, "-update"}, extra...), strings.NewReader(sweep), &out) + after, rerr := os.ReadFile(bp) + if rerr != nil { + t.Fatal(rerr) + } + return out.String(), err, string(after) +} + +func TestARerecordWillNotSilentlyBankALoss(t *testing.T) { + // a_spec goes 10 → 9 while b_spec gains: a NET of zero, which is exactly the + // shape no total can see. + lost := "OK\tcore/a_spec.rb\t9\nOK\tcore/b_spec.rb\t21\nFILEFAIL\tcore/c_spec.rb\n" + + out, err, after := recordOver(t, lost) + if !errors.Is(err, errWouldAbsorb) { + t.Fatalf("err = %v, want errWouldAbsorb; out:\n%s", err, out) + } + if !errors.Is(err, errWouldAbsorb) || errors.Is(err, errRegression) { + t.Errorf("the refusal must be distinct from a plain regression verdict") + } + if !strings.Contains(out, "core/a_spec.rb: 9 passing, baseline 10 (-1)") { + t.Errorf("the refusal does not name the file that went backwards:\n%s", out) + } + // ⛔ And it must not have half-written. A refusal that truncated the file + // would destroy the very record it declined to replace. + if after != baseline3 { + t.Errorf("the refused re-record changed the baseline:\n%q", after) + } + + // -absorb is the named derogation, and it STAMPS THE ARTEFACT: the console + // scrolls away, the baseline outlives the run by months. + out, err, after = recordOver(t, lost, "-absorb") + if err != nil { + t.Fatalf("-absorb: %v\n%s", err, out) + } + if !strings.Contains(after, "ABSORBED 1 file(s)") || !strings.Contains(after, "core/a_spec.rb: 9 passing, baseline 10 (-1)") { + t.Errorf("-absorb did not stamp the baseline with what it absorbed:\n%s", after) + } + if !strings.Contains(after, "OK\tcore/a_spec.rb\t9\n") { + t.Errorf("-absorb did not actually record the sweep:\n%s", after) + } + + // The CONTROL, without which the above only proves the gate always fires: + // a re-record that merely locks in a win is accepted, and is NOT stamped. + won := "OK\tcore/a_spec.rb\t12\nOK\tcore/b_spec.rb\t20\nFILEFAIL\tcore/c_spec.rb\n" + out, err, after = recordOver(t, won) + if err != nil { + t.Fatalf("a winning re-record was refused: %v\n%s", err, out) + } + if strings.Contains(after, "ABSORBED") { + t.Errorf("a clean re-record was stamped:\n%s", after) + } + if !strings.Contains(out, "improved (1)") { + t.Errorf("the re-record does not report what it locked in:\n%s", out) + } +} + +// The other two ways a file goes backwards are the same refusal, and a first +// recording -- with no baseline to be worse than -- is not one at all. +func TestTheRerecordRefusalCoversEveryWayBackwards(t *testing.T) { + for _, tc := range []struct { + name, sweep, want string + }{ + { + "a file stops loading", + "OK\tcore/a_spec.rb\t10\nFILEFAIL\tcore/b_spec.rb\nFILEFAIL\tcore/c_spec.rb\n", + "core/b_spec.rb", + }, + { + "a file vanishes from the sweep", + "OK\tcore/a_spec.rb\t10\nFILEFAIL\tcore/c_spec.rb\n", + "core/b_spec.rb", + }, + } { + t.Run(tc.name, func(t *testing.T) { + out, err, after := recordOver(t, tc.sweep) + if !errors.Is(err, errWouldAbsorb) { + t.Fatalf("err = %v, want errWouldAbsorb; out:\n%s", err, out) + } + if !strings.Contains(out, tc.want) { + t.Errorf("the refusal does not name %s:\n%s", tc.want, out) + } + if after != baseline3 { + t.Errorf("the refused re-record changed the baseline") + } + }) + } + + t.Run("the first recording has nothing to go backwards from", func(t *testing.T) { + bp := filepath.Join(t.TempDir(), "BASELINE") + var out strings.Builder + if err := run([]string{"-baseline", bp, "-update"}, strings.NewReader(baseline3), &out); err != nil { + t.Fatalf("a first recording was refused: %v\n%s", err, out.String()) + } + got, err := os.ReadFile(bp) + if err != nil { + t.Fatal(err) + } + if !strings.Contains(string(got), "OK\tcore/a_spec.rb\t10\n") { + t.Errorf("the first recording did not write the sweep:\n%s", got) + } + }) +} + // TestRatchetMissingBaselineIsAnError: pointing at a baseline that is not there // must fail loudly. A tool that treats an absent baseline as "nothing to beat" // would pass forever.