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

if both types are proto messages, use proto.Equal #1

Merged
merged 3 commits into from
May 9, 2019

Conversation

evie404
Copy link

@evie404 evie404 commented Apr 22, 2019

Context

golang/protobuf 1.0.0 contained a few major changes, one of which was the addition of a new field:

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.

As noted, the presence of these new fields breaks test assertions.

Description

This PR modifies the behavior for ObjectsAreEqual which is used by assertion methods like assert.Equal to support equality checks for protobuf messages.

This allows us to transparently compare protobufs messages without breaking existing tests even if they have different underlying XXX_ values. This is recommended by the protobuf team.

The alternative is create another assertion method specific to protobuf, then migrate every existing assertions that compares two protobuf messages to use the new assertion method. Even then, it does not cover cases like slices or maps that may contain protobuf messages. I think forking the assertion library is the most straight-forward solution with relative low-cost, as this is a relatively simple library, it should be easy to keep the forks in sync.

Also, I don't think it's likely that this will be accepted upstream as it would introduce the protobuf dependency to all users even thought they may not be using protobuf. I might try to suggest using a compiler flag upstream to make this functionality optional, but I don't think it should block our own upgrades.

Testing

  • unit tests in the library
  • internal tests against the go repo

@evie404
Copy link
Author

evie404 commented Apr 29, 2019

Upstream issue opened in stretchr#758 to solicit possibility of PR

@evie404 evie404 merged commit d23661d into vsco:master May 9, 2019
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

Successfully merging this pull request may close these issues.

2 participants