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/go: omit non-executable "func _()" functions from coverage profiles #36264

Closed
zikaeroh opened this issue Dec 24, 2019 · 7 comments
Closed
Labels
FeatureRequest FrozenDueToAge help wanted NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@zikaeroh
Copy link
Contributor

Currently, coverage profiles include coverage for functions that cannot ever run (func _() { ... }). These sorts of functions are commonly used to generate compile-time checks that aren't expected to actually execute. For example, code generated by stringer:

// Code generated by "stringer -type=accessLevel -trimprefix=level"; DO NOT EDIT.

package bot

import "strconv"

func _() {
	// An "invalid array index" compiler error signifies that the constant values have changed.
	// Re-run the stringer command to generate them again.
	var x [1]struct{}
	_ = x[levelUnknown-0]
	_ = x[levelEveryone-1]
	_ = x[levelSubscriber-2]
	_ = x[levelModerator-3]
	_ = x[levelBroadcaster-4]
	_ = x[levelAdmin-99]
}

const (
	_accessLevel_name_0 = "UnknownEveryoneSubscriberModeratorBroadcaster"
	_accessLevel_name_1 = "Admin"
)

var (
	_accessLevel_index_0 = [...]uint8{0, 7, 15, 25, 34, 45}
)

func (i accessLevel) String() string {
	switch {
	case 0 <= i && i <= 4:
		return _accessLevel_name_0[_accessLevel_index_0[i]:_accessLevel_index_0[i+1]]
	case i == 99:
		return _accessLevel_name_1
	default:
		return "accessLevel(" + strconv.FormatInt(int64(i), 10) + ")"
	}
}

Could have a coverage profile of:

github.com/hortbot/hortbot/internal/bot/accesslevel_string.go:7.10,17.2 7 0
github.com/hortbot/hortbot/internal/bot/accesslevel_string.go:28.38,29.9 1 23
github.com/hortbot/hortbot/internal/bot/accesslevel_string.go:30.24,31.80 1 21
github.com/hortbot/hortbot/internal/bot/accesslevel_string.go:32.15,33.29 1 2
github.com/hortbot/hortbot/internal/bot/accesslevel_string.go:34.10,35.64 1 0

Note the first line, which says 7.10,17.2 7 0, i.e. the _ function has zero lines covered.

I propose that coverage data should be omitted for these _ functions. My rationale:

  • Because these functions show up in coverage profiles, they "fool" commonly used coverage visualization / tracking systems into reporting a lower-than-true percentage. Here's my profile example on coveralls: https://coveralls.io/builds/27776643/source?filename=internal/bot/accesslevel_string.go
  • These functions cannot run; their benefit is already gained at compile time.
  • Due to the simplicity of the coverage profiles (just line info, no context), it's not straightforward to post-process coverage data to get this behavior without changing go test itself.

I'm not sure if this is a proper "proposal", but I didn't think it was exactly a "bug" either. Feel free to retitle as needed.

@gopherbot gopherbot added this to the Proposal milestone Dec 24, 2019
@zikaeroh
Copy link
Contributor Author

I get the feeling that this missed getting put into the Proposals project. Maybe I shouldn't have made it a proposal at all.

@bcmills bcmills changed the title proposal: cmd/go: omit non-executable "func _()" functions from coverage profiles cmd/go: omit non-executable "func _()" functions from coverage profiles Mar 10, 2020
@bcmills bcmills added FeatureRequest help wanted NeedsFix The path to resolution is known, but the work has not been done. and removed Proposal labels Mar 10, 2020
@bcmills bcmills modified the milestones: Proposal, Backlog Mar 10, 2020
@bcmills
Copy link
Contributor

bcmills commented Mar 10, 2020

This doesn't really need to be a proposal. If you'd like to send a fix, the tree is open now. 🙂

@zikaeroh
Copy link
Contributor Author

Thanks, that's what I figured. I'm still not sure what should and shouldn't be a proposal... 😄

I'll take a look, though I'm definitely not familiar with this particular code.

@gopherbot
Copy link

Change https://golang.org/cl/223117 mentions this issue: cmd/cover: skip function declarations with blank names

@ibraheemdev
Copy link

ibraheemdev commented Aug 10, 2020

I am still getting this issue. When I run the file with go test -cover ..., only get 10% test coverage. Removing the blank function gives me 100% cov. Wasn't this issue fixed?

Screen Shot 2020-08-09 at 9 11 50 PM

I am running go1.14.4 on macOS Catalina.

@zikaeroh
Copy link
Contributor Author

My CL was merged during the Go 1.15 cycle; you should see it fixed in an RC or on gotip. It wouldn't be fixed in Go 1.14.

@ibraheemdev
Copy link

Oh. Ok, thanks for the quick reply!

@golang golang locked and limited conversation to collaborators Aug 10, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FeatureRequest FrozenDueToAge help wanted NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

4 participants