assert: add NoFieldIsZero

[PeterEFinch]

EDIT: 2025-05-29 Originally this PR was to introduce the assertion NoFieldIsEmpty but has been replaced with NoFieldIsZero and check now unexported fields as well as exported.

Summary

Proposal to introduce an assertion that checks that no (exported) fields are empty zero in a struct.

Changes

1. A function NoFieldIsEmpty NoFieldIsZero is introduced into into the assert package. This function fails the test and returns false if any of the fields in the inputed struct (or reference to struct) are empty, aligning with pre-existing definition of empty zero.
2. Tests for the newly introduced NoFieldIsEmpty NoFieldIsZero function were added.
3. go generate ./... was run.

Motivation

I found myself repeatedly rewriting similar code in tests across different repositories in an effort to ensure that tests do not become out-of-date and no longer align 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 align 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.NoFieldIsZero(t, entity)

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

assert.NoFieldIsZero(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.

Related issues

None.

Additional comments

1. This form of assertion does not appear to be wide spread from what I have seen and thus maybe is of low impact - however, maybe that is just due to a dislike of reflection 😅.
2. Although I find such an assertion useful there are times when I want all bar some fields populated and it might be better to have a function with the signature

func NoFieldIsZero(t TestingT, object interface{}, exclude []string, msgAndArgs ...interface{}) bool

3. This assertion doesn't solve the issue of nested structs are populated too (although it does help). I avoided approaching addressing this because it was unclear of how to express nested field names and handle arrays of structs. Also, having some feedback on the the simple case seemed to be a good starting point.
4. Thank you for reading & reviewing 😄

[PeterEFinch]

Please let me know if I should also create an issue alongside this PR as it is for proposed functionality.

EDIT: I created an issue for this as well #1601.

[brackendawson]

What does "empty" mean? It's not in the language spec. From reading the code it seems to be "zero values for most types or with length of zero for some of the container types".

This assertion's behaviour needs to be clearly documented. I can't review it as it is because I can't tell what empty is supposed to mean. I think it's supposed to mean unpopulated struct fields? If that is the case then it doesn't work, see discussion in #1601.

[PeterEFinch]

I am happy to have more explicit documentation.

One option would be to provide a godoc link to the current assert.Empty function e.g.

NoFieldIsEmpty asserts that object, which must be a struct or eventually reference to one, has no exported field with a value that is empty (following the definition of empty used in [Empty]).

Another option is to include the same definition used in assert.Empty in the documentation for this function e.g.

NoFieldIsEmpty asserts that object, which must be a struct or eventually reference to one, has no exported field with a value that is empty I.e. nil, "", false, 0 or either a slice or a channel with len == 0.

Do find either these clear enough?

P.S. I am aware that "empty" is not part of the go spec. My intent was to follow a definition already used by this library (that I have found useful) rather than invent something myself.

[brackendawson]

The godoc link would be sufficient.

[dolmen]

This implementation checks only for Empty-ness only at the first struct level. For deeper structured types, the zero value rule applies, and thay may or may not be the user expectation.

At this point I don't want to expose more of the Empty inconsistent rules in a new API with its own weiredness.

[dolmen]

Move this block above at the top of the function. This is important for the case of pointer to struct to be reported at the right place.

[dolmen]

As isEmpty is called, if the field type is a struct or a pointer to struct, NoFieldIsEmpty is not called.

So the documentation should point out that NoFieldIsEmpty is not applied recursively.

[dolmen]

Related: #1753 improves documentation and test coverage of assert.Empty.

[PeterEFinch]

@dolmen Thanks for the feedback. I will make the changes you suggested.

I also intend to change this PR to introduce NoFieldIsZero. This is based on @brackendawson's reservation about the use of empty, #1601 (comment), and your on comment on Empty inconsistent rules.

[brackendawson]

I'm dismissing my previous review because it was resolved. I still have comments though.

I still don't like this assertion very much, I was considering how I would do this in the standard library and came up with:

// Tests that all fields in entity can be stored and loaded
func TestPersistence(t *testing.T) {
	var entity Entity = struct {
		// If you add fields to Entity then populate them in this test
		ID, Name string
	}{
		ID: "1",
		Name: "Jimbob",
	}
	
	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")
}

Which will fail to compile if you added new fields.

[brackendawson]

The change to use zero value rather than the existing but extremely weird testify "empty" definition is good and resolves my previous comments.

I think this should be recursive though, it doesn't solve the example use case well when it is not.

[PeterEFinch]

If you think it is an improvement I can change the implementation (my original implementations were).

I am slightly hesitant about it though due to naming and go conventions. The convention of the reflect package is the fields of a struct do not include fields of nested structs. Taking the recursive approach could lead to confusion.

[brackendawson]

The use case you provided as an example would fail as soon as any future developer nests a struct, it could fail to store/load and your NoFielIsZero asserion would not protect your test.

If following the conventions of reflect is a hard requirement, and there is no other use case for which this feature works well, then we should not add this feature.

I don't think testify is at all consistent with depth in its assertions. assert.Equal uses reflect.DeepEqual, but assert.ElementsMatch is shallow.

[PeterEFinch]

The proposed assertion in this PR is aligns with property-based testing.

That alternative works in that scenario.

Consider a function

// FakeEntity returns an Entity with every field filled with random values.
func FakeEntity() Entity {...}

I can see how to use the the approach you put forth in a test in this case - I think it would need to be in the function itself. By construction it would do the right thing but it would be tested to do so.

I still don't like this assertion very much

Honestly, I see this approach as uncommon in the go community. I think that is because example based testing is generally used over property based testing (although I dislike full PBT frameworks because it they force too much on the user).