Open Limero opened 7 months ago
Hi!
Thank you for feedback. Related to https://github.com/Antonboom/testifylint/blob/master/CONTRIBUTING.md#error-compare
I see that assert.ErrorContains(t, err, "not found")
is discouraged in favor of ErrorIs
. I agree with that guidance when it's an option, but sometimes errors are unexported, meaning you have no choice but to use ErrorContains
. I understand that checking the error string is a bit fragile, but what alternatives would you suggest? Perhaps simply assert.Error
?
Some justification I can think of for using assert.Error
alone and ignoring the content of the error is that if your actual code can't check the type of error that is returned, that means that the content of the error doesn't affect the control flow, and thus the unit tests shouldn't need to be coupled to the human-readable error messages.
For records, I started working on the implementation of this feature request.
https://github.com/Antonboom/testifylint/blob/master/CONTRIBUTING.md#error-compare
Stay tuned, and ping me if you see no progress in the next month.
I agree with @Limero @nickajacks1 there is a need for a checker that will look for detecting this.
assert.Contains(t, err.Error(), "error") require.Contains(t, err.Error(), "error") s.Contains(err.Error(), "error")
This should be a checker activated by default.
As @Antonboom said, there is a need for a checker these by .ErrorContains(t, err, ErrSentinel)
But these are two separate needs. While the first one is legitimate and should be activated by default. The second one is more opinionated and could lead to problems.
We have to consider ErrorContains could be used for wrapped error to check one word is present in the error message
Let's consider a code returning that
fmt.Errorf("error %w - code %s, ErrSentinel, "foo")
I would use
assert.ErrorIs(t, err, ErrSentinel)
but then I would use
assert.ErrorContains(t, err, "foo")
These use cases are legitimate.
But I agree we should also prevent the following pattern and suggest to use Error.Is
assert.Contains(t, err.Error(), ErrSentinel.Error())
This is pattern was sometimes used before testify.Error.Is and even errors.Is existed in standard lib.
I have found this:
A comparison of errors messages
The complexity involved in this is insane 😁
Shall there be a translator asking to use
err.Error()
Instead of
fmt.Sprintf("%v", err)
Then unwrapped-error can be applied
Yes, I also found fmt.Sprint(err)
https://github.com/search?q=language%3Ago+%22fmt.Sprint%28err%29%22+testify&type=code
assert.Contains(t, fmt.Sprint(err), "no project token(s) found for provided projects")
Running
testifylint
on a large codebase at work I noticed that people sometimes useContains
to check error messages, something thattestifylint
currently doesn't detectExpected results: