diff --git a/src/cmd/go/alldocs.go b/src/cmd/go/alldocs.go index a6166a7fdb034..dba4f7badfb0c 100644 --- a/src/cmd/go/alldocs.go +++ b/src/cmd/go/alldocs.go @@ -1801,8 +1801,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, -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 32822950f10a4..5f90949aa9391 100644 --- a/src/cmd/go/go_test.go +++ b/src/cmd/go/go_test.go @@ -6,6 +6,19 @@ package main_test import ( "bytes" + cmdgo "cmd/go" + "cmd/go/internal/base" + "cmd/go/internal/cache" + "cmd/go/internal/cfg" + "cmd/go/internal/gover" + "cmd/go/internal/robustio" + "cmd/go/internal/search" + "cmd/go/internal/toolchain" + "cmd/go/internal/vcs" + "cmd/go/internal/vcweb/vcstest" + "cmd/go/internal/web" + "cmd/go/internal/work" + "cmd/internal/sys" "debug/elf" "debug/macho" "debug/pe" @@ -29,21 +42,6 @@ import ( "strings" "testing" "time" - - "cmd/go/internal/base" - "cmd/go/internal/cache" - "cmd/go/internal/cfg" - "cmd/go/internal/gover" - "cmd/go/internal/robustio" - "cmd/go/internal/search" - "cmd/go/internal/toolchain" - "cmd/go/internal/vcs" - "cmd/go/internal/vcweb/vcstest" - "cmd/go/internal/web" - "cmd/go/internal/work" - "cmd/internal/sys" - - cmdgo "cmd/go" ) func init() { @@ -2801,3 +2799,71 @@ func TestExecInDeletedDir(t *testing.T) { // `go version` should not fail tg.run("version") } + +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") +} diff --git a/src/cmd/go/internal/test/cover.go b/src/cmd/go/internal/test/cover.go index f614458dc46a1..a79de11da6f29 100644 --- a/src/cmd/go/internal/test/cover.go +++ b/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,19 @@ 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) error { if coverMerge.f == nil { - return + return nil } coverMerge.Lock() defer coverMerge.Unlock() @@ -57,28 +59,41 @@ 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) } diff --git a/src/cmd/go/internal/test/test.go b/src/cmd/go/internal/test/test.go index b2b5d340278a2..0df6e09d27191 100644 --- a/src/cmd/go/internal/test/test.go +++ b/src/cmd/go/internal/test/test.go @@ -6,9 +6,21 @@ package test import ( "bytes" + "cmd/go/internal/base" + "cmd/go/internal/cache" + "cmd/go/internal/cfg" + "cmd/go/internal/load" + "cmd/go/internal/lockedfile" + "cmd/go/internal/modload" + "cmd/go/internal/search" + "cmd/go/internal/str" + "cmd/go/internal/trace" + "cmd/go/internal/work" + "cmd/internal/test2json" "context" "errors" "fmt" + "golang.org/x/mod/module" "internal/coverage" "internal/platform" "io" @@ -22,20 +34,6 @@ import ( "strings" "sync" "time" - - "cmd/go/internal/base" - "cmd/go/internal/cache" - "cmd/go/internal/cfg" - "cmd/go/internal/load" - "cmd/go/internal/lockedfile" - "cmd/go/internal/modload" - "cmd/go/internal/search" - "cmd/go/internal/str" - "cmd/go/internal/trace" - "cmd/go/internal/work" - "cmd/internal/test2json" - - "golang.org/x/mod/module" ) // Break init loop. @@ -125,8 +123,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, -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 @@ -1334,6 +1333,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 @@ -1387,13 +1393,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 { @@ -1417,7 +1416,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) } } } @@ -1607,7 +1606,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(a.Objdir + "_cover_.out"); 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 := "" @@ -1629,7 +1631,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 { base.SetExitStatus(1) if cancelSignaled { @@ -1723,7 +1725,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 { @@ -1835,6 +1844,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 } @@ -1981,7 +2012,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 } @@ -2002,12 +2038,31 @@ 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 { @@ -2015,6 +2070,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) } }