error-compare: detect contains of err.Error()

[Limero]

Running testifylint on a large codebase at work I noticed that people sometimes use Contains to check error messages, something that testifylint currently doesn't detect

Expected results:

❌   assert.Contains(t, err.Error(), "error")
     require.Contains(t, err.Error(), "error")
     s.Contains(err.Error(), "error")

✅   assert.ErrorContains(t, err, "error")
     require.ErrorContains(t, err, "error")
     s.ErrorContains(err, "error")

[Antonboom]

Hi!

Thank you for feedback.
Related to https://github.com/Antonboom/testifylint/blob/master/CONTRIBUTING.md#error-compare

[nickajacks1]

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.

[ccoVeille]

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.

[ccoVeille]

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.

[mmorel-35]

I have found this:

https://github.com/kubernetes/kubernetes/blob/9039d71dd7b9916e4faba00a60c86f0c2e45ed89/pkg/controller/podautoscaler/metrics/client_test.go#L213

A comparison of errors messages

[ccoVeille]

The complexity involved in this is insane 😁

[mmorel-35]

Shall there be a translator asking to use

err.Error()

Instead of

fmt.Sprintf("%v", err)

Then unwrapped-error can be applied

[ccoVeille]

Yes, I also found fmt.Sprint(err)

https://github.com/search?q=language%3Ago+%22fmt.Sprint%28err%29%22+testify&type=code

https://github.com/SAP/jenkins-library/blob/b6b366066f1ff819bb2b8f43c18e0a748ba432a4/pkg/whitesource/whitesource_test.go#L271

		assert.Contains(t, fmt.Sprint(err), "no project token(s) found for provided projects")

[Antonboom]

+ review of https://github.com/Antonboom/testifylint/pull/162/files