From d8307ea39b71f20b30b2150bcaf9755de0dd6a55 Mon Sep 17 00:00:00 2001 From: Ryan Currah Date: Tue, 12 Aug 2025 21:12:32 -0400 Subject: [PATCH] cmd/go: invalidate coverage report cache when coverpkg sources change When using -coverpkg with coverage profiling, the coverage cache keys did not include the source file hashes of the covered packages. This could result in stale coverage reports that reference outdated line numbers when covered package sources were modified between test runs. This change adds a cached hash of the covered packages' source files that gets included in the coverage cache key computation. The hash is computed once per test execution using sync.Once to avoid redundant file system operations across multiple test packages. The fix ensures that coverage profiles are properly invalidated when any source file in the covered packages changes, preventing cache hits with stale line number references. Fixes #74873 --- src/cmd/go/internal/test/test.go | 44 +++++- .../script/test_cache_coverpkg_bug.txt | 137 ++++++++++++++++++ 2 files changed, 180 insertions(+), 1 deletion(-) create mode 100644 src/cmd/go/testdata/script/test_cache_coverpkg_bug.txt diff --git a/src/cmd/go/internal/test/test.go b/src/cmd/go/internal/test/test.go index 6c4a6a574d10ef..69a7d162458bfb 100644 --- a/src/cmd/go/internal/test/test.go +++ b/src/cmd/go/internal/test/test.go @@ -554,6 +554,10 @@ var ( testTimeout time.Duration // -timeout flag testV testVFlag // -v flag testVet = vetFlag{flags: defaultVetFlags} // -vet flag + + // Cache for covered packages hash to avoid recomputing for each test package + testCoverPkgsHash cache.ActionID + testCoverPkgsHashOnce sync.Once ) type testVFlag struct { @@ -1927,6 +1931,37 @@ func (c *runCache) tryCacheWithID(b *work.Builder, a *work.Action, id string) bo var errBadTestInputs = errors.New("error parsing test inputs") var testlogMagic = []byte("# test log\n") // known to testing/internal/testdeps/deps.go +// hashCoveredPackages writes to h a hash of the source files of the covered packages. +func hashCoveredPackages(h io.Writer, pkgs []*load.Package) { + for _, pkg := range pkgs { + fmt.Fprintf(h, "coverpkg %s", pkg.ImportPath) + // Include source file hashes to detect changes + for _, file := range pkg.GoFiles { + if fh := hashStat(filepath.Join(pkg.Dir, file)); fh != (cache.ActionID{}) { + fmt.Fprintf(h, " %x", fh) + } + } + for _, file := range pkg.CgoFiles { + if fh := hashStat(filepath.Join(pkg.Dir, file)); fh != (cache.ActionID{}) { + fmt.Fprintf(h, " %x", fh) + } + } + fmt.Fprintf(h, "\n") + } +} + +// getCoveredPackagesHash returns the cached hash of covered packages, computing it only once. +func getCoveredPackagesHash() cache.ActionID { + testCoverPkgsHashOnce.Do(func() { + if len(testCoverPkgs) > 0 { + h := cache.NewHash("coverpkgs") + hashCoveredPackages(h, testCoverPkgs) + testCoverPkgsHash = h.Sum() + } + }) + return testCoverPkgsHash +} + // computeTestInputsID computes the "test inputs ID" // (see comment in tryCacheWithID above) for the // test log. @@ -2071,7 +2106,14 @@ func testAndInputKey(testID, testInputsID cache.ActionID) cache.ActionID { // coverProfileAndInputKey returns the "coverprofile" cache key for the pair (testID, testInputsID). func coverProfileAndInputKey(testID, testInputsID cache.ActionID) cache.ActionID { - return cache.Subkey(testAndInputKey(testID, testInputsID), "coverprofile") + // Include covered packages hash to invalidate coverage reports when covered packages change + h := cache.NewHash("coverprofile") + testAndInputs := testAndInputKey(testID, testInputsID) + h.Write(testAndInputs[:]) + // Use cached hash instead of recomputing for each test package + coveredPkgsHash := getCoveredPackagesHash() + h.Write(coveredPkgsHash[:]) + return h.Sum() } func (c *runCache) saveOutput(a *work.Action) { diff --git a/src/cmd/go/testdata/script/test_cache_coverpkg_bug.txt b/src/cmd/go/testdata/script/test_cache_coverpkg_bug.txt new file mode 100644 index 00000000000000..f1ada56b3f4a90 --- /dev/null +++ b/src/cmd/go/testdata/script/test_cache_coverpkg_bug.txt @@ -0,0 +1,137 @@ +env GO111MODULE=on + +# Test for bug where cached coverage profiles with -coverpkg can contain +# outdated line references when source files are modified. +# This reproduces the issue where coverage data from cache may reference +# lines that no longer exist in the updated source files. + +[short] skip +[GODEBUG:gocacheverify=1] skip + +# We're testing cache behavior, so start with a clean GOCACHE. +env GOCACHE=$WORK/cache + +# Create a project structure with multiple packages +# proj/ +# some_func.go +# some_func_test.go +# sub/ +# sub.go +# sub_test.go +# sum/ +# sum.go + +# Switch to the proj directory +cd proj + +# Run tests with -coverpkg to collect coverage for all packages +go test -coverpkg=proj/... -coverprofile=cover1.out ./... +stdout 'coverage:' + +# Verify the first coverage profile exists and has expected content +exists cover1.out +grep -q 'proj/sub/sub.go:' cover1.out + +# Run again to ensure caching works +go test -coverpkg=proj/... -coverprofile=cover1_cached.out ./... +stdout '\(cached\)' +stdout 'coverage:' + +# Note: Due to the bug, cached coverage profiles may have duplicate entries +# This is the issue we're trying to fix + +# Now modify sub.go to change line structure - this will invalidate +# the cache for sub package but not for proj package +cp ../sub_modified.go sub/sub.go + +# The bug manifests as duplicate or invalid line references in the coverage profile +# After modifying sub.go, we should not have both old and new line references +go test -coverpkg=proj/... -coverprofile=cover2.out ./... +stdout 'coverage:' + +# With the bug present, we would see duplicate entries for the same lines +# This demonstrates the bug - there should be duplicates in the coverage profile +grep 'proj/sub/sub.go:' cover2.out +# The fix should ensure that only the new line format exists, not the old one +grep 'proj/sub/sub.go:3.24,4.35' cover2.out +# This should fail if the stale coverage line exists (the bug is present) +! grep 'proj/sub/sub.go:3.24,4.22' cover2.out + +-- proj/go.mod -- +module proj + +go 1.21 + +-- proj/some_func.go -- +package proj + +import "proj/sum" + +func SomeFunc(a, b int) int { + if a == 0 && b == 0 { + return 0 + } + return sum.Sum(a, b) +} + +-- proj/some_func_test.go -- +package proj + +import ( + "testing" +) + +func Test_SomeFunc(t *testing.T) { + t.Run("test1", func(t *testing.T) { + result := SomeFunc(1, 1) + if result != 2 { + t.Errorf("Expected 2, got %d", result) + } + }) +} + +-- proj/sub/sub.go -- +package sub + +func Sub(a, b int) int { + if a == 0 && b == 0 { + return 0 + } + return a - b +} + +-- proj/sub/sub_test.go -- +package sub + +import ( + "testing" +) + +func Test_Sub(t *testing.T) { + t.Run("test_sub1", func(t *testing.T) { + result := Sub(1, 1) + if result != 0 { + t.Errorf("Expected 0, got %d", result) + } + }) +} + +-- proj/sum/sum.go -- +package sum + +func Sum(a, b int) int { + if a == 0 { + return b + } + return a + b +} + +-- sub_modified.go -- +package sub + +func Sub(a, b int) int { + if a == 0 && b == 0 || a == -100 { + return 0 + } + return a - b +}