Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow specifying default deep-equality comparison options? #212

Open
glenjamin opened this issue Nov 5, 2020 · 2 comments
Open

Allow specifying default deep-equality comparison options? #212

glenjamin opened this issue Nov 5, 2020 · 2 comments

Comments

@glenjamin
Copy link

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

google/go-cmp#241 (comment)

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.

@dnephin
Copy link
Member

dnephin commented Nov 7, 2020

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.

@glenjamin
Copy link
Author

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 - golang/protobuf#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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants