build: make the gate reproducible, pinned and complete - #344
Open
OmarAlJarrah wants to merge 1 commit into
Open
Conversation
The gate was written down in three places and executed in one, and the one execution left several of the repo's own tools unreached. A Makefile now holds the definition and .github/workflows/gate.yml calls its targets, so `make gate` and the CI job are the same commands rather than two lists that agree until somebody edits one. check-coverage.sh mints its coverage profile with mktemp instead of a fixed relative cover.out. Two runs sharing that path interleaved into a profile that was neither run's: three concurrent runs on one tree reported 15220, 15219 and 15215 statements for a tree whose real total is 4942. It also cds to the repo root, so running it from a subdirectory can no longer measure a subtree and report a pass. The suite now runs under -race, in the same execution the coverage profile comes from rather than a second one. scripts/fuzz.sh gives every fuzz target a bounded search, and scripts/bench.sh runs the benchmarks and refuses a run that measured nothing. golangci-lint is pinned to the release CI resolves today, with the version defined once in the Makefile and read back by the workflow. CheckPath's read-error branch is covered by a unix socket rather than a chmod 0o000 file, so it no longer depends on not being root; a pristine checkout previously failed the 100% gate under euid 0 by exactly one statement. BenchmarkAnchorWalk asked to be compared against a whole compile, which did not exist; BenchmarkCompile_Petstore is that number, alongside marshal, unmarshal and validate benchmarks. BenchmarkAnchorWalk itself had never executed — its fixture path was two levels short and it skipped, silently and with exit 0.
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.
Summary
The gate was written down in three places, executed in one, and that one
execution never reached several of the repo's own tools.
One definition (#90). A
Makefileholds the gate; every check in.github/workflows/gate.ymlruns one of its targets.make gateand the CI jobare now the same commands by construction. The suite also stops running twice —
the README sequence's standalone
go test ./...duplicated the run insidecheck-coverage.sh, and the coverage run is the only one left.A reproducible coverage number (#59).
check-coverage.shmints its profilewith
mktempinstead of a fixed relativecover.out.go test -coverprofiletruncates that file at start and appends each package's blocks as it finishes,
so two runs sharing the path interleave into a profile that is neither run's:
three staggered concurrent runs on one tree reported 15220 / 15219 / 15215
statements for a tree whose real total is 4942. It cannot fail a covered tree —
hit and total inflate together — but the number a human reads is fiction. The
script also
cds to the repo root, so running it from a subdirectory can nolonger measure a subtree and still report a pass.
A checkout that passes as root (#59).
TestCheckPath_UnreadableFileIsErrorused a
chmod 0o000file and skipped under euid 0, which leftinternal/harness/path.go:34-35uncovered there — a pristine checkout failed the100% gate by exactly one statement for anyone building as root, as a container
commonly does. It now uses a unix socket: refusing to open one for reading is not
a permission check, so no euid bypasses it and the skip is gone.
A pinned linter (#61).
golangci-lint-actionhad noversion:, so itinstalled whatever it resolved as latest. The pin (
v2.12.2, the release CIresolves today) is defined once in the Makefile and read back by the workflow.
make lintwarns — it does not fail — when the local binary differs.Race and fuzz (#77).
-racerides along with the coverage run rather thangetting a second full suite execution; the profile is identical either way.
scripts/fuzz.shgives each fuzz target a bounded-fuzztimesearch, derivingthe target list from the source so a new target is fuzzed the moment it lands,
and a finding is uploaded as an artifact so the reproducer survives a red run.
Benchmarks that run (#78).
BenchmarkCompile_Petstoreis the whole-compilenumber
anchorindex_bench_test.gohas been asking readers to compare against,alongside marshal, unmarshal and validate benchmarks.
scripts/bench.shrunsthem all once in the gate and refuses a run that measured nothing.
What was already fixed, and is not claimed here
check-coverage.shhas usedset -euo pipefailwith a bare
go testsincea0ca360, so a failing test cannot be swallowed;and there is no per-package name parsing left to misname anything — the gate
prints profile coordinates.
cmd/morphicandcmd/morphic-harnessdo appearin a full run, alphabetically first.
golangci-lint runexits 0 againstgo 1.26.3with v2.12.2. Only the pin was missing.FuzzCycleDetectoralready has 2 committed reproducers.tracked Go files (not 129). The gap it describes is real but narrower.
Deliberately out of scope
seedless:
FuzzCompileandFuzzLowerSchemaseed from the whole committedspec corpus via
f.Add, and the twoir/targets seed from adversarial runeand numeric-literal tables. Files under
testdata/fuzz/would duplicate thoseseeds rather than add coverage; that directory is where a found reproducer
gets pinned, which is what
FuzzCycleDetector's two entries are.FuzzCanonicalWords_Propertiesis held out of the gate's search, named inscripts/fuzz.shwith the issue that must close first. The very first boundedrun found a real idempotence bug in
ir.CanonicalWords—"ℤℤA"→"ℤℤa"→"ℤ_ℤa"— filed as ir: CanonicalWords is not idempotent when lowercasing creates an acronym tail #336. It is a surviving instance of ir: CanonicalWords is not idempotent for an uppercase rune with no lowercase form #187's mechanism at adifferent position, out of scope for a build change, and searching that target
would redden every unrelated PR until it is fixed. Its seeds still run on every
go test.README.mdis untouched. Its Building block still lists the six commandsand still says
gofmt -l .where the gate usesgofmt -l $(git ls-files '*.go'); docs: architecture and README drift from the implementation #64/docs: test and verification workflows are undocumented #65 are rewriting that section and this change would collide.CLAUDE.mdanddocs/micro-compiler-plan.mdcarried the same list and nowpoint at
make gate.BenchmarkAnchorWalk's fixture path was two directory levels short, so itskipped — silently, and with exit 0 — and had never executed. Fixed here rather
than filed, because the benchmark step added by this change is what surfaced it
and shipping a CI step that runs a known-dead benchmark would prove nothing.
Its
b.Skipfcalls are nowb.Fatalffor the same reason.Test plan
Every claim below was run in a worktree at this branch.
#59, non-determinism. Three staggered concurrent
./scripts/check-coverage.shruns on one tree, before and after:
#59, root. With both
os.Geteuid() == 0skips forced toif true,./scripts/check-coverage.shreported before:and after:
Coverage gate passed: all 4942 statements covered.Addingt.Skipto the new socket test reproduces that same one-block failure, so the test is
what covers the branch.
#77, race. A planted package-level counter written from two goroutines makes
make coverageexit 1 withWARNING: DATA RACE; removing it restores green.#77, fuzz.
make fuzzsearches 4 of 5 targets for 10s each and reportsfuzzed 4 of 5 target(s). Renaming everyfunc Fuzzmakes it exit 1 withno fuzz target was searched; adding a name to the quarantine that is not a targetmakes it exit 1 with
quarantine names ... remove it.#78, benchmarks.
make bench-smokeruns all 5. Making one skip fails withthese benchmarks skipped, so they measured nothing; renaming everyfunc Benchmarkfails withno benchmark reported a result; a plantedb.Fatalpropagates.
make benchgives the ratioanchorindex_bench_test.goasks for:BenchmarkAnchorWalk5.4 µs againstBenchmarkCompile_Petstore2.85 ms.#61, pin.
make -s print-lint-versionprintsv2.12.2, which is what theworkflow puts in the action's
version:input.make lint GOLANGCI_LINT_VERSION=v9.9.9prints the mismatch warning and still runs.#90, full gate.
make gateexits 0 in ~80 s and leaves the worktree clean.The repo's own gate script (gofmt, vet, golangci-lint, build, coverage) prints
### GATE PASSED.Closes #59
Closes #61
Closes #77
Closes #78
Closes #90