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

Jump to bottom

cmd/go: -coverpkg=all is not useful #10271

Closed
dvyukov opened this issue Mar 27, 2015 · 10 comments
Closed

cmd/go: -coverpkg=all is not useful #10271

dvyukov opened this issue Mar 27, 2015 · 10 comments
Labels
FrozenDueToAge
Milestone
Go1.10

Comments

@dvyukov
Copy link
Member

dvyukov commented Mar 27, 2015 

$ go test -c -coverpkg=all fmt

tries to build ALL packages that you have in GOROOT and GOPATH. I have tons of stuff there, significant portion of which does not build for various reasons. So the build either fails or takes enormous amount of time and produces tons of "warning: no packages being tested depend on something".

-coverpkg=all should mean "all packages used by the test" rather than "all packages". Note that -coverpkg=all is useful for any kind of coverage-guided fuzzing, and in this case you want to instrument all packages used by the program.

Below is a temporal patch that I use as a workaround. Note that I also exclude "runtime" as it is not useful for coverage-based fuzzing -- e.g. a suddenly triggered GC increases coverage; or receive from empty/non-empty chan gives different coverage; or creation of a goroutine with empty/non-empty local goroutine header cache gives different coverage, etc.
I don't know what is the right solution here: on one hand excluding runtime from all looks illogical; on the other hand if runtime is included into all, -coverpkg=all again becomes useless for coverage-guided fuzzing.

diff --git a/src/cmd/go/test.go b/src/cmd/go/test.go
index c44a219..e310087 100644
--- a/src/cmd/go/test.go
+++ b/src/cmd/go/test.go
@@ -409,11 +409,6 @@ func runTest(cmd *Command, args []string) {
        var builds, runs, prints []*action

        if testCoverPaths != nil {
-               // Load packages that were asked about for coverage.
-               // packagesForBuild exits if the packages cannot be loaded.
-               testCoverPkgs = packagesForBuild(testCoverPaths)
-
-               // Warn about -coverpkg arguments that are not actually used.
                used := make(map[string]bool)
                for _, p := range pkgs {
                        used[p.ImportPath] = true
@@ -421,6 +416,22 @@ func runTest(cmd *Command, args []string) {
                                used[dep] = true
                        }
                }
+
+               if len(testCoverPaths) == 1 && testCoverPaths[0] == "all" {
+                       testCoverPaths = nil
+                       for p := range used {
+                               if p == "command-line-arguments" || p == "unsafe" || p == "runtime" {
+                                       continue
+                               }
+                               testCoverPaths = append(testCoverPaths, p)
+                       }
+               }
+
+               // Load packages that were asked about for coverage.
+               // packagesForBuild exits if the packages cannot be loaded.
+               testCoverPkgs = packagesForBuild(testCoverPaths)
+
+               // Warn about -coverpkg arguments that are not actually used.
                for _, p := range testCoverPkgs {
                        if !used[p.ImportPath] {
                                log.Printf("warning: no packages being tested depend on %s", p.ImportPath)
@dvyukov dvyukov added this to the Go1.5 milestone Mar 27, 2015
@robpike
Copy link
Contributor

robpike commented Mar 27, 2015

Working as intended. The word "all" is known to the go command and it's the go command that's interpreting it, not cover. It would be confusing to change its meaning here, I believe.

@robpike robpike closed this as completed Mar 27, 2015
@dvyukov
Copy link
Member Author

dvyukov commented Mar 27, 2015

What is the intention here? What does it mean and how is it useful to build 10 packages but cover all packages?

@dvyukov
Copy link
Member Author

dvyukov commented Mar 27, 2015

The current behavior makes absolutely no sense to me. What am I missing?

@minux
Copy link
Member

minux commented Mar 27, 2015 via email

@dvyukov
Copy link
Member Author

dvyukov commented Mar 28, 2015

A new mode would work for me. But I still don't see the intention behind current -coverpkg=all behavior and how it can be useful.

@robpike
Copy link
Contributor

robpike commented Mar 28, 2015

Your complaint is like complaining that "cc *" fails because some of the files aren't .c files. "all" is a wildcard and it means the same thing everywhere.

I'm reluctant to add more wildcards to the cover tool's support in the go command, which is too complex already. And even if I weren't, I wouldn't know which ones to add. "deps" sounds ok but means we will cover things like runtime and reflect, which would be pointless.

Solution: List the packages you care about. They're easy to find and easy to specify. Don't use "all".

@dvyukov
Copy link
Member Author

dvyukov commented Mar 29, 2015

No, I am complaining about a different thing. I am complaining how the packages in -coverpkg are used.
When I do:
$ go test -c fmt
crypto/md5 package is not build at all. However, when I do:
$ go test -c -coverpkg=crypto/md5 fmt
crypto/md5 is suddenly built. And the result of the build is unused. Why is that?

The documentation says:
-coverpkg pkg1,pkg2,pkg3
Apply coverage analysis in each test to the given list of packages.

So I would assume that whatever -coverpkg value is or is expanded to, only an intersection of the list and the packages that would be built otherwise should matter.

@Julio-Guerra
Copy link

@robpike
Packages not listed as package dependencies simply shouldn't be included in the final coverage results... They cannot be covered, so the result will always be 0%. It is wrong including it in the resulting coverprofile (or test build)...

So, go test -coverpkg top-level-package/... top-level-package/... should be: "given the package list top-level-package/..., test the list top-level-package/... and cover the intersection between the -coverpkg list and the dependencies of the package being tested".

@golang golang locked and limited conversation to collaborators May 23, 2017
@rsc
Copy link
Contributor

rsc commented Nov 10, 2017

I agree with the disposition of this issue at the time given the go command at the time, but the answer is different today.

We just introduced using the package list syntax as a filter language (for example, go build -gcflags=net/...=-N myprog to build myprog with optimization disabled only in the net packages).

Now that package lists as matchers is an established part of the go command, it makes sense for -coverpkg to be interpreted as filters as well, and then -coverpkg=all means "all the packages being built anyway" and not "all the packages in the world".

I will send a CL doing this.

@rsc rsc reopened this Nov 10, 2017
@rsc rsc modified the milestones: Go1.5, Go1.11, Go1.10 Nov 10, 2017
@rsc rsc assigned rsc and unassigned robpike Nov 10, 2017
jpoirier pushed a commit to jpoirier/go that referenced this issue Nov 28, 2017
@rsc
cmd/go: redefine -coverpkg to be a pattern list
e33794f 
If you run

	go test -coverpkg=all fmt

one possible interpretation is that you want coverage for all the
packages involved in the fmt test, not all the packages in the world.
Because coverpkg was previously defined as a list of packages
to be loaded, however, it meant all packages in the world.

Now that the go command has a concept of package notation
being used as a matching filter instead of a direct enumeration,
apply that to -coverpkg, so that -coverpkg=all now has the
more useful filter interpretation.

Fixes golang#10271.
Fixes golang#21283.

Change-Id: Iddb77b21ba286d3dd65b62507af27e244865072d
Reviewed-on: https://go-review.googlesource.com/76876
Run-TryBot: Russ Cox <rsc@golang.org>
Reviewed-by: David Crawshaw <crawshaw@golang.org>
@rsc
Copy link
Contributor

rsc commented Dec 1, 2017

Fixed by CL 76876 (e33794f). Not sure why it was not closed.

@rsc rsc closed this as completed Dec 1, 2017
@rsc rsc removed their assignment Jun 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge
Projects
None yet
Milestone
Go1.10
Development

No branches or pull requests
6 participants
@rsc @minux @Julio-Guerra @dvyukov @robpike @gopherbot