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

runtime/pprof: testCPUProfile hides real issues #13817

Closed
aclements opened this issue Jan 4, 2016 · 3 comments
Closed

runtime/pprof: testCPUProfile hides real issues #13817

aclements opened this issue Jan 4, 2016 · 3 comments
Labels
FrozenDueToAge Testing An issue that has been verified to require only test changes, not just a test failure.
Milestone

Comments

@aclements
Copy link
Member

In response to #7095, we made the profiling tests pass if they get at least one sample. This is masking real issues. For example, on ARM, TestStackBarrierProfiling usually gets fewer than 10 samples, when it should be getting around 500 (discovered in #13405). It would be better if, when this got fewer samples than expected, it simply retried the test a few times. This would keep the test fast under common circumstances while still keeping it robust. (This is technically a repeated fixed-horizon hypothesis test, which technically means it's bad. What we really want is a sequential hypothesis test, but as long as the difference between "working" and "broken" [effect size] we're looking for is large enough, the simple thing is fine.)

@randall77 @RLH @dvyukov

@bradfitz bradfitz added this to the Go1.6Maybe milestone Jan 21, 2016
@aclements aclements modified the milestones: Go1.7Early, Go1.6Maybe Jan 21, 2016
@aclements
Copy link
Member Author

I'm sure this is going to turn up issues that are best dealt with by keeping an eye on the builders, so I've moved it to 1.7Early. It's also likely there are going to be issues we simply can't fix in the less used platforms because of kernel problems.

@bradfitz bradfitz modified the milestones: Go1.7Maybe, Go1.7Early May 5, 2016
@bradfitz bradfitz added the Testing An issue that has been verified to require only test changes, not just a test failure. label May 5, 2016
@bradfitz
Copy link
Contributor

bradfitz commented May 5, 2016

We still have a few months to watch builders if you want to do this now-ish. Otherwise feel free to kick it down the road more.

@aclements
Copy link
Member Author

As of commit 7037c15, we started checking that we get at least 25% of the expected number of samples. That's been in for a while and seems to be working fine.

@golang golang locked and limited conversation to collaborators May 25, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge Testing An issue that has been verified to require only test changes, not just a test failure.
Projects
None yet
Development

No branches or pull requests

3 participants