Skip to content

Commit

Permalink
address code review
Browse files Browse the repository at this point in the history
  • Loading branch information
sywhang committed Sep 6, 2022
1 parent e2de3bd commit 3289f99
Show file tree
Hide file tree
Showing 4 changed files with 48 additions and 34 deletions.
6 changes: 4 additions & 2 deletions leaks.go
Expand Up @@ -21,6 +21,7 @@
package goleak

import (
"errors"
"fmt"

"go.uber.org/goleak/internal/stack"
Expand Down Expand Up @@ -83,6 +84,7 @@ type testHelper interface {
// defer VerifyNone(t)
func VerifyNone(t TestingT, options ...Option) {
opts := buildOpts(options...)
teardown := opts.teardown
if h, ok := t.(testHelper); ok {
// Mark this function as a test helper, if available.
h.Helper()
Expand All @@ -92,7 +94,7 @@ func VerifyNone(t TestingT, options ...Option) {
t.Error(err)
}

if opts.teardown != nil {
opts.teardown(0)
if teardown != nil {
teardown(0)
}
}
66 changes: 38 additions & 28 deletions leaks_test.go
Expand Up @@ -40,21 +40,26 @@ func testOptions() Option {
}

func TestFind(t *testing.T) {
require.NoError(t, Find(), "Should find no leaks by default")
t.Run("Should find no leaks by default", func(t *testing.T) {
require.NoError(t, Find())
})

bg := startBlockedG()
err := Find(testOptions())
require.Error(t, err, "Should find leaks with leaked goroutine")
assert.Contains(t, err.Error(), "blockedG")
assert.Contains(t, err.Error(), "created by go.uber.org/goleak.startBlockedG")

// Once we unblock the goroutine, we shouldn't have leaks.
bg.unblock()
require.NoError(t, Find(), "Should find no leaks by default")

// Find can't take in Cleanup option
err = Find(Cleanup(func(int) { assert.Fail(t, "this should not be called") }))
require.Error(t, err, "Should exit with invalid option")
t.Run("Find leaks with leaked goroutine", func(t *testing.T) {
bg := startBlockedG()
err := Find(testOptions())
require.Error(t, err, "Should find leaks with leaked goroutine")
assert.Contains(t, err.Error(), "blockedG")
assert.Contains(t, err.Error(), "created by go.uber.org/goleak.startBlockedG")

// Once we unblock the goroutine, we shouldn't have leaks.
bg.unblock()
require.NoError(t, Find(), "Should find no leaks by default")
})

t.Run("Find can't take in Cleanup option", func(t *testing.T) {
err := Find(Cleanup(func(int) { assert.Fail(t, "this should not be called") }))
require.Error(t, err, "Should exit with invalid option")
})
}

func TestFindRetry(t *testing.T) {
Expand All @@ -78,21 +83,26 @@ func (ft *fakeT) Error(args ...interface{}) {
}

func TestVerifyNone(t *testing.T) {
ft := &fakeT{}
VerifyNone(ft)
require.Empty(t, ft.errors, "Expect no errors from VerifyNone")
t.Run("VerifyNone finds leaks", func(t *testing.T) {
ft := &fakeT{}
VerifyNone(ft)
require.Empty(t, ft.errors, "Expect no errors from VerifyNone")

bg := startBlockedG()
VerifyNone(ft, testOptions())
require.NotEmpty(t, ft.errors, "Expect errors from VerifyNone on leaked goroutine")
bg.unblock()
})

bg := startBlockedG()
VerifyNone(ft, testOptions())
require.NotEmpty(t, ft.errors, "Expect errors from VerifyNone on leaked goroutine")
bg.unblock()

cleanupCalled := false
VerifyNone(ft, Cleanup(func(c int) {
assert.Equal(t, 0, c)
cleanupCalled = true
}))
require.True(t, cleanupCalled, "expect cleanup registered callback to be called")
t.Run("cleanup registered callback should be called", func(t *testing.T) {
ft := &fakeT{}
cleanupCalled := false
VerifyNone(ft, Cleanup(func(c int) {
assert.Equal(t, 0, c)
cleanupCalled = true
}))
require.True(t, cleanupCalled, "expect cleanup registered callback to be called")
})
}

func TestIgnoreCurrent(t *testing.T) {
Expand Down
4 changes: 2 additions & 2 deletions options.go
Expand Up @@ -46,7 +46,7 @@ type opts struct {

// implement apply so that opts struct itself can be used as
// an Option.
func (o opts) apply(opts *opts) {
func (o *opts) apply(opts *opts) {
opts.filters = o.filters
opts.maxRetries = o.maxRetries
opts.maxSleep = o.maxSleep
Expand All @@ -68,7 +68,7 @@ func IgnoreTopFunction(f string) Option {
}

// Cleanup sets up a cleanup function that will be executed at the
// end of the leak.
// end of the leak check.
// When passed to [VerifyTestMain], the exit code passed to cleanupFunc
// will be set to the exit code of TestMain.
// When passed to [VerifyNone], the exit code will be set to 0.
Expand Down
6 changes: 4 additions & 2 deletions testmain.go
Expand Up @@ -52,15 +52,17 @@ type TestingM interface {
func VerifyTestMain(m TestingM, options ...Option) {
exitCode := m.Run()
opts := buildOpts(options...)
teardown := opts.teardown

if exitCode == 0 {
opts.teardown = nil
if err := Find(opts); err != nil {
fmt.Fprintf(_osStderr, "goleak: Errors on successful test run: %v\n", err)
exitCode = 1
}
}
if opts.teardown != nil {
opts.teardown(exitCode)
if teardown != nil {
teardown(exitCode)
} else {
_osExit(exitCode)
}
Expand Down

0 comments on commit 3289f99

Please sign in to comment.