fixing panic in calls to assertion with nil m.mutex #1212
Conversation
This reverts a change that was made in stretchr#1182 The PR makes m.mutex a pointer which now needs to be checked but it's not checked for nil everywhere. This should also help with these issues: - stretchr#1208 - stretchr#1210
…dy have it locked
|
Hey @boyan-soubachov sorry to ping you directly but since you were the original author for #1182 I thought I'd bring this one to your attention. Thanks for any review you might be able to offer. |
| @@ -526,9 +519,9 @@ func AssertExpectationsForObjects(t TestingT, testObjects ...interface{}) bool { | |||
| h.Helper() | |||
| } | |||
| for _, obj := range testObjects { | |||
| if m, ok := obj.(Mock); ok { | |||
| if m, ok := obj.(*Mock); ok { | |||
There was a problem hiding this comment.
Are you sure this should be changed? IIRC I didn't change this logic in the PR I merged?
There was a problem hiding this comment.
Yes, because we want to refer to the same instance of sync.Mutex for the copy that is a type check here.
There was a problem hiding this comment.
Also, go vet ./... will show that warning for us ( It was caught in CI).
There was a problem hiding this comment.
Fair enough; I see it's a test helper anyways.
Thank you for catching this!
There was a problem hiding this comment.
🤦 . I must've missed this when doing the Go 1.18 PR (which broke this) and 'fixed it' the long way around. Thanks
Summary
@CleanCut and I found these changes that should be reverted as they are causing panics after library updates.
This reverts a change that was made in #1182
That PR makes m.mutex a pointer which now needs to be checked but it's not checked for nil everywhere.
It also reset the mutex in the
.Onfunction when it could actually have locks from other callers.Changes
m.mutexback to a non pointer value.OnfunctionMotivation
Fixing panics and crashes in the library after a minor update.
Related issues
This should also help with these issues: