fix: count each coverage block once regardless of cache warmth - #381
Merged
Conversation
The gate counted statements straight off the lines of the merged ./... profile, and that profile can carry the same block several times. go test builds the -coverprofile output by concatenating each package's fragment, and cmd/go appends a cached fragment before the checks that decide whether the cached result is usable, once for each of the two keys it consults. Right after go clean -testcache every block lands three times, so the same tree reported 4942 statements warm and 14826 expired. Merge blocks by identity before counting. Counts combine with max rather than the sum go tool cover uses in atomic mode, because the repeats are one run's data re-emitted; the covered/uncovered verdict is the same either way.
The merge added by the parent commit had no test and could not be given one: `go test ./...` ran unconditionally, so driving the counting over a profile meant editing the script. Every shape the merge exists for — a block repeated three times, a block one fragment covered and another did not, a span two fragments size differently — is a shape no committed spec produces, so the merge was the only untested part of the gate that guards every other test. check-coverage.sh now takes an optional profile argument that skips the suite, and scripts/verify-coverage-count.sh drives twelve cases through it. Each case is paired with a mutation of the merge that must turn it red, so a case that stops reaching the decision it names fails rather than staying quietly green. The mutations replace exact text rather than matching a regex, since a pattern that means something else under GNU sed would be indistinguishable from a case that catches nothing. Wired into the gate ahead of the coverage step. Also fixed, all found by running the script rather than reading it: - A conflicting statement count was the one failure that printed nothing to stdout: the message went to stderr and `set -e` aborted the assignment before any summary. It now reports on stdout with a summary line, like every other failure here. - The note claimed every block lands three times after `go clean -testcache`. With no cached fragment to find every block lands once, which is what a runner with a cold build cache — CI's usual state — actually sees. Replaced the count with the mechanism. - Counts merged by max, and four lines defended max over sum, but nothing reads the result as anything but zero or non-zero: `count[$1] = 1` gives identical output on real warm and cold profiles. The merge now records whether some fragment ran the block, which is the only question the gate asks, and says so. - "added 2 to the total and 1 to the hits" holds only for a one-statement block; an N-statement block added 2N and N. - The note read as though repeated fragments come from one run. They need not: cmd/go merges a cached fragment before rejecting the cached result as expired, so a block only a replayed fragment covered now counts as covered. Stated where a reader will reach it. - The merged profile was re-emitted through three further passes to filter, total, and list it; one sorted pass does all three.
Three defects, all found by feeding the counter profiles rather than reading it.
**A block with no statements was listed as uncovered beside a passing verdict.**
`go` emits `numstmt=0` blocks for an empty body — an unreached `case x:`, say —
and 16 of them are in this tree's own profile today. Collapsing the reporting
into one streaming pass made the listing print such a block while `missed`
stayed 0, so the run ended:
COVERAGE FAIL: zeroblk/z.go:8.9,8.9 (0 statement(s) uncovered)
Coverage gate passed: all 3 statements covered.
exit 0
A COVERAGE FAIL beside a pass and a zero exit is the worst shape this script has.
It is a regression from the previous commit, which reached the listing only once
the totals already disagreed. A zero-statement block cannot be uncovered and
contributes nothing either way, so it is now skipped outright, which puts the
listing and the verdict back in lockstep.
**A profile with no `mode:` header passed while hiding an uncovered block.** The
body was read from line 2 unconditionally, so a body-only file lost its first
block: a two-block profile whose first block was uncovered reported `all 5
statements covered` instead of failing 5 of 10. That predates this branch, but
nothing could reach it while the script always wrote the profile it read; taking
a profile argument is what made it reachable. The first line must now be a
`mode:` header.
**A line with the wrong field count was read positionally anyway**, quietly
becoming a zero-statement block. Block lines must now have exactly three fields.
Both refusals report on stdout with a summary, like every other failure here.
The verifier grows six assertions and four mutations to match, and gains two
things it needed regardless: assertions that a substring is *absent*, without
which the abort flag that keeps merged blocks out of a failure report had no
test at all; and an anchor counter that counts occurrences rather than lines,
so an anchor appearing twice on one line can no longer read as unique.
`sort`'s collation follows the locale. en_US.UTF-8 folds case and skips punctuation
where C compares bytes, so four uncovered blocks in one file come out in two
different orders on two machines:
LC_ALL=C LC_ALL=en_US.UTF-8
x/a-c.go x/a_b.go
x/aB.go x/a-c.go
x/a_b.go x/aa.go
x/aa.go x/aB.go
The comment above the sort claimed the opposite — that sorting makes the same
failure read the same way on every run. It does within a machine; across two it
did not, and because the listing is capped at 25 blocks that decided which blocks
a reader is shown at all, not merely their order. Two people looking at the same
failing profile could see disjoint subsets.
The verifier checks this by running one profile under two locales and comparing,
rather than by inspecting the pin. A locale that is not installed falls back to C
silently, which would make the comparison pass without comparing anything, so the
second locale is chosen by its observed effect on sort rather than by name — and
if none of the candidates collates differently, the check says it is skipping
instead of reporting a pass. It also mutates the pin away first and requires the
two runs to disagree, since a probe that cannot see an unpinned sort proves
nothing about a pinned one. Both of those refusals were confirmed by forcing them.
Locally this changes nothing: C.UTF-8 and C order these identically.
A case names the profile it runs against. If nothing builds that profile, the
counter is handed a path that does not exist — and its answer to that is
"COVERAGE FAIL: cannot read profile ..." with exit 1, which satisfies any case
expecting a failure and a COVERAGE FAIL substring. So a typo in the case table
passed, quietly retiring the check it was meant to be:
ok: daed: COVERAGE FAIL # 'dead' misspelled; nothing ran
The mutation sweep does not cover this. It proves a case notices a broken
counter, and only for the cases a mutation targets — a case that never ran at
all still reports ok. Cases carry the profile check themselves now, so it holds
wherever they are used.
CLAUDE.md's command block says it is "the same checks CI's gate job runs (.github/workflows/gate.yml), in that order". Adding a step to that job without adding it here made the claim false, and a contributor following the block would pass locally and then fail on a check they were never told to run. README's pre-landing list has the same job and the same omission. Realigned CLAUDE.md's trailing comments to the longest entry so the column holds.
OmarAlJarrah
added a commit
that referenced
this pull request
Aug 13, 2026
The comment said two concurrent runs inflate the total several times over. That was true when it was written and is not now: the block merge landed in #381 counts a block once however many times it appears, and five staggered runs against a shared cover.out all report the same 6132 on this tree. What the unique path still buys is that a run judges the blocks it produced. Two runs sharing one path truncate each other mid-write, and a profile missing the blocks another run had already written reads as a pass when those were the uncovered ones.
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
scripts/check-coverage.shcounted statements straight off the lines of the merged./...coverage profile. That profile can carry the same block several times, so thenumber it printed described the test cache rather than the tree. Measured at
b1c7a5c,macOS, go1.26.4,
GOFLAGSempty — same tree, same commit, the old script:all 6132 statements coveredgo clean -testcacheall 18396 statements coveredA partially warm cache lands in between, in proportion to how many packages re-ran: a
run during which only
passand what depends on it re-ran gave a 7954-line profile,which the old script read as 11422 statements. (That tree carried the probe from the
test plan below, so its true figure is 6134 rather than 6132.)
Cause.
go testbuilds the-coverprofileoutput by concatenating each package'sprofile fragment —
mergeCoverProfileincmd/go/internal/test/cover.gocopies thefragment body onto the output and deduplicates nothing.
cmd/goappends a cachedfragment before the checks that decide whether the cached result is usable
(
test.go:1944merges;test.go:1966is where an expired entry is rejected), and itconsults the cache under two keys per package. So a package whose cached fragment is
found but whose cached result is then rejected contributes its blocks before the test
re-runs and again afterwards.
How many times a given block lands is a property of what the cache happens to hold, not
a constant. At
b1c7a5cevery block landed three times right aftergo clean -testcache— 29 of the 30 packages have tests, and
2 x 29 = 58lookups are refused:That snippet needs running twice to show anything:
GODEBUGis part of the test-cachekey, so the first run finds nothing under it, reports
0, and produces a profile withno repeats at all.
Which is the same reason CI was never affected. A repeat needs a cached fragment to
be found and the cached result then rejected. On a runner whose build cache is cold the
lookup returns before the merge, and the profile holds one copy of each block:
The wrong numbers were a local-developer phenomenon.
The fix. The script merges blocks by identity (the
<file>.go:<span>field) beforecounting, which is what
go tool coverdoes when it reads a profile. An explicit-coverpkgis not the fix: the repeats are re-emissions of a package's own fragment,not a question of what is instrumented, and
-coverpkgwould additionally make everytest binary emit every package's blocks.
The merge keeps only whether some fragment ran a block, which is the one question a
100% gate asks. It deliberately does not carry an execution count forward: nothing
downstream reads the value as anything but zero or non-zero, so a rule for combining
counts would be a distinction the script cannot observe.
Behaviour change, stated plainly. After merging, a block executed by one fragment
and not another counts as covered. Worth stating because the fragments need not come
from the same run —
cmd/gomerges a cached fragment before rejecting the cached resultas expired — so a block that a replayed fragment covered and this run did not now counts
as covered. Previously that divergence failed the gate, by charging the block's
statements to the total twice and to the hits once. A block no fragment ran still merges
to 0, is still listed, and still fails.
Profiles the counter would misread are now refused rather than counted anyway. The
body was read from line 2 unconditionally, so a file with no
mode:header lost itsfirst block — a two-block profile whose first block was uncovered reported
all 5 statements coveredinstead of failing 5 of 10. And a line with the wrong field countwas still read positionally, quietly becoming a zero-statement block. Both are now
refused, on stdout with a summary.
A block with no statements is no longer listed as uncovered.
goemitsnumstmt=0blocks for an empty body — an unreached
case x:— and 16 are in this tree's profiletoday. Such a block cannot be uncovered and contributes nothing, but it was being listed,
which put a
COVERAGE FAILline beside a passing verdict and a zero exit.The failure listing's sort is pinned to
LC_ALL=C. Collation follows the locale —en_US.UTF-8 folds case and skips punctuation where C compares bytes — so the same four
uncovered blocks came out in two different orders on two machines. Since the listing is
capped at 25 blocks, that decided which blocks a reader is shown, not merely their order.
Also fixed here. A profile claiming two different statement counts for one span was
the single failure path that printed nothing to stdout — the message went to stderr and
set -eaborted the assignment before any summary line. It now reports on stdout with asummary, like every other failure in the script. And the merged profile was re-emitted
through three further passes to filter, total and list it; one sorted pass does all three.
Test plan
The counting now has a test.
scripts/verify-coverage-count.shdrives eighteenassertions over twelve profiles through the gate script: a block repeated three times
counted once, a block one fragment covered and another did not merging to covered, a
block every fragment left at zero still failing, two spans in one file not collapsing
into each other, a span two fragments size differently failing loudly on stdout, a
profile with no
mode:header, a line with the wrong field count, a zero-statementblock that must not be listed, an empty profile body, a profile whose blocks hold no
statements, and the 25-block cap on the failure listing truncating. An assertion can
require a substring to be absent, which is what holds the zero-statement block and
the merged-block leak out of failure output.
None of those shapes come out of a real run, so the merge previously had no test at all —
and it could not be given one, because
go test ./...ran unconditionally. The script nowtakes an optional profile argument that skips the suite. It is documented as not being a
gate, since it runs nothing.
Each case is paired with a mutation that must break it, so a case that stops reaching
the decision it names fails rather than staying quietly green:
tripledistinctconflictpartialmode:header guardnoheaderbadfieldsempty-blockconflictThe sweep refuses to score a mutation it cannot trust: one whose anchor text no longer
appears exactly once in the script is reported rather than counted — counting
occurrences, not lines — and one that breaks the single-block case is treated as broken
outright rather than as evidence about its target. Mutations replace exact text rather
than matching a regex, so they read the same under BSD and GNU userlands. All five of
those guard paths were confirmed by forcing them.
What the sweep cannot cover is a case that never ran. A case names the profile it runs
against, and the counter's answer to a path that does not exist is
COVERAGE FAIL: cannot read profile ...with exit 1 — which satisfies any case expecting a failure, soa typo in the table read as
ok. Cases now check that their profile was built.Locale independence is checked separately, by running one profile under two locales and
comparing rather than by inspecting the pin. An uninstalled locale falls back to C
silently, which would compare nothing, so the second locale is chosen by its observed
effect on
sort; if no candidate collates differently the check says it is skippingrather than reporting a pass. It also mutates the pin away first and requires the two
runs to disagree, since a probe blind to an unpinned sort proves nothing about a pinned
one. Both refusals were confirmed by forcing them.
Wired into
gate.ymlahead of the coverage step.The count no longer moves with the cache. Same tree, back to back —
4322and12966profile lines respectively:6132 is the tree's own figure, computed independently of the script: summing
numstmtover first occurrences of each block identity gives 6132 for both profiles, and both
contain 4322 unique blocks.
An uncovered statement is still caught, in both cache states. Planted an unreachable
branch in
pass/refs.go(if len(path) == 1<<20 { return nil, false }) and ran the gatewarm and again after
go clean -testcache. Identical output both times, exit 1, fromprofiles of 7954 and 12972 lines:
The probe was reverted; the diff is the two scripts, the gate workflow, and the two
command lists that enumerate the gate (CLAUDE.md and README.md).
Output is unchanged on real profiles. The rewritten script and the previous revision
produce byte-identical stdout and exit status on a real warm profile and a real cold one.
Gate:
gofmtclean,go vet ./...clean,golangci-lint runreports0 issues.,go build ./...clean,./scripts/verify-coverage-count.shpasses,./scripts/check-coverage.shpasses at 6132.Upstream
The duplication is a
cmd/godefect, not something this repo's flags control: the fixthere would be to move
mergeCoverProfilebelow the checks that can reject a cachedresult, and to merge rather than concatenate. golang/go#74873 reports the same
non-deduplicating merge from the
-coverpkgside, and golang/go#23076 records thatprofile consumers have to cope with repeated blocks regardless. Merging on our side is
correct either way — a profile is defined by its blocks, not by its lines — so this stays
right whenever the toolchain changes.
Closes #369