Support protobuf equality

[evie404]

There's a subtle issue when using assertions with protobuf structs, because protobuf structs contain internal fields that should be ignored during assertions.

An XXX_sizecache field that is part of the internal implementation of the table-driven serializer. The presence of this field can break tests that use reflect.DeepEqual to compare some received messages with some golden message. It is recommended that proto.Equal be used instead for this situation.

• https://groups.google.com/forum/#!topic/golang-nuts/F5xFHTfwRnY

I think it'd be nice for testify to support this. Particularly, we can add this line in ObjectsAreEqual to check if both objects are protobuf messages, and if so, proto.Equal as suggested.

	expProto, ok := expected.(proto.Message)
	if ok {
		actProto, ok := actual.(proto.Message)
		if ok {
			// if both are protobuf messages, use `proto.Equal`
			return proto.Equal(expProto, actProto)
		}
	}

The problem would be introducing a dependency on golang/protobuf, which I'm not sure would be acceptable.

If not, one way to address this is using build constraints. We can split the ObjectsAreEqual method into a file equal.go that is compiled by default. And with a build constraint, we can toggle whether to include the protobuf support:

equal.go:

// +build !protobuf

equal_protos.go:

// +build protobuf

This way, we can include support for protobuf without introducing the dependency for everyone.

[evie404]

we've been using a fork in case interested parties want to try it out: vsco@d23661d

[vincentbernat]

Until #535 is implemented, this would be a nice workaround to have.

[glesica]

It'd be great to see a PR to resolve #535, but barring that, I'm not sure we want to introduce a protobuf dependency. A build constraint might be OK, though. @boyan-soubachov thoughts?

[boyan-soubachov]

@glesica , I would agree that this is a bit too use-case specific and would want to avoid having protobuf-specific checks & code.

I am, however completely on-board with moving to go-cmp. I am afraid, however that it would be a big project and almost certainly necessitate a /v2

[glesica]

OK cool, let's try to pull that into the next release then, even if it doesn't actually go out with the next release it will keep it from getting lost.

[dsnet]

