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

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

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

Comments

@ailurarctos
Copy link

@ailurarctos ailurarctos commented Jan 14, 2020

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

This comment has been minimized.

Copy link
Author

@ailurarctos ailurarctos commented Jan 14, 2020

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

@jayconrod

This comment has been minimized.

Copy link
Contributor

@jayconrod jayconrod commented Jan 14, 2020

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.

@jayconrod jayconrod closed this Jan 14, 2020
@ailurarctos

This comment has been minimized.

Copy link
Author

@ailurarctos ailurarctos commented Jan 14, 2020

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

This comment has been minimized.

Copy link
Author

@ailurarctos ailurarctos commented Jan 15, 2020

@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

This comment has been minimized.

Copy link
Contributor

@jayconrod jayconrod commented Jan 17, 2020

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

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Jan 17, 2020

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

@ailurarctos

This comment has been minimized.

Copy link
Author

@ailurarctos ailurarctos commented Jan 18, 2020

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

This comment has been minimized.

Copy link
Contributor

@jayconrod jayconrod commented Jan 22, 2020

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants
You can’t perform that action at this time.