From ef43cf01719b1529da17650ab792ea523bf67673 Mon Sep 17 00:00:00 2001 From: Ryan Currah Date: Sat, 7 Sep 2024 19:45:49 -0400 Subject: [PATCH 1/4] cmd/go: cache coverage profile with tests This CL stores coverage profile data in the GOCACHE under the 'coverprofile' subkey alongside tests. This makes tests which use coverage profiles cacheable. The values of the -coverprofile and -outputdir flags are not included in the cache key to allow cached profile data to be written to any output file. Note: This is a rebase and squash from the original PRs below that was created/closed/abandoned by @jproberts and @macnibblet that I plan to maintain. - https://github.com/golang/go/pull/50483 - https://github.com/golang/go/pull/65657 I made improvements to the change based on feedback from @bcmills in Gerrit https://go-review.googlesource.com/c/go/+/563138. From @macnibblet: I don't know if anyone has considered the environmental impact (Yes, of course, dev experience too), but on a team with 3 backend developers, when I replaced our CI Golang version with this build, it reduced the build time by 50%, which would have equated to about 5000 hours of CI reduced in the past year. Fixes #23565 --- src/cmd/go/alldocs.go | 5 +- src/cmd/go/internal/test/cover.go | 23 ++++-- src/cmd/go/internal/test/test.go | 78 +++++++++++++++---- .../go/testdata/script/test_cache_inputs.txt | 63 +++++++++++++++ 4 files changed, 147 insertions(+), 22 deletions(-) diff --git a/src/cmd/go/alldocs.go b/src/cmd/go/alldocs.go index 830bac2b2f8965..e7f0e606c6681a 100644 --- a/src/cmd/go/alldocs.go +++ b/src/cmd/go/alldocs.go @@ -1889,8 +1889,9 @@ // // The rule for a match in the cache is that the run involves the same // test binary and the flags on the command line come entirely from a -// restricted set of 'cacheable' test flags, defined as -benchtime, -cpu, -// -list, -parallel, -run, -short, -timeout, -failfast, -fullpath and -v. +// restricted set of 'cacheable' test flags, defined as -benchtime, -cover, +// -covermode, -coverprofile, -cpu, -list, -parallel, -run, -short, -timeout, +// -failfast, -fullpath and -v. // If a run of go test has any test or non-test flags outside this set, // the result is not cached. To disable test caching, use any test flag // or argument other than the cacheable flags. The idiomatic way to disable diff --git a/src/cmd/go/internal/test/cover.go b/src/cmd/go/internal/test/cover.go index f614458dc46a15..c5444cc9b49147 100644 --- a/src/cmd/go/internal/test/cover.go +++ b/src/cmd/go/internal/test/cover.go @@ -14,9 +14,13 @@ import ( "sync" ) +// coverMerge manages the state for merging test coverage profiles. +// It ensures thread-safe operations on a single coverage profile file +// across multiple test runs and packages. var coverMerge struct { f *os.File - sync.Mutex // for f.Write + fsize int64 // Tracks the size of valid data written to f + sync.Mutex // for f.Write } // initCoverProfile initializes the test coverage profile. @@ -36,16 +40,17 @@ func initCoverProfile() { if err != nil { base.Fatalf("%v", err) } - _, err = fmt.Fprintf(f, "mode: %s\n", cfg.BuildCoverMode) + s, err := fmt.Fprintf(f, "mode: %s\n", cfg.BuildCoverMode) if err != nil { base.Fatalf("%v", err) } coverMerge.f = f + coverMerge.fsize = int64(s) } // mergeCoverProfile merges file into the profile stored in testCoverProfile. // It prints any errors it encounters to ew. -func mergeCoverProfile(ew io.Writer, file string) { +func mergeCoverProfile(file string) { if coverMerge.f == nil { return } @@ -66,19 +71,25 @@ func mergeCoverProfile(ew io.Writer, file string) { return } if err != nil || string(buf) != expect { - fmt.Fprintf(ew, "error: test wrote malformed coverage profile %s.\n", file) + base.Errorf("error: test wrote malformed coverage profile %s: header %q, expected %q: %v", file, string(buf), expect, err) return } - _, err = io.Copy(coverMerge.f, r) + s, err := io.Copy(coverMerge.f, r) if err != nil { - fmt.Fprintf(ew, "error: saving coverage profile: %v\n", err) + base.Errorf("error: saving coverage profile: %v", err) + return } + coverMerge.fsize += s } func closeCoverProfile() { if coverMerge.f == nil { return } + // Discard any partially written data from a failed merge. + if err := coverMerge.f.Truncate(coverMerge.fsize); err != nil { + base.Errorf("closing coverage profile: %v", err) + } if err := coverMerge.f.Close(); err != nil { base.Errorf("closing coverage profile: %v", err) } diff --git a/src/cmd/go/internal/test/test.go b/src/cmd/go/internal/test/test.go index 90f2d88d6b8a45..9ab397bc4da3ba 100644 --- a/src/cmd/go/internal/test/test.go +++ b/src/cmd/go/internal/test/test.go @@ -126,8 +126,9 @@ elapsed time in the summary line. The rule for a match in the cache is that the run involves the same test binary and the flags on the command line come entirely from a -restricted set of 'cacheable' test flags, defined as -benchtime, -cpu, --list, -parallel, -run, -short, -timeout, -failfast, -fullpath and -v. +restricted set of 'cacheable' test flags, defined as -benchtime, -cover, +-covermode, -coverprofile, -cpu, -list, -parallel, -run, -short, -timeout, +-failfast, -fullpath and -v. If a run of go test has any test or non-test flags outside this set, the result is not cached. To disable test caching, use any test flag or argument other than the cacheable flags. The idiomatic way to disable @@ -1377,6 +1378,13 @@ type runCache struct { id2 cache.ActionID } +func coverProfTempFile(a *work.Action) string { + if a.Objdir == "" { + panic("internal error: objdir not set in coverProfTempFile") + } + return a.Objdir + "_cover_.out" +} + // stdoutMu and lockedStdout provide a locked standard output // that guarantees never to interlace writes from multiple // goroutines, so that we can have multiple JSON streams writing @@ -1478,13 +1486,6 @@ func (r *runTestActor) Act(b *work.Builder, ctx context.Context, a *work.Action) return nil } - coverProfTempFile := func(a *work.Action) string { - if a.Objdir == "" { - panic("internal error: objdir not set in coverProfTempFile") - } - return a.Objdir + "_cover_.out" - } - if p := a.Package; len(p.TestGoFiles)+len(p.XTestGoFiles) == 0 { reportNoTestFiles := true if cfg.BuildCover && cfg.Experiment.CoverageRedesign && p.Internal.Cover.GenMeta { @@ -1508,7 +1509,7 @@ func (r *runTestActor) Act(b *work.Builder, ctx context.Context, a *work.Action) if err := work.WriteCoverageProfile(b, a, mf, cp, stdout); err != nil { return err } - mergeCoverProfile(stdout, cp) + mergeCoverProfile(cp) } } } @@ -1671,7 +1672,7 @@ func (r *runTestActor) Act(b *work.Builder, ctx context.Context, a *work.Action) a.TestOutput = &buf t := fmt.Sprintf("%.3fs", time.Since(t0).Seconds()) - mergeCoverProfile(cmd.Stdout, a.Objdir+"_cover_.out") + mergeCoverProfile(a.Objdir + "_cover_.out") if err == nil { norun := "" @@ -1693,7 +1694,7 @@ func (r *runTestActor) Act(b *work.Builder, ctx context.Context, a *work.Action) cmd.Stdout.Write([]byte("\n")) } fmt.Fprintf(cmd.Stdout, "ok \t%s\t%s%s%s\n", a.Package.ImportPath, t, coveragePercentage(out), norun) - r.c.saveOutput(a) + r.c.saveOutput(a, coverProfTempFile(a)) } else { if testFailFast { testShouldFailFast.Store(true) @@ -1791,7 +1792,18 @@ func (c *runCache) tryCacheWithID(b *work.Builder, a *work.Action, id string) bo // Note that this list is documented above, // so if you add to this list, update the docs too. cacheArgs = append(cacheArgs, arg) - + case "-test.coverprofile", + "-test.outputdir": + // The `-coverprofile` and `-outputdir` arguments, which + // only affect the location of profile output, are cacheable. This + // is based on the process where, upon a cache hit, stored profile + // data is copied to the specified output location. This mechanism + // ensures that output location preferences are honored without + // modifying the profile's content, thereby justifying their + // cacheability without impacting cache integrity. This allows + // cached coverage profiles to be written to different files. + // Note that this list is documented above, + // so if you add to this list, update the docs too. default: // nothing else is cacheable if cache.DebugTest { @@ -1903,6 +1915,21 @@ func (c *runCache) tryCacheWithID(b *work.Builder, a *work.Action, id string) bo j++ } c.buf.Write(data[j:]) + + // Write coverage data to profile. + if cfg.BuildCover { + // The cached coverprofile has the same expiration time as the + // test result it corresponds to. That time is already checked + // above, so we can ignore the entry returned by GetFile here. + f, _, err := cache.GetFile(cache.Default(), coverProfileAndInputKey(testID, testInputsID)) + if err != nil { + if cache.DebugTest { + fmt.Fprintf(os.Stderr, "testcache: %s: test coverage profile not found: %v\n", a.Package.ImportPath, err) + } + return false + } + mergeCoverProfile(f) + } return true } @@ -2051,7 +2078,12 @@ func testAndInputKey(testID, testInputsID cache.ActionID) cache.ActionID { return cache.Subkey(testID, fmt.Sprintf("inputs:%x", testInputsID)) } -func (c *runCache) saveOutput(a *work.Action) { +// 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") +} + +func (c *runCache) saveOutput(a *work.Action, coverProfileFile string) { if c.id1 == (cache.ActionID{}) && c.id2 == (cache.ActionID{}) { return } @@ -2072,12 +2104,29 @@ func (c *runCache) saveOutput(a *work.Action) { if err != nil { return } + var saveCoverProfile = func(testID cache.ActionID) {} + if coverProfileFile != "" { + coverProfile, err := os.Open(coverProfileFile) + if err == nil { + saveCoverProfile = func(testID cache.ActionID) { + cache.Default().Put(coverProfileAndInputKey(testID, testInputsID), coverProfile) + } + defer func() { + if err := coverProfile.Close(); err != nil && cache.DebugTest { + fmt.Fprintf(os.Stderr, "testcache: %s: closing temporary coverprofile: %v", a.Package.ImportPath, err) + } + }() + } else if cache.DebugTest { + fmt.Fprintf(os.Stderr, "testcache: %s: failed to open temporary coverprofile: %s", a.Package.ImportPath, err) + } + } if c.id1 != (cache.ActionID{}) { if cache.DebugTest { fmt.Fprintf(os.Stderr, "testcache: %s: save test ID %x => input ID %x => %x\n", a.Package.ImportPath, c.id1, testInputsID, testAndInputKey(c.id1, testInputsID)) } cache.PutNoVerify(cache.Default(), c.id1, bytes.NewReader(testlog)) cache.PutNoVerify(cache.Default(), testAndInputKey(c.id1, testInputsID), bytes.NewReader(a.TestOutput.Bytes())) + saveCoverProfile(c.id1) } if c.id2 != (cache.ActionID{}) { if cache.DebugTest { @@ -2085,6 +2134,7 @@ func (c *runCache) saveOutput(a *work.Action) { } cache.PutNoVerify(cache.Default(), c.id2, bytes.NewReader(testlog)) cache.PutNoVerify(cache.Default(), testAndInputKey(c.id2, testInputsID), bytes.NewReader(a.TestOutput.Bytes())) + saveCoverProfile(c.id2) } } diff --git a/src/cmd/go/testdata/script/test_cache_inputs.txt b/src/cmd/go/testdata/script/test_cache_inputs.txt index 68a700b1160763..b5c2286ea24950 100644 --- a/src/cmd/go/testdata/script/test_cache_inputs.txt +++ b/src/cmd/go/testdata/script/test_cache_inputs.txt @@ -128,6 +128,45 @@ go test testcache -run=TestOSArgs -fullpath go test testcache -run=TestOSArgs -fullpath stdout '\(cached\)' +# Ensure that specifying a cover profile does not prevent test results from being cached. +go test testcache -run=TestCoverageCache -coverprofile=coverage.out +go test testcache -run=TestCoverageCache -coverprofile=coverage.out +stdout '\(cached\)' +exists coverage.out +grep -q 'mode: set' coverage.out +grep -q 'testcache/hello.go:' coverage.out + +# A new -coverprofile file should use the cached coverage profile contents. +go test testcache -run=TestCoverageCache -coverprofile=coverage2.out +stdout '\(cached\)' +cmp coverage.out coverage2.out + +# Explicitly setting the default covermode should still use cache. +go test testcache -run=TestCoverageCache -coverprofile=coverage_set.out -covermode=set +stdout '\(cached\)' +cmp coverage.out coverage_set.out + +# A new -covermode should not use the cached coverage profile. +go test testcache -run=TestCoverageCache -coverprofile=coverage_atomic.out -covermode=atomic +! stdout '\(cached\)' +! cmp coverage.out coverage_atomic.out +grep -q 'mode: atomic' coverage_atomic.out +grep -q 'testcache/hello.go:' coverage_atomic.out + +# A new -coverpkg should not use the cached coverage profile. +go test testcache -run=TestCoverageCache -coverprofile=coverage_pkg.out -coverpkg=all +! stdout '\(cached\)' +! cmp coverage.out coverage_pkg.out + +# Test that -v doesn't prevent caching. +go test testcache -v -run=TestCoverageCache -coverprofile=coverage_v.out +go test testcache -v -run=TestCoverageCache -coverprofile=coverage_v2.out +stdout '\(cached\)' +cmp coverage_v.out coverage_v2.out + +# Test that -count affects caching. +go test testcache -run=TestCoverageCache -coverprofile=coverage_count.out -count=2 +! stdout '\(cached\)' # Executables within GOROOT and GOPATH should affect caching, # even if the test does not stat them explicitly. @@ -159,6 +198,18 @@ This file is outside of GOPATH. -- testcache/script.sh -- #!/bin/sh exit 0 +-- testcache/hello.go -- +package testcache + +import "fmt" + +func HelloWorld(name string) string { + if name == "" { + return "Hello, World!" + } + return fmt.Sprintf("Hello, %s!", name) +} + -- testcache/testcache_test.go -- // Copyright 2017 The Go Authors. All rights reserved. // Use of this source code is governed by a BSD-style @@ -262,6 +313,18 @@ func TestOSArgs(t *testing.T) { func TestBenchtime(t *testing.T) { } +func TestCoverageCache(t *testing.T) { + result := HelloWorld("") + if result != "Hello, World!" { + t.Errorf("Expected 'Hello, World!', got '%s'", result) + } + + result = HelloWorld("Go") + if result != "Hello, Go!" { + t.Errorf("Expected 'Hello, Go!', got '%s'", result) + } +} + -- mkold.go -- package main From c77f836c653b1d5b6c0b4da36741b03902c0387e Mon Sep 17 00:00:00 2001 From: Ryan Currah Date: Mon, 9 Sep 2024 17:07:34 -0400 Subject: [PATCH 2/4] remove tracking of cover profile size and raise error using base.Errorf and return --- src/cmd/go/internal/test/cover.go | 19 ++++++------------- src/cmd/go/internal/test/test.go | 5 +++-- 2 files changed, 9 insertions(+), 15 deletions(-) diff --git a/src/cmd/go/internal/test/cover.go b/src/cmd/go/internal/test/cover.go index c5444cc9b49147..bc06133efda894 100644 --- a/src/cmd/go/internal/test/cover.go +++ b/src/cmd/go/internal/test/cover.go @@ -19,8 +19,7 @@ import ( // across multiple test runs and packages. var coverMerge struct { f *os.File - fsize int64 // Tracks the size of valid data written to f - sync.Mutex // for f.Write + sync.Mutex // for f.Write } // initCoverProfile initializes the test coverage profile. @@ -40,16 +39,15 @@ func initCoverProfile() { if err != nil { base.Fatalf("%v", err) } - s, err := fmt.Fprintf(f, "mode: %s\n", cfg.BuildCoverMode) + _, err = fmt.Fprintf(f, "mode: %s\n", cfg.BuildCoverMode) if err != nil { base.Fatalf("%v", err) } coverMerge.f = f - coverMerge.fsize = int64(s) } // mergeCoverProfile merges file into the profile stored in testCoverProfile. -// It prints any errors it encounters to ew. +// Errors encountered are logged and cause a non-zero exit status. func mergeCoverProfile(file string) { if coverMerge.f == nil { return @@ -71,25 +69,20 @@ func mergeCoverProfile(file string) { return } if err != nil || string(buf) != expect { - base.Errorf("error: test wrote malformed coverage profile %s: header %q, expected %q: %v", file, string(buf), expect, err) + base.Errorf("test wrote malformed coverage profile %s: header %q, expected %q: %v", file, string(buf), expect, err) return } - s, err := io.Copy(coverMerge.f, r) + _, err = io.Copy(coverMerge.f, r) if err != nil { - base.Errorf("error: saving coverage profile: %v", err) + base.Errorf("saving coverage profile: %v", err) return } - coverMerge.fsize += s } func closeCoverProfile() { if coverMerge.f == nil { return } - // Discard any partially written data from a failed merge. - if err := coverMerge.f.Truncate(coverMerge.fsize); err != nil { - base.Errorf("closing coverage profile: %v", err) - } if err := coverMerge.f.Close(); err != nil { base.Errorf("closing coverage profile: %v", err) } diff --git a/src/cmd/go/internal/test/test.go b/src/cmd/go/internal/test/test.go index 9ab397bc4da3ba..13952067d789c5 100644 --- a/src/cmd/go/internal/test/test.go +++ b/src/cmd/go/internal/test/test.go @@ -2113,11 +2113,12 @@ func (c *runCache) saveOutput(a *work.Action, coverProfileFile string) { } defer func() { if err := coverProfile.Close(); err != nil && cache.DebugTest { - fmt.Fprintf(os.Stderr, "testcache: %s: closing temporary coverprofile: %v", a.Package.ImportPath, err) + base.Errorf("closing temporary coverprofile: %v", err) } }() } else if cache.DebugTest { - fmt.Fprintf(os.Stderr, "testcache: %s: failed to open temporary coverprofile: %s", a.Package.ImportPath, err) + base.Errorf("failed to open temporary coverprofile: %s", err) + return } } if c.id1 != (cache.ActionID{}) { From 948f23b160cc30e58e8cac3340e8413c5453b329 Mon Sep 17 00:00:00 2001 From: Ryan Currah Date: Mon, 16 Sep 2024 08:18:13 -0400 Subject: [PATCH 3/4] dont return error when no coverage info exists yet --- src/cmd/go/internal/test/test.go | 6 +++--- src/cmd/go/testdata/script/test_cache_inputs.txt | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/cmd/go/internal/test/test.go b/src/cmd/go/internal/test/test.go index 13952067d789c5..5eef3f888fc984 100644 --- a/src/cmd/go/internal/test/test.go +++ b/src/cmd/go/internal/test/test.go @@ -1924,7 +1924,7 @@ func (c *runCache) tryCacheWithID(b *work.Builder, a *work.Action, id string) bo f, _, err := cache.GetFile(cache.Default(), coverProfileAndInputKey(testID, testInputsID)) if err != nil { if cache.DebugTest { - fmt.Fprintf(os.Stderr, "testcache: %s: test coverage profile not found: %v\n", a.Package.ImportPath, err) + fmt.Fprintf(os.Stderr, "testcache: %s: cached test result valid but cached coverage profile missing: %v\n", a.Package.ImportPath, err) } return false } @@ -2117,8 +2117,8 @@ func (c *runCache) saveOutput(a *work.Action, coverProfileFile string) { } }() } else if cache.DebugTest { - base.Errorf("failed to open temporary coverprofile: %s", err) - return + // Not indicative of a problem, as the test may not have generated a cover profile yet. + fmt.Fprintf(os.Stderr, "testcache: %s: opening temporary coverprofile: %s\n", a.Package.ImportPath, err) } } if c.id1 != (cache.ActionID{}) { diff --git a/src/cmd/go/testdata/script/test_cache_inputs.txt b/src/cmd/go/testdata/script/test_cache_inputs.txt index b5c2286ea24950..ee0c2c66bc6a68 100644 --- a/src/cmd/go/testdata/script/test_cache_inputs.txt +++ b/src/cmd/go/testdata/script/test_cache_inputs.txt @@ -128,7 +128,7 @@ go test testcache -run=TestOSArgs -fullpath go test testcache -run=TestOSArgs -fullpath stdout '\(cached\)' -# Ensure that specifying a cover profile does not prevent test results from being cached. +# Ensure that coverage profiles are being cached. go test testcache -run=TestCoverageCache -coverprofile=coverage.out go test testcache -run=TestCoverageCache -coverprofile=coverage.out stdout '\(cached\)' From a3c91979252fb8d4866b9cd3d8f580c9721b4961 Mon Sep 17 00:00:00 2001 From: Ryan Currah Date: Sat, 18 Jan 2025 15:52:36 -0500 Subject: [PATCH 4/4] ensure the coverprofile flag is not empty before retrieving the cover profile cache data --- src/cmd/go/alldocs.go | 6 +-- src/cmd/go/internal/test/cover.go | 3 -- src/cmd/go/internal/test/test.go | 79 +++++++++++++------------------ 3 files changed, 37 insertions(+), 51 deletions(-) diff --git a/src/cmd/go/alldocs.go b/src/cmd/go/alldocs.go index e7f0e606c6681a..547c1002ae827c 100644 --- a/src/cmd/go/alldocs.go +++ b/src/cmd/go/alldocs.go @@ -1889,9 +1889,9 @@ // // The rule for a match in the cache is that the run involves the same // test binary and the flags on the command line come entirely from a -// restricted set of 'cacheable' test flags, defined as -benchtime, -cover, -// -covermode, -coverprofile, -cpu, -list, -parallel, -run, -short, -timeout, -// -failfast, -fullpath and -v. +// restricted set of 'cacheable' test flags, defined as -benchtime, +// -coverprofile, -outputdir, -cpu, -list, -parallel, -run, -short, +// -timeout, -failfast, -fullpath and -v. // If a run of go test has any test or non-test flags outside this set, // the result is not cached. To disable test caching, use any test flag // or argument other than the cacheable flags. The idiomatic way to disable diff --git a/src/cmd/go/internal/test/cover.go b/src/cmd/go/internal/test/cover.go index bc06133efda894..e295c2d90fe40e 100644 --- a/src/cmd/go/internal/test/cover.go +++ b/src/cmd/go/internal/test/cover.go @@ -14,9 +14,6 @@ import ( "sync" ) -// coverMerge manages the state for merging test coverage profiles. -// It ensures thread-safe operations on a single coverage profile file -// across multiple test runs and packages. var coverMerge struct { f *os.File sync.Mutex // for f.Write diff --git a/src/cmd/go/internal/test/test.go b/src/cmd/go/internal/test/test.go index 5eef3f888fc984..5a043fd6f4b063 100644 --- a/src/cmd/go/internal/test/test.go +++ b/src/cmd/go/internal/test/test.go @@ -126,9 +126,9 @@ elapsed time in the summary line. The rule for a match in the cache is that the run involves the same test binary and the flags on the command line come entirely from a -restricted set of 'cacheable' test flags, defined as -benchtime, -cover, --covermode, -coverprofile, -cpu, -list, -parallel, -run, -short, -timeout, --failfast, -fullpath and -v. +restricted set of 'cacheable' test flags, defined as -benchtime, +-coverprofile, -outputdir, -cpu, -list, -parallel, -run, -short, +-timeout, -failfast, -fullpath and -v. If a run of go test has any test or non-test flags outside this set, the result is not cached. To disable test caching, use any test flag or argument other than the cacheable flags. The idiomatic way to disable @@ -1672,7 +1672,7 @@ func (r *runTestActor) Act(b *work.Builder, ctx context.Context, a *work.Action) a.TestOutput = &buf t := fmt.Sprintf("%.3fs", time.Since(t0).Seconds()) - mergeCoverProfile(a.Objdir + "_cover_.out") + mergeCoverProfile(coverProfTempFile(a)) if err == nil { norun := "" @@ -1694,7 +1694,7 @@ func (r *runTestActor) Act(b *work.Builder, ctx context.Context, a *work.Action) cmd.Stdout.Write([]byte("\n")) } fmt.Fprintf(cmd.Stdout, "ok \t%s\t%s%s%s\n", a.Package.ImportPath, t, coveragePercentage(out), norun) - r.c.saveOutput(a, coverProfTempFile(a)) + r.c.saveOutput(a) } else { if testFailFast { testShouldFailFast.Store(true) @@ -1794,14 +1794,7 @@ func (c *runCache) tryCacheWithID(b *work.Builder, a *work.Action, id string) bo cacheArgs = append(cacheArgs, arg) case "-test.coverprofile", "-test.outputdir": - // The `-coverprofile` and `-outputdir` arguments, which - // only affect the location of profile output, are cacheable. This - // is based on the process where, upon a cache hit, stored profile - // data is copied to the specified output location. This mechanism - // ensures that output location preferences are honored without - // modifying the profile's content, thereby justifying their - // cacheability without impacting cache integrity. This allows - // cached coverage profiles to be written to different files. + // These are cacheable and do not invalidate the cache when they change. // Note that this list is documented above, // so if you add to this list, update the docs too. default: @@ -1875,6 +1868,20 @@ func (c *runCache) tryCacheWithID(b *work.Builder, a *work.Action, id string) bo // Parse cached result in preparation for changing run time to "(cached)". // If we can't parse the cached result, don't use it. data, entry, err = cache.GetBytes(cache.Default(), testAndInputKey(testID, testInputsID)) + + // Merge cached cover profile data to cover profile. + if testCoverProfile != "" { + // Specifically ignore entry as it will be the same as above. + cpData, _, err := cache.GetFile(cache.Default(), coverProfileAndInputKey(testID, testInputsID)) + if err != nil { + if cache.DebugTest { + fmt.Fprintf(os.Stderr, "testcache: %s: cached cover profile missing: %v\n", a.Package.ImportPath, err) + } + return false + } + mergeCoverProfile(cpData) + } + if len(data) == 0 || data[len(data)-1] != '\n' { if cache.DebugTest { if err != nil { @@ -1915,21 +1922,6 @@ func (c *runCache) tryCacheWithID(b *work.Builder, a *work.Action, id string) bo j++ } c.buf.Write(data[j:]) - - // Write coverage data to profile. - if cfg.BuildCover { - // The cached coverprofile has the same expiration time as the - // test result it corresponds to. That time is already checked - // above, so we can ignore the entry returned by GetFile here. - f, _, err := cache.GetFile(cache.Default(), coverProfileAndInputKey(testID, testInputsID)) - if err != nil { - if cache.DebugTest { - fmt.Fprintf(os.Stderr, "testcache: %s: cached test result valid but cached coverage profile missing: %v\n", a.Package.ImportPath, err) - } - return false - } - mergeCoverProfile(f) - } return true } @@ -2083,7 +2075,7 @@ func coverProfileAndInputKey(testID, testInputsID cache.ActionID) cache.ActionID return cache.Subkey(testAndInputKey(testID, testInputsID), "coverprofile") } -func (c *runCache) saveOutput(a *work.Action, coverProfileFile string) { +func (c *runCache) saveOutput(a *work.Action) { if c.id1 == (cache.ActionID{}) && c.id2 == (cache.ActionID{}) { return } @@ -2104,21 +2096,14 @@ func (c *runCache) saveOutput(a *work.Action, coverProfileFile string) { if err != nil { return } - var saveCoverProfile = func(testID cache.ActionID) {} - if coverProfileFile != "" { - coverProfile, err := os.Open(coverProfileFile) - if err == nil { - saveCoverProfile = func(testID cache.ActionID) { - cache.Default().Put(coverProfileAndInputKey(testID, testInputsID), coverProfile) + var coverProfile []byte + if testCoverProfile != "" { + coverProfile, err = os.ReadFile(coverProfTempFile(a)) + if err != nil { + if cache.DebugTest { + fmt.Fprintf(os.Stderr, "testcache: %s: reading cover profile: %v\n", a.Package.ImportPath, err) } - defer func() { - if err := coverProfile.Close(); err != nil && cache.DebugTest { - base.Errorf("closing temporary coverprofile: %v", err) - } - }() - } else if cache.DebugTest { - // Not indicative of a problem, as the test may not have generated a cover profile yet. - fmt.Fprintf(os.Stderr, "testcache: %s: opening temporary coverprofile: %s\n", a.Package.ImportPath, err) + return } } if c.id1 != (cache.ActionID{}) { @@ -2127,7 +2112,9 @@ func (c *runCache) saveOutput(a *work.Action, coverProfileFile string) { } cache.PutNoVerify(cache.Default(), c.id1, bytes.NewReader(testlog)) cache.PutNoVerify(cache.Default(), testAndInputKey(c.id1, testInputsID), bytes.NewReader(a.TestOutput.Bytes())) - saveCoverProfile(c.id1) + if coverProfile != nil { + cache.PutNoVerify(cache.Default(), coverProfileAndInputKey(c.id1, testInputsID), bytes.NewReader(coverProfile)) + } } if c.id2 != (cache.ActionID{}) { if cache.DebugTest { @@ -2135,7 +2122,9 @@ func (c *runCache) saveOutput(a *work.Action, coverProfileFile string) { } cache.PutNoVerify(cache.Default(), c.id2, bytes.NewReader(testlog)) cache.PutNoVerify(cache.Default(), testAndInputKey(c.id2, testInputsID), bytes.NewReader(a.TestOutput.Bytes())) - saveCoverProfile(c.id2) + if coverProfile != nil { + cache.PutNoVerify(cache.Default(), coverProfileAndInputKey(c.id2, testInputsID), bytes.NewReader(coverProfile)) + } } }