(I'm the author of both cmp and one of the primary authors of Go protobufs)

Google heavily uses protobufs and I have been asked many times for cmp to provide native support for them. However, I've denied such requests because adding support for them would require a dependency on protobufs. Furthermore, it introduces a dangerous slippery slope: if protobufs can have special treatment, can other Go types have special treatment as well? What about Cap'n Proto or Thrift? Specialized support for any type sets precedence for adding special handling of other types resulting in cmp having a massive dependency tree. Presumably this same argument applies to the assert project.

BTW, for cmp to better support protobufs, there is a protocmp package that is specifically designed to support protobuf messages with cmp.

[boyan-soubachov]

(I'm the author of both cmp and one of the primary authors of Go protobufs)

Google heavily uses protobufs and I have been asked many times for cmp to provide native support for them. However, I've denied such requests because adding support for them would require a dependency on protobufs. Furthermore, it introduces a dangerous slippery slope: if protobufs can have special treatment, can other Go types have special treatment as well? What about Cap'n Proto or Thrift? Specialized support for any type sets precedence for adding special handling of other types resulting in cmp having a massive dependency tree. Presumably this same argument applies to the assert project.

BTW, for cmp to better support protobufs, there is a protocmp package that is specifically designed to support protobuf messages with cmp.

Fair point. I've had some more time to think about this one as well and it seems like makes more sense for testify to remain a simple/thin functionality package rather than aiming to cater to all use cases. I definitely agree with supporting what the community wants and if there is an overwhelming voice for adding protobufs, then we should do it.

Thank you for the input, @dsnet

[ocavue]

I'm using embedding type to "inherit" assert.Assertions with my extra method to support protobuf equality.

Define a new internal package called expect (to not conflict with the name assert) and write our custom assertion method:

package expect

import (
	"fmt"

	"github.com/stretchr/testify/assert"
	"google.golang.org/protobuf/proto"
)

// Expectation is a embedding type for assert.Assertions
type Expectation struct {
	*assert.Assertions
}

// New makes a new Expectation object for the specified TestingT.
func New(t assert.TestingT) *Expectation {
	assert := assert.New(t)
	return &Expectation{assert}
}

// ProtoEqual asserts that the specified protobuf messages are equal.
func (a *Expectation) ProtoEqual(expected, actual proto.Message) bool {
	return a.True(
		proto.Equal(expected, actual),
		fmt.Sprintf("These two protobuf messages are not equal:\nexpected: %v\nactual:  %v", expected, actual),
	)
}

Then in the test files, I can use both the origin assert functions from github.com/stretchr/testify/assert as well as my custom assert function.

func TestBaseWorkflow(t *testing.T) {
	expect := expect.New(t)
	expect.NotEqual(1, 2)  // 
	expect.ProtoEqual(&pb.Pod{}, &pb.Pod{})  // 
}

[bogdandrutu]

@boyan-soubachov I think having a small extensibility to Equal like what cmp-go does, would make a lot of sense:

func Diff(x, y interface{}, opts ...Option) string

type Option interface {
	// filter applies all filters and returns the option that remains.
	// Each option may only read s.curPath and call s.callTTBFunc.
	//
	// An Options is returned only if multiple comparers or transformers
	// can apply simultaneously and will only contain values of those types
	// or sub-Options containing values of those types.
	filter(s *state, t reflect.Type, vx, vy reflect.Value) applicableOption
}

Maybe the interface will be different, similar to ObjectsAreEqual would make everyone happy:

• Reduce dependency on custom libraries like proto
• Allow users to write their own custom Equaler for things like proto.

assert.CustomEqual(t, want, got, MyCustomProtoEqualer)

[torkelrogstad]

A solution to this would be to allow people to provide custom equality and serialization options.

Could look like this:

        // applied to assert.New constructor
	assert := assert.New(t, assert.WithEqualCheck(func(expected, actual interface{}) (result, ok bool) {
		expectedProto, ok := expected.(proto.Message)
		if !ok {
			return false, false
		}

		actualProto, ok := actual.(proto.Message)
		if !ok {
			return false, false
		}

		return proto.Equal(expectedProto, actualProto), true
	})
	
        // with package level option, if not using the assert.New constructor
	assert.AddEqualCheck(func(expected, actual interface{}) (result, ok bool) {
		expectedProto, ok := expected.(proto.Message)
		if !ok {
			return false, false
		}

		actualProto, ok := actual.(proto.Message)
		if !ok {
			return false, false
		}

		return proto.Equal(expectedProto, actualProto), true
	})

The first boolean result is the equality check, and the second is if it "should count", i.e. can we bail on comparisons or should we continue with other comparisons.

This would let people pull in their own dependencies, while still leveraging this great library.

[FarhanSajid1]

Any updates to this?

[Seklfreak]

I believe by now EqualExportedValues does the trick? https://pkg.go.dev/github.com/stretchr/testify/assert#EqualExportedValues

[stephenpmurray]

I believe by now EqualExportedValues does the trick? https://pkg.go.dev/github.com/stretchr/testify/assert#EqualExportedValues

Good point. Unfortunately, this doesn't work for pointers.

Perhaps the feature request now is to make this function handle pointers?

[redachl]

I believe by now EqualExportedValues does the trick? https://pkg.go.dev/github.com/stretchr/testify/assert#EqualExportedValues

Good point. Unfortunately, this doesn't work for pointers.

Perhaps the feature request now is to make this function handle pointers?

I just opened a pull request for this. Thanks for the suggestion :).

[redachl]

Sorry just closed the PR.
Actually the PR #1517 already allows EqualExportedValues to accept pointers, and it was approved 🎉

Let's wait for the PR to be merged

[brackendawson]

I've merged that change and you should expect it in testify v1.8.5. Does EqaulExpertedValues fully resolve this Issue or are there some comparisons that protobuf users still can't make?

[redachl]

I've merged that change and you should expect it in testify v1.8.5. Does EqaulExpertedValues fully resolve this Issue or are there some comparisons that protobuf users still can't make?

Thanks for the change it's really useful!

Something unrelated to protobuf is still missing: Equal and EqualValues can be run on slices, but EqualExportedValues doesn't run on slices. Is there a specific reason for this?

Let me know if I should create another issue.

[brackendawson]

Yes, create a new issue.

I'm going to mark this as completed. Please let me know if protobuf equality is still an issue.