Skip to content

Commit

Permalink
Fix generic comparisons on protobuf messages
Browse files Browse the repository at this point in the history
Generated protobuf messages contain internal data structures
that general purpose comparison functions (e.g., reflect.DeepEqual,
pretty.Compare, etc) do not properly compare. It is already the case
today that these functions may report a difference when two messages
are actually semantically equivalent.

Fix all usages by either calling proto.Equal directly if
the top-level types are themselves proto.Message, or by calling
cmp.Equal with the cmp.Comparer(proto.Equal) option specified.
This option teaches cmp to use proto.Equal anytime it encounters
proto.Message types.
  • Loading branch information
dsnet committed Nov 6, 2019
1 parent ddf53b6 commit c5b53be
Show file tree
Hide file tree
Showing 3 changed files with 9 additions and 11 deletions.
9 changes: 5 additions & 4 deletions fswalker_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import (
"testing"
"time"

"github.com/golang/protobuf/proto"
"github.com/google/go-cmp/cmp"

fspb "github.com/google/fswalker/proto/fswalker"
Expand Down Expand Up @@ -133,7 +134,7 @@ func TestReadTextProtoReviews(t *testing.T) {
if err := readTextProto(ctx, filepath.Join(testdataDir, "reviews.asciipb"), reviews); err != nil {
t.Errorf("readTextProto() error: %v", err)
}
diff := cmp.Diff(reviews, wantReviews)
diff := cmp.Diff(reviews, wantReviews, cmp.Comparer(proto.Equal))
if diff != "" {
t.Errorf("readTextProto(): unexpected content: diff (-want +got):\n%s", diff)
}
Expand All @@ -156,7 +157,7 @@ func TestReadTextProtoConfigs(t *testing.T) {
if err := readTextProto(ctx, filepath.Join(testdataDir, "defaultReportConfig.asciipb"), config); err != nil {
t.Fatalf("readTextProto(): %v", err)
}
diff := cmp.Diff(config, wantConfig)
diff := cmp.Diff(config, wantConfig, cmp.Comparer(proto.Equal))
if diff != "" {
t.Errorf("readTextProto(): unexpected content: diff (-want +got):\n%s", diff)
}
Expand Down Expand Up @@ -185,7 +186,7 @@ func TestReadPolicy(t *testing.T) {
t.Errorf("readTextProto() error: %v", err)
return
}
diff := cmp.Diff(pol, wantPol)
diff := cmp.Diff(pol, wantPol, cmp.Comparer(proto.Equal))
if diff != "" {
t.Errorf("readTextProto() policy: diff (-want +got): \n%s", diff)
}
Expand Down Expand Up @@ -220,7 +221,7 @@ func TestWriteTextProtoReviews(t *testing.T) {
if err := readTextProto(ctx, tmpfile.Name(), gotReviews); err != nil {
t.Errorf("readTextProto() error: %v", err)
}
diff := cmp.Diff(gotReviews, wantReviews)
diff := cmp.Diff(gotReviews, wantReviews, cmp.Comparer(proto.Equal))
if diff != "" {
t.Errorf("writeTextProto() reviews: diff (-want +got): \n%s", diff)
}
Expand Down
5 changes: 1 addition & 4 deletions reporter_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ import (
"fmt"
"io/ioutil"
"os"
"strings"
"testing"
"time"

Expand Down Expand Up @@ -199,9 +198,7 @@ func TestReadWalk(t *testing.T) {
if got.Fingerprint.Value != wantFp {
t.Errorf("readwalk(): fingerprint value, got=%s, want=%s", got.Fingerprint.Value, wantFp)
}
diff := cmp.Diff(got.Walk, wantWalk, cmp.FilterPath(func(p cmp.Path) bool {
return strings.Contains(p.String(), "XXX_")
}, cmp.Ignore()))
diff := cmp.Diff(got.Walk, wantWalk, cmp.Comparer(proto.Equal))
if diff != "" {
t.Errorf("readwalk(): content diff (-want +got):\n%s", diff)
}
Expand Down
6 changes: 3 additions & 3 deletions walker_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ func TestWalkerFromPolicyFile(t *testing.T) {
t.Errorf("WalkerFromPolicyFile() error: %v", err)
return
}
diff := cmp.Diff(wlkr.pol, wantPol)
diff := cmp.Diff(wlkr.pol, wantPol, cmp.Comparer(proto.Equal))
if diff != "" {
t.Errorf("WalkerFromPolicyFile() policy: diff (-want +got):\n%s", diff)
}
Expand All @@ -111,7 +111,7 @@ func TestProcess(t *testing.T) {
continue
}
}
if diff := cmp.Diff(wlkr.walk.File, files); diff != "" {
if diff := cmp.Diff(wlkr.walk.File, files, cmp.Comparer(proto.Equal)); diff != "" {
t.Errorf("wlkr.walk.File != files: diff (-want +got):\n%s", diff)
}
}
Expand Down Expand Up @@ -293,7 +293,7 @@ func TestConvert(t *testing.T) {
}

gotFile = wlkr.convert(path, info)
diff := cmp.Diff(gotFile, wantFile)
diff := cmp.Diff(gotFile, wantFile, cmp.Comparer(proto.Equal))
if diff != "" {
t.Errorf("convert() File proto: diff (-want +got):\n%s", diff)
}
Expand Down

0 comments on commit c5b53be

Please sign in to comment.