empty: add Equal empty string

ccoVeille commented 5 months ago

This pattern is common usef

assert.Equal(t, "", whatever)
assert.Equal(t, ``, whatever)
assert.Equal(t, "", string(whatever))
assert.Equal(t, ``, string(whatever))
assert.Empty(t, string(whatever))

And it could be replaced

assert.Empty(t, whatever)

Examples: https://github.com/search?q=language%3Ago+%22assert.Equal%28t%2C+%5C%22%5C%22%22&type=code https://github.com/search?q=language%3Ago+%22assert.Equal%28t%2C+%5C%22%5C%22%2C+string%22&type=code https://github.com/search?q=language%3Ago+%22assert.Empty%28t%2C+string%28%22&type=code

https://github.com/search?q=%22assert.Equal%28t%2C+%60%60%2C%22&type=code

https://github.com/search?q=%22assert.Equal%28t%2C+%60%60%2C+string%28%22&type=code

Antonboom commented 5 months ago

+ string conversion

https://github.com/Antonboom/testifylint/issues/113#issue-2340406872

Antonboom commented 5 months ago

+ len support

assert.Empty(t, len(whateverStr))

Antonboom commented 5 months ago

@ccoVeille, hi!

This is thin ice.

Looks like if we will support string that it need to expand the checker 1) or to all zero-values (that I do not support) 2) or to all len-compatible types – strings, channels, maps

But I am not sure about 2), that this is the best/common practice. Maybe to introduce in the checker some configuration 🤔

ccoVeille commented 5 months ago

+ len support

https://github.com/Antonboom/testifylint/issues/119#issuecomment-2156152399

+ string conversion

https://github.com/Antonboom/testifylint/issues/119#issuecomment-2156141025

I updated issue description with both of them

ccoVeille commented 5 months ago

https://github.com/Antonboom/testifylint/issues/119#issuecomment-2156152600

Looks like if we will support string that it need to expand the checker\n\nor to all zero-values (that I do not support)\nor to all len-compatible types – strings, channels, maps

I'm OK with letting zero typed outside the scope of empty checker.

About the fact to handle other types that support len. I would say, let's start with string, we could add the other later.

What do you think?

There is no problem to iterate.

My remark is based on the fact that ot would require to check if there are real life examples where empty could be used with Len and other types that are compatible with len.

You somehow urged me not to implement, support strange use case such as zero and empty with compare and bool compare checkers. While they were valid wrong usages, you told me to forget them because they are likely to never have been used.

I found wrong usages of asserters on string that could be replaced by Empty, you agree to catch them. let's support them.

Other can be added later, if needed.

ccoVeille commented 5 months ago

I found new use cases to be added to empty

https://github.com/search?q=language%3Ago+%22assert.True%28t%2C+0+%3D%3D+len%28%22&type=code

    assert.True(t, 0 == len(read.RelatedSlice))

https://github.com/objectbox/objectbox-go/blob/fb4848df90642394c0800a7d8e9bc0e0743ce903/test/relations_test.go#L259

https://github.com/search?q=language%3Ago+%22assert.True%28t%2C+0+%3C+len%28%22&type=code

    assert.True(t, 0 < len(notify.Hash()))

https://github.com/33cn/plugin/blob/e87d9cc084028b3306185458fd0cdcda70f01438/plugin/consensus/dpos/types/signable_test.go#L114

Let me know if you want me to add them to #129

Antonboom commented 5 months ago

I found new use cases to be added to empty

I guess this is already covered by chain compares -> negative-positive -> empty

Antonboom commented 5 months ago

Agreed about

assert.Empty(t, string(whatever)) -> assert.Empty

But have doubts about

assert.Equal(t, "", whatever)         -> assert.Empty
assert.Equal(t, "", string(whatever)) -> assert.Empty

Do you have some proofs of this as a best practice? I am not sure.

Examples from your search:

assert.Nil(t, err, "send %v", err)
assert.Equal(t, 0, int(ret.ExitCode))
assert.Equal(t, 0, int(ret.GasUsed))
assert.Equal(t, "", string(ret.ReturnData))    // Why do require Empty here?

assert.EqualValues(t, 0, nextCreateAt, "should have finished iterating through drafts")
assert.Equal(t, "", nextUserId, "should have finished iterating through drafts") // Why do require Empty here?

ccoVeille commented 5 months ago

I found new use cases to be added to empty

I guess this is already covered by chain compares -> negative-positive -> empty

Not exactly

var arr []string
assert.True(t, 0 < len(arr))

becomes

var arr []string
assert.Positive(t, len(l))

But this one won't be detected, I agree with you, it could/should become

