cmd/go: cache coverage profile with tests

src/cmd/go/go_test.go

@@ -2447,6 +2447,68 @@ func TestCacheCoverage(t *testing.T) {
 	tg.run("test", "-cover", "-short", "math", "strings")
 }
 
+func TestCacheCoverageProfile(t *testing.T) {
+	tooSlow(t, "links and runs a test binary multiple times with coverage enabled")
+
+	if gocacheverify.Value() == "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)

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

@@ -16,7 +16,8 @@ import (
 
 var coverMerge struct {
 	f          *os.File
-	sync.Mutex // for f.Write
+	fsize      int64 // size of valid data written to f
+	sync.Mutex       // for f.Write
 }
 
 // initCoverProfile initializes the test coverage profile.
@@ -36,18 +37,18 @@ func initCoverProfile() {
 	if err != nil {
 		base.Fatalf("%v", err)
 	}
-	_, err = fmt.Fprintf(f, "mode: %s\n", cfg.BuildCoverMode)
+	n, err := fmt.Fprintf(f, "mode: %s\n", cfg.BuildCoverMode)
 	if err != nil {
 		base.Fatalf("%v", err)
 	}
 	coverMerge.f = f
+	coverMerge.fsize += int64(n)
 }
 
 // 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()
@@ -57,28 +58,37 @@ func mergeCoverProfile(ew io.Writer, file string) {
 	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 %s.\n", file)
-		return
+		return fmt.Errorf("test wrote malformed coverage profile %s", file)
 	}
-	_, err = io.Copy(coverMerge.f, r)
+	m, err := io.Copy(coverMerge.f, r)
 	if err != nil {
-		fmt.Fprintf(ew, "error: saving coverage profile: %v\n", err)
+		if m > 0 {
+			// Attempt to rollback partial write.
+			coverMerge.f.Seek(coverMerge.fsize, 0)
+		}
+		return fmt.Errorf("saving coverage profile: %w", err)
 	}
+	coverMerge.fsize += m
+	return nil
 }
 
 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)
 	}

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

@@ -124,8 +124,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, 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
@@ -1342,11 +1343,13 @@ func (r *runTestActor) Act(b *work.Builder, ctx context.Context, a *work.Action)
 	}
 	args := str.StringList(execCmd, a.Deps[0].BuiltTarget(), testlogArg, panicArg, fuzzArg, coverdirArg, testArgs)
 
+	var tempCoverProfile string
 	if testCoverProfile != "" {
 		// Write coverage to temporary profile, for merging later.
+		tempCoverProfile = 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=" + tempCoverProfile
 			}
 		}
 	}
@@ -1426,7 +1429,10 @@ 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")
+	if coverErr := mergeCoverProfile(tempCoverProfile); coverErr != nil {
+		// TODO: consider whether an error to merge cover profiles should fail the test.
+		fmt.Fprintf(cmd.Stdout, "error: %v\n", coverErr)
+	}
 
 	if err == nil {
 		norun := ""
@@ -1448,7 +1454,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, tempCoverProfile)
 	} else {
 		base.SetExitStatus(1)
 		if cancelSignaled {
@@ -1541,7 +1547,14 @@ 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 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 {
@@ -1653,6 +1666,28 @@ 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(), 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 {
+			// TODO: consider whether an error to merge cover profiles should fail the test.
+			if cache.DebugTest {
+				fmt.Fprintf(os.Stderr, "testcache: %s: test coverage profile not merged: %v\n", a.Package.ImportPath, err)
+			}
+			return false
+		}
+	}
+
 	return true
 }
 
@@ -1799,7 +1834,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) {
+// 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
 	}
@@ -1820,19 +1860,37 @@ func (c *runCache) saveOutput(a *work.Action) {
 	if err != nil {
 		return
 	}
+	saveCoverProfile := func(testID cache.ActionID) {}
+	if coverprofileFile != "" {
+		coverprof, err := os.Open(coverprofileFile)
+		if err == nil {
+			saveCoverProfile = func(testID cache.ActionID) {
+				cache.Default().Put(testCoverProfileKey(testID, testInputsID), coverprof)
+			}
+			defer func() {
+				if err := coverprof.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 {
 			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()))
+		saveCoverProfile(c.id2)
 	}
 }