gotest.tools icon indicating copy to clipboard operation
gotest.tools copied to clipboard

Allow specifying default deep-equality comparison options?

Open glenjamin opened this issue 3 years ago • 2 comments

I originally tried pitching this to go-cmp, and they said no because they quite resonably want all comparison to be stateless.

https://github.com/google/go-cmp/issues/241#issue-736205887

The context from that issue is:

After upgrading to the most recent version of protobuf, we are finding all of our comparisons are failing due to some new unexported fields.

There is a cmp.Option implementation provided by the protobuf module in godoc.org/google.golang.org/protobuf/testing/protocmp#Transform - but I now need to go and add this option to every single compare.

This particular check is generic and safe to add on every compare - would it be reasonable to provide a way to install such global comparison options?

This feels like it might be slightly safer for a test assertion tool, as each "global" would be scoped to the test binary.

glenjamin avatar Nov 05 '20 14:11 glenjamin

This feels like it might be slightly safer for a test assertion tool, as each "global" would be scoped to the test binary.

I think that would be true of the go-cmp package as well, as long as the defaults are registered in a test file.

It was my understadning that go-cmp already worked well with protobuf, because all the types it generates implemented Equal. Is that no longer the case? Maybe it is specifically the gRPC types I am thinking of. If the comparison does use the Equal method I wonder if go-cmp should consider it the same as a cmp.Comparer which skips the check for unexported fields.

I'm also not keen on adding a global registry of cmp options. What I would probably do would be to create an assertDeepEqualPB func, and sed replace all the existing calls. I guess that is still some amount of work if there are lots of packages with these tests.

dnephin avatar Nov 07 '20 23:11 dnephin

From what I can tell, grpc/protobuf generated code didn't used to have any private fields - but the new rewrite they did recently now does.

I've opened an issue with them about implementing Equal - https://github.com/golang/protobuf/issues/1238 - answer so far is noncommittal in either direction, but it sounds like they're wary of adding more codegen in general.

In practice it turns out the affected codebase only tests protobuf generated structs at the edges - so adding compare options to each DeepEquals was merely annoying, but not too arduous.

glenjamin avatar Nov 08 '20 08:11 glenjamin