Proposal: assert.NoFieldIsZero

[PeterEFinch]

Description

Introducing a function to assert that no field in a struct is empty.

Proposed solution

Proposed implementation in #1591

Use case

Writing tests where it is important to assert all fields are populated, not just the fields present at the time of creating the test. Often used to avoid tests becoming out-of-date and no longer aligning with their original intention.

An example of this is writing tests that check that all fields in a struct can be stored and then loaded. Consider the test:

// Tests that all fields in entity can be stored and loaded
func TestPersistence(t *testing.T) {
	entity := Entity{
		// Filled with data
	}
	
	s := NewStore()
	err := s.Store(entity)
	require.NoError(t, err)
	
	result, err := s.Load(entity.ID)
	require.NoError(t, err)
	assert.Equal(t, entity, result, "result should match the entity stored")
}

This test is claiming to check that all fields can be stored and loaded but it not enforcing it. If the entity was not correctly populated initially or if new fields where added to the Entity and the test was not updated then the test would not be aligned with its stated purpose.

In this particular case the assertion could be used as a pre-condition to ensure we always start with all fields containing data

require.NoFieldIsEmpty(t, entity)

or as check at the end of test to ensure all fields where populated

assert.NoFieldIsEmpty(t, result)

I have found this assertion useful for when using tests that require populated structs including:

1. Testing the storing and loading of structs into database or persistence storage.
2. Testing the population of structs e.g. writing fakers.
3. Testing the marshalling/formatting of structs and unmarshalling/parsing data to structs.

[Antonboom]

Hi!

Using so common assertion looks like too smoke testing.

Why don't you use per field assertion?
Such tests more strict and easier to support and debug.

P.S. What is opposite analogue? assert.Zero?
P.P.S. Naming proposal – assert.Filled

[PeterEFinch]

Hi @Antonboom

Why don't you use per field assertion?

This does not exactly test the same thing and such assertions easily go out-of-date. It some cases it is critical that all fields are filled not just ones the developer remembers to include in the test.

For example, if I have a type T and with fields A and B and want to test a function that creates a fake T or a function that converts another type to T then I could assert that both A & B are non-empty but if someone adds another field to T without updating the test then the test is not following its original intention. The more developers there are the more chance this happens.

I think it is useful to know if something is zero/empty, non-zero/non-empty or maximal. This assert is one kind of "maximally filled" check.

P.S. What is opposite analogue?

It would be assert.SomeFieldsAreEmpty.

P.P.S. Naming proposal

