Skip to content

cmd/go: go test using -coverpkg can change the test result #36541

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.

Sign up for GitHub

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
Closed
ailurarctos opened this issue Jan 14, 2020 · 8 comments
Closed

cmd/go: go test using -coverpkg can change the test result #36541

ailurarctos opened this issue Jan 14, 2020 · 8 comments
Labels
FrozenDueToAge

Comments

@ailurarctos
Copy link

What version of Go are you using (go version)?

$ go version
go version go1.13.6 linux/amd64

Does this issue reproduce with the latest release?

Yes.

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/me/.cache/go-build"
GOENV="/home/me/.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/me/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/lib/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/lib/go/pkg/tool/linux_amd64"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build723393417=/tmp/go-build -gno-record-gcc-switches"

What did you do?

Construct a module where an init() function in one package, a, modifies state in another package, b. Make a test in b that uses this state:

$ find . -type f -exec sh -c 'echo === {}; cat {}; echo' \;
=== ./go.mod
module example.com

go 1.13

=== ./a/a.go
package a

import "example.com/b"

func init() {
	b.B = append(b.B, "a")
}

=== ./b/b_test.go
package b

import "testing"

func Test_B(t *testing.T) {
	if len(B) != 0 {
		t.Fail()
	}
}

=== ./b/b.go
package b

var B []string

This test will fail when -coverpkg ./... is used and succeed when it is not used:

$ go test ./...
?   	example.com/a	[no test files]
ok  	example.com/b	(cached)

$ go test -coverpkg=./... ./...
?   	example.com/a	[no test files]
--- FAIL: Test_B (0.00s)
FAIL
coverage: 100.0% of statements in ./...
FAIL	example.com/b	0.002s
FAIL

What did you expect to see?

I expected the outcome of the test to be the same regardless of the -coverpkg argument.

What did you see instead?

Using -coverpkg will cause the init() functions of matching packages to be called. This can change the test result.
@ailurarctos
Copy link
Author

I think this is the same underlying issue as described in #21283.

@jayconrod
Copy link
Contributor

This is working as designed (and as you say, is a duplicate of #21283).

Packages instrumented for coverage with -coverpkg are imported by the generated test main package and are linked into the test binary. It's not feasible to skip their init functions.

@ailurarctos
Copy link
Author

Thanks for the quick response @jayconrod. #21283 was closed by https://go-review.googlesource.com/c/go/+/76876/ which neither fixed the issue nor mentioned deliberately not fixing it so I wasn't sure if it was working as designed.

Do you think it's worth updating the documentation for -coverpkg to mention this? It seems counter-intuitive that using -coverpkg could change the test result.

@ailurarctos
Copy link
Author

@jayconrod would it be reasonable to put the coverage state in a generates sub-package (instead of the package itself) and import that from the test main package?

@jayconrod
Copy link
Contributor

Do you think it's worth updating the documentation for -coverpkg to mention this? It seems counter-intuitive that using -coverpkg could change the test result.

Sure, it's definitely worth updating the documentation. From go help testflag, it's not really clear how this work. I'll put together a CL for this.

would it be reasonable to put the coverage state in a generates sub-package (instead of the package itself) and import that from the test main package?

I think it would be feasible. Coverage data already needs to be across packages since it's imported by the generated test main package. The package holding coverage data wouldn't need to import anything else, so it wouldn't create dependency problems.

That said, this would be a substantial change with some visible side effects. If you're interesting in pursuing this, I'd suggest opening a proposal issue.

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/215317 mentions this issue: cmd/go: document how -coverpkg links instrumented packages

@ailurarctos
Copy link
Author

That said, this would be a substantial change with some visible side effects. If you're interesting in pursuing this, I'd suggest opening a proposal issue.

I'll try to investigate what this change would look like and if it seems doable I'll make a proposal issue. Thanks @jayconrod!

@jayconrod
Copy link
Contributor

@ailurarctos

No proposal needed. @bcmills, @matloob and I discussed this today. It doesn't seem like the current behavior is actually intended or desired, and if anyone is depending on it, it's probably only by mistake. So we'll try and fix this for the 1.15 cycle.

There are a number of related issues, so rather than re-opening this one, let's consider #23910 to be the canonical tracking issue.

@golang golang locked and limited conversation to collaborators Jan 21, 2021
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
No milestone
Development

No branches or pull requests
3 participants
@jayconrod @ailurarctos @gopherbot