Skip to content

Commit

Permalink
assert.NotNil should only check if the value is not nil
Browse files Browse the repository at this point in the history
Currently, it uses a try + Nil assertion to check if a value was nil,
but as a side effect, the Nil helper also prints the internals.
This could potentially result in unanticipated race conditions
for values that are perfectly fine since they aren't nil.
We should be cautious about such potential issues.
  • Loading branch information
adamluzsi committed Jun 13, 2023
1 parent feedd0b commit fcb3b04
Show file tree
Hide file tree
Showing 2 changed files with 30 additions and 1 deletion.
2 changes: 1 addition & 1 deletion assert/Asserter.go
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,7 @@ func (a Asserter) Nil(v any, msg ...any) {

func (a Asserter) NotNil(v any, msg ...any) {
a.TB.Helper()
if !a.try(func(a Asserter) { a.Nil(v) }) {
if !reflects.IsNil(v) {
return
}
a.fn(fmterror.Message{
Expand Down
29 changes: 29 additions & 0 deletions assert/Asserter_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,10 @@ import (
"context"
"errors"
"fmt"
"github.com/adamluzsi/testcase"
"io"
"math/big"
"math/rand"
"net"
"reflect"
"strings"
Expand Down Expand Up @@ -169,6 +171,33 @@ func TestAsserter_NotNil(t *testing.T) {
subject.NotNil("", "foo", "bar", "baz")
Equal(t, dtb.IsFailed, false)
})
t.Run("race", func(t *testing.T) {
type T struct{ V *int }

var v = T{V: func() *int {
n := 42
return &n
}()}

done := make(chan struct{})
defer close(done)
go func() {
for {
select {
case <-done:
return
default:
*v.V = rand.Int()
}
}
}()

time.Sleep(time.Microsecond)

blk := func() { assert.NotNil(t, &v) }

testcase.Race(blk, blk, blk)
})
}

func TestAsserter_Panics(t *testing.T) {
Expand Down

0 comments on commit fcb3b04

Please sign in to comment.