-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Improved error reporting of unexpected method calls #375
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?
Conversation
Fixes stretchr#373. Panic now tells us the method was called with [] instead of an obscure index out of range error.
ernesto-jimenez
left a comment
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.
@seppestas thank you very much for the contribution, the refactoring and changes to the messages are good, but we should make a couple of tweaks to have good readability when methods accept []byte or are receiving big arguments.
| argVals = append(argVals, ",") | ||
| } | ||
| argValsString = fmt.Sprintf("\n\t\t%s", strings.Join(argVals, "\n\t\t")) | ||
| argVals = append(argVals, fmt.Sprintf("%#v", arg)) |
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.
We should:
- Use
%sinstead of%#v: it is pretty common methods to receive[]byteand%#vwould print output like[]byte{0x6f, 0x6b} - Split arguments in different lines: it is common for people to receive big structs or long strings. Having all the arguments in a single line will affect readability.
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.
Mmh, I don't really agree. Depending on what the []byte contains, I'd actually preter it to be printed the way %#v prints it over printing it as a string. IMO, if you're using a []byte to hold text Go has to manipulate or print, you're doing something wrong. Relevant: https://blog.golang.org/strings. Also, what if the argument is a string nor []byte?
Splitting the arguments makes sense though.
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 understand []byte can contain other data beyond strings, but it is extremely common in developer's programs to have []byte containing text.
Some common examples:
json.Marshalreturns[]byte
I would argue that it is more common for most developers to have a []byte variable holding text than some other data.
In those cases, you want to see something like:
Expected: foo
Received: bar
Rather than:
Expected: []byte{0x66, 0x6f, 0x6f}
Received: []byte{0x62, 0x61, 0x72}
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.
For me that argument doesn't make a lot of sense. I could argue most numbers used by developers are smaller than 1000, but that doesn't mean making something that can only handle numbers up to 999 is a good idea, even if it makes things "simpler" for "most" people.
I can come up with plenty of functions returning a []byte that do not contain plain text like:
x509.CreateCertificateelf.section.Data- Good ol'
io.Reader.read, arguably the most versatile interace in golang, used for everything from bzip2 to png, and yes, also for text.
Also, when I'd pass []byte{0x62, 0x61, 0x72} to a function, and some error tells me I passed a string containing bar, I don't consider that expected or something I'd want. Let alone I want an error spewing out a bunch of random crapp when I passed binary data to a function expecting, and using a []byte for this is perfectly valid, binary data.
As a last argument: the callString function shows arguments to mocked functions, not return types. Any sane function doing something with text should take a string (or even better, a text.Scanner), not a []byte. So while json.Marshal might return a []byte, if you want to make a function that does something with the marshalled text, it should take a string, and the output of json.Marshal should be wrapped with a string().
Trust me, as someone having to deal with char * and uint8_t * on a daily basis, be glad Go has a special type for text.
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 also use []byte mostly for binary data over text. (There is string after all..)
However, the %#v notation is quite verbose. "0x%x" for hexadecimal might be a good compromise between the two.
EDIT: just realize that this is more general than just for []byte types. I guess %s for byte slices is just not suitable whenever someone is not storing text in it. While getting a byte slice representation when using strings inside is kinda what you can expect.
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.
But the nice feature of %v is that it works for pretty much all types, not only []byte or string.
I also think that using Go's default formatting for types is more consistent than trying to come up with new formats, and it makes for a more "expected" result, even if it's a bit verbose.
A good alternative might be to shorten the argVals so a big []byte is not completely printed, but limited to e.g 60 or so characters. This would mean seeing something like:
Expected: []byte{0x66, 0x6f, 0x6f, 0x66, 0x6f, 0x6f...
Received: []byte{0x62, 0x61, 0x72, 0x62, 0x61, 0x72...
|
Any chance this will still be merged? I just spent a while trying to debug an |
Fixes #373. Panic now tells us the method was called with [] instead
of an obscure index out of range error.