assert.NotEmpty(t, l)

So it's a new use case to add to `empty` checker, I opened :

https://github.com/Antonboom/testifylint/issues/139

But this pattern is detected

    assert.True(t, 0 == len(read.RelatedSlice))

len will fix like this

    assert.Len(t, read.RelatedSlice, 0)

empty will fix like this

    assert.Empty(t, read.RelatedSlice)

ccoVeille commented 5 months ago

Agreed about
assert.Empty(t, string(whatever)) -> assert.Empty
But have doubts about
assert.Equal(t, "", whatever)         -> assert.Empty
assert.Equal(t, "", string(whatever)) -> assert.Empty
Do you have some proofs of this as a best practice? I am not sure.

Maybe we are facing the same debate as should we check an empty string with:

if len(str) == 0 {

}

or

if str == "" {

}

This is defintely a debate. But here we have testify, it exists and provides asserters, let's consider them

Equal(t, "", str)
Zero(t, str)
Empty(t, str)

Let's put aside Zero, we already talked about it multiple times this one is not adapted.

Empty exists, and it's documentation talk about:

Empty asserts that the specified object is empty. I.e. nil, "", false, 0 or either a slice or a channel with len == 0.

Let's take each example described one by one:

Empty with nil (empty slice/map excluded), I would use Nil in most case, and NoError if it's a nil error
false, I would use False, but I won't fix code using Empty
0, I would use Zero, but I wouldn't enforce to use it, and code using Empty would be fine for me.
channel I would use Empty
slice I would use Empty, and we enforce it when we find len(arr)

So except channel, map, slices would be fine.

Also a string is a slice of characters, so somehow a slice of runes/bytes in Go

When we are facing this "", we designate it as an empty string, so using Empty sounds logic.

Even more, if you look about the usage of Equal(t, "", whatever), especially the message they add https://github.com/search?q=language%3Ago+%22Equal%28t%2C+%5C%22%5C%22%2C%22&type=code https://github.com/search?q=language%3Ago+%22Equalf%28t%2C+%5C%22%5C%22%2C%22&type=code https://github.com/search?q=language%3Ago+%22NotEqual%28t%2C+%5C%22%5C%22%2C%22&type=code

https://github.com/rclone/rclone/blob/300851e8bfe3b587a51c89dff6e84fb57929350f/fstest/fstests/fstests.go#L2043

require.NotEqual(t, "", link3, "Link should not be empty")

https://github.com/mbland/elistman/blob/25a6cea40f7690f23ac52cabde79a419e78444ca/cmd/testutils.go#L46

assert.Equal(t, "", f.Stderr.String(), "stderr should be empty")

https://github.com/helmfile/helmfile/blob/0d79d14841b4ae19cb5ca2256cecc76835e23147/pkg/filesystem/fs_test.go#L49

    require.Equalf(t, "", path, "Expected empty path but got %s", path)

https://github.com/linkedin/Burrow/blob/8690c5844cab06601634bf21fa07591f951068fc/core/internal/consumer/kafka_client_test.go#L294

    assert.Equalf(t, "", errorAt, "Expected decodeMetadataValueHeader to return empty errorAt, not %v", errorAt)

https://github.com/firecracker-microvm/firecracker-containerd/blob/a3e402334342d109ddeeba2c65187eb3fe9ec83a/runtime/service_integ_test.go#L2232

    require.NotEqualf(t, "", line, "%s must not be empty", path)

For me, we should add Equal(t, "", str) detection to Empty

and by extension Equal(t, "", string(whatever))

ccoVeille commented 5 months ago

BTW, we do not detect error used with empty, right now

var err error
assert.Empty(t, err)

We should recommend using NoError

and this pattern is widely used https://github.com/search?q=language%3Ago+%22assert.Empty%28t%2C+err%22&type=code

Same for

var err error
assert.Zero(t, err)

https://github.com/search?q=language%3Ago+%22assert.Zero%28t%2C+err%29%22&type=code

These two are candidates for error-nil, so I opened:

138

breml commented 4 months ago

To add to this discussion, I just stumbled over this code in our tests:

assert.NotEqual(t, ``, somevar)

and I think, the linter should suggest

assert.NotEmpty(t, somevar)

in this case.

ccoVeille commented 4 months ago

Indeed this pattern is present

https://github.com/search?q=%22assert.Equal%28t%2C+%60%60%2C%22&type=code

I edited the issue description to support it.

And I also added

https://github.com/search?q=%22assert.Equal%28t%2C+%60%60%2C+string%28%22&type=code

(I'm unsure if it would differ in term of detection, but at least we will add it to the unit tests)

Antonboom / testifylint