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: go test reports panics on wrong line when using -covermode #15757

Closed
sarahhodne opened this issue May 19, 2016 · 4 comments
Closed

cmd/cover: go test reports panics on wrong line when using -covermode #15757

sarahhodne opened this issue May 19, 2016 · 4 comments

Comments

@sarahhodne
Copy link

@sarahhodne sarahhodne commented May 19, 2016

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

Reproduced on both:

  • go version go1.6.2 linux/amd64
  • go version devel +3b50adb Thu May 19 18:40:53 2016 +0000 linux/amd64
  1. What operating system and processor architecture are you using (go env)?
GOARCH="amd64"
GOBIN=""
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/henrik/gopath"
GORACE=""
GOROOT="/home/henrik/.gimme/versions/go"
GOTOOLDIR="/home/henrik/.gimme/versions/go/pkg/tool/linux_amd64"
CC="gcc"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build860467575=/tmp/go-build -gno-record-gcc-switches"
CXX="g++"
CGO_ENABLED="1"
  1. What did you do?

Project to reproduce is at https://github.com/henrikhodne/go-cover-test-panic (requires multiple files, otherwise I'd use play.golang.org).

  1. What did you expect to see?

I'd expect the stack trace from go test with and without -covermode=count to report the panic on the same line (foo.go:7).

  1. What did you see instead?

When enabling the coverage tool (go test -covermode=count ./...), the stack trace indicates that the panic is on foo.go:9, which is incorrect (probably due to the way the coverage tool injects code to mark lines as called?)

Interestingly, the line numbers are correct when I move the Foo() function into foo_test.go.
#6329 might be related.

@sarahhodne sarahhodne changed the title go test reports panics on wrong line when using -coverprofile go test reports panics on wrong line when using -covermode May 19, 2016
@ianlancetaylor ianlancetaylor changed the title go test reports panics on wrong line when using -covermode cmd/cover: go test reports panics on wrong line when using -covermode May 20, 2016
@ianlancetaylor ianlancetaylor added this to the Go1.8 milestone May 20, 2016
@quentinmit quentinmit added the NeedsFix label Oct 6, 2016
@rsc
Copy link
Contributor

@rsc rsc commented Nov 3, 2016

This is due to an underlying bug, maybe, in go/printer. Probably the right thing to do is not to use go/printer at all and just insert the counter manipulation into the original file text (knowing the positions from go/ast). That would solve a few problems related to comments as well. But that's a pretty invasive change and won't happen for Go 1.8 or maybe even beyond. Marking Go1.9Early so we look at this early in the Go 1.9 cycle.

Note that when you do run the test you gave, the file names are different in the output as well: the line number in the -cover output refers to the cover-generated source file as well, so it's technically accurate. Of course, that source file has been deleted, so it's not terribly useful.

@rsc rsc modified the milestones: Go1.9Early, Go1.8 Nov 3, 2016
@dhananjay92
Copy link
Contributor

@dhananjay92 dhananjay92 commented Dec 26, 2016

This isn't a bug in cmd/cover or go/printer. The the actual code moves around a bit because of added counter increment statements.

Instrumented code,

$ go tool cover -mode=count foo.go | cat -n
     1	package foo
     2	
     3	func Foo() int {
     4		GoCover.Count[0]++
     5		a := 1 + 1
     6	
     7		if a == 2 {
     8			GoCover.Count[2]++
     9			panic("oops")
    10		}
    11		GoCover.Count[1]++
    12	
    13		return a
    14	}
    15	
    16	var GoCover = struct {
    17		Count     [3]uint32
    18		Pos       [3 * 3]uint32
    19		NumStmt   [3]uint16
    20	} {
    21		Pos: [3 * 3]uint32{
    22			3, 6, 0xc0010, // [0]
    23			10, 10, 0xa0002, // [1]
    24			6, 7, 0x10000c, // [2]
    25		},
    26		NumStmt: [3]uint16{
    27			2, // 0
    28			1, // 1
    29			1, // 2
    30		},
    31	}

Notice that in the generated code, panic("oops") has moved to 9th line because of two counter increment statements added before it.

There're two ways to improve debuggability of go test -cover:

  1. Make instrumentation tool preserve the original line numbers. This is slightly tricky, but achievable with //line directive.
  2. In case the test fails with -cover, keep the instrumented code around for later inspection.

I like the second one, because it's simple and doesn't add any more complexity into coverage instrumentation tool.

@gopherbot
Copy link

@gopherbot gopherbot commented Nov 10, 2017

Change https://golang.org/cl/77151 mentions this issue: cmd/cover: add //line comment pointing to original file

@gopherbot
Copy link

@gopherbot gopherbot commented Nov 10, 2017

Change https://golang.org/cl/77150 mentions this issue: cmd/cover: modify source as text, not as AST

@gopherbot gopherbot closed this in 2c00dea Nov 13, 2017
gopherbot pushed a commit that referenced this issue Nov 16, 2017
Now that cover does not modify the formatting of the original file
or add any newline characters, we can make it print a //line comment
pointing back at the original, and compiler errors and panics will
report accurate line numbers.

Fixes #6329.
Fixes #15757.

Change-Id: I7b0e386112c69beafe69e0d47c5f9e9abc87c0f5
Reviewed-on: https://go-review.googlesource.com/77151
Run-TryBot: Russ Cox <rsc@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
@golang golang locked and limited conversation to collaborators Nov 13, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

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