Fix a couple of reflection based comparisons on protos in tests #1706
Conversation
This is not safe as protos have additional internal fields.
server/log_rpc_server_test.go
Outdated
| @@ -409,7 +408,7 @@ func TestGetLeavesByRange(t *testing.T) { | |||
| if test.wantErr != "" { | |||
| t.Errorf("GetLeavesByRange(%d, %+d)=_,nil; want nil, err containing %q", req.StartIndex, req.Count, test.wantErr) | |||
| } | |||
| if got := rsp.Leaves; !reflect.DeepEqual(got, test.want) { | |||
| if got := rsp.Leaves; !leavesEqual(got, test.want) { | |||
There was a problem hiding this comment.
| if got := rsp.Leaves; !leavesEqual(got, test.want) { | |
| if got := rsp.Leaves; !go-cmp.Equal(got, test.want, cmp.Comparer(proto.Equal)) { |
consider using go-cmp?
There was a problem hiding this comment.
I was put off by their doc that says it "may" use Equal methods. As protos have exported internal fields if it doesn't use Equal it's still wrong.
There was a problem hiding this comment.
My understanding was that it would use the comparer when possible, and reflect.Equal otherwise, which in the case of protos, is what we want IIUC.
https://godoc.org/github.com/google/go-cmp/cmp#Comparer
Comparer returns an Option that determines whether two values are equal to each other.
The comparer f must be a function "func(T, T) bool" and is implicitly filtered to input values assignable to T. If T is an interface, it is possible that f is called with two values of different concrete types that both implement T.
There was a problem hiding this comment.
It could just be poor wording in the go-cmp docs. We need to prevent reflect.Equal being used on protos as it contains exported fields that are not really part of the public state. If go-cmp always uses Equal then it's probably the right choice.
There was a problem hiding this comment.
cmp.Comparer(proto.Equal) would prevent reflect.Equal from being used on protos.
I've had troubles before where reflect.Equal complained about the XXX_ fields not being the same and I resolved it with cmp.Comparer(proto.Equal)
|
Towards #1738. |
This is not safe as protos have additional internal fields.
Checklist