mock.AnythingOfType fix panic when matching nil type#1799
mock.AnythingOfType fix panic when matching nil type#1799brackendawson wants to merge 1 commit intostretchr:masterfrom
Conversation
mock/mock_test.go
Outdated
| assert.Contains(t, diff, `(int)(456) != (int)(123)`) | ||
| assert.Contains(t, diff, `(string)(false) != (bool)(true)`) |
There was a problem hiding this comment.
This format seems awkward to me.
I feel like we are using something like this everywhere else
| assert.Contains(t, diff, `(int)(456) != (int)(123)`) | |
| assert.Contains(t, diff, `(string)(false) != (bool)(true)`) | |
| assert.Contains(t, diff, `456(int) != 123(int)`) | |
| assert.Contains(t, diff, `false(string) != true(bool)`) |
There was a problem hiding this comment.
Think about something storing "bool" in a string. The result is confusing
There was a problem hiding this comment.
(string)(false) is a bit confusing. However *int(1) is also confusing because it's invalid in go. I'm going to restore the original format.
There was a problem hiding this comment.
I'm a bit lost there
I was suggesting to use "false (string)"
I felt like it was better than "(false)(string)"
So the example of "*int(1)" is incorrect. I was asking to use "1 (*int)" (even if I'm unsure this format exists as you said)
My point was to use something like "%v (%T)" that we are already using. And fix the strange format that exists in code for this method.
There was a problem hiding this comment.
I mis-read your suggestion, %v(%T) is also quite weird to me because I was hoping to find something with a grounding in Go syntax.
This probably warrants de-coupling into a separate issue where we can consider the format we want testify to move to as a whole. As I said for now this PR just fixes the panic and leaves the format alone.
a81fbc5 to
6b9ffdf
Compare
AnythingOfType would panic if the actual arg value was of type nil. Handle the nil type case before calling reflect.Type.String. Because (<nil>=<nil>) looks confusing, change the failure message value format to the more idiomatic (%T)(%v). In order to better test the failure message, change the mockTestingT to capture the messages passed to it.
6b9ffdf to
564214b
Compare
Fix a panic in the AnythingOfType matcher when matching nil type.
It's reported that IsType had the same panic but a refactor between then and now appears to have fixed this.
Summary
Changes
Motivation
AnythingOfType would panic if the actual arg value was of type nil.
Related issues