Skip to content

fix: count each coverage block once regardless of cache warmth - #381

Open
OmarAlJarrah wants to merge 1 commit into
mainfrom
fix/coverage-count-dedupe
Open

fix: count each coverage block once regardless of cache warmth#381
OmarAlJarrah wants to merge 1 commit into
mainfrom
fix/coverage-count-dedupe

Conversation

@OmarAlJarrah

@OmarAlJarrah OmarAlJarrah commented Aug 9, 2026

Copy link
Copy Markdown
Member

Summary

scripts/check-coverage.sh counted statements straight off the lines of the merged
./... coverage profile. That profile can carry the same block several times, so the
number it printed described the test cache rather than the tree. On this branch's parent,
same commit, same script:

test cache profile lines unique blocks reported
warm 3517 3517 all 4942 statements covered
partially warm 8023 3517 all 11426 statements covered
after go clean -testcache 10551 3517 all 14826 statements covered

Cause. go test builds the -coverprofile output by concatenating each package's
profile fragment — mergeCoverProfile in cmd/go/internal/test/cover.go copies the
fragment body onto the output and deduplicates nothing. cmd/go appends a cached
fragment before the checks that decide whether the cached result is usable, and it
consults the cache under two keys per package (build action ID, then build content ID).
So a package whose cached coverage fragment is found but whose cached result is then
rejected contributes its blocks twice before the test even re-runs. Right after
go clean -testcache — which sets an expiry marker rather than deleting entries — that
is every package, and every block lands exactly three times. The toolchain's own tracing
shows it directly:

$ GODEBUG=gocachetest=1 go test ./... -covermode=atomic -coverprofile=cover.out 2>&1 >/dev/null |
    grep -c 'test output expired due to go clean -testcache'
52                                    # 26 packages x 2 cache lookups
$ tail -n +2 cover.out | wc -l ; tail -n +2 cover.out | awk '{print $1}' | sort -u | wc -l
   10551
    3517

When the cached fragment is instead absent, the lookup returns before the merge and no
duplicate appears — 52 x cached cover profile missing gives a 3517-line profile. That is
the whole difference between the two numbers.

An explicit -coverpkg is not the fix: the repeats are re-emissions of a package's own
fragment, not a question of what is instrumented, and -coverpkg would additionally make
every test binary emit every package's blocks. So the script now merges blocks by identity
(the <file>.go:<span> field) before counting, which is what go tool cover does when it
reads a profile.

Counts combine with max rather than the sum go tool cover uses in atomic mode.
The duplicates here are one run's data re-emitted, so summing would multiply genuine
execution counts by however many times a fragment happened to be appended. The
covered/uncovered verdict is identical under either rule, since the gate only asks whether
a block ever ran.

Behaviour change, stated plainly. After merging, a block executed by one fragment and
not another counts as covered. That is the correct reading of "some test executed this
statement", and it retires a class of false failure: previously such a block added 2 to
total and 1 to hit, failing the gate. Duplicates could only ever cause a false failure,
never a false pass, so the gate was sound before and stays sound now — a block that no
fragment ran still merges to 0, is still listed, and still fails.

Test plan

All of the below on this branch, macOS, go1.26.4, GOFLAGS empty.

The count no longer moves with the cache. Same tree, back to back:

$ ./scripts/check-coverage.sh | tail -1
Coverage gate passed: all 4942 statements covered.        # profile: 3517 lines
$ go clean -testcache
$ ./scripts/check-coverage.sh | tail -1
Coverage gate passed: all 4942 statements covered.        # profile: 10551 lines

4942 is the tree's own figure, computed independently of the script — summing numstmt
over first occurrences of each block identity gives 4942 for both profiles, and both
contain 3517 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 gate
warm and again after go clean -testcache. Identical output both times, exit 1:

COVERAGE FAIL: github.com/dexpace/morphic/pass/refs.go:50.24,52.3 (1 statement(s) uncovered)
Coverage gate failed: 1 of 4944 statements uncovered; 100% is required.

On the second run the profile held 10557 lines. The old script, given that very same
profile, printed the block three times and miscounted the totals:

COVERAGE FAIL: github.com/dexpace/morphic/pass/refs.go:50.24,52.3 (1 statement(s) uncovered)
COVERAGE FAIL: github.com/dexpace/morphic/pass/refs.go:50.24,52.3 (1 statement(s) uncovered)
COVERAGE FAIL: github.com/dexpace/morphic/pass/refs.go:50.24,52.3 (1 statement(s) uncovered)
Coverage gate failed: 3 of 14832 statements uncovered; 100% is required.

A package with no coverage still fails. Added a temporary pass/nocov package whose
test executes none of its statements:

COVERAGE FAIL: github.com/dexpace/morphic/pass/nocov/nocov.go:11.2,11.14 (1 statement(s) uncovered)
COVERAGE FAIL: github.com/dexpace/morphic/pass/nocov/nocov.go:5.24,7.23 (2 statement(s) uncovered)
COVERAGE FAIL: github.com/dexpace/morphic/pass/nocov/nocov.go:7.23,9.3 (1 statement(s) uncovered)
Coverage gate failed: 4 of 4946 statements uncovered; 100% is required.

Both probes were reverted; the diff is the script alone.

Counting logic, against hand-built profiles (the script with its go test line stubbed
out, driven through COVER_FILE): a block repeated three times with identical counts is
counted once; a block repeated with one zero and one non-zero count merges to covered; a
block repeated with every count zero is still reported and still fails; a profile with an
empty body still trips "the profile records no statements"; the 25-block cap on the failure
listing still truncates; and a profile claiming two different statement counts for one span
fails loudly rather than silently letting line order pick a total.

Gate (.github/workflows/gate.yml, unchanged): gofmt clean, go vet ./... clean,
golangci-lint run reports 0 issues., go build ./... clean, ./scripts/check-coverage.sh
passes at 4942.

Upstream

The duplication is a cmd/go defect, not something this repo's flags control: the fix
there would be to move mergeCoverProfile below the checks that can reject a cached
result, and to merge rather than concatenate. golang/go#74873 reports the same
non-deduplicating merge from the -coverpkg side, and golang/go#23076 records that
profile 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

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

build: the coverage gate's statement count depends on test-cache warmth

1 participant