Go: cmd/go: -coverpkg=all is not useful

[dvyukov]

$ 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)

[robpike]

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.

[dvyukov]

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

[dvyukov]

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

[minux]

Perhaps it's time to introduce -coverpkg=deps option, which means
all packages depended by the packages on the command line.

[dvyukov]

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]

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]

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]

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

[rsc]

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]

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