testing/protocmp: add IgnoreUnsetFields

[MasterMedo]

Is your feature request related to a problem? Please describe.
Comparing protobuf types in full with the cmp.Diff or cmp.Equal makes for a change detector test. This is because every time a new field is added to the protobuf all old tests need to be updated to have this field as well. This doesn't improve the confidence that the code is correct and introduces operational load on the engineers. Additionally, the lack of this feature causes many side values of the return type to be mocked. The problem with that is that often the mocked values don't contribute to the validity of the test and when they change in the implementation all tests need to be changed as well.

Describe the solution you'd like
I'd like an option like protocmp.IngoreUnsetFields(m protobufMessage) to exist. It would be used by passing in the want protobuf message as the argument. E.g.

func TestMessageContent(t *testing.T) {
  content := "content"
  want := pb.Message_builder{
    Content: content,
  }
  got := MessageFromContent(content)
  if diff := cmp.Diff(want, got, protocmp.Transform(), protocmp.IgnoreUnsetFields(want)); diff != "" {
    t.Errorf(...)
  }
}

Describe alternatives you've considered
Alternatively, one has to manually implement the function by using the cmp.FilterField function.

Additional context
This started as a document created internally in Google because often when a field was added to a protobuf old tests needed to be changed. The addition of this function only partially solves the issues in that document, but it's a great first step!

[stapelberg]

Implementing an IgnoreUnsetFields function in protocmp seems straight-forward:

func IgnoreUnsetFields(want proto.Message) cmp.Option {
	msg := want.ProtoReflect()
	md := msg.Descriptor()
	fields := md.Fields()
	var names []protoreflect.Name
	for i := 0; i < fields.Len(); i++ {
		fd := fields.Get(i)
		if msg.Has(fd) {
			continue // field is set, do not ignore
		}
		names = append(names, fd.Name())
	}
	return protocmp.IgnoreFields(want, names...)
}

…but I’m curious to hear what others think about this addition.

cc @neild @dsnet

[dsnet]

What's the expected behavior with field extensions?

[stapelberg]

What's the expected behavior with field extensions?

Good question. Can you spell out what the concerns and options are?

It sounds like maybe we’ll need to use protoreflect.Range instead of walking only the msg.Descriptor().Fields(). Or were you thinking of something else/more?

Thanks

[dsnet]

I don't have any specific concerns, but just trying to remember things that would often bite us later on.

Overall this feature sounds fine. It reminds me of @neild's "messagediff" package, which IIRC had an asymmetrical comparison that did something similar.

[neild]

For extensions, I'd expect IgnoreUnsetFields(want) to ignore any extensions not set in want. I think that might require some additional handling beyond @stapelberg's implementation above.

The messagediff package (Google-internal) was based on a C++ library (not sure if that one is internal or open source) that contained a "partial compare" which compares two messages, considering only fields set on the right-hand message. This is quite convenient in tests.

I wonder if the IgnoreUnsetFields feature could work as a general-purpose cmp or cmpopt option, not a protocmp proto-specific one.

got := map[string]int{"a": 1, "b": 2}
want := map[string]int{"a": 1}
diff := cmp.Diff(got, want, cmpopts.IgnoreZero(want)) // ignores "b"

And perhaps a PartialDiff helper:

func PartialDiff(got, want any, opts ...Option) string {
  return cmp.Diff(x, y, IgnoreZero(y), cmp.Options(opts))
}

[MasterMedo]

I was curious about the messagediff package, thanks for mentioning it. It seems like the idea there is similar to the cmpopts.IgnoreFields. Can you link the partial compare you're referring to @neild? I thought that wouldn't be possible because default field values and unset values would be treated equally.

The way I see it, the biggest value of having an IgnoreUnsetFields (or maybe a better name would be IgnoreFieldsFrom) function over other approaches that can tackle the same problem is that the proposed approach here is compatible with parametrized tests that are so popular in Go.

In the table below all 'yes' values are positive, all 'no' values are negative.

	full structure comparison (status quo)	field-by-field comparison	filter field names you care about explicitly – FilterField(fieldName string)	filter field names you don't care about – IgnoreField(fieldName string)	[PROPOSED HERE] ignoreUnsetFields(msg proto.Message)
