From 43600e7a4750662154667eaa194d9600a2e59063 Mon Sep 17 00:00:00 2001 From: tannevaled Date: Sun, 11 Oct 2026 17:51:45 +0200 Subject: [PATCH] test: no test may skip on a failure to build, and a guard says so #687 named two sites that turned a build failure into a PASS: if out, err := build.CombinedOutput(); err != nil { t.Skipf("cannot build cmd/rbgo (%v): %s", err, out) } If cmd/rbgo stopped compiling, both reported SKIP, the lane stayed green, and the binary-level HTTP and socket coverage silently stopped being exercised. `t.Skipf` there says "this machine cannot build it"; what it reports is "the product is broken", and only one of those may be green. Both are already t.Fatalf -- measured, not assumed. So this commit adds what closes the CLASS rather than the two lines: a walk over every *_test.go that reads each t.Skip/Skipf/SkipNow's REASON and refuses a vocabulary which can only mean the code did not compile. It judges the reason and not the surrounding code, deliberately. A skip is legitimate when what is missing belongs to the MACHINE -- no symlinks, no /dev/zero, not root -- and never when what is missing is this repository's own build, because every lane builds it and a developer who cannot wants to be told. Inferring that distinction from the call site would be a judgement call dressed up as a check. Three things it does to avoid being decorative: WORD BOUNDARIES, not substrings. The first version used strings.Contains and reported cmd/rbgo/build_test.go's "set RBGO_BUILD_IT=1 to run the rbgo build integration test" as an offence, because "go build" is a substring of "rb|go build". That is the third substring-blindness of this kind in two days. ITS OWN CONTROL runs before it judges anything: seven reasons, three that must match and four that must not, including that false positive. A vocabulary nobody tested is a guess, and a guess that matches nothing looks exactly like a clean tree. IT COUNTS WHAT IT READ and fails at zero. 59 skips examined, 0 offences. A walk that parsed nothing finds no offence either. Proved in the other direction too: replanting the exact shape #687 described, at the site it described, turns it red and names the line -- `internal/vm/socket_test.go:433:3: skips on a build failure`. Also from #687's inventory: grape_internal_test.go's `if r.handler != nil { t.Skip }` could not fire, since handler is set to nil on the line above. It is an assertion now, so a change that did leave a handler there goes red rather than quiet. The two "gap recorded" skips in scripts/conformance/webapp are left alone on purpose: they record a known missing binding with a long diagnostic, and turning them red would make the lane fail for a gap nobody is hiding. Co-Authored-By: Claude Opus 5 --- internal/vm/grape_internal_test.go | 10 +- skip_vocabulary_test.go | 172 +++++++++++++++++++++++++++++ 2 files changed, 180 insertions(+), 2 deletions(-) create mode 100644 skip_vocabulary_test.go diff --git a/internal/vm/grape_internal_test.go b/internal/vm/grape_internal_test.go index dc6be7af..77b1cfc0 100644 --- a/internal/vm/grape_internal_test.go +++ b/internal/vm/grape_internal_test.go @@ -166,9 +166,15 @@ func (errStub) Error() string { return "boom" } // Match#handler / Match#route where a matched route carries no handler value. func TestGrapeRouteHandlerNil(t *testing.T) { r := &GrapeRoute{rt: grape.NewRoute("GET", "/x", nil), handler: nil} - // handler is nil -> the accessor returns Ruby nil, not a Go nil interface. + // Assert it, rather than skipping when it is not so. This read + // + // if r.handler != nil { t.Skip("handler set") } + // + // which cannot fire -- handler is set to nil on the line above -- so the + // test read as conditional when it is not, and a future change that DID + // leave a handler here would have gone quiet instead of red (#687). if r.handler != nil { - t.Skip("handler set") + t.Fatalf("handler = %v, want nil: the literal above sets it to nil", r.handler) } m := &GrapeMatch{} // no route, no handler if m.route != nil || m.handler != nil { diff --git a/skip_vocabulary_test.go b/skip_vocabulary_test.go new file mode 100644 index 00000000..cfb9fa8d --- /dev/null +++ b/skip_vocabulary_test.go @@ -0,0 +1,172 @@ +// Copyright (c) the go-embedded-ruby/ruby authors +// +// SPDX-License-Identifier: BSD-3-Clause + +package ruby_test + +import ( + "go/ast" + "go/parser" + "go/token" + "io/fs" + "path/filepath" + "regexp" + "strconv" + "strings" + "testing" +) + +// TestNoTestSkipsOnAFailureToBuild (#687). +// +// Two tests used to turn a build failure into a PASS: +// +// build := exec.Command("go", "build", "-o", bin, "./cmd/rbgo") +// if out, err := build.CombinedOutput(); err != nil { +// t.Skipf("cannot build cmd/rbgo (%v): %s", err, out) +// } +// +// If `cmd/rbgo` stopped compiling, both reported **skip**, the lane stayed +// green, and the binary-level HTTP and socket coverage silently stopped being +// exercised. `t.Skipf` there says "this machine cannot build it"; what it +// reports is "the product is broken". Those are opposite meanings and only one +// of them may be green. +// +// Both sites are `t.Fatalf` now. This exists so they cannot come back, and so +// the next test that reaches for the same shape fails instead of going quiet -- +// a fix to two lines closes two lines; a guard closes the class. +// +// WHAT IT JUDGES, and what it deliberately does not. A skip is legitimate when +// the thing it skips over is genuinely absent from the MACHINE: no symlink +// support, no /dev/zero, not root. It is never legitimate when the thing absent +// is THIS REPOSITORY'S OWN BUILD, because every lane builds it and a developer +// who cannot wants to be told. So the test reads the skip's REASON and refuses a +// vocabulary that can only describe the second case. It does not try to infer +// intent from the surrounding code, which would be a judgement call dressed up +// as a check. +func TestNoTestSkipsOnAFailureToBuild(t *testing.T) { + // Each alternative is a phrase that can only mean "the code did not + // compile". "build" alone is not among them: "no build tag", "build + // directory" and "builder" are ordinary and innocent, and a guard that fires + // on those gets edited away rather than obeyed. + // + // The match is on WORD BOUNDARIES, and that is not fussiness. The first + // version used strings.Contains and reported + // cmd/rbgo/build_test.go's "set RBGO_BUILD_IT=1 to run the rbgo build + // integration test" as an offence -- because "go build" is a substring of + // "rb|go build". One false positive on a guard like this is enough to get it + // deleted, so the two cases below are pinned as a self-test. + forbidden := regexp.MustCompile(`(?i)\b(` + strings.Join([]string{ + `cannot build`, + `could not build`, + `failed to build`, + `build failed`, + `go build`, + `cannot compile`, + `could not compile`, + `does not compile`, + `compile failed`, + `failed to compile`, + }, `|`) + `)\b`) + + // The guard's own control, run before it judges anything: a known positive + // it must catch and a known negative it must not. Without these the + // vocabulary is a guess, and a guess that matches nothing looks exactly like + // a clean tree. + for _, c := range []struct { + reason string + want bool + }{ + {"cannot build cmd/rbgo (exit status 1): ...", true}, + {"could not compile the fixture", true}, + {"go build failed", true}, + {"set RBGO_BUILD_IT=1 to run the rbgo build integration test (needs the Go toolchain)", false}, + {"no build tag for this platform", false}, + {"builder not available", false}, + {"requires root", false}, + } { + if got := forbidden.MatchString(c.reason); got != c.want { + t.Fatalf("the vocabulary itself is wrong: MatchString(%q) = %v, want %v", c.reason, got, c.want) + } + } + + var offences []string + skipsSeen := 0 + + fset := token.NewFileSet() + err := filepath.WalkDir(".", func(path string, d fs.DirEntry, err error) error { + if err != nil { + return err + } + if d.IsDir() { + // testdata holds fixtures, not tests; .git and the corpus caches are + // not ours to judge. + switch d.Name() { + case ".git", "testdata", "vendor", "node_modules": + return fs.SkipDir + } + return nil + } + if !strings.HasSuffix(path, "_test.go") { + return nil + } + f, perr := parser.ParseFile(fset, path, nil, 0) + if perr != nil { + // A test file that does not parse is a failure of this guard's input, + // not a pass. Say so rather than skipping the file. + offences = append(offences, path+": does not parse: "+perr.Error()) + return nil + } + ast.Inspect(f, func(n ast.Node) bool { + call, ok := n.(*ast.CallExpr) + if !ok { + return true + } + sel, ok := call.Fun.(*ast.SelectorExpr) + if !ok { + return true + } + switch sel.Sel.Name { + case "Skip", "Skipf", "SkipNow": + default: + return true + } + skipsSeen++ + // The reason is the first argument when there is one, and it is a + // literal in every site in this repository. A computed reason cannot + // be judged here and is left alone rather than guessed at. + if len(call.Args) == 0 { + return true + } + lit, ok := call.Args[0].(*ast.BasicLit) + if !ok || lit.Kind != token.STRING { + return true + } + reason, uerr := strconv.Unquote(lit.Value) + if uerr != nil { + return true + } + if forbidden.MatchString(reason) { + pos := fset.Position(call.Pos()) + offences = append(offences, pos.String()+": skips on a build failure: "+strconv.Quote(reason)) + } + return true + }) + return nil + }) + if err != nil { + t.Fatalf("walking the tree: %v", err) + } + + // "N of M, never N alone": a walk that parsed nothing finds no offence and + // is indistinguishable from a clean tree. This repository has dozens of + // legitimate skips, so zero means the walk is broken. + if skipsSeen == 0 { + t.Fatalf("the walk found no t.Skip call anywhere, which cannot be right; "+ + "it has not read the tree (offences collected so far: %v)", offences) + } + for _, o := range offences { + t.Errorf("%s\n a build failure is the product being broken, not this machine "+ + "lacking something: use t.Fatalf. See #687.", o) + } + t.Logf("%d t.Skip call(s) examined, %d offence(s)", skipsSeen, len(offences)) +}