Happy for alternative names. I also thought of
2. assert.MaximallyFilled (opposite analogue assert.NotMaximallyFilled
3. assert.AllFieldsAreNonEmpty (opposite analogue assert.SomeFieldsAreEmpty)

[PeterEFinch]

P.S. I definitely think that this is niche but it has come up a few times at different companies where I have worked. It has been implemented as a helper but there is an aversion to maintaining helpers that rely on reflection.

The key thing is we want check for this kind of maximality... however, maximality is usually a lot more context dependent and not as straight-forward compared to the empty/zero case.

[brackendawson]

I understand your anxiety, I get the same feeling sometimes. For sure this would be really niche. There's a reason I'm not keen on this assertion, which is that it makes tests that all break whenever someone adds a field and those aren't cost effective tests¹. A conscientious developer updating your package will be doing TDD and updating tests first. This also relies on the type being defined in the package which accepts it as an argument to the Store function (dedicated types packages are bad).

What about unconscientious developers? Well, there's a lot of ways they can break your code.

This assertion would also make it impossible to test your type with any fields set to a zero value?

These are just my opinions. I'm open to approaches or demonstrations of this approach which play well with best practices. But I think this problem is in the domain of writing unit tests well and not putting a block in the way of people updating the type. If you have really obvious fixtures/testdata and you test all of them against Store (maybe by listing the testdata directory); will it be obvious to future developers to add testdata for their new field?

Footnotes

1. poodr chapter 9 ↩

[PeterEFinch]

Thanks for the feedback @brackendawson 😄

I want to clarify/reiterate that the key goal is to be able to have the contract of being "maximally filled" expressible in tests, rather than it living in someone's head or just in documentation. It is more about rigour - I trust devs work to the best of their ability and know mistakes will still be made.

There's a reason I'm not keen on this assertion, which is that it makes tests that all break whenever someone adds a field and those aren't cost effective tests.

Understandable. It is true that it will break the tests if someone adds a field without other changes meaning more work. However, the cost effectiveness will depend on the relative importance of the property compared to other factors. Sometimes that breakage is important, for example:

1. If the assertion is used to test a contract and it breaks then we want to know it because there is now a bug.
2. If the assertion is used as a pre-condition e.g. in a property based test, and it breaks then it means the tests are no longer correct.

Regarding blocking devs, I would say that the block should only go as far preventing the dev from introducing a bug or unwanted side-affects (including adversely affecting tests).

Coming up with an example that illustrates the usefulness while still being simple is rather tricky. I have written a working example (https://go.dev/play/p/rvHXuz97gFS) which includes the place where the assertion would be included. Hopefully, this also makes it clear that it would not (significantly) disrupt the DX. It is not about putting this assertion everywhere but just in the places where it adds value.

This assertion would also make it impossible to test your type with any fields set to a zero value?

I don't think I fully understand your comment here... This assertion should only be used if there is a requirement all fields are non-empty. One could test the type with some fields set to zero and not use this assertion.

If you are talking about a requirement to have only certain fields empty or non-empty then this becomes much trickier. In the past I have written an inspect package (I had also thought about suggesting it be added to this repo but wasn't sure it was the right place) which contains helper functions using reflection, such as

package inspect

import (...)

func IsEmpty(a any) bool {...}

func ListEmptyFields(a any) []string {...}

func ListNonEmptyFields(a any) []string {...}

In the cases where you have sets of test data, you could also potentially get the lists for each object and take intersections to ensure that each field is covered once.

[brackendawson]

Ah, I had assumed that by "non-empty" you meant "not-the-zero-value".

Unfortunately after compilation, which is when testify runs, there is no way of knowing if a struct field was set or not in source. Observe: https://go.dev/play/p/_mpXvOv-jX8 Even with reflection you cannot tell the difference between a field which is not set and one which is set to the zero-value for its type.

That somewhat fundamentally prevents testify from implementing this, you'd need to implement it as a linter.

[PeterEFinch]

Unless there is a flaw in my implementation, it should be possible to use reflection to check if exported fields are non-empty. See #1591. The limitation to checking exported only fields aligns to black box testing techniques.

I have followed the testify assert definition of empty/non-empty

testify/assert/assertions.go

Lines 717 to 744 in 1b4fca7

	// isEmpty gets whether the specified object is considered empty or not.
	func isEmpty(object interface{}) bool {
	// get nil case out of the way
	if object == nil {
	return true
	}
	objValue := reflect.ValueOf(object)
	switch objValue.Kind() {
	// collection types are empty when they have no element
	case reflect.Chan, reflect.Map, reflect.Slice:
	return objValue.Len() == 0
	// pointers are empty if nil or if the value they point to is empty
	case reflect.Ptr:
	if objValue.IsNil() {
	return true
	}
	deref := objValue.Elem().Interface()
	return isEmpty(deref)
	// for all other types, compare against the zero value
	// array types are empty when they match their zero-initialized state
	default:
	zero := reflect.Zero(objValue.Type())
	return reflect.DeepEqual(object, zero.Interface())
	}
	}

Using non-zero instead of non-empty could also be useful.

[brackendawson]

It doesn't work: https://go.dev/play/p/-vfKFT2QcBn

Your implementations appears to be checking that fields are non-zero except for maps, slices, and channels which have to be more than non-zero, they need to contain an element.

So Go has no concept of "empty", this assertion would need a different name. What would it be called? And how would its behaviour be documented? It's very complicated.

[PeterEFinch]

It isn't clear to me which of your comments relate to the code I have proposed.

The concept of "empty" is from this library (see here and here).

My proposed assertion does two things:

1. Checks the input is a struct or eventual references one (following a pattern already used in this library).
2. Iterates the fields of that struct and applies the pre-existing definition of isEmpty to exported fields.

Could you please say which part you think doesn't work? 1? 2? or the pre-existing isEmpty function? Likewise, with your comment about being complicated.

[brackendawson]

If I understand your proposal correctly then this test should pass, but using your branch it does not:

package kata_test

import (
	"testing"

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

type MyStruct struct {
	MistakeCount int
}

func TestIfy(t *testing.T) {
	// No field is "empty"
	m := MyStruct{
		MistakeCount: 0,
	}
	assert.NoFieldIsEmpty(t, m) // returns false
}

[PeterEFinch]

Unfortunately, your interpretation is not what I was after. Hopefully one of the alternate documentations in #1591 (comment) makes the expected behaviour clear.

In the details below I have used your example and written an equivalent to NoFieldIsEmpty using reflection and NotEmpty. This should hopefully illuminate the difference. EDIT: I am hiding it, in-case you think it is useful to first understand the assertion purely by the documentation.

package kata_test

import (
	"reflect"
	"testing"

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

type MyStruct struct {
	MistakeCount int
}

func TestIfy(t *testing.T) {
	m := MyStruct{
		MistakeCount: 1, // If this is 0 both sub-tests will fail.
	}

	// The "proposed_way" test will pass if and only if the "current_way_using_reflection" test passes.
	// Only equivalent for m being a struct. References to structs require de-referencing in "current_way_using_reflection". 

	t.Run("proposed_way", func(t *testing.T) {
		assert.NoFieldIsEmpty(t, m, "MyStruct contains empty field(s)")
	})

	t.Run("current_way_using_reflection", func(t *testing.T) {
		objectType := reflect.TypeOf(m)
		objectValue := reflect.ValueOf(m)
		for i := 0; i < objectType.NumField(); i++ {
			field := objectType.Field(i)
			if !field.IsExported() {
				continue
			}

			assert.NotEmptyf(t, objectValue.Field(i).Interface(), "The field %s in MyStruct is empty", field.Name)
		}
	})
}

[brackendawson]

So this NoFieldIsEmpty can guard against an exported field being added to a struct without an important test being updated. But, only if this important test populates the struct's fields with what NotEmpty considers to be not empty.

Go has no concept of emptiness, the Empty and NotEmpty assertions have a strange definition of empty which is sometimes a zero value and sometimes a length. Honestly those assertions should have never been added.

I can see that this has utility for you. I have pretty steep concerns about any given testify user figuring out what this assertion is for and correctly using it.

I'd be slightly more comfortable with a NoZeroValues assertion which deeply checks all values in a structure are not the zero value for their type, because that sits better with the language spec.

[PeterEFinch]

That is an accurate summary of its purpose!

I can see that this has utility for you. I have pretty steep concerns about any given testify user figuring out what this assertion is for and correctly using it.

Absolutely fair. I do find it very useful and simultaneously think that this approach is uncommon and can be unclear to others without clear communication or examples.

I'd be slightly more comfortable with a NoZeroValues assertion which deeply checks all values in a structure are not the zero value for their type, because that sits better with the language spec.

That is reasonable and am I am willing have a NoFieldsAreZeroValues assertion. I agree that checking for zero values sits better with the language specs, however, I think checking for "emptiness" provides more utility and aligns more with practical use-cases. The reasons I chose "empty" over zero is:

1. I see it used more. In the codebases I work in assert.Empty & assert.NotEmpty are used approximately 8 times more often than assert.Zero & assert.NotZero.
2. Checking slice for being "empty", i.e. having a length of zero, rather than being the zero value, i.e. nil, aligns closer to what I see done in practice. In application code and tests, it is usually the case the developers I work/have worked with check whether or not a slice has length zero rather than is the zero value or not as it is more important to know whether or not the slice has elements.

All that being said, I have my preferences, priorities & reasons, and understand that others do to. Typically, at this stage I would reach out to others to get their thoughts - not sure what the process is for this library.