Is it possible to test all deterministic behavior?	yes	yes	yes	yes	yes
Readability-wise, is it obvious what the code is doing?	yes	yes	yes	yes	yes
Does the test return all values that are different in the want and got part of the test?	yes	no	yes	yes	yes
When a new field is added to the structure a function is returning and it starts populating it, does the test still pass?	no, the hard-coded value of the test needs to be updated (yellow because this is the intended behavior)	yes	yes	no, the test fails because a new field has to be ignored	yes
Can you avoid mocking fields the test isn't concerned with?	no	yes	yes	yes	yes
Can the comparison library avoid using reflection?	yes	yes	yes	yes	yes
How many times do you need to refer to the same fields?	once	twice – once in the want assignment and once in the comparison	twice – once in the want assignment and once in the filters	once	once
How do the lines of code needed to write a test depend on the number of fields that need (n) or don't need to be compared (m)?	n + m – specifying the fields of the want value	2m – specifying the fields of the want value and number of comparison lines for each field	2m – specifying the fields of the want value and number of filters	n + m – specifying the fields of th want value and number of filters	m – specifying the fields of the want value
Works well with parameterized tests?	yes	no	no	no	yes

I couldn't find a downside with the proposed approach, but there might be something I'm not considering and I should. I share the concern with @dsnet that there might be some unforeseen complications that will come to bite us later on.

The way I'll proceed is by creating this function in my team/org and popularizing it there.

[stapelberg]

The messagediff package (Google-internal) was based on a C++ library (not sure if that one is internal or open source) that contained a "partial compare" which compares two messages, considering only fields set on the right-hand message. This is quite convenient in tests.

Are you referring to https://github.com/protocolbuffers/protobuf/blob/main/src/google/protobuf/util/message_differencer.h perhaps? It is a C++ message diffing library with partial comparison support.

[comparison table] Can the comparison library avoid using reflection?

I don’t think your “yes”es in this row are correct. The cmp library certainly uses reflection, and so does protoreflect.

I wonder if the IgnoreUnsetFields feature could work as a general-purpose cmp or cmpopt option, not a protocmp proto-specific one.

Perhaps! Though it might be prudent to start with a protocmp option that knows about protobuf messages, and later replace it with a call to the general-purpose IgnoreUnsetFields() if that is indeed a drop-in replacement.

@MasterMedo filed google/go-cmp#345 and retracted it with this comment:

I'm retracting this feature request. Structs are significantly different from Protobuf messages in the sense that the primitive values of structs don't have corresponding HasField functions. That would introduce a challenge in the current proposal because one couldn't distinguish between an unset primitive field and a field set to the default value of the primitive.

…but that’s not inherent to structs. proto3 does not have presence either (except for fields explicitly marked as “optional”), so it does not distinguish between an unset field and a default-valued field either. So, we’ll face the same issue with a protocmp option.

Returning to my original implementation:

Implementing an IgnoreUnsetFields function in protocmp seems straight-forward:

func IgnoreUnsetFields(want proto.Message) cmp.Option { … }

This function signature is different from the other protocmp signatures: the currently existing ones take a parameter to work with the type, whereas mine takes a parameter to work with the specific message. My signature is problematic because it requires an odd structure for table-driven tests: instead of preparing opts := []cmp.Option{protocmp.Transform(), …} once and then calling cmp.Diff(tt.want, got, opts) for each table entry, my signature requires re-constructing the opts based on tt.want for each table entry.

I think we should try to come up with an implementation that matches the semantics of the existing options better. I had a quick look at it, but diving into cmp’s bowels requires more time than I can spend on this right now. Suggestions and contributions welcome.

[neild]

Are you referring to https://github.com/protocolbuffers/protobuf/blob/main/src/google/protobuf/util/message_differencer.h perhaps? It is a C++ message diffing library with partial comparison support.

Yes, that's the one. The Google-internal messagediff package was my attempt at providing a Go alternative to it. (This predated official protobuf reflection, so the messagediff internals were quite a mess. Joe rewrote messagediff to use protobuf reflection at some point, and designed cmp/protocmp as a more general-purpose replacement.)