[go] cmd/go: cache coverage profile with tests

Gerrit Dou (Gerrit)

unread,

Jan 6, 2022, 6:15:36 PM1/6/22

to goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com

Gerrit Dou has uploaded this change for review.

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.

Fixes #23565

Change-Id: Ifcf8999fa6de917ea20a52a3e3293d9ded0bf080
GitHub-Last-Rev: 9ada3115023e79fe43311b684fef21ebcc63e1a0
GitHub-Pull-Request: golang/go#50483
---
M src/cmd/go/internal/test/test.go
M src/cmd/go/internal/test/cover.go
M src/cmd/go/go_test.go
M src/cmd/go/alldocs.go
4 files changed, 154 insertions(+), 17 deletions(-)

diff --git a/src/cmd/go/alldocs.go b/src/cmd/go/alldocs.go
index d903214..a145bc1 100644
--- a/src/cmd/go/alldocs.go
+++ b/src/cmd/go/alldocs.go
@@ -1681,8 +1681,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, and -v.
+// restricted set of 'cacheable' test flags, defined as -benchtime, -cover,
+// -covermode, -coverprofile, -cpu, -list, -parallel, -run, -short, -timeout,
+// -failfast, -v, -vet and -outputdir.
 // 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/go_test.go b/src/cmd/go/go_test.go
index 170c882..625c854 100644
--- a/src/cmd/go/go_test.go
+++ b/src/cmd/go/go_test.go
@@ -2320,6 +2320,68 @@
 	tg.run("test", "-cover", "-short", "math", "strings")
 }
 
+func TestCacheCoverageProfile(t *testing.T) {
+	tooSlow(t)
+
+	if godebug.Get("gocacheverify") == "1" {
+		t.Skip("GODEBUG gocacheverify")
+	}
+
+	tg := testgo(t)
+	defer tg.cleanup()
+	tg.parallel()
+	tg.setenv("GOPATH", filepath.Join(tg.pwd(), "testdata"))
+	tg.makeTempdir()
+	tg.setenv("GOCACHE", tg.path("c1"))
+
+	// checkProfile asserts that the given profile contains the given mode
+	// and coverage lines for all given files.
+	checkProfile := func(t *testing.T, profile, mode string, files ...string) {
+		t.Helper()
+		if out, err := os.ReadFile(profile); err != nil {
+			t.Fatalf("failed to open coverprofile: %v", err)
+		} else {
+			if n := bytes.Count(out, []byte("mode: "+mode)); n != 1 {
+				if n == 0 {
+					t.Fatalf("missing mode: %s", mode)
+				} else {
+					t.Fatalf("too many mode: %s", mode)
+				}
+			}
+			for _, fname := range files {
+				if !bytes.Contains(out, []byte(fname)) {
+					t.Fatalf("missing file in coverprofile: %s", fname)
+				}
+			}
+		}
+	}
+
+	tg.run("test", "-coverprofile="+tg.path("cover.out"), "-x", "-v", "-short", "strings")
+	tg.grepStdout(`ok  \t`, "expected strings test to succeed")
+	checkProfile(t, tg.path("cover.out"), "set", "strings/strings.go")
+	// Repeat commands should use the cache.
+	tg.run("test", "-coverprofile="+tg.path("cover.out"), "-x", "-v", "-short", "strings")
+	tg.grepStdout(`ok  \tstrings\t\(cached\)`, "expected strings test results to be cached")
+	checkProfile(t, tg.path("cover.out"), "set", "strings/strings.go")
+	// Cover profiles should be cached independently. Since strings is already cached,
+	// only math should need to run.
+	tg.run("test", "-coverprofile="+tg.path("cover.out"), "-x", "-v", "-short", "strings", "math")
+	tg.grepStdout(`ok  \tstrings\t\(cached\)`, "expected strings test results to be cached")
+	checkProfile(t, tg.path("cover.out"), "set", "strings/strings.go", "math/mod.go")
+	// A new -coverprofile file should use the cached coverage profile contents.
+	tg.run("test", "-coverprofile="+tg.path("cover1.out"), "-x", "-v", "-short", "strings")
+	tg.grepStdout(`ok  \tstrings\t\(cached\)`, "expected cached strings test results to be used regardless of -coverprofile")
+	checkProfile(t, tg.path("cover1.out"), "set", "strings/strings.go")
+	// A new -covermode should not use the cached coverage profile, since the covermode changes
+	// the profile output.
+	tg.run("test", "-covermode=count", "-coverprofile="+tg.path("cover.out"), "-x", "-v", "-short", "strings")
+	tg.grepStdoutNot(`ok  \tstrings\t\(cached\)`, "cached strings test results should not be used with different -covermode")
+	// A new -coverpkg should not use the cached coverage profile, since the coverpkg changes
+	// the profile output.
+	tg.run("test", "-coverpkg=math", "-coverprofile="+tg.path("cover.out"), "-x", "-v", "-short", "strings")
+	tg.grepStdoutNot(`ok  \tstrings\t\(cached\)`, "cached strings test results should not be used with different -coverpkg")
+}
+
 func TestIssue22588(t *testing.T) {
 	// Don't get confused by stderr coming from tools.
 	tg := testgo(t)
diff --git a/src/cmd/go/internal/test/cover.go b/src/cmd/go/internal/test/cover.go
index 657d22a..6be2e41 100644
--- a/src/cmd/go/internal/test/cover.go
+++ b/src/cmd/go/internal/test/cover.go
@@ -6,6 +6,7 @@
 
 import (
 	"cmd/go/internal/base"
+	"errors"
 	"fmt"
 	"io"
 	"os"
@@ -43,10 +44,9 @@
 }
 
 // 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) error {
 	if coverMerge.f == nil {
-		return
+		return nil
 	}
 	coverMerge.Lock()
 	defer coverMerge.Unlock()
@@ -56,22 +56,22 @@
 	r, err := os.Open(file)
 	if err != nil {
 		// Test did not create profile, which is OK.
-		return
+		return nil
 	}
 	defer r.Close()
 
 	n, err := io.ReadFull(r, buf)
 	if n == 0 {
-		return
+		return nil
 	}
 	if err != nil || string(buf) != expect {
-		fmt.Fprintf(ew, "error: test wrote malformed coverage profile.\n")
-		return
+		return errMalformedCoverProfile
 	}
 	_, err = io.Copy(coverMerge.f, r)
 	if err != nil {
-		fmt.Fprintf(ew, "error: saving coverage profile: %v\n", err)
+		return fmt.Errorf("saving coverage profile: %v\n", err)
 	}
+	return nil
 }
 
 func closeCoverProfile() {
@@ -82,3 +82,5 @@
 		base.Errorf("closing coverage profile: %v", err)
 	}
 }
