Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cmd/go: coverage profile should be cached with tests #23565

Open
bbrks opened this issue Jan 26, 2018 · 28 comments · May be fixed by #65657 or #69339
Open

cmd/go: coverage profile should be cached with tests #23565

bbrks opened this issue Jan 26, 2018 · 28 comments · May be fixed by #65657 or #69339
Labels
NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@bbrks
Copy link

bbrks commented Jan 26, 2018

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
@ALTree ALTree changed the title Coverage profile should be cached with tests cmd/go: coverage profile should be cached with tests Jan 26, 2018
@ALTree ALTree added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Jan 26, 2018
@ALTree ALTree added this to the Go1.11 milestone Jan 26, 2018
@ianlancetaylor
Copy link
Contributor

CC @rsc

@rsc rsc added the NeedsFix The path to resolution is known, but the work has not been done. label Jan 29, 2018
@gopherbot gopherbot removed the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Jan 29, 2018
@gopherbot
Copy link
Contributor

Change https://golang.org/cl/90955 mentions this issue: cmd/go: coverage profile use cache if the set of arguements equals

@ianlancetaylor ianlancetaylor modified the milestones: Go1.11, Go1.12 Jul 6, 2018
@fgrosse
Copy link

fgrosse commented Aug 6, 2018

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?

@bbrks
Copy link
Author

bbrks commented Aug 6, 2018

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

@fgrosse
Copy link

fgrosse commented Aug 6, 2018 via email

@nightlyone
Copy link
Contributor

Is this still planned for Go1.12 ? I guess not...

@andybons andybons modified the milestones: Go1.12, Go1.13 Feb 12, 2019
2opremio added a commit to eksctl-io/eksctl that referenced this issue Jun 27, 2019
It wasn't used and it prevents tests from being cached.

See golang/go#23565 for more details.
2opremio added a commit to eksctl-io/eksctl that referenced this issue Jun 27, 2019
It wasn't used and it prevents tests from being cached.

See golang/go#23565 for more details.
2opremio added a commit to eksctl-io/eksctl that referenced this issue Jun 27, 2019
It wasn't used and it prevents tests from being cached.

See golang/go#23565 for more details.
2opremio added a commit to eksctl-io/eksctl that referenced this issue Jun 28, 2019
It wasn't used and it prevents tests from being cached.

See golang/go#23565 for more details.
@andybons andybons modified the milestones: Go1.13, Go1.14 Jul 8, 2019
@rsc rsc modified the milestones: Go1.14, Backlog Oct 9, 2019
@dinvlad
Copy link

dinvlad commented Sep 23, 2020

Any updates on this? Thanks

@heppu
Copy link

heppu commented Dec 14, 2020

Any plans on this? We have a huge go monorepo and this prevents us from leveragin test cache in our tooling.

@heppu
Copy link

heppu commented Sep 16, 2021

Is this something that could be still implemented or is the idea abandoned?

@simonjpartridge
Copy link

simonjpartridge commented Nov 19, 2021

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.

jproberts added a commit to jproberts/go that referenced this issue Jan 6, 2022
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 golang#23565
tarasmadan added a commit to tarasmadan/syzkaller that referenced this issue Jul 27, 2023
If some job is not a build bottleneck - let's remove caching.
The main focus points then are "build" and "race" then.
To cache test results independently let's use 2 different cache prefixes.
See https://markphelps.me/posts/speed-up-your-go-builds-with-actions-cache/.

The idea is to always generate cash miss using the second precision key.
After the miss we download latest uploaded one using "restore-keys".

Potential improvements:
1. golang/go#23565 asks to cache -coverprofile results since 2018.
2. golang/go#61608 to cache -race results.
@prochac
Copy link

prochac commented Aug 5, 2023

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

github-merge-queue bot pushed a commit to google/syzkaller that referenced this issue Aug 23, 2023
If some job is not a build bottleneck - let's remove caching.
The main focus points then are "build" and "race" then.
To cache test results independently let's use 2 different cache prefixes.
See https://markphelps.me/posts/speed-up-your-go-builds-with-actions-cache/.

The idea is to always generate cash miss using the second precision key.
After the miss we download latest uploaded one using "restore-keys".

