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.

- #50483
- #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: a5a1d1b
GitHub-Pull-Request: #69339
Reviewed-on: https://go-review.googlesource.com/c/go/+/610564
Reviewed-by: Michael Matloob <[email protected]>
LUCI-TryBot-Result: Go LUCI <[email protected]>
Reviewed-by: Than McIntosh <[email protected]>
Reviewed-by: Junyang Shao <[email protected]>

Author: ryancurrah

src/cmd/go/alldocs.go

@@ -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
 	}
 }
 

src/cmd/go/internal/test/cover.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, -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,19 +2095,35 @@ 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 {
 			fmt.Fprintf(os.Stderr, "testcache: %s: save test ID %x => input ID %x => %x\n", a.Package.ImportPath, c.id2, testInputsID, testAndInputKey(c.id2, testInputsID))
 		}
 		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))
+		}
 	}
 }
 

src/cmd/go/internal/test/test.go

@@ -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