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.

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

cmd/go: -cover stdout format is not consistent between failed & pass #39070

Closed
christophermancini opened this issue May 14, 2020 · 9 comments
Closed

cmd/go: -cover stdout format is not consistent between failed & pass #39070

christophermancini opened this issue May 14, 2020 · 9 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone
Backlog

Comments

@christophermancini
Copy link

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

$ go version
go version go1.14.2 windows/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
set GO111MODULE=
set GOARCH=amd64
set GOBIN=
set GOCACHE=C:\Users\\AppData\Local\go-build
set GOENV=C:\Users\\AppData\Roaming\go\env
set GOEXE=.exe
set GOFLAGS=
set GOHOSTARCH=amd64
set GOHOSTOS=windows
set GOINSECURE=
set GONOPROXY=
set GONOSUMDB=
set GOOS=windows
set GOPATH=C:\Users\Chris\go
set GOPRIVATE=
set GOPROXY=https://proxy.golang.org,direct
set GOROOT=c:\go
set GOSUMDB=sum.golang.org
set GOTMPDIR=
set GOTOOLDIR=c:\go\pkg\tool\windows_amd64
set GCCGO=gccgo
set AR=ar
set CC=gcc
set CXX=g++
set CGO_ENABLED=1
set GOMOD=
set CGO_CFLAGS=-g -O2
set CGO_CPPFLAGS=
set CGO_CXXFLAGS=-g -O2
set CGO_FFLAGS=-g -O2
set CGO_LDFLAGS=-g -O2
set PKG_CONFIG=pkg-config
set GOGCCFLAGS=-m64 -mthreads -fmessage-length=0 -fdebug-prefix-map=C:\Users\\AppData\Local\Temp\go-build342408862=/tmp/go-build -gno-record-gcc-switches

What did you do?

Run go test -cover on any set of packages including a failed tests.

What did you expect to see?

$ go test -count=1 -cover -tags="sample" github.com/digitalocean/gocop/sample/...
# cover github.com/digitalocean/gocop/sample/failbuild
2020/05/14 09:18:17 cover: C:\Users\Chris\dev\code\do\gocop\sample\failbuild\broken.go: C:\Users\Chris\dev\code\do\gocop\sample\failbuild\broken.go:5:1: expected declaration, found fun
--- FAIL: TestWillFail (0.00s)
    failing_test.go:14: number does equal eleven
--- FAIL: TestWillFailComplex (0.00s)
    --- FAIL: TestWillFailComplex/my_simple_test (0.00s)      
        failing_test.go:37: 423 (int) to equal 10 (int)       
            failing_test.go:37
    --- FAIL: TestWillFailComplex/my_simple_test2 (0.00s)     
        failing_test.go:37: 517 (int) to equal 12 (int)       
            failing_test.go:37
FAIL
FAIL    github.com/digitalocean/gocop/sample/fail       0.028s  coverage: 100.0% of statements
FAIL    github.com/digitalocean/gocop/sample/failbuild [build failed]
ok      github.com/digitalocean/gocop/sample/flaky      0.024s  coverage: [no statements]        
?       github.com/digitalocean/gocop/sample/numbers    [no test files]
ok      github.com/digitalocean/gocop/sample/pass       0.018s  coverage: 100.0% of statements   
ok      github.com/digitalocean/gocop/sample/pass/nostatements  0.025s  coverage: [no statements]
FAIL

What did you see instead?

$ go test -count=1 -cover -tags="sample" github.com/digitalocean/gocop/sample/...
# cover github.com/digitalocean/gocop/sample/failbuild
2020/05/14 09:18:17 cover: C:\Users\Chris\dev\code\do\gocop\sample\failbuild\broken.go: C:\Users\Chris\dev\code\do\gocop\sample\failbuild\broken.go:5:1: expected declaration, found fun
--- FAIL: TestWillFail (0.00s)
    failing_test.go:14: number does equal eleven
--- FAIL: TestWillFailComplex (0.00s)
    --- FAIL: TestWillFailComplex/my_simple_test (0.00s)      
        failing_test.go:37: 423 (int) to equal 10 (int)       
            failing_test.go:37
    --- FAIL: TestWillFailComplex/my_simple_test2 (0.00s)     
        failing_test.go:37: 517 (int) to equal 12 (int)       
            failing_test.go:37
