cmd/go: coverpkg doesn't ignore directories starting with '.' #66038

matthewhughes934 · 2024-02-29T20:12:29Z

Go version

go version go1.22.0 linux/amd64

Output of `go env` in your module/workspace:

GO111MODULE=''
GOARCH='amd64'
GOBIN=''
GOCACHE='/home/mjh/.cache/go-build'
GOENV='/home/mjh/.config/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFLAGS=''
GOHOSTARCH='amd64'
GOHOSTOS='linux'
GOINSECURE=''
GOMODCACHE='/home/mjh/.local/share/go/pkg/mod'
GONOPROXY=''
GONOSUMDB=''
GOOS='linux'
GOPATH='/home/mjh/.local/share/go'
GOPRIVATE=''
GOPROXY='https://proxy.golang.org,direct'
GOROOT='/home/mjh/sdk/go1.22.0'
GOSUMDB='sum.golang.org'
GOTMPDIR=''
GOTOOLCHAIN='auto'
GOTOOLDIR='/home/mjh/sdk/go1.22.0/pkg/tool/linux_amd64'
GOVCS=''
GOVERSION='go1.22.0'
GCCGO='gccgo'
GOAMD64='v1'
AR='ar'
CC='gcc'
CXX='g++'
CGO_ENABLED='1'
GOMOD='/home/mjh/src/personal/go-cov/go.mod'
GOWORK=''
CGO_CFLAGS='-O2 -g'
CGO_CPPFLAGS=''
CGO_CXXFLAGS='-O2 -g'
CGO_FFLAGS='-O2 -g'
CGO_LDFLAGS='-O2 -g'
PKG_CONFIG='pkg-config'
GOGCCFLAGS='-fPIC -m64 -pthread -Wl,--no-gc-sections -fmessage-length=0 -ffile-prefix-map=/tmp/go-build3982892531=/tmp/go-build -gno-record-gcc-switches'

What did you do?

