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: TestCoverageRuns fails #17699

Closed
josharian opened this issue Oct 31, 2016 · 9 comments
Closed

cmd/go: TestCoverageRuns fails #17699

josharian opened this issue Oct 31, 2016 · 9 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. Testing An issue that has been verified to require only test changes, not just a test failure.
Milestone
Go1.8

Comments

@josharian
Copy link
Contributor

josharian commented Oct 31, 2016 •
edited

Broken out from #17472.

$ go test -run=TestCoverageRuns cmd/go
--- FAIL: TestCoverageRuns (1.41s)
	go_test.go:251: running testgo [test -short -coverpkg=strings strings regexp]
	go_test.go:266: standard output:
	go_test.go:267: --- FAIL: TestIndexRune (0.00s)
			strings_test.go:298: expected no allocations, got 1.000000
		FAIL
		coverage: 97.9% of statements in strings
		FAIL	strings	0.082s
		ok  	regexp	0.099s	coverage: 19.2% of statements in strings
		
	go_test.go:280: go [test -short -coverpkg=strings strings regexp] failed unexpectedly: exit status 1
FAIL
FAIL	cmd/go	3.513s

Looks like the failure is because the coverage instrumentation causes an allocation in TestIndexRune where there wasn't one before, causing the test itself to fail.
@josharian josharian added the NeedsFix The path to resolution is known, but the work has not been done. label Oct 31, 2016
@josharian josharian added this to the Go1.8 milestone Oct 31, 2016
@bradfitz bradfitz added the Testing An issue that has been verified to require only test changes, not just a test failure. label Oct 31, 2016
@bradfitz
Copy link
Contributor

So IndexRune is allocation-free, except when it's compiled in cover mode?

We could just skip that alloc test in that mode, but how do we detect that we're in cover mode at runtime?

/cc @dsnet

@josharian
Copy link
Contributor Author

Yep. Alternatively, we could have TestCoverageRuns skip the test, but (frustratingly) there's no way to tell go test to skip tests that match a regexp.

@dsnet
Copy link
Member

dsnet commented Oct 31, 2016 •
edited

My understanding is that coverage instruments the code and inserts logic to do some bookkeeping. What is it doing that's causing the allocation? (I can look into this).

I see at least 50 usages of testing.AllocsPerRun in stdlib. Do those also fail when we run them with coverage enabled or is IndexRune just unlucky?

I'm wondering if there's a bigger problem where it is flaky to do AllocsPerRun when in coverage mode.

@bradfitz
Copy link
Contributor

If only AllocsPerRun took a *testing.T, then it could just always t.Skip in coverage mode. :)

@josharian
Copy link
Contributor Author

I'm wondering if there's a bigger problem where it is flaky to do AllocsPerRun when in coverage mode.

Probably. Unfortunately, pprof doesn't help much here. Running 'go test -run=TestIndexRune -memprofile=m -coverprofile=c strings' yields line numbers that are useless, because they're post-cover-rewrite line numbers. But there are four sources of allocations there, somewhere, so the complaint from AllocsPerRun is legit.

@josharian
Copy link
Contributor Author

If only AllocsPerRun took a *testing.T, then it could just always t.Skip in coverage mode. :)

And if *testing.T exposed whether it was in coverage mode.

@bradfitz
Copy link
Contributor

/cc @robpike

@gopherbot
Copy link

CL https://golang.org/cl/32483 mentions this issue.

gopherbot pushed a commit that referenced this issue Nov 1, 2016
@robpike
testing: add a method testing.CoverMode
daf3f08 
This makes it possible to avoid tests where coverage affects the test
results by skipping them (or otherwise adjusting them) when coverage
is enabled.

Update #17699

Change-Id: Ifcc36cfcd88ebd677890e82ba80ee3d696ed3d7c
Reviewed-on: https://go-review.googlesource.com/32483
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
@gopherbot
Copy link

CL https://golang.org/cl/32484 mentions this issue.

@golang golang locked and limited conversation to collaborators Nov 1, 2017
@rsc rsc unassigned dsnet Jun 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. Testing An issue that has been verified to require only test changes, not just a test failure.
Projects
None yet
Milestone
Go1.8
Development

No branches or pull requests
4 participants
@bradfitz @josharian @dsnet @gopherbot