empty: add Equal empty string

[ccoVeille]

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]

+ string conversion

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

[Antonboom]

+ len support

assert.Empty(t, len(whateverStr))

[Antonboom]

@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]

+ 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]

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]

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]

I found new use cases to be added to empty

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

[Antonboom]

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]

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]

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]

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]

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]

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]

I reviewed it again more closely 🔍

I noticed that I contradict myself and we already have checkers that introduce inconsistency into a group of checks -https://github.com/Antonboom/testifylint/issues/14 (a separate holy war issue, which we may return to in the future) 😭

So, I agree with

1. (Not)?Equal|EqualValues|Exactly + ""|`` + s|string(s) = (Not)?Empty + s
2. (Not)?Empty|Zero + len(s)|string(s)|len(string(s)) = (Not)?Empty + s (I agree with Zero-to-Empty support for strings and slices)
3. error assertions must be ignored by empty, some already covered by error-nil
4. True|False + len must be already covered by positive-negative
5. Ignore everything else that does not fall under the points above.

I will update the related PR and merge it soon 👌

[ccoVeille]

I'm glad you take a look at this.

I will wait for the PR to see in details.

[ccoVeille]

I think I remember there is an issue with Empty being used with byte instead of string. I have no computer with me to test or find back the discussion that may have occurred either here on testifylint or on testify repository.

The error message is then cryptic when it fails because it lists the ASCII code is empty/not empty.

Please note, I'm doing this feedback because you mentioned string(s) in your comment, so you made me think about it.

Maybe you don't plan to transform Equal(t, "", string(somebytes)) to Empty(t, somebytes), and/or it's not in your plan to remove the string conversion when it is present.

But I preferred to raise the point again

[Antonboom]

UPD, to simplify code and tests:

• Do not support string(bytes), to keep readability
• Do not support string(str), in favor of https://golangci-lint.run/usage/linters/#unconvert and consistency with no-untyping in negative-positive