]> Cypherpunks repositories - gostls13.git/commitdiff
cmd/go: cache coverage profile with tests
authorRyan Currah <ryan@currah.ca>
Tue, 25 Feb 2025 15:51:56 +0000 (15:51 +0000)
committerMichael Matloob <matloob@golang.org>
Thu, 6 Mar 2025 16:27:15 +0000 (08:27 -0800)
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

Change-Id: I59a20af5ea156f990a17544cf06dc667ae7f8aa3
GitHub-Last-Rev: a5a1d1b9c87ff433d16f656fc8988e1cb1ce7100
GitHub-Pull-Request: golang/go#69339
Reviewed-on: https://go-review.googlesource.com/c/go/+/610564
Reviewed-by: Michael Matloob <matloob@golang.org>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Than McIntosh <thanm@golang.org>
Reviewed-by: Junyang Shao <shaojunyang@google.com>
src/cmd/go/alldocs.go
src/cmd/go/internal/test/cover.go
src/cmd/go/internal/test/test.go
src/cmd/go/testdata/script/test_cache_inputs.txt

index dba37e891def0229c7b95c74d359485c7cbe7c7e..7063a9f216f07c203d444424a3a019a2f0136b1c 100644 (file)
 //
 // 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, -skip, -timeout, -failfast, -fullpath and -v.
+// restricted set of 'cacheable' test flags, defined as -benchtime,
+// -coverprofile, -cpu, -failfast, -fullpath, -list, -outputdir, -parallel,
+// -run, -short, -skip, -timeout 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
index f614458dc46a15baead3d1efa7839e0593894eee..e295c2d90fe40e315e116526a0118b85a915bc66 100644 (file)
@@ -44,8 +44,8 @@ func initCoverProfile() {
 }
 
 // mergeCoverProfile merges file into the profile stored in testCoverProfile.
-// It prints any errors it encounters to ew.
-func mergeCoverProfile(ew io.Writer, file string) {
+// Errors encountered are logged and cause a non-zero exit status.
+func mergeCoverProfile(file string) {
        if coverMerge.f == nil {
                return
        }
@@ -66,12 +66,13 @@ 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("test wrote malformed coverage profile %s: header %q, expected %q: %v", file, string(buf), expect, err)
                return
        }
        _, err = io.Copy(coverMerge.f, r)
        if err != nil {
-               fmt.Fprintf(ew, "error: saving coverage profile: %v\n", err)
+               base.Errorf("saving coverage profile: %v", err)
+               return
        }
 }
 
index 2ee2aa6f415d5ccc6239cc5e8266768e64158bb3..b842c2f48ecabdcc9d081d933786c3c4913f96dc 100644 (file)
@@ -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, -skip, -timeout, -failfast, -fullpath and -v.
+restricted set of 'cacheable' test flags, defined as -benchtime,
+-coverprofile, -cpu, -failfast, -fullpath, -list, -outputdir, -parallel,
+-run, -short, -skip, -timeout 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
@@ -1375,6 +1376,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
@@ -1476,13 +1484,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 && p.Internal.Cover.GenMeta {
@@ -1506,7 +1507,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)
                                }
                        }
                }
@@ -1669,7 +1670,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(coverProfTempFile(a))
 
        if err == nil {
                norun := ""
@@ -1790,7 +1791,11 @@ 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":
+                       // 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:
                        // nothing else is cacheable
                        if cache.DebugTest {
@@ -1862,6 +1867,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 {
@@ -2050,6 +2069,11 @@ func testAndInputKey(testID, testInputsID cache.ActionID) cache.ActionID {
        return cache.Subkey(testID, fmt.Sprintf("inputs:%x", testInputsID))
 }
 
+// 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) {
        if c.id1 == (cache.ActionID{}) && c.id2 == (cache.ActionID{}) {
                return
@@ -2071,12 +2095,25 @@ func (c *runCache) saveOutput(a *work.Action) {
        if err != nil {
                return
        }
+       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)
+                       }
+                       return
+               }
+       }
        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()))
+               if coverProfile != nil {
+                       cache.PutNoVerify(cache.Default(), coverProfileAndInputKey(c.id1, testInputsID), bytes.NewReader(coverProfile))
+               }
        }
        if c.id2 != (cache.ActionID{}) {
                if cache.DebugTest {
@@ -2084,6 +2121,9 @@ 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()))
+               if coverProfile != nil {
+                       cache.PutNoVerify(cache.Default(), coverProfileAndInputKey(c.id2, testInputsID), bytes.NewReader(coverProfile))
+               }
        }
 }
 
index 796a5880eb6146be16d7a1a01bc9ad25f32541c8..29e538c11ece709f7c86bd414b4f5c985eae46da 100644 (file)
@@ -134,6 +134,46 @@ go test testcache -run=TestOdd -skip=TestOddFile
 go test testcache -run=TestOdd -skip=TestOddFile
 stdout '\(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\)'
+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.
 
@@ -164,6 +204,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
@@ -267,6 +319,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