Antonboom / testifylint

The Golang linter that checks usage of github.com/stretchr/testify.
https://github.com/stretchr/testify
MIT License
101 stars 8 forks source link

empty: report special message for len and string-cast cases #149

Closed ldez closed 1 month ago

ldez commented 5 months ago
package sandbox

import (
    "testing"

    "github.com/stretchr/testify/assert"
)

func TestName(t *testing.T) {
    assert.NotEmptyf(t, len([]string{"e"}), "text: %s", "a")
    assert.NotEmptyf(t, []string{"e"}, "text: %s", "a")
}
$ ./golangci-lint run --enable-only testifylint
main_test.go:10:2: empty: use assert.NotEmptyf (testifylint)
        assert.NotEmptyf(t, len([]string{"e"}), "text: %s", "a")
        ^

I used a version of golangci-lint that uses testifylint v1.4.3.

ldez commented 5 months ago

I think you already know that but it's the same for NotEmpty, Emptyf, Empty.

I have not tested all the assertions.

package sandbox

import (
    "testing"

    "github.com/stretchr/testify/assert"
)

func TestName(t *testing.T) {
    assert.NotEmptyf(t, len([]string{"e"}), "text: %s", "a")
    assert.NotEmptyf(t, []string{"e"}, "text: %s", "a")

    assert.NotEmpty(t, len([]string{"e"}))
    assert.NotEmpty(t, []string{"e"})

    assert.Emptyf(t, len([]string{"e"}), "text: %s", "a")
    assert.Emptyf(t, []string{"e"}, "text: %s", "a")

    assert.Empty(t, len([]string{"e"}))
    assert.Empty(t, []string{"e"})
}
$ ./golangci-lint run --enable-only testifylint
main_test.go:10:2: empty: use assert.NotEmptyf (testifylint)
        assert.NotEmptyf(t, len([]string{"e"}), "text: %s", "a")
        ^
main_test.go:13:2: empty: use assert.NotEmpty (testifylint)
        assert.NotEmpty(t, len([]string{"e"}))
        ^
main_test.go:16:2: empty: use assert.Emptyf (testifylint)
        assert.Emptyf(t, len([]string{"e"}), "text: %s", "a")
        ^
main_test.go:19:2: empty: use assert.Empty (testifylint)
        assert.Empty(t, len([]string{"e"}))
        ^
ccoVeille commented 5 months ago

Good catch, the way it reports now could let people think it's a false negative, while it's not, testifylint suggests to simplify.

So I agree with you the message needs to be adapted

Antonboom commented 1 month ago

After fix:

$ testifylint --disable-all --enable=empty ./...                                                                        
/tmp/sandbox/main_test.go:10:2: empty: remove unnecessary len
/tmp/sandbox/main_test.go:13:2: empty: remove unnecessary len
/tmp/sandbox/main_test.go:16:2: empty: remove unnecessary len
/tmp/sandbox/main_test.go:19:2: empty: remove unnecessary len