As briefly discussed here: https://twitter.com/_rsc/status/956888213314068481
I don't see why Go shouldn't cache the results of -coverprofile when running tests, as test coverage shouldn't vary from run to run, given the same set of arguments.
What version of Go are you using (go version)?
go version go1.10rc1 darwin/amd64
Does this issue reproduce with the latest release?
Yes
What operating system and processor architecture are you using (go env)?
$GOOS="darwin"
$GOARCH="amd64"
What did you do?
Call go test -coverprofile=coverprofile.out ./... multiple times.
What did you expect to see?
Test results cached between runs.
$ go test -coverprofile=coverprofile.out ./...
ok github.com/bbrks/tmp 2.893s coverage: 46.1% of statements
$ go test -coverprofile=coverprofile.out ./...
ok github.com/bbrks/tmp (cached)
What did you see instead?
Test results were not cached between runs.
$ go test -coverprofile=coverprofile.out ./...
ok github.com/bbrks/tmp 2.893s coverage: 46.1% of statements
$ go test -coverprofile=coverprofile.out ./...
ok github.com/bbrks/tmp 2.893s coverage: 46.1% of statements
Comment From: ianlancetaylor
CC @rsc
Comment From: gopherbot
Change https://golang.org/cl/90955 mentions this issue: cmd/go: coverage profile use cache if the set of arguements equals
Comment From: fgrosse
I was hoping this change would make it into go1.11 but it seems its not working yet in go1.11beta3. Looking at the CL it seems it got stuck due to merge conflicts? Any chance we can pick up this issue and still include it in v11?
Comment From: bbrks
@fgrosse Looks like it's being targeted for 1.12 now. I don't think it will be rescheduled this close to 1.11's release.
Comment From: fgrosse
Alright, thanks for the update on this issue.
Ben Brooks notifications@github.com schrieb am Mo., 6. Aug. 2018, 13:34:
@fgrosse https://github.com/fgrosse Looks like it's being targeted for 1.12 now. I don't think it will be rescheduled this close to 1.11's release.
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/golang/go/issues/23565#issuecomment-410678519, or mute the thread https://github.com/notifications/unsubscribe-auth/AAsvTP9Zd6Qik3yRHeE5td0dAye-tjLeks5uOCnbgaJpZM4RuY2- .
Comment From: nightlyone
Is this still planned for Go1.12 ? I guess not...
Comment From: dinvlad
Any updates on this? Thanks
Comment From: heppu
Any plans on this? We have a huge go monorepo and this prevents us from leveragin test cache in our tooling.
Comment From: heppu
Is this something that could be still implemented or is the idea abandoned?
Comment From: simonjpartridge
This is a feature I'd love to see make it into go one day. We run our unit tests against a large monorepo and would like to be able to generate a coverage report for every PR but it's not feasible to do for the entire repo if the test results aren't cached.
Comment From: gopherbot
Change https://golang.org/cl/376134 mentions this issue: cmd/go: cache coverage profile with tests
Comment From: vearutop
Would it be possible to review proposed CL? In my case this feature would allow to save a lot of resources in CI.
Comment From: jproberts
Sadly, I think it's unlikely that the CL will be reviewed now that we're more than a month into the freeze period for 1.19.
When the 1.20 tree is open for development, I'll ping the reviewers again in hopes of moving the CL forward. I'll also propose it as a "early-in-cycle" change on the golang-dev mailing list once that thread is created, which will hopefully raise its priority. If a month passes after the 1.20 tree opens and there are still no reviews, I'll send a new message in the golang-dev mailing list requesting reviews. That seems to be the most effective way to prompt reviews, but it's also noisy so I hope it won't be necessary. If any of these steps sound inappropriate, please let me know. This is my first CL to the Go project and I haven't found clear documentation on what to do when a CL is left in limbo.
In the meantime, I know @vearutop has already tested the changes and seen positive results. If anyone else is willing to test or review the CL, I'm happy to address feedback. Community testing can help strengthen the CL, resulting in fewer iterations needed once members of the Go team have time to review.
Aside from reviews, one other potential blocker could be the WIP changes for #51430. When I read the original proposal, I thought the changes would be independent of this CL but I haven't kept up with the stack of changes in the proposed implementation to be certain. It's also possible I misunderstood the proposal. Either way, @thanm if you can find the time to take a look at CL 376134 and comment on whether it conflicts with your changes for #51430, I'd appreciate it. I understand if that informal review needs to be pushed until 1.20 opens.
Comment From: thanm
@jproberts I looked over your CL and I don't see any significant conflicts with my work, LGTM. I'll leave a couple of minor comments on your CL.
Comment From: karelbilek
@jproberts how is the PR going? I see that it is in the gerrit but no change/messages since October 2022? Which is sad as the PR is basically done?
Comment From: alvaroaleman
What is the general approach in go in case of patches, where the original author doesn't respond anymore? is it ok if someone else picks them up?
Comment From: jproberts
@alvaroaleman I'm not sure the general approach but I've closed my PR. From my perspective, anyone is welcome to attempt to fix this issue.
I've resolved the long existing merge conflicts and pushed them to https://github.com/jproberts/go/commit/ace4a178499cf382b7efed63239818bac4bebc92 . I think that's a good starting point but the impression I've gotten from the reviewers is that they'd prefer to make the current test caching more robust before extending it to profiles. So while I hope my code can still be used, I think there's more work to do and I can't commit to taking that on. I've blocked this long enough. Sorry y'all.
The 1.22 tree is open for development for the next 5 months, so hopefully someone else can push this work through in that time.
Comment From: prochac
Could we, at least, add a note that test cache doesn't work with -race and -coverprofile?
I had to find the hard way, after long time trying and thinking that it's me who's wrong.
Maybe here would be the proper place? https://pkg.go.dev/cmd/go#hdr-Build_and_test_caching
Comment From: gopherbot
Change https://go.dev/cl/563138 mentions this issue: cmd/go: cache coverage profile with tests
Comment From: ryancurrah
I agree with the ChangeList comment—this enhancement does more than just save developer time; it also offers significant environmental benefits. I urge the Go development team to consider this matter with the gravity it deserves. While I understand that it may not be as appealing as a new feature or a runtime issue, and thus might not be prioritized as highly, it nonetheless warrants serious consideration.
Comment From: ryancurrah
@macnibblet you have some feedback on the CL. Not sure if you got notified.
Comment From: awartoft
@ryancurrah I have, but I have been swamped with work, but this is still high up on my to do list.
Comment From: ryancurrah
@macnibblet Brian, who is reviewing your CR is leaving Google. Maybe if we can get the CR comments addressed we can get it merged before he leaves. I have addressed his comments in my diff.
diff --git a/src/cmd/go/alldocs.go b/src/cmd/go/alldocs.go
index dde47ac1b8..f45a85256a 100644
--- a/src/cmd/go/alldocs.go
+++ b/src/cmd/go/alldocs.go
@@ -1803,8 +1803,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 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
diff --git a/src/cmd/go/internal/test/cover.go b/src/cmd/go/internal/test/cover.go
index f614458dc4..461b60c3ca 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,33 @@ 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("error: test wrote malformed coverage profile %s: %w", file, err)
}
_, err = io.Copy(coverMerge.f, r)
if err != nil {
- fmt.Fprintf(ew, "error: saving coverage profile: %v\n", err)
+ return fmt.Errorf("error: saving coverage profile: %w", err)
}
+
+ 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 08fac5f395..f877d27c1a 100644
--- a/src/cmd/go/internal/test/test.go
+++ b/src/cmd/go/internal/test/test.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, -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 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
@@ -1337,6 +1338,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
@@ -1395,13 +1403,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 && p.Internal.Cover.GenMeta {
@@ -1425,7 +1426,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)
+ if err = mergeCoverProfile(cp); err != nil {
+ return err
+ }
}
}
}
@@ -1615,7 +1616,9 @@ 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 {
+ return coverErr
+ }
if err == nil {
norun := ""
@@ -1637,7 +1640,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 {
if testFailFast {
testShouldFailFast.Store(true)
@@ -1735,6 +1738,18 @@ 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, which
+ // only affect the location of profile output, are cacheable. This
+ // is based on the process where, upon a cache hit, stored profile
+ // data is copied to the specified output location. This mechanism
+ // ensures that output location preferences are honored without
+ // modifying the profile's content, thereby justifying their
+ // cacheability without impacting cache integrity. 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
@@ -1847,6 +1862,26 @@ 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 {
+ if cache.DebugTest {
+ fmt.Fprintf(os.Stderr, "testcache: %s: test coverage profile not merged: %v\n", a.Package.ImportPath, err)
+ }
+ return false
+ }
+ }
return true
}
@@ -1993,7 +2028,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
}
@@ -2014,12 +2054,29 @@ 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 {
@@ -2027,6 +2084,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)
}
}
diff --git a/src/cmd/go/testdata/script/test_cache_inputs.txt b/src/cmd/go/testdata/script/test_cache_inputs.txt
index 3705c700d1..1e9ed4cc93 100644
--- a/src/cmd/go/testdata/script/test_cache_inputs.txt
+++ b/src/cmd/go/testdata/script/test_cache_inputs.txt
@@ -114,6 +114,32 @@ go test testcache -run=TestOSArgs -failfast
go test testcache -run=TestOSArgs -failfast
stdout '\(cached\)'
+# Ensure that specifying a cover profile does not prevent test results from being cached.
+go test testcache -run=none -coverprofile=cover.out
+! stdout '\(cached\)'
+go test testcache -run=none -coverprofile=cover.out
+stdout '\(cached\)'
+
+# A new -coverprofile file should use the cached coverage profile contents.
+go test testcache -run=none -coverprofile=cover.out
+go test testcache -run=none -coverprofile=cover1.out
+stdout '\(cached\)'
+
+# Cached coverage profile contents should be the same as the first result.
+go test testcache -run=none -coverprofile=cover.out
+go test testcache -run=none -coverprofile=cover1.out
+cmp cover.out cover1.out
+
+# A new -covermode should not use the cached coverage profile.
+go test testcache -run=none -coverprofile=cover.out
+go test testcache -run=none -covermode=count -coverprofile=cover.out
+! stdout '\(cached\)'
+
+# A new -coverpkg should not use the cached coverage profile.
+go test testcache -run=none -coverprofile=cover.out
+go test testcache -run=none -coverpkg=math -coverprofile=cover.out
+! stdout '\(cached\)'
+
# Executables within GOROOT and GOPATH should affect caching,
# even if the test does not stat them explicitly.
Comment From: thanm
I am willing to pitch in with reviews etc as well if you think it would help. Thanks.
Comment From: prochac
This is such a long-lasting painful issue 😭 For years, so close to solution, yet so far.
Comment From: ryancurrah
@thanm I have a new PR that I will look to see through the review process https://github.com/golang/go/pull/69339. If your generous offer of review is still on the table I would appreciate it.
Comment From: gopherbot
Change https://go.dev/cl/610564 mentions this issue: cmd/go: cache coverage profile with tests
Comment From: thanm
@thanm I have a new PR that I will look to see through the review process #69339. If your generous offer of review is still on the table I would appreciate it.
Happy to review. I am tied up at the moment but I should be able to take a look in a couple of days. Thanks for sending the CL.
Comment From: chudilka1
How the review is going?
Comment From: ryancurrah
I have to get a test working on amd64. One test is failing on amd64 but it works on arm64. I have not had a chance to troubleshoot it, I will try to get to it soon now that it's getting quiet at work with the holidays.
Comment From: ryancurrah
I haven't been able to figure out why the test fails on MacOS amd64. I also ran the test on Linux amd64 and it fails.
https://github.com/golang/go/blob/master/src/cmd/go/testdata/script/cover_statements.txt
# Second run to make sure that caching works properly.
go test -x -cover ./pkg1 ./pkg2 ./pkg3 ./pkg4
[!GOEXPERIMENT:coverageredesign] stdout 'pkg1 \[no test files\]'
[GOEXPERIMENT:coverageredesign] stdout 'pkg1 coverage: 0.0% of statements'
stdout 'pkg2 \S+ coverage: 0.0% of statements \[no tests to run\]'
stdout 'pkg3 \S+ coverage: 100.0% of statements'
stdout 'pkg4 \S+ coverage: \[no statements\]'
[GOEXPERIMENT:coverageredesign] ! stderr 'link(\.exe"?)? -'
! stderr 'compile(\.exe"?)? -'
! stderr 'cover(\.exe"?)? -'
[GOEXPERIMENT:coverageredesign] stderr 'covdata(\.exe"?)? percent'
go test cmd/go -run=TestScript/cover_statements
...
> [!GOEXPERIMENT:coverageredesign] stdout 'pkg1 \[no test files\]'
[condition not met]
> [GOEXPERIMENT:coverageredesign] stdout 'pkg1 coverage: 0.0% of statements'
matched: m/pkg1 coverage: 0.0% of statements
> stdout 'pkg2 \S+ coverage: 0.0% of statements \[no tests to run\]'
matched: ok m/pkg2 (cached) coverage: 0.0% of statements [no tests to run]
> stdout 'pkg3 \S+ coverage: 100.0% of statements'
matched: ok m/pkg3 (cached) coverage: 100.0% of statements
> stdout 'pkg4 \S+ coverage: \[no statements\]'
matched: ok m/pkg4 (cached) coverage: [no statements]
> [GOEXPERIMENT:coverageredesign] ! stderr 'link(\.exe"?)? -'
matched: GOROOT='/Users/ryancurrah/git/github.com/ryancurrah/go' /Users/ryancurrah/git/github.com/ryancurrah/go/pkg/tool/darwin_amd64/link -o $WORK/b137/pkg3.test -importcfg $WORK/b137/importcfg.link -s -w -X=runtime.godebugDefault=asynctimerchan=1,gotestjsonbuildtext=1,gotypesalias=0,httplaxcontentlength=1,httpmuxgo121=1,httpservecontentkeepheaders=1,multipathtcp=0,netedns0=0,panicnil=1,randseednop=0,rsa1024min=0,tls10server=1,tls3des=1,tlsmlkem=0,tlsrsakex=1,tlsunsafeekm=1,winreadlinkvolume=0,winsymlink=0,x509keypairleaf=0,x509negativeserial=1,x509rsacrt=0,x509usepolicies=0 -buildmode=pie -buildid=RR0fN6oYEe615rhfYRgT/f_g3z2Gqfk81p-gSE70t/O9xIcWV6MwuOHOuTbvOZ/RR0fN6oYEe615rhfYRgT -X testing.testBinary=1 -extld=clang /var/folders/fk/8lgm5_252mb0ln68l_z9wlz80000gn/T/cmd-go-test-3563203134/tmpdir2014567110/cover_statements2789456339/cache/ea/ea95a7c155f132e6d1c31c0298df3bbcb4aad1d5df4b580a3c8c9f68769e9cde-d
script_test.go:163: FAIL: testdata/script/cover_statements.txt:26: stderr 'link(\.exe"?)? -': unexpected success
FAIL
FAIL cmd/go 13.300s
FAIL
Comment From: ryancurrah
I figured out that tryCacheWithID was checking cfg.BuildCover to determine whether to retrieve cached coverage profile data, but this wasn’t quite right. The correct variable to check is testCoverProfile, as it specifically indicates whether the current test run is generating a coverage profile. This change ensures coverage data is only cached and retrieved when appropriate, fixing the caching behavior and making it consistent with how saveOutput works. It also resolves issues under GOEXPERIMENT=coverageredesign.
╰─ go test cmd/go -run=TestScript/cover_statements
ok cmd/go 18.393s
╰─ go test cmd/go -run=TestScript/test_cache_inputs
ok cmd/go 63.606s
@thanm, would you be able to take a look at this when you have a chance? I know it’s been a while, so if someone else should review it instead, just let me know!
- Gerrit CR: https://go-review.googlesource.com/c/go/+/610564
- GH PR: https://github.com/golang/go/pull/69339
Comment From: AKhozya
Amazing!
Comment From: thanm
@thanm, would you be able to take a look at this when you have a chance?
I'll look it over some time this week. Thanks for your persistence on this--
Comment From: thanm
I'll look it over some time this week. Thanks for your persistence on this--
Apologies, I have been tied up with other things. Will try to get to this next week.
Comment From: ryancurrah
I'll look it over some time this week. Thanks for your persistence on this--
Apologies, I have been tied up with other things. Will try to get to this next week.
No problem I know 1.24 is coming out soon so it's probably really busy.
Comment From: hihoak
hello, guys, is there any update? Sorry for ping, It is very important feature for us. Can you tell me, feature will work only for -covermode set flag since it set by default with -coverprofile flag?
Comment From: ianlancetaylor
@hihoak Can you explain why this is a very important feature? It seems like a useful but minor optimization. Maybe I am missing something.
Comment From: hihoak
@ianlancetaylor We care a lot about speed of pipelines in our organization and this feature would help us to enable caching of go tests test results, which will significantly increase speed of test jobs. We have around 4000 Go projects in our Gitlab
Comment From: ryancurrah
Companies have code coverage requirements as part of their software quality standards, which mandate a specific percentage of code coverage.
As a result, continuous integration pipelines always run unit tests with coverage profiling enabled.
However, when coverage reporting is enabled, go test does not utilize the build cache. This leads to longer build times and compute resource usage.
For those using the new GOCACHEPROG feature, this issue is exacerbated because the remote build cache fills up quickly due to Go not reusing the test build cache.
This change does not affect every Go developer. It does impact those who use Go in a professional environment.
Comment From: ianlancetaylor
Thanks.
Comment From: ianlancetaylor
CC @matloob @samthanawalla
Comment From: prochac
Companies have code coverage requirements as part of their software quality standards, which mandate a specific percentage of code coverage.
If you need 100% test coverage, feel free to hire me. The correctness of the tested code is not guaranteed tho, just the coverage.
Comment From: slsyy
@ianlancetaylor in my company there was discussion about turning the coverage off:
* without coverage tests are well cached and run almost instantly
* with coverage tests are slow, but we really like features like diff lines are marked 🟢/🔴 based on a coverage
With this feature there is no need for a difficult choice between goodies and performance
Comment From: chudilka1
Right to a bull's eye from @ryancurrah
This change does not affect every Go developer. It does impact those who use Go in a professional environment.
The very same case, one of our pipelines ran up to 45 mins w/o caching, and up to 10 with it. So, it is not a minor feature, it is underevaluated here.
Comment From: ryancurrah
CL 610564 got a thumbs up. Waiting on another review.
Comment From: ryancurrah
@thanm sorry to bug you again. I had to rebase my change because there was a conflict in the docs. I ordered the flags alphabetically which requires your re-review. http://go-review.googlesource.com/c/go/+/610564/16/src/cmd/go/alldocs.go
CL: https://go-review.googlesource.com/c/go/+/610564
Comment From: thanm
@ryancurrah I'll take a look. Thank you for your persistence in getting this CL moved forward, I believe it will be a valuable change to make.
Comment From: ruoibmt
@thanm On March 6, this modification was merged to master. Do you intend to update it to the upcoming minor version of 1.23 or 1.24?
Comment From: thanm
It is not my intent to back-port this change. The general policy on back-porting is that the fix needs to address a serious problem (compiler or runtime crash, computing incorrect results, that sort of thing). Performance improvements typically don't meet those criteria. I could nominate the bug for back-porting, but I would be surprised if the release team approved it.
Comment From: hihoak
Hello guys,
Thanks a lot for resolving this issue.
I started testing this feature and encountered a problem that occurs when I use the go test command with the -coverpkg flag.
When coverage is collected with the -coverpkg flag and test result caching is enabled, the generated report may include lines that no longer exist. This happens because:
-
Each test package attempts to collect coverage for all packages matching the
-coverpkgpattern. -
If the test result is loaded from the cache, the coverage data may be outdated—especially if the test package does not directly or indirectly depend on the modified code, leaving the cache uninvalidated.
for example we have project with layout
Project layout
proj/
some_func.go
some_func_test.go
sub/
sub.go
sub_test.go
sum/
sum.go
Files Content
some_func.go
package proj
import "proj/sum"
func SomeFunc(a, b int) int {
if a == 0 && b == 0 {
return 0
}
return sum.Sum(a, b)
}
sub.go
package sub
func Sub(a, b int) int {
if a == 0 && b == 0 {
return 0
}
return a - b
}
sum.go
package sum
func Sum(a, b int) int {
if a == 0 {
return b
}
return a + b
}
some_func_test.go
package proj
import (
"github.com/stretchr/testify/require"
"testing"
)
func Test_SomeFunc(t *testing.T) {
t.Run("test1", func(t *testing.T) {
require.Equal(t, 2, SomeFunc(1, 1))
})
}
sub_test.go
package sub
import (
"github.com/stretchr/testify/require"
"testing"
)
func Test_Sub(t *testing.T) {
t.Run("test_sub1", func(t *testing.T) {
require.Equal(t, 0, Sub(1, 1))
})
}
Coverage result of this tests
mode: set
proj/some_func.go:5.29,6.22 1 1
proj/some_func.go:6.22,8.3 1 0
proj/some_func.go:9.2,9.22 1 1
proj/sub/sub.go:3.24,4.22 1 0
proj/sub/sub.go:4.22,6.3 1 0
proj/sub/sub.go:7.2,7.14 1 0
proj/sum/sum.go:3.24,4.12 1 1
proj/sum/sum.go:4.12,6.3 1 0
proj/sum/sum.go:7.2,7.14 1 1
proj/some_func.go:5.29,6.22 1 0
proj/some_func.go:6.22,8.3 1 0
proj/some_func.go:9.2,9.22 1 0
proj/sub/sub.go:3.24,4.22 1 1
proj/sub/sub.go:4.22,6.3 1 0
proj/sub/sub.go:7.2,7.14 1 1
proj/sum/sum.go:3.24,4.12 1 0
proj/sum/sum.go:4.12,6.3 1 0
proj/sum/sum.go:7.2,7.14 1 0
proj/sum/sum.go:3.24,4.12 1 0
proj/sum/sum.go:4.12,6.3 1 0
proj/sum/sum.go:7.2,7.14 1 0
Change a bit sub.go
sub.go
package sub
func Sub(a, b int) int {
if a == 0 && b == 0 || a == -100 {
return 0
}
return a - b
}
Coverage result after change
mode: set
proj/some_func.go:5.29,6.22 1 1
proj/some_func.go:6.22,8.3 1 0
proj/some_func.go:9.2,9.22 1 1
proj/sub/sub.go:3.24,4.22 1 0
proj/sub/sub.go:4.22,6.3 1 0
proj/sub/sub.go:7.2,7.14 1 0
proj/sum/sum.go:3.24,4.12 1 1
proj/sum/sum.go:4.12,6.3 1 0
proj/sum/sum.go:7.2,7.14 1 1
proj/some_func.go:5.29,6.22 1 0
proj/some_func.go:6.22,8.3 1 0
proj/some_func.go:9.2,9.22 1 0
proj/sub/sub.go:3.24,4.35 1 1
proj/sub/sub.go:4.35,6.3 1 0
proj/sub/sub.go:7.2,7.14 1 1
proj/sum/sum.go:3.24,4.12 1 0
proj/sum/sum.go:4.12,6.3 1 0
proj/sum/sum.go:7.2,7.14 1 0
proj/sum/sum.go:3.24,4.12 1 0
proj/sum/sum.go:4.12,6.3 1 0
proj/sum/sum.go:7.2,7.14 1 0
Conclusion
so there are now 2 versions of sub.go:3.24 line in one report and proj/sub/sub.go:3.24,4.22 1 0 doesn't exist
as a result it can breaks up coverage tools to generate cobertura coverage
Comment From: ryancurrah
Can you provide the go test commands your running to get these results.
Comment From: hihoak
first run
go test -coverpkg=proj/... -coverprofile=cover.out ./proj/...
ok proj (cached) coverage: 44.4% of statements in proj/...
ok proj/sub (cached) coverage: 22.2% of statements in proj/...
proj/sum coverage: 0.0% of statements
second run
go test -coverpkg=proj/... -coverprofile=cover.out ./proj/...
ok proj (cached) coverage: 44.4% of statements in proj/...
ok proj/sub 0.005s coverage: 22.2% of statements in proj/...
proj/sum coverage: 0.0% of statements
Comment From: ryancurrah
My immediate thought here is to disable coverage caching when -coverpkg is used. Otherwise we will need to figure out how to determine if any of the packages found in coverpkg changed. Which I'm not sure how to do. If none of these options sound good we should revert this CL.
Comment From: hihoak
All of these "phantom" lines always have 0 hits; otherwise, they would be invalidated (since they would have been imported by modified code).
This fact can be used to ignore them in tools that generate reports.
That said, I agree, this behavior does seem unintuitive and non-obvious at first glance