+
+var errMalformedCoverProfile = errors.New("test wrote malformed coverage profile")
diff --git a/src/cmd/go/internal/test/test.go b/src/cmd/go/internal/test/test.go
index 50e6d52..2d34a73 100644
--- a/src/cmd/go/internal/test/test.go
+++ b/src/cmd/go/internal/test/test.go
@@ -123,8 +123,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, and -v.
+restricted set of 'cacheable' test flags, defined as -benchtime, -cover,
+-covermode, -coverprofile, -cpu, -list, -parallel, -run, -short, -timeout,
+-failfast, -v, -vet and -outputdir.
 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
@@ -1336,11 +1337,13 @@
 	}
 	args := str.StringList(execCmd, a.Deps[0].BuiltTarget(), testlogArg, panicArg, fuzzArg, testArgs)
 
+	var coverprofileFile string
 	if testCoverProfile != "" {
 		// Write coverage to temporary profile, for merging later.
+		coverprofileFile = a.Objdir + "_cover_.out"
 		for i, arg := range args {
 			if strings.HasPrefix(arg, "-test.coverprofile=") {
-				args[i] = "-test.coverprofile=" + a.Objdir + "_cover_.out"
+				args[i] = "-test.coverprofile=" + coverprofileFile
 			}
 		}
 	}
@@ -1417,7 +1420,9 @@
 	a.TestOutput = &buf
 	t := fmt.Sprintf("%.3fs", time.Since(t0).Seconds())
 
-	mergeCoverProfile(cmd.Stdout, a.Objdir+"_cover_.out")
+	if coverErr := mergeCoverProfile(coverprofileFile); coverErr != nil {
+		fmt.Fprintf(cmd.Stdout, "error: %v\n", coverErr)
+	}
 
 	if err == nil {
 		norun := ""
@@ -1439,7 +1444,7 @@
 			cmd.Stdout.Write([]byte("\n"))
 		}
 		fmt.Fprintf(cmd.Stdout, "ok  \t%s\t%s%s%s\n", a.Package.ImportPath, t, coveragePercentage(out), norun)
