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/cover: regression in 1.20, cover tool no longer includes packages without tests #58770

Closed
abraithwaite opened this issue Feb 27, 2023 · 11 comments
Assignees
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.

Comments

@abraithwaite
Copy link
Contributor

abraithwaite commented Feb 27, 2023

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

11:55:14 $ go version
go version go1.20.1 darwin/arm64

Does this issue reproduce with the latest release?

Yes

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

go env Output
11:56:28 $ go env
GO111MODULE=""
GOARCH="arm64"
GOBIN=""
GOCACHE="/Users/abraithwaite/Library/Caches/go-build"
GOENV="/Users/abraithwaite/Library/Application Support/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="arm64"
GOHOSTOS="darwin"
GOINSECURE=""
GOMODCACHE="/Users/abraithwaite/dev/golang/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="darwin"
GOPATH="/Users/abraithwaite/dev/golang"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/opt/homebrew/Cellar/go/1.20.1/libexec"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/opt/homebrew/Cellar/go/1.20.1/libexec/pkg/tool/darwin_arm64"
GOVCS=""
GOVERSION="go1.20.1"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD="/Users/abraithwaite/dev/github.com/runreveal/runreveal/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 -arch arm64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/x5/_x8ny1d57k717yy0m92c02zh0000gn/T/go-build1870615753=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

I ran our unit tests under both go1.19 and go1.20 and observed different coverage results.

To calculate coverage, we have this makefile testing target:

test:
    $(GO) test -vet=off -tags='$(GOTAGS)' $(GOTESTFLAGS) -coverpkg="./..." -coverprofile=.coverprofile ./...
    grep -v 'cmd' < .coverprofile > .covprof && mv .covprof .coverprofile
    $(GO) tool cover -func=.coverprofile

What did you expect to see?

I expected the coverage reporting to stay the same between releases, assuming no code changes.

What did you see instead?

The reported coverage falsely increased with go1.20 because it's no longer tracking coverage for packages that are missing tests.

As a workaround, I've put TODO tests in every package missing tests, but can't count on that manual practice to catch packages not being covered that I want to include in the coverage report.

@thanm thanm self-assigned this Feb 27, 2023
@thanm
Copy link
Contributor

thanm commented Feb 27, 2023

Thanks for the report; I will take a look.

@thanm thanm added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Feb 27, 2023
@thanm
Copy link
Contributor

thanm commented Mar 2, 2023

I took a look at this issue.

What's happening here has to do with how the coverage machinery is handling "-coverpkg", specifically regarding imports.

You haven't supplied any specific code examples, but I will assume that this example will do for the discussion:

https://go.dev/play/p/253gWw8ItV5

Code from the link above:

-- go.mod --
module bug.example

go 1.20
-- a/a.go --
package a

func A() int {
	return 42
}
-- a/a_test.go --
package a

import "testing"

func TestPackageCoverageInA(t *testing.T) {
	A()
}
-- b/b.go --
package b

func B() int {
	return -42
}
-- main/main.go --
package main

import (
	"bug.example/a"
	"bug.example/b"
)

func main() {
	println(a.A() + b.B())
}

We have three packages: "a", "b" and "main", where main imports "a" and "b" but neither "a" nor "b" imports anything. If you run this with Go 1.20 at the top level (where the "go.mod" is) you get:

$ go test -count=1 -coverpkg=./...  -cover ./...
ok  	bug.example/a	0.018s	coverage: 100.0% of statements in ./...
?   	bug.example/b	[no test files]
ok  	bug.example/main	0.033s	coverage: 100.0% of statements in ./...
$

whereas in Go 1.19 you would get

$ go test -count=1 -coverpkg=./...  -cover ./...
ok  	bug.example/a	0.017s	coverage: 33.3% of statements in ./...
?   	bug.example/b	[no test files]
ok  	bug.example/main	0.028s	coverage: 100.0% of statements in ./...
$

