fix: count each coverage block once regardless of cache warmth - #381
Open
OmarAlJarrah wants to merge 1 commit into
Open
fix: count each coverage block once regardless of cache warmth#381OmarAlJarrah wants to merge 1 commit into
OmarAlJarrah wants to merge 1 commit into
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.
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. On this branch's parent,
same commit, same script:
all 4942 statements coveredall 11426 statements coveredgo clean -testcacheall 14826 statements coveredCause.
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, 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 — thatis every package, and every block lands exactly three times. The toolchain's own tracing
shows it directly:
When the cached fragment is instead absent, the lookup returns before the merge and no
duplicate appears — 52 x
cached cover profile missinggives a 3517-line profile. That isthe whole difference between the two numbers.
An explicit
-coverpkgis not the fix: the repeats are re-emissions of a package's ownfragment, not a question of what is instrumented, and
-coverpkgwould additionally makeevery test binary emit every package's blocks. So the script now merges blocks by identity
(the
<file>.go:<span>field) before counting, which is whatgo tool coverdoes when itreads a profile.
Counts combine with max rather than the sum
go tool coveruses 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
totaland 1 tohit, 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,
GOFLAGSempty.The count no longer moves with the cache. Same tree, back to back:
4942 is the tree's own figure, computed independently of the script — summing
numstmtover 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 gatewarm and again after
go clean -testcache. Identical output both times, exit 1: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:
A package with no coverage still fails. Added a temporary
pass/nocovpackage whosetest executes none of its statements:
Both probes were reverted; the diff is the script alone.
Counting logic, against hand-built profiles (the script with its
go testline stubbedout, driven through
COVER_FILE): a block repeated three times with identical counts iscounted 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):gofmtclean,go vet ./...clean,golangci-lint runreports0 issues.,go build ./...clean,./scripts/check-coverage.shpasses at 4942.
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