Repository navigation
test: no test may skip on a failure to build, and a guard says so - #812
Merged
Merged
Conversation
#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 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes issue 687.
The defect it closes
Two tests turned a build failure into a PASS:
If
cmd/rbgostopped compiling, both reported skip, the lane stayed green,and the binary-level HTTP and socket coverage silently stopped being exercised.
t.Skipfthere says "this machine cannot build it"; what it reports is "theproduct is broken". Only one of those may be green.
Both are already
t.Fatalf— measured onmain, not assumed. So this PRadds what closes the class rather than the two lines.
What the guard judges, and what it deliberately does not
It walks every
*_test.go, reads eacht.Skip/Skipf/SkipNow's reason,and refuses a vocabulary that can only mean the code did not compile.
A skip is legitimate when what is missing belongs to the machine — no
symlinks, no
/dev/zero, not root. It is never legitimate when what is missingis this repository's own build, because every lane builds it and a developer
who cannot wants to be told. Inferring that from the surrounding code would be a
judgement call dressed as a check, so it reads the reason and nothing else.
Three things so it is not decorative
Word boundaries, not substrings. The first version used
strings.Containsand reported
cmd/rbgo/build_test.go'sas an offence — because
go buildis a substring ofrb|go build. One falsepositive is enough to get a guard deleted. That is the third
substring-blindness of this kind in two days, after
-vinside--versionand a README phrase inside its own removal note.
Its own control, run before it judges anything. Seven reasons — three that
must match, 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 t.Skip call(s) examined, 0 offence(s). A walk that parsed nothing finds no offence either.Proved in the other direction
Replanting the exact shape issue 687 described, at the site it described:
Also from that issue's inventory
grape_internal_test.go'sif r.handler != nil { t.Skip("handler set") }couldnot fire —
handleris set tonilon the line above — so the test read asconditional when it is not. 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/webappare left aloneon purpose: they record a known missing binding with a long diagnostic, and
turning them red would fail the lane for a gap nobody is hiding. Said here so
the next sweep does not treat them as an oversight.
🤖 Generated with Claude Code