FAIL
coverage: 100.0% of statements
FAIL    github.com/digitalocean/gocop/sample/fail       0.028s
FAIL    github.com/digitalocean/gocop/sample/failbuild [build failed]
ok      github.com/digitalocean/gocop/sample/flaky      0.024s  coverage: [no statements]        
?       github.com/digitalocean/gocop/sample/numbers    [no test files]
ok      github.com/digitalocean/gocop/sample/pass       0.018s  coverage: 100.0% of statements   
ok      github.com/digitalocean/gocop/sample/pass/nostatements  0.025s  coverage: [no statements]
FAIL

The same behavior is observed via json formatting.
@christophermancini christophermancini changed the title cmd/go: -cover stdout format is not consistent between failed & pawss cmd/go: -cover stdout format is not consistent between failed & pass May 14, 2020
@christophermancini
Copy link
Author

christophermancini commented May 14, 2020 •
edited

We wrote a tool, GoCop, to enable us to identify and track flaky tests. It is designed to parse stdout of go test so then we can redirect the output to file during CI runs, use the tool to identify packages which failed and attempt to rerun them N times to determine if they are flaky. We also use this tool to persist the test outcome per package to a Postgres database.

When tests fail, the coverage % output being inconsistently placed leads to data that is missed by the tool. When considering flakiness of a package, the coverage info is something we want to track regardless of the package result.

@christophermancini
Copy link
Author

I submitted a change to address this issue, I will update the change with integration tests as requested.

https://go-review.googlesource.com/c/go/+/233617/1

@cagedmantis cagedmantis added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label May 18, 2020
@cagedmantis cagedmantis added this to the Backlog milestone May 18, 2020
@cagedmantis
Copy link
Contributor

/cc @bcmills @jayconrod @matloob

@gopherbot
Copy link

Change https://golang.org/cl/233617 mentions this issue: cmd/go: include cover percent on failures

@bcmills
Copy link
Contributor

bcmills commented May 19, 2020

As I asked on the CL review: why do we believe that the coverage numbers are meaningful for failed tests?

I suspect that in practice the coverage for failed tests will be systematically lower than for passing tests, because many tests use t.Fatalf or similar on failure — skipping the remainder of the test (and any associated coverage for that test).

So it generally will not be meaningful to compare coverage between two test runs with different failing tests or different failure modes — and if you have a flaky test with a consistent failure mode over time, why not fix the test instead of worrying about its coverage?

@christophermancini
Copy link
Author

@bcmills Thanks for the question. The way I see it, the coverage data reported for a test run is unique to each test run and should be recorded regardless of the outcome of a test. The coverage being potentially less for a failed test run is no different than coverage being less for a passing run if -short is used. Or the coverage being less based on the build constraints that were used to execute the tests. Test coverage can also differ for different platforms. Each run of a suite of tests has the possibility of having a different coverage % due to many factors.

If the expectation is that the coverage % is always reflective of the parameters/factors of the test execution, then there is no logical reason to have inconsistencies between reporting this value based on the outcome of the execution.

@rsc
Copy link
Contributor

rsc commented Jun 4, 2020

FWIW, I agree that reporting coverage % on failure is more likely to be misleading than helpful. If the tests are failing, the coverage doesn't mater. And if they are failing early, the coverage will be somewhat meaningless. Doesn't seem worth the churn.

@christophermancini
Copy link
Author

christophermancini commented Jun 18, 2020 •
edited

@rsc the same then could be said for execution duration then, could it not?

@rsc
Copy link
Contributor

rsc commented Sep 18, 2020

Maybe, but "how long did it run before it failed?" is a more meaningful question than "what level of code coverage did it achieve before it failed?"

This discussion kind of trailed off, but I think the rough consensus here is that the output should be left as is.

@golang golang locked and limited conversation to collaborators Sep 18, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Milestone
Backlog
Development

No branches or pull requests
5 participants
@rsc @christophermancini @cagedmantis @bcmills @gopherbot