Add option to ignore current goroutines#49
Conversation
…ojects that recently started to use go-leak check. Signed-off-by: denis-tingajkin <denis.tingajkin@xored.com>
|
|
Codecov Report
@@ Coverage Diff @@
## master #49 +/- ##
==========================================
+ Coverage 95.20% 95.42% +0.21%
==========================================
Files 4 4
Lines 146 153 +7
==========================================
+ Hits 139 146 +7
Misses 4 4
Partials 3 3
Continue to review full report at Codecov.
|
Signed-off-by: denis-tingajkin <denis.tingajkin@xored.com>
|
@prashantv Is this patch make sense? |
prashantv
left a comment
There was a problem hiding this comment.
Thank you for the contribution @denis-tingajkin
I think this change makes sense, and I think it's a clean and simple API that doesn't expose too much, nice work!
I have some small comments (rename the method, some additional test cases), I think it would also benefit from an example test, so users can see that it can be used in the same way with defer.
leaks_test.go
Outdated
| go func() { | ||
| <-done | ||
| }() | ||
| VerifyNone(t, IgnoreCurrentStacks()) |
There was a problem hiding this comment.
Can you add a couple more test cases here:
- Add a goroutine after
IgnoreCurrentStacksand ensure that it is captured (you can useFind) - Ensure that if a goroutine is started before
IgnoreCurrentStacks, it ends, and another goroutine is started, the other goroutine is noticed. (This should help ensure that goroutine IDs are not reused and so we won't have false negatives)
I'd suggest making them independent tests using t.Run rather than one long test as well, will be easier to understand what each test is testing
There was a problem hiding this comment.
Added tests for proposed use cases, please take a look :)
Signed-off-by: denis-tingajkin <denis.tingajkin@xored.com>
prashantv
left a comment
There was a problem hiding this comment.
Some small suggestions to make the tests easier to read, but otherwise looks good.
| <-done | ||
| wg.Done() | ||
| }() | ||
| } |
There was a problem hiding this comment.
can you add some whitespace to this test to separate out the different parts, and add comments on what we are trying to test?
There was a problem hiding this comment.
can you add some whitespace to this test to separate out the different parts, and add comments on what we are trying to test?
Sure, done. Please take a look :)
Signed-off-by: denis-tingajkin <denis.tingajkin@xored.com>
prashantv
left a comment
There was a problem hiding this comment.
LGTM, just one minor nit, thanks!
Signed-off-by: denis-tingajkin <denis.tingajkin@xored.com>
This allows usage specific tests in big projects that recently started to use go-leak check.
Signed-off-by: denis-tingajkin denis.tingajkin@xored.com
Motivation
#48