Use goleak to capture goroutine leak #2606
Conversation
Signed-off-by: Ashmita Bohara <ashmita.bohara152@gmail.com>
| func TolerantVerifyLeakMain(m *testing.M) { | ||
| goleak.VerifyTestMain(m, | ||
| // https://github.com/census-instrumentation/opencensus-go/blob/d7677d6af5953e0506ac4c08f349c62b917a443a/stats/view/worker.go#L34 | ||
| goleak.IgnoreTopFunction("go.opencensus.io/stats/view.(*worker).start"), |
There was a problem hiding this comment.
where do we even use opencensus?
|
|
||
| // TolerantVerifyLeakMain verifies go leaks but excludes the go routines that are | ||
| // launched as side effects of some of our dependencies. | ||
| func TolerantVerifyLeakMain(m *testing.M) { |
There was a problem hiding this comment.
This is unfortunate. Seems to indicate that the tests are not closing the resources properly, or the dependencies are not exposing proper clean-up functions.
| goleak.IgnoreTopFunction("k8s.io/klog/v2.(*loggingT).flushDaemon"), | ||
| goleak.IgnoreTopFunction("k8s.io/klog.(*loggingT).flushDaemon"), | ||
| // https://github.com/dgraph-io/badger/blob/v1.5.3/y/watermark.go#L54 | ||
| goleak.IgnoreTopFunction("github.com/dgraph-io/badger/y.(*WaterMark).process"), |
There was a problem hiding this comment.
if feels like badger should really have proper clean-ups, maybe we're not invoking them?
| ) | ||
|
|
||
| func TestMain(m *testing.M) { |
There was a problem hiding this comment.
Manually adding these functions is not a scalable approach, next time someone will surely forget to add one.
I would prefer to have a CI check, similar to make nocover, that will fail if some packages don't have this function (assuming we want to go with checking these leaks).
Codecov Report
@@ Coverage Diff @@
## master #2606 +/- ##
==========================================
- Coverage 95.31% 94.87% -0.45%
==========================================
Files 208 210 +2
Lines 9285 9383 +98
==========================================
+ Hits 8850 8902 +52
- Misses 356 402 +46
Partials 79 79
Continue to review full report at Codecov.
|
|
I am very new to golang but I saw goleak is adopted by multiple open-source golang projects like thanos. I just thought it will be good to catch such leaks in unittests. I have no experience with it. |
|
How did you arrive at the exclusion list, by testing packages individually? I would prefer not to have any exclusions and just not enable goleaks for packages that leak, for now. |
|
Right, I added that |
Signed-off-by: Ashmita Bohara ashmita.bohara152@gmail.com
Which problem is this PR solving?
This PR uses goleak (https://github.com/uber-go/goleak) to identify goroutine leak as part of unit tests.
Short description of the changes
We are adding a
TestMainfunction for each test package. Goleak provides a way to exclude top functions, hence we are addingTolerantVerifyLeakMainwith those exceptions. If this is accepted change, I'll add changes for rest of the test packages too.