-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
test: remove api that should not be used #15678
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Function name and doc could be slightly more clear, but everything else looks like a simple mechanical refactor.
testutil/compare.go
Outdated
) | ||
|
||
// ProtoDeepEqual is a helper function that uses the protocmp package to compare two protobuf messages. | ||
func ProtoDeepEqual(t *testing.T, p1, p2 interface{}) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RequireProtoDeepEqual
is probably a better function name.
RequireProtoDeepEqual fails the test t if p1 and p2 are not equivalent protobuf messages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And this is why I am going to request reviews from you more often :D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For that matter, I think p1
and p2
should be of type proto.Message
iirc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cmp.Diff
accepts interface{}
, so I think we should accept interfaces as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cmp.Diff
does accept any
, but I'm not sure what the intended behavior is if a programmer mistakenly passes a non-proto.Message
type when using protocmp.Transform
. If there is a runtime failure in that case, then it would be better to fail earlier, at compile time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, maybe proto.Message
would be too restrictive if you were comparing slices instead. Maybe the doc should mention that the function can handle values or slices in the case of continuing to accept any
for p1
and p2
.
Description
ref: #12332 (comment)
Given the sync in the standup, just removing the helper with had introduced for v0.48, for the API lacking of gotest.tools.
As going forward, we still use testify assertions, we can simply delete them, and add a proto deep equal helper.
We need to remove them now for ensuring that no one uses them.
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!
to the type prefix if API or client breaking changeCHANGELOG.md
Reviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...
!
in the type prefix if API or client breaking change