require-error: detect if !assert.NoError

[mmorel-35]

While applying require-error on https://github.com/argoproj/argo-cd I have seen this pattern many times

if !assert.NoError(t, err) {
    return
}

I fixed require-error rule by replacing it with

require.NoError(t, err)

There was also some variations like

if !assert.NoError(t, err) || !assert.Len(t, objs, 2) {
    return
}

That could be replaced with

require.NoError(t, err)
require.Len(t, objs, 2)

Also, the return was replaced by a continue in a loop.

I’m wondering it would be pertinent to identify this with testifylint ?

[ccoVeille]

This use case is interesting, yes.

[zak-pawel]

This is interesting:

For:

testifylint --enable-all ./... doesn't find any problems.

For:

I have:

/home/pzak/test/some_test.go:12:6: require-error: for error assertions use require

For:

I have:

/home/pzak/test/some_test.go:12:6: require-error: for error assertions use require
/home/pzak/test/some_test.go:16:6: require-error: for error assertions use require

It seems that only the last !assert in the function is being ignored (i.e., not identified as an issue to be fixed), whereas I would expect all occurrences to be ignored.

[ccoVeille]

The issue you report here is a bit different than the issue described in this issue I think.

Please note, I'm not saying you didn't find an issue. I'm saying it's different.

While they are both related to require-error, the error reported by @mmorel-35 is about a detection improvement that should consider the fact an assert test followed by a return could be suggested to be replaced by a require. So it's a suggestion of improvement about something that is not coded now.

What you found is an "oddities" with the current require-error checker.

I'm surprised by two things:

• as you do, I don't get why the last is not reported
• I'm suspicious the checker should report the usage of assert.Error/NoError in a if statement, because a if assert.Error { something } might be tricky to be refactored with a require (unless there is a simple return in it, as the request of enhancement suggest)

[zak-pawel]

While they are both related to require-error, the error reported by @mmorel-35 is about a detection improvement that should consider the fact an assert test followed by a return could be suggested to be replaced by a require. So it's a suggestion of improvement about something that is not coded now.

@ccoVeille Yes, you're right, I should have opened a new PR.

However, regarding the mentioned improvement ("an assert test followed by a return could be suggested to be replaced by a require"), it's also worth considering that such an assert with a return might be one of the solutions to address findings related to go-require.
So (in short), a code like this:

if !assert.NoError(t, err) {
    errs <- err
    return
}

which is run in a goroutine, in my opinion, shouldn't be flagged as a problem.

I understand that the examples presented by @mmorel-35 only involve a simple return (without any arguments), but it might be worth considering whether this could also be one of the ways to fix findings of a different type.

[ccoVeille]

Exactly, thanks for providing a good example of what should not be reported.

Matthieu and I were only talking about reporting code exactly like this one

if assert.Whatever(t, … {
    return
}

To report using

require.Whatever(t, …

I use pseudocode here, the whatever could be an asserter about errors (assert.Error, assert.ErrorIs, …) or something as simple as assert.Equal

Because the pattern about using if + return is about using require, no matter the asserter. I agree go-require seems the right candidate. (So not require-error)

[ccoVeille]

Your example showed that we shouldn't report an if + assert.Error with require.Error in require-error checker

[zak-pawel]

• as you do, I don't get why the last is not reported

@ccoVeille Quick debugging and it seems the last is not reported because of this:

https://github.com/Antonboom/testifylint/blob/master/internal/checkers/require_error.go#L163-L193 :)
(you even write this in README: "the last assertion in the block, if there are no methods/functions calls after it;")

And none of these if statements with negation are being treated as if statements (i.e. inIfCond == true) because the handling of *ast.UnaryExpr:

is missing here:

https://github.com/Antonboom/testifylint/blob/master/internal/checkers/require_error.go#L71-L74

[ccoVeille]

Thank you, it will help us.

I don't have a computer with me for the next few days. Maybe Matthieu or Anton will look at it

Of course, you can open a PR after reading the contribution guide