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: lines that are unreached (due to panic() ) appear as covered by test #48147

Open
holubond opened this issue Sep 2, 2021 · 2 comments
Open
Assignees
Labels
NeedsInvestigation
Milestone

Comments

@holubond
Copy link

@holubond holubond commented Sep 2, 2021

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

$ go version
go version go1.17 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\ondre\AppData\Local\go-build
set GOENV=C:\Users\ondre\AppData\Roaming\go\env
set GOEXE=.exe
set GOEXPERIMENT=
set GOFLAGS=
set GOHOSTARCH=amd64
set GOHOSTOS=windows
set GOINSECURE=
set GOMODCACHE=C:\Users\ondre\go\pkg\mod
set GONOPROXY=
set GONOSUMDB=
set GOOS=windows
set GOPATH=C:\Users\ondre\go
set GOPRIVATE=
set GOPROXY=https://proxy.golang.org,direct
set GOROOT=C:\Program Files\Go
set GOSUMDB=sum.golang.org
set GOTMPDIR=
set GOTOOLDIR=C:\Program Files\Go\pkg\tool\windows_amd64
set GOVCS=
set GOVERSION=go1.17
set GCCGO=gccgo
set AR=ar
set CC=gcc
set CXX=g++
set CGO_ENABLED=1
set GOMOD=C:\Users\ondre\OneDrive\Desktop\workplace\mladata-bang\bang-backend\go.mod
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\ondre\AppData\Local\Temp\go-build1886039437=/tmp/go-build -gno-record-gcc-switches
gdb --version: GNU gdb (GDB) 7.9.1

What did you do?

When running tests with coverage, the lines after calling a panicking function appear to be covered, even though the test could never reach them. They are not executed, just appear to be covered. This happens to every line until a branching statement is reached as can be seen when adding regular statements above the branching statement.

An example can be found at https://github.com/mazanec202/go-wrong-panic-coverage. In this case I had to place the panicking function to a separate package, however on my personal project was the logic complex enough not to be marked by go vet as unused while residing in a single package.

Using the following commands on the project, it's possible to verify the problem:
go test -coverprofile=coverage.out
go tool cover -html=coverage.out

I believe this issue could provide a false sense of safety and could therefore lead to potential security issues.

What did you expect to see?

Only a line 16 (main.go) containing panic.DoPanic(true) to be marked as covered. Line 17 (main.go) containing shouldBeZero = 1 should be marked as not covered.

What did you see instead?

Line 17 (main.go) containing shouldBeZero = 1 is marked as covered, even though the tests prove that the panic was executed and value of shouldBeZero is unchanged - 0.

@holubond holubond changed the title Lines that are unreached (due to panic() ) appear as covered by test cmd/go/internal/test: lines that are unreached (due to panic() ) appear as covered by test Sep 2, 2021
@bcmills
Copy link
Member

@bcmills bcmills commented Sep 2, 2021

CC @thanm

@bcmills bcmills added the NeedsInvestigation label Sep 2, 2021
@bcmills bcmills added this to the Backlog milestone Sep 2, 2021
@bcmills bcmills changed the title cmd/go/internal/test: lines that are unreached (due to panic() ) appear as covered by test cmd/cover: lines that are unreached (due to panic() ) appear as covered by test Sep 2, 2021
@thanm
Copy link
Contributor

@thanm thanm commented Sep 2, 2021

Thanks for the report. This is a known flaw with the current implementation. There are a number of other coverage testing frameworks (such as the support offered by LLVM/clang) that have the same limitation.

Providing support for this use case would require substantial effort, probably not something that would be workable with the existing cmd/cover source-to-source translation scheme. I'll add this to the potential list of features to support if/when we refresh Go's coverage implementation.

@thanm thanm self-assigned this Sep 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsInvestigation
Projects
None yet
Development

No branches or pull requests

3 participants