Skip to content

build: make the gate reproducible, pinned and complete - #344

Open
OmarAlJarrah wants to merge 1 commit into
mainfrom
build/reproducible-and-complete-gate
Open

build: make the gate reproducible, pinned and complete#344
OmarAlJarrah wants to merge 1 commit into
mainfrom
build/reproducible-and-complete-gate

Conversation

@OmarAlJarrah

Copy link
Copy Markdown
Member

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 Makefile holds the gate; every check in
.github/workflows/gate.yml runs one of its targets. make gate and the CI job
are now the same commands by construction. The suite also stops running twice —
the README sequence's standalone go test ./... duplicated the run inside
check-coverage.sh, and the coverage run is the only one left.

A reproducible coverage number (#59). check-coverage.sh mints its profile
with mktemp instead of a fixed relative cover.out. go test -coverprofile
truncates 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 no
longer measure a subtree and still report a pass.

A checkout that passes as root (#59). TestCheckPath_UnreadableFileIsError
used a chmod 0o000 file and skipped under euid 0, which left
internal/harness/path.go:34-35 uncovered there — a pristine checkout failed the
100% 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-action had no version:, so it
installed whatever it resolved as latest. The pin (v2.12.2, the release CI
resolves today) is defined once in the Makefile and read back by the workflow.
make lint warns — it does not fail — when the local binary differs.

Race and fuzz (#77). -race rides along with the coverage run rather than
getting a second full suite execution; the profile is identical either way.
scripts/fuzz.sh gives each fuzz target a bounded -fuzztime search, deriving
the 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_Petstore is the whole-compile
number anchorindex_bench_test.go has been asking readers to compare against,
alongside marshal, unmarshal and validate benchmarks. scripts/bench.sh runs
them all once in the gate and refuses a run that measured nothing.

What was already fixed, and is not claimed here

Deliberately out of scope

  • No committed seed corpora for the four bare fuzz targets. They are not
    seedless: FuzzCompile and FuzzLowerSchema seed from the whole committed
    spec corpus via f.Add, and the two ir/ targets seed from adversarial rune
    and numeric-literal tables. Files under testdata/fuzz/ would duplicate those
    seeds rather than add coverage; that directory is where a found reproducer
    gets pinned, which is what FuzzCycleDetector's two entries are.
  • FuzzCanonicalWords_Properties is held out of the gate's search, named in
    scripts/fuzz.sh with the issue that must close first. The very first bounded
    run 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 a
    different 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.md is untouched. Its Building block still lists the six commands
    and still says gofmt -l . where the gate uses gofmt -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.md and docs/micro-compiler-plan.md carried the same list and now
    point at make gate.
  • BenchmarkAnchorWalk's fixture path was two directory levels short, so it
    skipped — 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.Skipf calls are now b.Fatalf for 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.sh
runs on one tree, before and after:

before:  15220 / 15219 / 15215 statements   (serial baseline: 4942)
after:    4942 /  4942 /  4942 /  4942      (four staggered runs)

#59, root. With both os.Geteuid() == 0 skips forced to if true,
./scripts/check-coverage.sh reported before:

COVERAGE FAIL: .../internal/harness/path.go:34.17,36.4 (1 statement(s) uncovered)
Coverage gate failed: 1 of 4942 statements uncovered; 100% is required.

and after: Coverage gate passed: all 4942 statements covered. Adding t.Skip
to 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 coverage exit 1 with WARNING: DATA RACE; removing it restores green.

#77, fuzz. make fuzz searches 4 of 5 targets for 10s each and reports
fuzzed 4 of 5 target(s). Renaming every func Fuzz makes it exit 1 with no fuzz target was searched; adding a name to the quarantine that is not a target
makes it exit 1 with quarantine names ... remove it.

#78, benchmarks. make bench-smoke runs all 5. Making one skip fails with
these benchmarks skipped, so they measured nothing; renaming every func Benchmark fails with no benchmark reported a result; a planted b.Fatal
propagates. make bench gives the ratio anchorindex_bench_test.go asks for:
BenchmarkAnchorWalk 5.4 µs against BenchmarkCompile_Petstore 2.85 ms.

#61, pin. make -s print-lint-version prints v2.12.2, which is what the
workflow puts in the action's version: input. make lint GOLANGCI_LINT_VERSION=v9.9.9 prints the mismatch warning and still runs.

#90, full gate. make gate exits 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

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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment