Catch panic when using go-spew #1824
Conversation
go-spew can panic when trying to diff certain types of values, there are open issues about this on their GitHub repository. go-spew is unfortunately unmaintained, we cannot expect a fix any time soon. Also, because of go-spew's design, there are multiple causes for a panic, and fixing all of them would be a huge undertaking. We already return an empty diff when the types are not comparable, or when the values are not from types that can be easily diffed by go-spew. Let's hide the panic by recovering from it, and returning an empty diff instead. This is not ideal, but at least it prevents the entire program from crashing. The expected/actual values will still be printed, just without the diff.
ccoVeille
force-pushed
the
gospew-panic
branch
from
15c59da to
dfdbd7f
Compare
| a = spewConfig.Sdump(actual) | ||
| } | ||
| }) | ||
| if panicked { |
There was a problem hiding this comment.
Panics are not exceptions, we should not be blindly ignoring them. If there is a case we know of that spew can't handle then we should avoid it or fix spew, potentially by vendoring it.
There was a problem hiding this comment.
It's what I suggested in the PR that was created earlier
@brackendawson do you remember the discussion we had about importing the code of go-spew, the same way we imported difflib ?
The conclusion was not yet, because we don't know yet the "unforseen consequences" of importing the code of go-difflib, so we don't have enough information and experience on this.
Maybe it's time to reconsider it.
Importing the code of go-spew diff and and fixing the bug in the code could be considered.
Originally posted by @ccoVeille in #1816 (comment)
I'm glad we converge here.
It would also help to remove the imperfect hack about time.Time by patching it to use the stringer even when DisableMethods is true.
|
I will work on vendoring go-spew then |
2 participants
Summary
Avoid panic on go-spew errors
Changes
wrap the calls to go-spew into a call to
didPanicMotivation
go-spew can panic when trying to diff certain types of values, there are open issues about this on their GitHub repository.
go-spew is unfortunately unmaintained, we cannot expect a fix any time soon. Also, because of go-spew's design, there are multiple causes for a panic, and fixing all of them would be a huge undertaking.
We already return an empty diff when the types are not comparable, or when the values are not from types that can be easily diffed by go-spew.
Let's hide the panic by recovering from it, and returning an empty diff instead. This is not ideal, but at least it prevents the entire program from crashing. The expected/actual values will still be printed, just without the diff.
Related issues
Closes #480
Related to