Some background on how "go test" works: when you kick off a run of "go test -cover" in package "a", the Go command builds the test binary by combining the code from the package of interest (package 'a' in this case) with a test harness (`_testmain.go') that includes the test code, the testing package, and a synthetically generated "main" routine.

In Go 1.19 and prior versions, when given a command like

go test -coverpkg=./... -cover ./...

When building the test binary for "a", the Go command would construct a _testmain.go file that did a force import of all the packages matching the "./..." pattern, e.g.


// Code generated by 'go test'. DO NOT EDIT.
package main

import (
	"os"
	"testing"
	"testing/internal/testdeps"
	_test "bug.example/a"
	_cover0 "bug.example/b"
	_cover1 "bug.example/main"
	_cover2 "bug.example/a"
)
...

and would register the coverage counter variables for all of these packages with the testing runtime.

Forcing imports of "b" and "main" into the package "a" test binary is strange, since package "a" has no imports, and in particular importing "main" into "a" is especially weird given that normally it is the other way around.

The forced imports does provide a mechanism for computing the correct result (here 33% of all statements in "./...") for package "a", but it also creates lots of confusion for users, who are surprise when various init functions in the "./..." packages that take actions that perturb the semantics of the run when testing "a".

Here are some of the bugs that users have filed regarding this quirk:

#21283
#23883
#23910
#27336
#46212

There are others as well (variants of this problem have been reported as bugs by Go users internal to Google). For this reason, in the coverage redesign rolled out in Go 1.20 we did away with the forced import mechanism.

Getting back to the bug at hand: you are quite correct however that this is a regression, we should be reporting the 33% number for package "a" and not 100%.

Rather than going back to forced imports, I believe that the best way to handle this is to have the Go command delay reporting of coverage numbers in this case (when "-coverpkg" is used). Internally the new implementation collects raw profiles for each tested packed ("a" and "main") into a separate directory, so if we process the directories in combinatoin at the end of all test runs it should be possible to get the right result.

In the mean time, as a workaround you can restore the 1.19 behavior by setting adding this to your environment:

export GOEXPERIMENT=nocoverageredesign

This will turn off the new implementation and revert back to the 1.19 scheme. Let me know if this works for you until the bug can be fixed.

@thanm thanm added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Mar 2, 2023
@onsi
Copy link

onsi commented Mar 15, 2023

thanks for the clear explanation @thanm - I was about to submit a related issue but came across this and figured I'd provide an additional data point. I've put together a reproducer here (which you can clone and run make on) for what I'm seeing but tl;dr I have the following setup:

---A/A.go---
package A
func A() bool { return true }

---B/B.go---
package B
func B() bool { return true }
   
---tests/tests.go---
package tests

import (
	"testing"
	"github.com/onsi/test-coverage/A"
)

func TestA(t *testing.T) {
	A.A()
}

when I run:

go test  -coverpkg=./... ./tests

I always get 100% coverage on both 1.19 and 1.20

When I run:

go test  -coverpkg=./... ./...

I get 50% coverage on 1.19 and 100% coverage on 1.20

That 100% coverage on 1.20 is this issue. But the fact that 1.19 behaves differently for ./tests vs ./... (which is kind of reported here but I don't see the root issue being addressed there) is confusing - particularly in light of your explanation. it seems that tests is built differently when invoked as single package than when invoked as ./...

I'm primarily sharing this here to call out that there is an opportunity in 1.20 not simply to fix the regression that's been identified but also to improve on the behavior in 1.19. I'm also trying to avoid opening a near-duplicate issue but would be happy to do so if y'all would prefer.

@thanm
Copy link
Contributor

thanm commented Mar 15, 2023

Thanks for that addition @onsi .

New issue: probably not needed, but up to you.

Regarding fixing things in 1.19, it would be nice, but in general the Golang policy on back-porting fixes is fairly conservative/restrictive. Unless a change is to fix a compiler error or runtime crash or some other serious problem (where there are no workarounds), the release team tends to be discourage this sort of back-port. We'll see.

@onsi
Copy link

onsi commented Mar 15, 2023

oh for sure - i wouldn't backport it to 1.19 either. just noting that previous behavior wasn't quite working here either.

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/495452 mentions this issue: cmd/go: fix percent covered problems with -coverpkg

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/495446 mentions this issue: cmd/cover: add new "emit meta file" mode for packages without tests

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/495447 mentions this issue: cmd/go: improve handling of no-test packages for coverage

@thanm
Copy link
Contributor

thanm commented May 17, 2023

An update on this issue:

I have a CL stack (rooted at CL 495452) that should resolve the problem.

There is one area where the coverage percentage numbers are going to be slightly different for the "go test -coverpkg=./..." case, which has to do with how init functions are handled.

In the example below, we have a top level dir containing five package subdirectories, "a", "b", "main", "n", and "x". The first three packages have tests and have a user-written init function. If you count up the total number of statements, there are a total of 9 (2 in "a", 2 in "b", 1 in "x", and 4 in "main").


-- go.mod --
module M

go 1.21
-- a/a.go --
package a

func init() {
	println("package 'a' init: launch the missiles!")
}

func AFunc() int {
	return 42
}
-- a/a_test.go --
package a

import "testing"

func TestA(t *testing.T) {
	if AFunc() != 42 {
		t.Fatalf("bad!")
	}
}
-- b/b.go --
package b

func init() {
	println("package 'b' init: release the kraken")
}

func BFunc() int {
	return -42
}
-- b/b_test.go --
package b

import "testing"

func TestB(t *testing.T) {
	if BFunc() != -42 {
		t.Fatalf("bad!")
	}
}
-- main/main.go --
package main

import (
	"M/a"
	"M/b"
)

func MFunc() string {
	return "42"
}

func M2Func() int {
	return a.AFunc() + b.BFunc()
}

func init() {
	println("package 'main' init")
}

func main() {
	println(a.AFunc() + b.BFunc())
}
-- main/main_test.go --
package main

import "testing"

func TestMain(t *testing.T) {
	if MFunc() != "42" {
		t.Fatalf("bad!")
	}
	if M2Func() != 0 {
		t.Fatalf("also bad!")
	}
}
-- n/n.go --
package n

type N int
-- x/x.go --
package x

func XFunc() int {
	return 2 * 2
}

Here is the output from a test run with my fix:

$ go test -coverprofile=tip.cov  -count=1 -coverpkg=./... ./...  ; go tool cover -func=tip.cov
?   	M/n	[no test files]
ok  	M/a	0.028s	coverage: 22.2% of statements in ./...
ok  	M/b	0.015s	coverage: 22.2% of statements in ./...
ok  	M/main	0.040s	coverage: 77.8% of statements in ./...
	M/x		coverage: 0.0% of statements
M/a/a.go:3:		init		100.0%
M/a/a.go:7:		AFunc		100.0%
M/b/b.go:3:		init		100.0%
M/b/b.go:7:		BFunc		100.0%
M/main/main.go:8:	MFunc		100.0%
M/main/main.go:12:	M2Func		100.0%
M/main/main.go:16:	init		100.0%
M/main/main.go:20:	main		0.0%
M/x/x.go:3:		XFunc		0.0%
total:			(statements)	77.8%
$

Note the coverage percentage for package "a": 22%, or 2 out of 9 statements total. With Go 1.19, the coverage number is 44%, or 4 out of 9 (this is due to the artifact of force-importing all covered packages into testmain for each test).

Also worth noting is that for packages like "x", which have statements but no tests, we don't link or run a test binary for the package but we do report a coverage percentage for them (this is different from a previous attempt in CL 115095 (checked in and then rreverted).

@abraithwaite
Copy link
Contributor Author

abraithwaite commented May 17, 2023

I think that's totally reasonable. Might be confusing for some, but the results I value most are the total coverages after all the tests have ran since I often have code exercising other packages in opaque and distant ways.

Looking great! Thanks so much for taking a look at this @thanm! Really appreciate your work. 💯

@Nov1kov
Copy link

Nov1kov commented Jun 7, 2023

A dirty workaround is to add an empty file with the name *_test.go to each package that is not included in coverage.

gopherbot pushed a commit that referenced this issue Sep 14, 2023
Introduce a new mode of execution for instrumenting packages that have
no test files. Instead of just skipping packages with no test files
(during "go test -cover" runs), the go command will invoke cmd/cover
on the package passing in an option in the config file indicating that
it should emit a coverage meta-data file directly for the package (if
the package has no functions, an empty file is emitted). Note that
this CL doesn't actually wire up this functionality in the Go command,
that will come in a later patch.

Updates #27261.
Updates #58770
Updates #24570.

Cq-Include-Trybots: luci.golang.try:gotip-linux-amd64-longtest,gotip-windows-amd64-longtest
Change-Id: I01e8a3edb62441698c7246596e4bacbd966591c3
Reviewed-on: https://go-review.googlesource.com/c/go/+/495446
Reviewed-by: Bryan Mills <bcmills@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
gopherbot pushed a commit that referenced this issue Sep 30, 2023
This patch improves the way the go command handles coverage testing
of packages that have functions but don't have any test files. Up to
this point if you ran "go test -cover" on such a package, you would
see:

  ?   	mymod/mypack	[no test files]

While "no test files" is true, it is also very unhelpful; if the
package contains functions, it would be better instead to capture the
fact that these functions are not executed when "go test -cover" is
run on the package.

With this patch, for the same no-test package "go test -cover" will
output:

	mymod/mypack	coverage: 0.0% of statements

The inclusion of such packages in coverage reporting also extends to
"-coverprofile" as well (we'll see entries for the "mypack" functions
in this case.

Note that if a package has no functions at all, then we'll still fall
back to reporting "no test files" in this case; it doesn't make sense
to report "0.0% statements covered" if there are no statements.

Updates #27261.
Updates #58770.
Updates #18909.
Fixes #24570.

Cq-Include-Trybots: luci.golang.try:gotip-linux-amd64-longtest,gotip-windows-amd64-longtest
Change-Id: I8e916425f4f2beec65861df78265e93db5ce001a
Reviewed-on: https://go-review.googlesource.com/c/go/+/495447
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Bryan Mills <bcmills@google.com>
@golang golang locked and limited conversation to collaborators Sep 29, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

5 participants