From 24eb0ee58e941428de5d9e88f260168051cbaf8a Mon Sep 17 00:00:00 2001 From: tannevaled Date: Sun, 1 Nov 2026 11:22:28 +0100 Subject: [PATCH 1/3] =?UTF-8?q?conformance:=20cmd/specdelta=20=E2=80=94=20?= =?UTF-8?q?hold=20the=20corpus=20still=20and=20vary=20only=20the=20binary?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `cmd/ratchet` judges one sweep against `BASELINE`. That is the right gate for *"may this land"* and the wrong instrument for *"what does this change do"*: a baseline comparison folds in every difference between the recording machine and this one, and the version move in #799 needs the other reading — hold the corpus and the machine fixed, vary only the interpreter, and name every file that moved. ```console $ specdelta -root ~/ruby-spec-corpus -dir core/set -quiet ./rbgo-main ./rbgo-409 file control treat delta core/set/flatten_merge_spec.rb 3 0 -3 core/set/to_s_spec.rb 2 4 +2 core/set/divide_spec.rb 9 8 -1 core/set/equal_value_spec.rb 4 3 -1 core/set/exclusion_spec.rb 3 2 -1 core/set/flatten_spec.rb 10 9 -1 core/set/join_spec.rb 5 4 -1 core/set/proper_superset_spec.rb 3 2 -1 core/set/superset_spec.rb 3 2 -1 57 file(s): control 175 passing, treatment 167 passing, -8 9 file(s) moved, 8 regressed ``` That table took the `Set` half of the version move from "eight files to work through" to **two defects**, which is written up on #799. ## What it refuses, and why each refusal is not a 1 Exit **2**, never 1, for: an engine that would not start, output with an unreadable `RBGO_RESULT` line, two result lines in one run, and an **empty population**. The last one matters most — a mistyped `-dir` would otherwise print `0 files, no regressions` and exit 0, which is the most convincing possible way for this tool to tell someone their change is safe. `parse` refuses a missing or non-numeric field rather than defaulting it to zero, for `cmd/ratchet`'s reason: a zero meaning "unreadable" and a zero meaning "nothing passed" lead to opposite conclusions. A file that produced no result line at all is *named* — "DID NOT RUN on the treatment" — rather than counted as zero, which is the distinction that made an endless range look like a load failure in #853. ## ⛔ The second channel, and the invariant I got wrong first The `RBGO_RESULT pass=…` line comes out of the **subject's own stdout** (#843), so a subject that miscounts can report any total it likes. The `RBGO_DETAIL` lines are an independent channel, and reading them rather than the tally is what caught a figure misattributed between two branches earlier today. My first version asserted `details == fail + error` and fired on its first real run, against `core/set/set_spec.rb` — `pass=0 fail=0 error=0 skip=1` with one `skip` detail, entirely well-formed. Reading spec_helper.rb instead of assuming its shape gives this: | kind | counter | detail line | | |---|---|---|---| | `fail` | `$RB_FAIL` | yes | :983, :987 | | `error` | `$RB_ERROR` | yes | :995, :1029, :1102 | | `skip` | `$RB_SKIP` | yes | :990 | | `skip` | `$RB_SKIP` | **no** | :970 — a version/platform guard | | `hookerror` | **none** | yes | :965, :1020 — before/after `:all` | So no equality is derivable from the four printed numbers. What holds is `details >= fail + error`, because those two kinds each push exactly one line, and a shortfall means detail lines went missing. That is what the tool checks, and the test names every row above — including the two that made equality wrong in opposite directions. The last row of that table is a defect in the harness rather than in this tool: a `before(:all)` that raises is reported in the detail lines and counted in no tally at all. Filed separately. ## Tests `parse` first and hardest, since that is where a silent wrong answer would live: seven malformed result lines that must each be an error, the no-result case that must not be, the detail-shortfall table, the empty-population refusal, the four usage refusals, and an end-to-end pass over fake `/bin/sh` engines in both directions — an improvement must exit 0 and the same pair reversed must exit 1. The engines are shell scripts, not Ruby, because the tool knows nothing about Ruby and a test needing an interpreter installed would measure the machine. ⛔ One test failed first time and the tool was right: I had written a "good" result line as `fail=3 error=1` with three detail lines, which is four expected and three given. The message it printed said "check the arithmetic", and the arithmetic to check was mine. Co-authored-by: Claude Opus 5 --- .../rubyspec/cmd/specdelta/main.go | 316 ++++++++++++++++++ .../rubyspec/cmd/specdelta/main_test.go | 223 ++++++++++++ 2 files changed, 539 insertions(+) create mode 100644 scripts/conformance/rubyspec/cmd/specdelta/main.go create mode 100644 scripts/conformance/rubyspec/cmd/specdelta/main_test.go diff --git a/scripts/conformance/rubyspec/cmd/specdelta/main.go b/scripts/conformance/rubyspec/cmd/specdelta/main.go new file mode 100644 index 00000000..6ba8b72f --- /dev/null +++ b/scripts/conformance/rubyspec/cmd/specdelta/main.go @@ -0,0 +1,316 @@ +// Copyright (c) the go-embedded-ruby/ruby authors +// +// SPDX-License-Identifier: BSD-3-Clause + +// Command specdelta runs a set of ruby/spec files through TWO interpreters and +// reports, file by file, how many examples each one passed. +// +// It answers a question `cmd/ratchet` cannot. The ratchet judges one sweep +// against `BASELINE`, which is the right gate for "may this land" but the wrong +// instrument for "what does THIS change do" — a baseline comparison folds in +// every difference between the recording machine and this one, and #799's +// version move needed the other reading: hold the corpus and the machine fixed, +// vary only the binary, and name every file that moved. +// +// specdelta -dir core/set -root ~/ruby-spec ./rbgo-main ./rbgo-branch +// +// ⛔ Two channels, not one. The `RBGO_RESULT pass=…` line comes out of the +// SUBJECT's own stdout (#843), so a subject that miscounts can report any total +// it likes. specdelta also counts the `RBGO_DETAIL` lines, which the shim emits +// one per failure and error, and flags any file where the two disagree — +// `fail+error` claimed against detail lines seen. That is not a theoretical +// concern: reading the detail lines rather than the tally is what caught a +// figure misattributed between two branches on 2026-11-01. +// +// Exit status: +// +// 0 every file ran and no file passed fewer examples on the second engine +// 1 at least one file regressed +// 2 refused: no files, an engine that would not start, or output that could +// not be read — never reported as a regression, because "could not measure" +// and "measured worse" need opposite responses +package main + +import ( + "bytes" + "errors" + "flag" + "fmt" + "io" + "os" + "os/exec" + "path/filepath" + "sort" + "strconv" + "strings" + "time" +) + +// result is one spec file's outcome under one engine. +type result struct { + pass, fail, errs, skip int + details int // RBGO_DETAIL lines seen + ran bool // a RESULT line was found at all + killed bool +} + +// tallyDisagrees reports whether the subject printed FEWER detail lines than +// its own tally of failures and errors. That is a one-sided check, and the +// asymmetry is the shim's, not a hedge: +// +// kind counter detail line +// fail $RB_FAIL yes (spec_helper.rb:983, :987) +// error $RB_ERROR yes (:995, :1029, :1102) +// skip $RB_SKIP yes (:990, :993) +// skip $RB_SKIP NO (:970 — a version/platform guard) +// hookerror NONE yes (:965, :1020 — before/after :all) +// +// So `details == fail+error` is wrong in both directions: a guard-skipped file +// has a detail with no failure, and a `before(:all)` that raises has a detail +// counted in no tally at all. `details >= fail+error` is what holds, because +// those two kinds each push exactly one line — and a shortfall means detail +// lines went missing, which makes the whole report unreadable. +// +// ⛔ The first version of this asserted equality and fired on the first real +// run, against `core/set/set_spec.rb` — which is `pass=0 fail=0 error=0 skip=1` +// with one `skip` detail, entirely well-formed. Reading the shim rather than +// assuming its shape is what produced the table above; the line +// `hookerror` has no counter is a defect in the harness, filed separately. +func (r result) tallyDisagrees() bool { + return r.ran && r.details < r.fail+r.errs +} + +func main() { + os.Exit(main1(os.Args[1:], os.Stdout, os.Stderr)) +} + +func main1(args []string, out, errOut io.Writer) int { + fs := flag.NewFlagSet("specdelta", flag.ContinueOnError) + fs.SetOutput(errOut) + root := fs.String("root", ".", "the ruby/spec checkout") + dir := fs.String("dir", "", "a directory under -root to walk (recursively), e.g. core/set") + timeout := fs.Duration("timeout", 60*time.Second, "per-file limit for one engine") + quiet := fs.Bool("quiet", false, "print only the files that moved") + if err := fs.Parse(args); err != nil { + return 2 + } + engines := fs.Args() + if len(engines) != 2 { + fmt.Fprintln(errOut, "specdelta: name exactly two interpreters — the control first, the treatment second") + return 2 + } + if *dir == "" { + fmt.Fprintln(errOut, "specdelta: -dir is required") + return 2 + } + files, err := specFiles(*root, *dir) + if err != nil { + fmt.Fprintf(errOut, "specdelta: %v\n", err) + return 2 + } + if len(files) == 0 { + // ⛔ An empty population is a refusal, not a clean result: a mistyped + // -dir would otherwise print "0 files, no regressions" and pass. + fmt.Fprintf(errOut, "specdelta: no _spec.rb files under %s\n", filepath.Join(*root, *dir)) + return 2 + } + + type row struct { + file string + a, b result + } + rows := make([]row, 0, len(files)) + refused := false + for _, f := range files { + r := row{file: f} + var errA, errB error + r.a, errA = run(engines[0], *root, f, *timeout) + r.b, errB = run(engines[1], *root, f, *timeout) + for i, e := range []error{errA, errB} { + if e != nil { + fmt.Fprintf(errOut, "specdelta: %s on %s: %v\n", engines[i], f, e) + refused = true + } + } + rows = append(rows, r) + } + if refused { + return 2 + } + + // Loudest movement first: a reader wants the biggest change, not the + // alphabetically first one. + sort.SliceStable(rows, func(i, j int) bool { + di := rows[i].b.pass - rows[i].a.pass + dj := rows[j].b.pass - rows[j].a.pass + if di != dj { + return abs(di) > abs(dj) + } + return rows[i].file < rows[j].file + }) + + var totalA, totalB, moved, regressed, suspect int + fmt.Fprintf(out, "%-56s %7s %7s %6s\n", "file", "control", "treat", "delta") + for _, r := range rows { + d := r.b.pass - r.a.pass + if d != 0 { + moved++ + } + if d < 0 { + regressed++ + } + totalA += r.a.pass + totalB += r.b.pass + note := "" + switch { + case !r.a.ran && !r.b.ran: + note = " DID NOT RUN on either" + case !r.a.ran: + note = " DID NOT RUN on the control" + case !r.b.ran: + note = " DID NOT RUN on the treatment" + case r.a.killed || r.b.killed: + note = " KILLED on timeout" + } + if r.a.tallyDisagrees() || r.b.tallyDisagrees() { + note += " ⛔ fewer detail lines than its own fail+error tally" + suspect++ + } + if *quiet && d == 0 && note == "" { + continue + } + fmt.Fprintf(out, "%-56s %7d %7d %+6d%s\n", r.file, r.a.pass, r.b.pass, d, note) + } + fmt.Fprintf(out, "\n%d file(s): control %d passing, treatment %d passing, %+d\n", + len(rows), totalA, totalB, totalB-totalA) + fmt.Fprintf(out, "%d file(s) moved, %d regressed\n", moved, regressed) + if suspect > 0 { + // A total built on a tally the subject contradicts is not a measurement. + fmt.Fprintf(errOut, "specdelta: %d file(s) reported a tally their detail lines contradict\n", suspect) + return 2 + } + if regressed > 0 { + return 1 + } + return 0 +} + +func abs(n int) int { + if n < 0 { + return -n + } + return n +} + +// specFiles lists the *_spec.rb files under root/dir, as paths relative to root +// — which is how the shim expects to be handed them. +func specFiles(root, dir string) ([]string, error) { + base := filepath.Join(root, dir) + info, err := os.Stat(base) + if err != nil { + return nil, err + } + if !info.IsDir() { + return nil, fmt.Errorf("%s is not a directory", base) + } + var out []string + err = filepath.Walk(base, func(p string, fi os.FileInfo, err error) error { + if err != nil { + return err + } + if fi.IsDir() || !strings.HasSuffix(p, "_spec.rb") { + return nil + } + rel, err := filepath.Rel(root, p) + if err != nil { + return err + } + out = append(out, rel) + return nil + }) + if err != nil { + return nil, err + } + sort.Strings(out) + return out, nil +} + +// run executes one spec file under one engine, from the corpus root so the +// shim's relative requires resolve. +func run(engine, root, file string, timeout time.Duration) (result, error) { + var r result + var out bytes.Buffer + cmd := exec.Command(engine, file) + cmd.Dir = root + cmd.Stdin = nil + cmd.Stdout = &out + cmd.Stderr = io.Discard // the shim reports on stdout; stderr is warnings + cmd.WaitDelay = time.Second + + done := make(chan error, 1) + if err := cmd.Start(); err != nil { + return r, err + } + go func() { done <- cmd.Wait() }() + select { + case err := <-done: + var ee *exec.ExitError + if err != nil && !errors.As(err, &ee) { + return r, err + } + case <-time.After(timeout): + _ = cmd.Process.Kill() + <-done + r.killed = true + } + return parse(out.String()) +} + +// parse reads the shim's report. It REFUSES a RESULT line it cannot read rather +// than defaulting a field to zero, because a zero that means "unreadable" and a +// zero that means "nothing passed" lead to opposite conclusions. +func parse(s string) (result, error) { + var r result + for _, line := range strings.Split(s, "\n") { + switch { + case strings.HasPrefix(line, "RBGO_DETAIL"): + r.details++ + case strings.HasPrefix(line, "RBGO_RESULT"): + if r.ran { + return r, fmt.Errorf("two RBGO_RESULT lines in one run") + } + fields := strings.Fields(strings.TrimPrefix(line, "RBGO_RESULT")) + seen := map[string]bool{} + for _, f := range fields { + k, v, ok := strings.Cut(f, "=") + if !ok { + return r, fmt.Errorf("unreadable field %q in %q", f, line) + } + n, err := strconv.Atoi(v) + if err != nil { + return r, fmt.Errorf("field %q is not a number in %q", f, line) + } + seen[k] = true + switch k { + case "pass": + r.pass = n + case "fail": + r.fail = n + case "error": + r.errs = n + case "skip": + r.skip = n + default: + return r, fmt.Errorf("unknown field %q in %q", k, line) + } + } + for _, k := range []string{"pass", "fail", "error", "skip"} { + if !seen[k] { + return r, fmt.Errorf("%q missing from %q", k, line) + } + } + r.ran = true + } + } + return r, nil +} diff --git a/scripts/conformance/rubyspec/cmd/specdelta/main_test.go b/scripts/conformance/rubyspec/cmd/specdelta/main_test.go new file mode 100644 index 00000000..6e2d41f8 --- /dev/null +++ b/scripts/conformance/rubyspec/cmd/specdelta/main_test.go @@ -0,0 +1,223 @@ +// Copyright (c) the go-embedded-ruby/ruby authors +// +// SPDX-License-Identifier: BSD-3-Clause + +package main + +import ( + "bytes" + "os" + "path/filepath" + "runtime" + "strings" + "testing" +) + +// parse is where a silent wrong answer would live, so it is tested first and +// hardest: every field it cannot read must be an ERROR, never a zero. +func TestParseRefusesWhatItCannotRead(t *testing.T) { + // fail=2 + error=1 is THREE detail lines; the shim emits one per failure and + // one per error, so the consistent case has to add up. + good := "RBGO_RESULT pass=12 fail=2 error=1 skip=0\n" + r, err := parse(good + "RBGO_DETAIL\tfail\tX\ta\tb\nRBGO_DETAIL\tfail\tX\tc\td\nRBGO_DETAIL\terror\tY\te\tf\n") + if err != nil { + t.Fatalf("parse(good): %v", err) + } + if r.pass != 12 || r.fail != 2 || r.errs != 1 || r.skip != 0 || !r.ran { + t.Errorf("parse(good) = %+v", r) + } + if r.details != 3 { + t.Errorf("details = %d, want 3", r.details) + } + if r.tallyDisagrees() { + t.Error("fail=2 error=1 with three detail lines must agree") + } + + // ⛔ Each of these used to be a plausible "default it to zero" case. A zero + // that means "unreadable" and a zero that means "nothing passed" lead to + // opposite conclusions, so neither may be guessed. + for _, bad := range []string{ + "RBGO_RESULT pass=12 fail=3 error=1\n", // skip missing + "RBGO_RESULT pass=12 fail=3 skip=0\n", // error missing + "RBGO_RESULT fail=3 error=1 skip=0\n", // pass missing + "RBGO_RESULT pass=twelve fail=3 error=1 skip=0", // not a number + "RBGO_RESULT pass fail=3 error=1 skip=0", // no '=' + "RBGO_RESULT pass=1 fail=1 error=1 skip=1 oops=1", + good + good, // two runs in one output + } { + if _, err := parse(bad); err == nil { + t.Errorf("parse(%q) returned no error", bad) + } + } + + // No RESULT line at all is not an error — it is a file that did not run, + // which the caller reports differently from one that ran and scored zero. + r, err = parse("some warning on stdout\n") + if err != nil { + t.Fatalf("parse(no result): %v", err) + } + if r.ran { + t.Error("a run with no RESULT line must not be marked as having run") + } +} + +// ⛔ The tally comes out of the SUBJECT's stdout, so a subject that miscounts +// can claim anything. The detail lines are the independent channel — but the +// relationship is an INEQUALITY, not an equality, and getting that wrong is how +// the first version of this check fired on a perfectly well-formed file. +// +// The shim's shape, read from spec_helper.rb rather than assumed: `fail` and +// `error` each push exactly one detail line; a guard-driven `skip` bumps the +// counter with NO detail (:970) while a runtime skip pushes one (:990); and a +// `before/after(:all)` error pushes a detail counted in NO tally (:965, :1020). +func TestDetailShortfallIsDetected(t *testing.T) { + for _, tc := range []struct { + name string + out string + flag bool + why string + }{{ + name: "a missing detail line", + out: "RBGO_RESULT pass=1 fail=2 error=0 skip=0\nRBGO_DETAIL\tfail\tX\ta\tb\n", + flag: true, + why: "fail=2 with one detail line means a line went missing", + }, { + name: "exactly as many as claimed", + out: "RBGO_RESULT pass=1 fail=1 error=0 skip=0\nRBGO_DETAIL\tfail\tX\ta\tb\n", + flag: false, + why: "fail=1 with one detail line is consistent", + }, { + // ⛔ The row the first version got wrong: core/set/set_spec.rb, skipped + // by a guard, is pass=0 fail=0 error=0 skip=1 with one `skip` detail. + name: "a skip with its detail line", + out: "RBGO_RESULT pass=0 fail=0 error=0 skip=1\nRBGO_DETAIL\tskip\tSpecSkip\tx\ty\n", + flag: false, + why: "a skip carries a detail and no failure; that is well-formed", + }, { + name: "a guard skip with no detail line", + out: "RBGO_RESULT pass=0 fail=0 error=0 skip=3\n", + flag: false, + why: "spec_helper.rb:970 bumps $RB_SKIP without pushing a detail", + }, { + name: "a hook error, counted nowhere", + out: "RBGO_RESULT pass=0 fail=0 error=0 skip=0\nRBGO_DETAIL\thookerror\tX\t(before :all)\ty\n", + flag: false, + why: "a hookerror detail belongs to no counter; MORE details is legitimate", + }} { + t.Run(tc.name, func(t *testing.T) { + r, err := parse(tc.out) + if err != nil { + t.Fatal(err) + } + if got := r.tallyDisagrees(); got != tc.flag { + t.Errorf("tallyDisagrees() = %v, want %v — %s", got, tc.flag, tc.why) + } + }) + } +} + +// An empty population is a REFUSAL. A mistyped -dir would otherwise print +// "0 files, no regressions" and exit 0, which is the most convincing way for +// this tool to tell someone their change is safe. +func TestEmptyPopulationIsRefused(t *testing.T) { + root := t.TempDir() + if err := os.MkdirAll(filepath.Join(root, "core", "empty"), 0o755); err != nil { + t.Fatal(err) + } + var out, errOut bytes.Buffer + if code := main1([]string{"-root", root, "-dir", "core/empty", "/bin/echo", "/bin/echo"}, &out, &errOut); code != 2 { + t.Errorf("exit = %d, want 2; err %q", code, errOut.String()) + } + if !strings.Contains(errOut.String(), "no _spec.rb files") { + t.Errorf("the refusal does not say why: %q", errOut.String()) + } +} + +// Usage refusals, each exit 2 — distinct from 1, which means a real regression. +func TestUsageRefusals(t *testing.T) { + root := t.TempDir() + for _, tc := range []struct { + name string + args []string + }{ + {"one engine", []string{"-root", root, "-dir", "x", "/bin/echo"}}, + {"three engines", []string{"-root", root, "-dir", "x", "/bin/echo", "/bin/echo", "/bin/echo"}}, + {"no dir", []string{"-root", root, "/bin/echo", "/bin/echo"}}, + {"absent dir", []string{"-root", root, "-dir", "nope", "/bin/echo", "/bin/echo"}}, + } { + t.Run(tc.name, func(t *testing.T) { + var out, errOut bytes.Buffer + if code := main1(tc.args, &out, &errOut); code != 2 { + t.Errorf("exit = %d, want 2; out %q err %q", code, out.String(), errOut.String()) + } + }) + } +} + +// End to end against fake engines, because the tool must not know anything +// about Ruby. The control passes 2 of 3 in one file; the treatment passes 3 — +// an improvement, exit 0 — and then the reverse, which must be exit 1. +func TestReportsMovementAndRegression(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("the fake engines are POSIX shell scripts") + } + root := t.TempDir() + if err := os.MkdirAll(filepath.Join(root, "core", "x"), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(root, "core", "x", "a_spec.rb"), []byte("#\n"), 0o644); err != nil { + t.Fatal(err) + } + two := fakeEngine(t, root, "two", "echo 'RBGO_RESULT pass=2 fail=1 error=0 skip=0'\nprintf 'RBGO_DETAIL\\tfail\\tX\\ta\\tb\\n'\n") + three := fakeEngine(t, root, "three", "echo 'RBGO_RESULT pass=3 fail=0 error=0 skip=0'\n") + + var out, errOut bytes.Buffer + if code := main1([]string{"-root", root, "-dir", "core/x", two, three}, &out, &errOut); code != 0 { + t.Errorf("improvement: exit = %d, want 0; err %q", code, errOut.String()) + } + if !strings.Contains(out.String(), "+1") { + t.Errorf("the improvement is not reported:\n%s", out.String()) + } + + out.Reset() + errOut.Reset() + if code := main1([]string{"-root", root, "-dir", "core/x", three, two}, &out, &errOut); code != 1 { + t.Errorf("regression: exit = %d, want 1; err %q", code, errOut.String()) + } + if !strings.Contains(out.String(), "-1") || !strings.Contains(out.String(), "1 regressed") { + t.Errorf("the regression is not reported:\n%s", out.String()) + } +} + +// A file that produces no RESULT line is named as such rather than counted as +// zero — the distinction the ratchet's "FAILED TO LOAD" verdict exists for, and +// the one that made an endless range look like a load failure in #853. +func TestAFileThatDidNotRunIsNamed(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("the fake engines are POSIX shell scripts") + } + root := t.TempDir() + if err := os.MkdirAll(filepath.Join(root, "core", "y"), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(root, "core", "y", "b_spec.rb"), []byte("#\n"), 0o644); err != nil { + t.Fatal(err) + } + ok := fakeEngine(t, root, "ok", "echo 'RBGO_RESULT pass=1 fail=0 error=0 skip=0'\n") + mute := fakeEngine(t, root, "mute", "exit 1\n") + + var out, errOut bytes.Buffer + main1([]string{"-root", root, "-dir", "core/y", ok, mute}, &out, &errOut) + if !strings.Contains(out.String(), "DID NOT RUN on the treatment") { + t.Errorf("a silent engine is not named:\n%s", out.String()) + } +} + +func fakeEngine(t *testing.T, dir, name, body string) string { + t.Helper() + p := filepath.Join(dir, name) + if err := os.WriteFile(p, []byte("#!/bin/sh\n"+body), 0o755); err != nil { + t.Fatal(err) + } + return p +} From aef13a015f32d92ae6494f65668ab94b045778c3 Mon Sep 17 00:00:00 2001 From: tannevaled Date: Sun, 1 Nov 2026 11:54:47 +0100 Subject: [PATCH 2/3] specdelta: one run per engine cannot tell a flake from a difference MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The tool's first real use reported ``` core/random/bytes_spec.rb 9 8 -1 ``` between two binaries that differ by **one file and three constants** — a version string. Three runs of each said why: | | run 1 | run 2 | run 3 | |---|---|---|---| | control | 9 | 9 | **8** | | treatment | **8** | 9 | 9 | A coin flip, attributed to the change. `core/random/bytes_spec.rb` carries no version guard at all, which is what made the row suspicious enough to re-run — but a row that *did* sit behind a guard would have looked explained and gone into a report. ## `-repeat N` Above 1, each file runs N times per engine and the tool keeps the **lowest** pass count — not the mean and not the last, because the question a reader asks of this table is "did anything get worse" and the answer must not rest on a lucky run. An engine that disagrees with itself marks the file `UNSTABLE`, and its delta is **excluded from the verdict**, so the exit status stops depending on which way the coin landed: ```console $ specdelta -dir core/random -repeat 3 ./rbgo-main ./rbgo-409 core/random/bytes_spec.rb 8 8 +0 ⛔ UNSTABLE: an engine disagreed with itself, delta excluded ... 10 file(s): control 65 passing, treatment 65 passing, +0 0 file(s) moved, 0 regressed ``` The default stays 1, because three passes over `core/` is three times the twenty-odd minutes — and the flag's help says plainly that the regression verdict is only trustworthy above 1. `-repeat 0` is a usage refusal rather than a silent no-op. ## Tests The instability test asserts **both** modes, so the difference between them is visible rather than claimed: with `-repeat 1` the flake reads as a regression and exits 1; with `-repeat 3` the file is named `UNSTABLE`, counted as `0 regressed`, and exits 0. Its fake engine scores 8 the first time it is asked and 9 after, which is the direction that looks like a regression on the first run. Two controls beside it: a stable engine must not be called unstable, and a stable pair must stay at exit 0. Also pinned, because `abs` measured 0% and the ordering is documented behaviour: the loudest movement prints first. It needs two files — with one, the sort comparator is never called — and they are named so that alphabetical order and movement order disagree. Co-authored-by: Claude Opus 5 --- .../rubyspec/cmd/specdelta/main.go | 70 ++++++++-- .../rubyspec/cmd/specdelta/main_test.go | 120 ++++++++++++++++++ 2 files changed, 181 insertions(+), 9 deletions(-) diff --git a/scripts/conformance/rubyspec/cmd/specdelta/main.go b/scripts/conformance/rubyspec/cmd/specdelta/main.go index 6ba8b72f..45222648 100644 --- a/scripts/conformance/rubyspec/cmd/specdelta/main.go +++ b/scripts/conformance/rubyspec/cmd/specdelta/main.go @@ -91,6 +91,13 @@ func main1(args []string, out, errOut io.Writer) int { dir := fs.String("dir", "", "a directory under -root to walk (recursively), e.g. core/set") timeout := fs.Duration("timeout", 60*time.Second, "per-file limit for one engine") quiet := fs.Bool("quiet", false, "print only the files that moved") + // ⛔ One run per engine cannot tell a FLAKE from a difference, and the first + // real use of this tool reported `core/random/bytes_spec.rb` as -1 between two + // binaries that differ only in a version string. Three runs of each: control + // 9, 9, 8 and treatment 8, 9, 9 — a coin flip, attributed to the change. + // With -repeat above 1 a file whose own engine disagrees with itself is named + // UNSTABLE and kept out of the regression verdict. + repeat := fs.Int("repeat", 1, "run each file this many times per engine; above 1, detect flaky files") if err := fs.Parse(args); err != nil { return 2 } @@ -115,21 +122,33 @@ func main1(args []string, out, errOut io.Writer) int { return 2 } + if *repeat < 1 { + fmt.Fprintln(errOut, "specdelta: -repeat must be at least 1") + return 2 + } type row struct { - file string - a, b result + file string + a, b result // the LOWEST pass count seen, so a verdict is never built on a lucky run + unstable bool // an engine disagreed with itself across repeats } rows := make([]row, 0, len(files)) refused := false for _, f := range files { r := row{file: f} - var errA, errB error - r.a, errA = run(engines[0], *root, f, *timeout) - r.b, errB = run(engines[1], *root, f, *timeout) - for i, e := range []error{errA, errB} { - if e != nil { - fmt.Fprintf(errOut, "specdelta: %s on %s: %v\n", engines[i], f, e) + for i, engine := range engines { + lowest, varied, err := runRepeatedly(engine, *root, f, *timeout, *repeat) + if err != nil { + fmt.Fprintf(errOut, "specdelta: %s on %s: %v\n", engines[i], f, err) refused = true + continue + } + if varied { + r.unstable = true + } + if i == 0 { + r.a = lowest + } else { + r.b = lowest } } rows = append(rows, r) @@ -156,7 +175,9 @@ func main1(args []string, out, errOut io.Writer) int { if d != 0 { moved++ } - if d < 0 { + // ⛔ An unstable file is kept OUT of the verdict: its delta is noise, and + // counting it would make the exit status depend on a coin flip. + if d < 0 && !r.unstable { regressed++ } totalA += r.a.pass @@ -172,6 +193,9 @@ func main1(args []string, out, errOut io.Writer) int { case r.a.killed || r.b.killed: note = " KILLED on timeout" } + if r.unstable { + note += " ⛔ UNSTABLE: an engine disagreed with itself, delta excluded" + } if r.a.tallyDisagrees() || r.b.tallyDisagrees() { note += " ⛔ fewer detail lines than its own fail+error tally" suspect++ @@ -235,6 +259,34 @@ func specFiles(root, dir string) ([]string, error) { return out, nil } +// runRepeatedly runs one file under one engine `times` over, and returns the +// LOWEST-scoring result plus whether the engine disagreed with itself. +// +// The lowest rather than the mean or the last, because the question a reader +// asks of this table is "did anything get worse", and the answer must not rest +// on a lucky run. With times == 1 `varied` is always false, which is why the +// regression verdict is only trustworthy above 1 — said in the flag's help. +func runRepeatedly(engine, root, file string, timeout time.Duration, times int) (result, bool, error) { + var lowest result + varied := false + for i := 0; i < times; i++ { + r, err := run(engine, root, file, timeout) + if err != nil { + return lowest, false, err + } + switch { + case i == 0: + lowest = r + case r.pass != lowest.pass || r.ran != lowest.ran: + varied = true + if r.pass < lowest.pass { + lowest = r + } + } + } + return lowest, varied, nil +} + // run executes one spec file under one engine, from the corpus root so the // shim's relative requires resolve. func run(engine, root, file string, timeout time.Duration) (result, error) { diff --git a/scripts/conformance/rubyspec/cmd/specdelta/main_test.go b/scripts/conformance/rubyspec/cmd/specdelta/main_test.go index 6e2d41f8..5aefe3f7 100644 --- a/scripts/conformance/rubyspec/cmd/specdelta/main_test.go +++ b/scripts/conformance/rubyspec/cmd/specdelta/main_test.go @@ -189,6 +189,53 @@ func TestReportsMovementAndRegression(t *testing.T) { } } +// The loudest movement prints first, whichever direction it points, because a +// reader wants the biggest change and not the alphabetically first one. With a +// single file the comparator is never called, so this needs two — and it is the +// documented ordering, which is reason enough to pin it. +func TestTheBiggestMovementIsReportedFirst(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("the fake engines are POSIX shell scripts") + } + root := t.TempDir() + if err := os.MkdirAll(filepath.Join(root, "core", "w"), 0o755); err != nil { + t.Fatal(err) + } + // `zz` moves by one and `aa` by five, so alphabetical order and movement + // order disagree — which is what makes the assertion mean anything. + for name, body := range map[string]string{ + "aa_spec.rb": "#\n", + "zz_spec.rb": "#\n", + } { + if err := os.WriteFile(filepath.Join(root, "core", "w", name), []byte(body), 0o644); err != nil { + t.Fatal(err) + } + } + // One engine scores by file name; the other adds 5 to aa and 1 to zz. + low := fakeEngine(t, root, "low", `case "$1" in + *aa_spec.rb) echo 'RBGO_RESULT pass=10 fail=0 error=0 skip=0' ;; + *) echo 'RBGO_RESULT pass=10 fail=0 error=0 skip=0' ;; +esac +`) + high := fakeEngine(t, root, "high", `case "$1" in + *aa_spec.rb) echo 'RBGO_RESULT pass=15 fail=0 error=0 skip=0' ;; + *) echo 'RBGO_RESULT pass=11 fail=0 error=0 skip=0' ;; +esac +`) + var out, errOut bytes.Buffer + if code := main1([]string{"-root", root, "-dir", "core/w", low, high}, &out, &errOut); code != 0 { + t.Fatalf("exit = %d, want 0; err %q", code, errOut.String()) + } + iAA := strings.Index(out.String(), "aa_spec.rb") + iZZ := strings.Index(out.String(), "zz_spec.rb") + if iAA < 0 || iZZ < 0 { + t.Fatalf("both files must appear:\n%s", out.String()) + } + if iAA > iZZ { + t.Errorf("+5 must print before +1:\n%s", out.String()) + } +} + // A file that produces no RESULT line is named as such rather than counted as // zero — the distinction the ratchet's "FAILED TO LOAD" verdict exists for, and // the one that made an endless range look like a load failure in #853. @@ -213,6 +260,79 @@ func TestAFileThatDidNotRunIsNamed(t *testing.T) { } } +// ⛔ A single run per engine cannot tell a FLAKE from a difference. The first +// real use of this tool reported `core/random/bytes_spec.rb` as -1 between two +// binaries differing only in a version string; three runs of each gave control +// 9, 9, 8 and treatment 8, 9, 9 — a coin flip, attributed to the change. +// +// So above `-repeat 1` an engine that disagrees with ITSELF marks the file +// UNSTABLE, its delta is excluded from the verdict, and the exit status stops +// depending on which way the coin landed. +func TestAnUnstableFileIsExcludedFromTheVerdict(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("the fake engines are POSIX shell scripts") + } + root := t.TempDir() + if err := os.MkdirAll(filepath.Join(root, "core", "z"), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(root, "core", "z", "c_spec.rb"), []byte("#\n"), 0o644); err != nil { + t.Fatal(err) + } + stable := fakeEngine(t, root, "stable", "echo 'RBGO_RESULT pass=9 fail=0 error=0 skip=0'\n") + // Scores 8 the first time it is asked and 9 after: a flake, in the direction + // that looks like a regression on the first run. + flaky := fakeEngine(t, root, "flaky", `if [ -f "`+root+`/seen" ]; then + echo 'RBGO_RESULT pass=9 fail=0 error=0 skip=0' +else + : > "`+root+`/seen" + echo 'RBGO_RESULT pass=8 fail=1 error=0 skip=0' + printf 'RBGO_DETAIL\tfail\tX\ta\tb\n' +fi +`) + + // With one run the flake reads as a regression — which is the defect this + // flag exists for, asserted so the difference between the two modes is + // visible rather than claimed. + var out, errOut bytes.Buffer + if code := main1([]string{"-root", root, "-dir", "core/z", stable, flaky}, &out, &errOut); code != 1 { + t.Errorf("-repeat 1: exit = %d, want 1 (the flake reads as a regression); err %q", code, errOut.String()) + } + + // With three, the engine contradicts itself and the file is set aside. + if err := os.Remove(filepath.Join(root, "seen")); err != nil { + t.Fatal(err) + } + out.Reset() + errOut.Reset() + if code := main1([]string{"-root", root, "-dir", "core/z", "-repeat", "3", stable, flaky}, &out, &errOut); code != 0 { + t.Errorf("-repeat 3: exit = %d, want 0 (unstable, excluded); out %q", code, out.String()) + } + if !strings.Contains(out.String(), "UNSTABLE") { + t.Errorf("the unstable file is not named:\n%s", out.String()) + } + if !strings.Contains(out.String(), "0 regressed") { + t.Errorf("an excluded delta still counted as a regression:\n%s", out.String()) + } + + // A stable engine must NOT be called unstable, or every file would be. + out.Reset() + errOut.Reset() + if code := main1([]string{"-root", root, "-dir", "core/z", "-repeat", "3", stable, stable}, &out, &errOut); code != 0 { + t.Errorf("stable pair: exit = %d, want 0; out %q", code, out.String()) + } + if strings.Contains(out.String(), "UNSTABLE") { + t.Errorf("a stable engine was called unstable:\n%s", out.String()) + } + + // And -repeat below 1 is a usage refusal, not a silent no-op. + out.Reset() + errOut.Reset() + if code := main1([]string{"-root", root, "-dir", "core/z", "-repeat", "0", stable, stable}, &out, &errOut); code != 2 { + t.Errorf("-repeat 0: exit = %d, want 2; err %q", code, errOut.String()) + } +} + func fakeEngine(t *testing.T, dir, name, body string) string { t.Helper() p := filepath.Join(dir, name) From 802626944d699383723d32b84a91659db67ad716 Mon Sep 17 00:00:00 2001 From: tannevaled Date: Mon, 2 Nov 2026 11:05:56 +0100 Subject: [PATCH 3/3] =?UTF-8?q?conformance:=20README=20=E2=80=94=20what=20?= =?UTF-8?q?cmd/specdelta=20is=20for?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A section beside `cmd/oracle`'s, with the `core/set` table as the worked example and the three refusals spelled out: the flake detection, the two-channel check and the empty-population exit 2. The `Set` row is the point of the tool in one line — it took that half of the 4.0 version move from "eight files to work through" to **two defects**, because six of the eight are examples 4.0 stopped asking for and a delta cannot say so. Only the `ruby_version_is` guards can, which is why the section says to read them before reading the numbers. Co-authored-by: Claude Opus 5 --- scripts/conformance/rubyspec/README.md | 41 ++++++++++++++++++++++++++ 1 file changed, 41 insertions(+) diff --git a/scripts/conformance/rubyspec/README.md b/scripts/conformance/rubyspec/README.md index 14ed22ef..952f2827 100644 --- a/scripts/conformance/rubyspec/README.md +++ b/scripts/conformance/rubyspec/README.md @@ -212,6 +212,47 @@ Three engines is the shape the measurement rules want: reference, control (`main`), treatment (the branch). A difference between the last two that the reference does not share is the defect, or the fix. +## Holding the corpus still — `cmd/specdelta` + +`cmd/ratchet` judges one sweep against `BASELINE`. That is the right gate for +*"may this land"* and the wrong instrument for *"what does this change do"*: a +baseline comparison folds in every difference between the recording machine and +this one. `cmd/specdelta` asks the other question — hold the corpus and the +machine fixed, vary only the interpreter, and name every file that moved. + +```console +$ specdelta -root ~/ruby-spec-corpus -dir core/set -quiet -repeat 3 ./rbgo-main ./rbgo-409 +file control treat delta +core/set/flatten_merge_spec.rb 3 0 -3 +core/set/to_s_spec.rb 2 4 +2 +core/set/divide_spec.rb 9 8 -1 +... +57 file(s): control 175 passing, treatment 167 passing, -8 +9 file(s) moved, 8 regressed +``` + +That table took the `Set` half of the 4.0 version move from "eight files to work +through" to **two defects** — the other six are examples 4.0 stopped asking for, +which the deltas cannot tell you and the `ruby_version_is` guards can. + +**`-repeat N`.** One run per engine cannot tell a FLAKE from a difference. The +tool's first real use reported `core/random/bytes_spec.rb` as `-1` between two +binaries that differ by one file and three constants; three runs of each gave +control 9, 9, 8 and treatment 8, 9, 9. Above 1, each file runs N times per +engine, the **lowest** count is kept, and an engine that disagrees with itself +marks the file `UNSTABLE` and has its delta excluded from the verdict. + +**Two channels.** `RBGO_RESULT pass=…` comes out of the subject's own stdout, so +a subject that miscounts can claim anything. specdelta also counts the +`RBGO_DETAIL` lines and flags a file that printed **fewer** than its own +`fail + error` tally. It is an inequality and not an equality, because the shim +emits a detail with no counter for a `before(:all)` failure and a counter with +no detail for a guard-driven skip. + +Exit **2**, never 1, for a refusal: an engine that would not start, an +unreadable result line, two result lines in one run, or an **empty population** +— a mistyped `-dir` would otherwise print `0 files, no regressions` and exit 0. + ## Caveats The shim is not full MSpec: a few matchers (`complain`, `output`,