Ran Go tests with -coverpkg=./... in a directory which included GOMODCACHE/GOCACHE in a subdirectory starting with "." (the use case is for caching in GitLab, which requires directories stored in caches exist in the working directory, see their Go example https://docs.gitlab.com/ee/ci/caching/#cache-go-dependencies)

Here's a bash script to reproduce:

#!/usr/bin/env bash

set -o errexit -o pipefail -o nounset

mkdir proj
cd proj
go mod init example-proj

# create a trivial lib with 100% test coverage
cat <<EOF > lib.go
package lib

import (
	// add an unused dependency, just so something is stored in the cache directory
	_ "golang.org/x/tools/cover"
)

func Sum(a int, b int) int { 
	return a + b
}
EOF

cat <<EOF > lib_test.go
package lib

import (
	"testing"
)

func TestSum(t *testing.T) {
	if Sum(1, 2) != 3 {
		t.Error("fail")
	}
}
EOF

# .go contains directories we wish to cache
mkdir .go
export GOMODCACHE="$PWD/.go/mod_cache" 
export GOCACHE="$PWD/.go/cache"

go mod tidy
go test -coverprofile=coverage.out -coverpkg=./... ./...

What did you see happen?

The modules under .go were included in test coverage, output of the above script (note the coverage % in the last line):

go: creating new go.mod: module example-proj
go: finding module for package golang.org/x/tools/cover
go: downloading golang.org/x/tools v0.18.0
go: found golang.org/x/tools/cover in golang.org/x/tools v0.18.0
ok  	example-proj	0.002s	coverage: 0.8% of statements in ./...

inspecting the coverage profile:

$ grep --invert-match "$(go list -m)" coverage.out | head
mode: set
golang.org/x/tools/cover/profile.go:37.41,37.58 1 0
golang.org/x/tools/cover/profile.go:38.41,38.81 1 0
golang.org/x/tools/cover/profile.go:39.41,39.68 1 0
golang.org/x/tools/cover/profile.go:43.57,45.16 2 0
golang.org/x/tools/cover/profile.go:45.16,47.3 1 0
golang.org/x/tools/cover/profile.go:48.2,49.36 2 0
golang.org/x/tools/cover/profile.go:54.64,62.15 4 0
golang.org/x/tools/cover/profile.go:62.15,64.17 2 0
golang.org/x/tools/cover/profile.go:64.17,66.48 2 0

What did you expect to see?

Modules under the .go directory to be ignored, per https://pkg.go.dev/cmd/go#hdr-Package_lists_and_patterns

Directory and file names that begin with "." or "_" are ignored by the go tool, as are directories named "testdata".

EDIT: for anyone looking for a workaround, I just filtered out lines not belonging to my module in the coverage file:

awk -v go_mod="$(go list -m)" 'NR==1 || $0 ~ "^"go_mod' "coverage.in" > coverage.out

The text was updated successfully, but these errors were encountered:

matthewhughes934 · 2024-02-29T20:59:31Z

Observation, the behaviour changes if I vendor the dependencies:

$ GOMODCACHE="$PWD/.go/mod_cache" GOCACHE="$PWD/.go/cache" go mod vendor
$ GOMODCACHE="$PWD/.go/mod_cache" GOCACHE="$PWD/.go/cache" go test -coverprofile=coverage.out -coverpkg=./... ./...
ok  	example-proj	0.002s	coverage: 100.0% of statements in ./...

Digging further, here's the first quick-and-dirty (not acceptable, it breaks a test in cmd/go/internal/load) change I found to give the behaviour I expected:

diff --git a/src/cmd/go/internal/load/search.go b/src/cmd/go/internal/load/search.go
index 565996a21f..da533661c6 100644
--- a/src/cmd/go/internal/load/search.go
+++ b/src/cmd/go/internal/load/search.go
@@ -39,7 +39,7 @@ func MatchPackage(pattern, cwd string) func(*Package) bool {
                                return false
                        }
                        rel = filepath.ToSlash(rel)
-                       if rel == ".." || strings.HasPrefix(rel, "../") {
+                       if rel == ".." || strings.HasPrefix(rel, "../") || rel[0] == '.' || rel[0] == '_' {
                                return false
                        }
                        return matchPath(rel)

In the case of vendoring the rel path there is like "vendor/github.com/russross/blackfriday/v2" and matchPath will return false. In the other case the path is e.g. .go/mod_cache/github.com/russross/blackfriday/v2@v2.1.0"

bcmills · 2024-02-29T20:59:45Z

(attn @thanm)

matthewhughes934 · 2024-03-02T11:32:07Z

Here's an updated patch which doesn't break existing tests, and includes a new PerPackageFlag (just because that was the first place I found that was directly checking the behaviour of the package matching). Happy to submit for review if it is at all reasonable.

diff --git a/src/cmd/go/internal/load/flag_test.go b/src/cmd/go/internal/load/flag_test.go
index d3223e12d5..f5b38f5323 100644
--- a/src/cmd/go/internal/load/flag_test.go
+++ b/src/cmd/go/internal/load/flag_test.go
@@ -92,6 +92,7 @@ type ppfTest struct {
 	ppfDirTest("./...", 3, "/my/test/dir", "/my/test/dir/sub", "/my/test/dir/sub/sub", "/my/test/other", "/my/test/other/sub"),
 	ppfDirTest("../...", 4, "/my/test/dir", "/my/test/other", "/my/test/dir/sub", "/my/test/other/sub", "/my/other/test"),
 	ppfDirTest("../...sub...", 3, "/my/test/dir/sub", "/my/test/othersub", "/my/test/yellowsubmarine", "/my/other/test"),
+	ppfDirTest("./...", 1, "/my/test/dir", "/my/test/dir/.sub", "/test/dir/_sub", "/test/dir/testdata/sub"),
 }
 
 func ppfDirTest(pattern string, nmatch int, dirs ...string) ppfTest {
diff --git a/src/cmd/go/internal/load/search.go b/src/cmd/go/internal/load/search.go
index 565996a21f..3af07a9475 100644
--- a/src/cmd/go/internal/load/search.go
+++ b/src/cmd/go/internal/load/search.go
@@ -38,10 +38,20 @@ func MatchPackage(pattern, cwd string) func(*Package) bool {
 				// Cannot make relative - e.g. different drive letters on Windows.
 				return false
 			}
+
 			rel = filepath.ToSlash(rel)
 			if rel == ".." || strings.HasPrefix(rel, "../") {
 				return false
 			}
+
+			cwdRel, err := filepath.Rel(cwd, p.Dir)
+			if (
+				err == nil && cwdRel != "." && !strings.HasPrefix(cwdRel, "../") &&
+				// Avoid .foo, _foo, and testdata subdirectory trees.
+				(strings.HasPrefix(cwdRel, ".") || strings.HasPrefix(cwdRel, "_") || cwdRel == "testdata")) {
+				return false
+			}
+
 			return matchPath(rel)
 		}
 	case pattern == "all":

thanm · 2024-03-04T14:36:29Z

Thanks for the report, and thanks for sending a patch.

Happy to submit for review if it is at all reasonable.

That would be fine with me, please do. I tried your fix in my own repo and it looks good to me.

The pattern passed to `-coverpkg` when running `go test` would not ignore directories usually ignored by the `go` command, i.e. those beginning with "." or "_" are ignored by the go tool, as are directories named "testdata". Fix this by adding an explicit check for these (by following a similar check in `src/cmd/doc/dirs.go`[1]) allowing us to ignore them. See linked issue for a reproduction. Fixes golang#66038 [1] https://go.googlesource.com/go/+/16e5d24480dca7ddcbdffb78a8ed5de3e5155dec/src/cmd/doc/dirs.go#136

The pattern passed to `-coverpkg` when running `go test` would not ignore directories usually ignored by the `go` command, i.e. those beginning with "." or "_" are ignored by the go tool, as are directories named "testdata". Fix this by adding an explicit check for these (by following a similar check in `src/cmd/doc/dirs.go`[1]) allowing us to ignore them. Two tests are added for this change, one is a regression test attempting to directly replicate the behaviour described in the issue, the other is updating another test I saw fail when trialling other solutions to this issue so I thought it worthwhile to be explicit about the change there. See linked issue for a reproduction. Fixes golang#66038 [1] https://go.googlesource.com/go/+/16e5d24480dca7ddcbdffb78a8ed5de3e5155dec/src/cmd/doc/dirs.go#136

matthewhughes934 · 2024-03-07T20:07:06Z

Thanks for the report, and thanks for sending a patch.

Happy to submit for review if it is at all reasonable.

That would be fine with me, please do. I tried your fix in my own repo and it looks good to me.

Thanks, I've created #66171 for this

gopherbot · 2024-03-07T20:18:06Z

Change https://go.dev/cl/569895 mentions this issue: cmd/go: fix -coverpkg not ignoring special directories

thanm · 2024-03-07T20:26:44Z

Looks good. I left some comments on the CL.

The pattern passed to `-coverpkg` when running `go test` would not ignore directories usually ignored by the `go` command, i.e. those beginning with "." or "_" are ignored by the go tool, as are directories named "testdata". Fix this by adding an explicit check for these (by following a similar check in `src/cmd/doc/dirs.go`[1]) allowing us to ignore them. Two tests are added for this change, one is a regression test attempting to directly replicate the behaviour described in the issue, the other is updating another test I saw fail when trialling other solutions to this issue so I thought it worthwhile to be explicit about the change there. See linked issue for a reproduction. Fixes golang#66038 [1] https://go.googlesource.com/go/+/16e5d24480dca7ddcbdffb78a8ed5de3e5155dec/src/cmd/doc/dirs.go#136

The pattern passed to `-coverpkg` when running `go test` would not ignore directories usually ignored by the `go` command, i.e. those beginning with "." or "_" are ignored by the go tool, as are directories named "testdata". Fix this by adding an explicit check for these (by following a similar check in `src/cmd/doc/dirs.go`[1]) allowing us to ignore them. The scope of the change is limted to package matching to only the -coverpkg flag of `go test` to avoid impacting -gcflags and the other per package flags, e.g. we don't want to change behaviour for a user building something that imports a leading dot package who wants to set gcflags for it Two tests are added for this change, one is a regression test attempting to directly replicate the behaviour described in the issue, the other is updating another test I saw fail when trialling other solutions to this issue so I thought it worthwhile to be explicit about the change there. See linked issue for a reproduction. Fixes golang#66038 [1] https://go.googlesource.com/go/+/16e5d24480dca7ddcbdffb78a8ed5de3e5155dec/src/cmd/doc/dirs.go#136

bcmills added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. GoCommand cmd/go labels Feb 29, 2024

bcmills added this to the Backlog milestone Feb 29, 2024

thanm self-assigned this Mar 4, 2024

matthewhughes934 linked a pull request Mar 7, 2024 that will close this issue

cmd/go: make -coverpkg properly ignore special directories #66171

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cmd/go: coverpkg doesn't ignore directories starting with '.' #66038

cmd/go: coverpkg doesn't ignore directories starting with '.' #66038

matthewhughes934 commented Feb 29, 2024 •

edited

matthewhughes934 commented Feb 29, 2024 •

edited

bcmills commented Feb 29, 2024

matthewhughes934 commented Mar 2, 2024

thanm commented Mar 4, 2024

matthewhughes934 commented Mar 7, 2024

gopherbot commented Mar 7, 2024

thanm commented Mar 7, 2024

cmd/go: coverpkg doesn't ignore directories starting with '.' #66038

cmd/go: coverpkg doesn't ignore directories starting with '.' #66038

Comments

matthewhughes934 commented Feb 29, 2024 • edited

Go version

Output of go env in your module/workspace:

What did you do?

What did you see happen?

What did you expect to see?

matthewhughes934 commented Feb 29, 2024 • edited

bcmills commented Feb 29, 2024

matthewhughes934 commented Mar 2, 2024

thanm commented Mar 4, 2024

matthewhughes934 commented Mar 7, 2024

gopherbot commented Mar 7, 2024

thanm commented Mar 7, 2024

matthewhughes934 commented Feb 29, 2024 •

edited

Output of `go env` in your module/workspace:

matthewhughes934 commented Feb 29, 2024 •

edited