Potential improvements:
1. golang/go#23565 asks to cache -coverprofile results since 2018.
2. golang/go#61608 to cache -race results.
macnibblet added a commit to macnibblet/go that referenced this issue Feb 11, 2024
'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 golang#23565

Co-authored-by: James Roberts <jproberts.dev@gmail.com>
macnibblet added a commit to macnibblet/go that referenced this issue Feb 11, 2024
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 golang#23565

Co-authored-by: James Roberts <jproberts.dev@gmail.com>
macnibblet added a commit to macnibblet/go that referenced this issue Feb 11, 2024
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 golang#23565

Co-authored-by: James Roberts <jproberts.dev@gmail.com>
@macnibblet macnibblet linked a pull request Feb 11, 2024 that will close this issue
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/563138 mentions this issue: cmd/go: cache coverage profile with tests

macnibblet added a commit to macnibblet/go that referenced this issue Feb 11, 2024
'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 golang#23565

Co-authored-by: James Roberts <jproberts.dev@gmail.com>
macnibblet added a commit to macnibblet/go that referenced this issue Feb 11, 2024
'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 golang#23565

Co-authored-by: James Roberts <jproberts.dev@gmail.com>
@ryancurrah
Copy link

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.

@ryancurrah
Copy link

@macnibblet you have some feedback on the CL. Not sure if you got notified.

@macnibblet
Copy link

@ryancurrah I have, but I have been swamped with work, but this is still high up on my to do list.

@ryancurrah
Copy link

ryancurrah commented Mar 8, 2024

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

@thanm
Copy link
Contributor

thanm commented Mar 8, 2024

I am willing to pitch in with reviews etc as well if you think it would help. Thanks.

@prochac
Copy link

prochac commented Apr 25, 2024

This is such a long-lasting painful issue 😭 For years, so close to solution, yet so far.

ryancurrah added a commit to ryancurrah/go that referenced this issue Sep 7, 2024
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 golang#50483 and golang#65657 that was
created/closed by @jproberts and @macnibblet that I plan to maintain.

I made improvements to the PR based on feedback from @bcmills.

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 golang#23565

Signed-off-by: Ryan Currah <ryan@currah.ca>
@ryancurrah ryancurrah linked a pull request Sep 7, 2024 that will close this issue
ryancurrah added a commit to ryancurrah/go that referenced this issue Sep 7, 2024
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 golang#50483 and golang#65657 that was
created/closed/abandoned by @jproberts and @macnibblet that I plan to maintain.

I made improvements to the PR based on feedback from @bcmills here https://go-review.googlesource.com/c/go/+/563138.
y
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 golang#23565

Signed-off-by: Ryan Currah <ryan@currah.ca>
ryancurrah added a commit to ryancurrah/go that referenced this issue Sep 7, 2024
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 golang#50483 and golang#65657 that was
created/closed/abandoned by @jproberts and @macnibblet that I plan to maintain.

I made improvements to the PR based on feedback from @bcmills here 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 golang#23565

Signed-off-by: Ryan Currah <ryan@currah.ca>
@ryancurrah
Copy link

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

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/610564 mentions this issue: cmd/go: cache coverage profile with tests

ryancurrah added a commit to ryancurrah/go that referenced this issue Sep 8, 2024
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 golang#50483 and golang#65657 that was
created/closed/abandoned by @jproberts and @macnibblet that I plan to maintain.

I made improvements to the PR based on feedback from @bcmills here 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 golang#23565

Signed-off-by: Ryan Currah <ryan@currah.ca>
ryancurrah added a commit to ryancurrah/go that referenced this issue Sep 8, 2024
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.

- golang#50483
- golang#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 golang#23565
@thanm
Copy link
Contributor

thanm commented Sep 8, 2024

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

ryancurrah added a commit to ryancurrah/go that referenced this issue Sep 8, 2024
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.

- golang#50483
- golang#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 golang#23565
ryancurrah added a commit to ryancurrah/go that referenced this issue Sep 8, 2024
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.

- golang#50483
- golang#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 golang#23565
ryancurrah added a commit to ryancurrah/go that referenced this issue Sep 8, 2024
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.

- golang#50483
- golang#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 golang#23565
ryancurrah added a commit to ryancurrah/go that referenced this issue Sep 8, 2024
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.

- golang#50483
- golang#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 golang#23565
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet