Add call Empty(values ...any)

[skelouse]

var (
    emptyString string
    emptyInt int
    emptySlice []string
)

// This check
assert.Equal(t, emptyString, "")
assert.Equal(t, emptyInt, 0)
assert.Equal(t, emptySlice, nil)

// Becomes
assert.Empty(t, emptyString, emptyInt, emptySlice)

[dnephin]

Thank you for your interest in contributing to this project!

One of the primary design goals for this library is to keep the number of functions to a minimum. The only functions in the assert package should be ones that are necessary to provide helpful error messages for the most common cases. Fewer functions makes the library easier to use.

Another goal is to avoid having many ways to do something. As much as possible there should be only a single way of asserting some condition, to help make tests more consistent and easier to read.

I believe this proposal is not aligned with these goals. As you point out, there's already a really easy way to assert that these types have a zero or nil value. Another way to assert an empty slice would be:

assert.Equal(t, emptySlice, []string{})
// or
assert.Equal(t, len(emptySlice), 0)
// or, with more detail
assert.Equal(t, len(emptySlice), 0, "got %#v", emptySlice)

These should give you a similar error message, without the need for another function.

I think it would be valuable to add this example to the docs, and to add a design.md document to make the design goals more official.

Unfortunately I don't think it's a good idea to add an Empty method to the library.

[skelouse]

@dnephin Thanks for the speedy response to my requested issue and fix.

I understand having as few calls as possible, but this would improve test readability. In the cases of an empty pointer, taking and the length of a slice, or expanding on structs; assert.Empty would be a singular call with the goal of filling all nil, empty, and no item cases.

For example:

// will panic contrary to the example i gave originally
var emptyStringSlice []string
assert.Equal(t, emptyStringSlice, []string{})

// fails with +++ []string(nil){}
var emptyStringSlice []string
assert.DeepEqual(t, emptyStringSlice, []string{})
assert.DeepEqual(t, emptyStringSlice, nil)

// Or in the case of a pointer to a []string, assertion fails. +++ nil any( (*[]string)(nil)
var emptyStringSlice *[]string
assert.Equal(t, emptyStringSlice, nil)
// To check the above is nil, 
var emptyStringSlice *[]string
var anotherEmptySlice *[]string
assert.Equal(t, emptyStringSlice, anotherEmptySlice)

I agree that fewer calls makes assert simple to digest, but I also think that Empty would be a valuable addition to the package.

If you are unswayed I would be happy to add as an example, and can use the Empty in my own tests as an example. Where would I go about submitting to the docs?

[dnephin]

I guess the reason I've never run into these sub-optimal failure messages myself is because I always write the assertion as

assert.Equal(t, len(actual), 0, "got %#v", actual)

I believe it's generally a good practice to write Go code that doesn't care about the different between an empty slice and a nil slice, so the assertions I write follow the same rules.

I guess there may be some rare cases where that distinction is important, but I've never encountered them myself.

Pretty soon I think we'll be able to start using generics for assert.Equal (#249). The x/exp/slices, and x/exp/maps packages are on track to be added to the stdlib soon, and both of them have Equal functions.

I think with generics we may be able to extend assert.Equal to handle this comparison (instead of the panic that happens today), which would allow us to provide a better error message. I would prefer to explore that option before adding a new assert.Empty function (or more likely an assert.IsZero function, if we want to allow scalar types to be passed as arguments).

The place to document the existing options would be the examples in the godoc here: https://github.com/gotestyourself/gotest.tools/blob/f44676a54686a87dcccb32d90ec5d7dd78743332/assert/assert.go#L50-L59

[dolmen]

It looks like what you call "empty" is called "zero value" in the Go world. See for example time.Time.IsZero.

So I think an cmp.Zero that would handle the case where the value implements the interface { IsZero() bool } or fallbacks to reflect.Value.IsZero() would be more useful than this Empty.