-		c.saveOutput(a)
+		c.saveOutput(a, coverprofileFile)
 	} else {
 		base.SetExitStatus(1)
 		if len(out) == 0 {
@@ -1520,7 +1525,14 @@
 			// 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 are cacheable but
+			// only change where profiles are written. They don't change the
+			// profile contents, so they aren't added to the cacheArgs. 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 {
@@ -1640,6 +1652,26 @@
 		j++
 	}
 	c.buf.Write(data[j:])
+
+	// Write coverage data to profile.
+	if testCover {
+		// coverprofile cache expiration time should be coupled with the test data above, so
+		// the entry can be ignored.
+		f, _, err := cache.Default().GetFile(testCoverProfileKey(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
+		}
+		if err := mergeCoverProfile(f); err != nil {
+			if cache.DebugTest {
+				fmt.Fprintf(os.Stderr, "testcache: %s: test coverage profile not merged: %v\n", a.Package.ImportPath, err)
+			}
+			return false
+		}
+	}
+
 	return true
 }
 
@@ -1788,7 +1820,12 @@
 	return cache.Subkey(testID, fmt.Sprintf("inputs:%x", testInputsID))
 }
 
-func (c *runCache) saveOutput(a *work.Action) {
+// testCoverProfileKey returns the "coverprofile" cache key for the pair (testID, testInputsID).
+func testCoverProfileKey(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
 	}
@@ -1809,12 +1846,27 @@
 	if err != nil {
 		return
 	}
+	saveCoverProfile := func(testID cache.ActionID) {
+		if coverprofileFile == "" {
+			return
+		}
+		coverprof, err := os.Open(coverprofileFile)
+		if err != nil {
+			if cache.DebugTest {
+				fmt.Fprintf(os.Stderr, "testcache: %s: failed to open temporary coverprofile file: %s", a.Package.ImportPath, err)
+			}
+			return
+		}
+		defer coverprof.Close()
+		cache.Default().Put(testCoverProfileKey(testID, testInputsID), coverprof)
+	}
 	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.Default().PutNoVerify(c.id1, bytes.NewReader(testlog))
 		cache.Default().PutNoVerify(testAndInputKey(c.id1, testInputsID), bytes.NewReader(a.TestOutput.Bytes()))
+		saveCoverProfile(c.id1)
 	}
 	if c.id2 != (cache.ActionID{}) {
 		if cache.DebugTest {
@@ -1822,6 +1874,7 @@
 		}
 		cache.Default().PutNoVerify(c.id2, bytes.NewReader(testlog))
 		cache.Default().PutNoVerify(testAndInputKey(c.id2, testInputsID), bytes.NewReader(a.TestOutput.Bytes()))
+		saveCoverProfile(c.id2)
 	}
 }

To view, visit change 376134. To unsubscribe, or for help writing mail filters, visit settings.

Gopher Robot (Gerrit)

unread,

Jan 6, 2022, 6:16:12 PM1/6/22

to Gerrit Dou, goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com

Congratulations on opening your first change. Thank you for your contribution!

Next steps:
A maintainer will review your change and provide feedback. See
https://golang.org/doc/contribute.html#review for more info and tips to get your
patch through code review.

Most changes in the Go project go through a few rounds of revision. This can be
surprising to people new to the project. The careful, iterative review process
is our way of helping mentor contributors and ensuring that their contributions
have a lasting impact.

During May-July and Nov-Jan the Go project is in a code freeze, during which
little code gets reviewed or merged. If a reviewer responds with a comment like
R=go1.11 or adds a tag like "wait-release", it means that this CL will be
reviewed as part of the next development cycle. See https://golang.org/s/release
for more details.

[go] cmd/go: cache coverage profile with tests

Gerrit Dou (Gerrit)

Gopher Robot (Gerrit)

James Roberts (Gerrit)

Than McIntosh (Gerrit)

Gerrit Bot (Gerrit)

James Roberts (Gerrit)

Bryan Mills (Gerrit)

Gerrit Bot (Gerrit)

Gerrit Bot (Gerrit)

James Roberts (Gerrit)

Bryan Mills (Gerrit)

Gerrit Bot (Gerrit)

James Roberts (Gerrit)

Than McIntosh (Gerrit)

James Roberts (Gerrit)

Gerrit Bot (Gerrit)

Gerrit Bot (Gerrit)

James Roberts (Gerrit)

Gopher Robot (Gerrit)