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: incorrect coverage for source file generated by x/tools/cmd/goyacc #33690

Open
rillig opened this issue Aug 16, 2019 · 7 comments
Open

Comments

@rillig
Copy link
Contributor

@rillig rillig commented Aug 16, 2019

What version of Go are you using?

go version go1.12.7 windows/amd64

What did you do?

go get github.com/rillig/go-yacc-examples
cd $GOPATH/src/github.com/rillig/go-yacc-examples/json
go generate
go test -test.coverprofile coverage.out

What did you expect to see?

The coverage.out file contains coverage markers for json.y, as that is where the source code comes from, according to the //line comments.

What did you see instead?

The coverage.out file contains:

github.com/rillig/go-yacc-examples/json/y.go:25.0,27.0 1 0

y.go lines 25 to 27 contain:

var yyToknames = [...]string{
	"$end",
	"error",

There is no code to be covered here, and these lines do not form a block at all.

Some lines below that, y.go contains:

//line yaccpar:1

The output of go tool objdump on the test executable contains:

TEXT github.com/rillig/go-yacc-examples/json.(*yyParserImpl).Lookahead(SB) yaccpar
  yaccpar:25		0x586a00		b801000000		MOVL $0x1, AX
  yaccpar:25		0x586a05		488d0d74341e00		LEAQ something(SB), CX
  yaccpar:25		0x586a0c		f00fc101		LOCK XADDL AX, 0(CX)
  yaccpar:26		0x586a10		488b442408		MOVQ 0x8(SP), AX
  yaccpar:26		0x586a15		488b8010010000		MOVQ 0x110(AX), AX
  yaccpar:26		0x586a1c		4889442410		MOVQ AX, 0x10(SP)
  yaccpar:26		0x586a21		c3			RET

This makes me suspect that the file name yaccpar is not taken into account when generating the coverage data.

See https://youtrack.jetbrains.com/issue/GO-7513

@gopherbot gopherbot added this to the Unreleased milestone Aug 16, 2019
@bcmills bcmills changed the title x/golang/cmd/goyacc: coverage is calculated for wrong files x/tools/cmd/goyacc: coverage is calculated for wrong files Aug 19, 2019
@bcmills bcmills changed the title x/tools/cmd/goyacc: coverage is calculated for wrong files cmd/cover: incorrect coverage for source file generated by x/tools/cmd/goyacc Aug 19, 2019
@bcmills bcmills modified the milestones: Unreleased, Unplanned Aug 19, 2019
@bcmills bcmills added help wanted and removed help wanted labels Aug 19, 2019
@bcmills
Copy link
Member

@bcmills bcmills commented Aug 19, 2019

@adam-azarchs
Copy link
Contributor

@adam-azarchs adam-azarchs commented Oct 7, 2019

Run goyacc with the -l flag to disable //line comments if you want coverage. Otherwise the line numbers in the symbol tables are referring to a file which isn't even necessarily a go source file, and the coverage information won't make any sense.

@rillig
Copy link
Contributor Author

@rillig rillig commented Oct 7, 2019

@adam-azarchs I tried your suggestion, and it works partially. The thing that works is the coverage, in that I found two branches of my code that are not yet covered. The downside though is that to achieve 100% code coverage, I would now have to cover all goyacc error cases now, and maybe even trigger impossible situations. The goyacc code has worked for me, and I'm not willing to test it thoroughly. Therefore I consider the -l flag only a workaround. A nice one though.

@adam-azarchs
Copy link
Contributor

@adam-azarchs adam-azarchs commented Oct 7, 2019

My point is that this isn't a problem with cover, really - it can't be expected to get cover information when the symbol tables are lying about line numbers.

In terms of code coverage numbers, I'm not sure what you're expecting. Do you want to cover everything in the generated code or not? It sounds like you don't, but then what exactly do you want to ensure coverage of?

If there is a cover issue in here, it sounds like maybe there should maybe be an option to exclude generated code (detected based on https://golang.org/s/generatedcode) from coverage analysis. That said, I doubt that's actually what you want, because you probably do want to make sure your test cases are exercising every case in the grammar - if that isn't possible then that likely implies your grammar is wrong. There is error handling code that may not be possible to exercise fully (though more of it gets exercised if you set yyDebug = 4 and yyErrorVerbose = true), but that's more an argument for 100% code coverage being an inappropriate goal when testing defensively-coded applications.

@rillig
Copy link
Contributor Author

@rillig rillig commented Oct 11, 2019

what exactly do you want to ensure coverage of?

I want to ensure coverage of all the code I have written myself in the yacc source file, as you said.

I think that the output of gocover should include the yacc source file. I usually make the code in the yacc source file minimal, as that file has less IDE support than regular go source. Therefore having the coverage of the individual lines in the yacc file is sufficient to me. I don't need the column information.

rillig added a commit to rillig/pkglint that referenced this issue Oct 11, 2019
Without these line numbers, code coverage can be measured more
accurately, but that's likely a bug in gocover.

See golang/go#33690.
@adam-azarchs
Copy link
Contributor

@adam-azarchs adam-azarchs commented Oct 12, 2019

Not sure if this is what you're talking about but #32200 caused most of that code not to be covered. Have you tried with go1.13?

@rillig
Copy link
Contributor Author

@rillig rillig commented Oct 12, 2019

That's definitely an interesting bug, but not my point here. I tried again with go1.13, and the coverage data still references the wrong locations. I updated the original description, trying to express myself clearer.

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
4 participants
You can’t perform that action at this time.