Closed stanhu closed 3 years ago
@stanhu - My brief reading of the httptest.NewServer
function shows that it never returns a nil
value. Why do the assertion at all?
@mdwhatcott Thanks, good point. There's no need for the assertion. I guess the larger point that it was surprising to me that the assertion implementation is printing the string representation of the object just to validate it, and this race condition could be hit in any code that may have data access in the %v
output and Goroutines. I would expect this test case just to check the value of the object. require.NotNil(t, server)
from https://github.com/stretchr/testify works fine.
@stanhu - I would advise against performing assertions on things that may not have finished initializing but I can see your point. What change would like to see? Are you suggesting that we change just the ShouldNotBeNil
assertion, or are you proposing we eliminate any possibility of a similar race across the whole library? There are a great many usages of %v
and variants in the output messages as they currently stand, most of which are quite important and provide helpful information:
https://github.com/smartystreets/assertions/blob/master/messages.go
Sample code:
When run with
go test -race
, this fails becausehttptest.NewServer
has a Goroutine that initializes its connection state, butShouldNotBeNil
callsShouldBeNil
, which returns a string that looks like:However, calling
fmt.Sprintf(
%v, server)
isn't really a good idea here because it may not be finished initializing. This seems like a weakness in depending on strings for comparisons.