-
Notifications
You must be signed in to change notification settings - Fork 22
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
assert: Add Error and remove google/go-cmp #74
Conversation
sloggers/slogtest/assert/assert.go
Outdated
// ErrorContains asserts err != nil and err.Error() contains sub. | ||
// | ||
// The match will be case insensitive. | ||
func ErrorContains(t testing.TB, err error, sub, name string) { |
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 think I would prefer a function to ensure the two unwrapped errors are equal instead of just comparing strings
I often have to do things like this
https://github.com/cdr/enterprise/blob/master/manager/internal/database/teams_test.go#L46
Additionally, if you call xerrors.Unwrap
on a non-wrapped error it returns nil instead of the input err. Would be nice to handle that here.
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 was using ErrorContains in nhooyr/websocket but I can easily fix that. Will adjust this function.
94611f7
to
1e111e4
Compare
@coadler what do you think about adding ...cmp.Options to Equals? I think name is enough for adding detail instead of a ...slog.F. |
I could see some benefit in that. Right now we have to truncate all of our |
See google/go-cmp#174 In general its a very heavy library and I'm not convinced it offers enough.
@coadler Thoughts on removing google/go-cmp? Do you approve? |
No description provided.