-
Notifications
You must be signed in to change notification settings - Fork 1.7k
mock.AnythingOfType fix panic when matching nil type. #1799
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
mock.AnythingOfType fix panic when matching nil type. #1799
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Think about something storing "bool" in a string. The result is confusing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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