Skip to content

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

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

Closed
bbrks opened this issue Jan 26, 2018 · 50 comments · May be fixed by #65657
Closed

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

bbrks opened this issue Jan 26, 2018 · 50 comments · May be fixed by #65657
Labels
GoCommand cmd/go 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
Member

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
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
@chudilka1
Copy link

How the review is going?

@ryancurrah
Copy link
Contributor

ryancurrah commented Nov 29, 2024

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.

ryancurrah added a commit to ryancurrah/go that referenced this issue Dec 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
Copy link
Contributor

ryancurrah commented Jan 25, 2025

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

ryancurrah added a commit to ryancurrah/go that referenced this issue Jan 25, 2025
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
Copy link
Contributor

ryancurrah commented Jan 25, 2025

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!

@AKhozya
Copy link

AKhozya commented Jan 27, 2025

Amazing!

@thanm
Copy link
Contributor

thanm commented Jan 27, 2025

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

@thanm
Copy link
Contributor

thanm commented Feb 2, 2025

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.

@ryancurrah
Copy link
Contributor

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.

@hihoak
Copy link

hihoak commented Feb 19, 2025

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?

@ianlancetaylor
Copy link
Member

@hihoak Can you explain why this is a very important feature? It seems like a useful but minor optimization. Maybe I am missing something.

@hihoak
Copy link

hihoak commented Feb 19, 2025

@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

@ryancurrah
Copy link
Contributor

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.

@ianlancetaylor
Copy link
Member

Thanks.

@ianlancetaylor
Copy link
Member

CC @matloob @samthanawalla

@ianlancetaylor ianlancetaylor added the GoCommand cmd/go label Feb 19, 2025
@prochac
Copy link

prochac commented Feb 19, 2025

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.

@slsyy
Copy link

slsyy commented Feb 20, 2025

@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

@chudilka1
Copy link

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.

@ryancurrah
Copy link
Contributor

ryancurrah commented Feb 27, 2025

CL 610564 got a thumbs up. Waiting on another review.

@ryancurrah
Copy link
Contributor

ryancurrah commented Mar 5, 2025

@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

@thanm
Copy link
Contributor

thanm commented Mar 5, 2025

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

@ruoibmt
Copy link

ruoibmt commented Apr 17, 2025

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

@thanm
Copy link
Contributor

thanm commented Apr 18, 2025

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
GoCommand cmd/go NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet