-
Notifications
You must be signed in to change notification settings - Fork 151
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
Add option to ignore current goroutines #49
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? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a couple more test cases here:
- Add a goroutine after
IgnoreCurrentStacks
and 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added tests for proposed use cases, please take a look :)
Signed-off-by: denis-tingajkin <denis.tingajkin@xored.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some small suggestions to make the tests easier to read, but otherwise looks good.
<-done | ||
wg.Done() | ||
}() | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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