From 0247dd2930e4d9d554c9bb2c304497b0c73f194b Mon Sep 17 00:00:00 2001 From: "R.B. Boyer" <4903+rboyer@users.noreply.github.com> Date: Mon, 22 May 2023 13:50:00 -0500 Subject: [PATCH] [1.14.x] prototest: fix early return condition in AssertElementsMatch (#17418) manual backport of #17416 to 1.14.x --- .../services/peerstream/stream_test.go | 8 +- proto/prototest/testing.go | 23 ++-- proto/prototest/testing_test.go | 103 ++++++++++++++++++ 3 files changed, 124 insertions(+), 10 deletions(-) create mode 100644 proto/prototest/testing_test.go diff --git a/agent/grpc-external/services/peerstream/stream_test.go b/agent/grpc-external/services/peerstream/stream_test.go index b2afd3d64a..6030af2e22 100644 --- a/agent/grpc-external/services/peerstream/stream_test.go +++ b/agent/grpc-external/services/peerstream/stream_test.go @@ -547,7 +547,7 @@ func TestStreamResources_Server_StreamTracker(t *testing.T) { it := incrementalTime{ base: time.Date(2000, time.January, 1, 0, 0, 0, 0, time.UTC), } - waitUntil := it.FutureNow(6) + waitUntil := it.FutureNow(7) srv, store := newTestServer(t, nil) srv.Tracker.setClock(it.Now) @@ -1439,7 +1439,9 @@ func makeClient(t *testing.T, srv *testServer, peerID string) *MockClient { // Note that server address may not come as an initial message for _, resourceURL := range []string{ pbpeerstream.TypeURLExportedService, + pbpeerstream.TypeURLExportedServiceList, pbpeerstream.TypeURLPeeringTrustBundle, + // only dialers request, which is why this is absent below pbpeerstream.TypeURLPeeringServerAddresses, } { init := &pbpeerstream.ReplicationMessage{ @@ -1468,7 +1470,7 @@ func makeClient(t *testing.T, srv *testServer, peerID string) *MockClient { { Payload: &pbpeerstream.ReplicationMessage_Request_{ Request: &pbpeerstream.ReplicationMessage_Request{ - ResourceURL: pbpeerstream.TypeURLPeeringTrustBundle, + ResourceURL: pbpeerstream.TypeURLExportedServiceList, // The PeerID field is only set for the messages coming FROM // the establishing side and are going to be empty from the // other side. @@ -1479,7 +1481,7 @@ func makeClient(t *testing.T, srv *testServer, peerID string) *MockClient { { Payload: &pbpeerstream.ReplicationMessage_Request_{ Request: &pbpeerstream.ReplicationMessage_Request{ - ResourceURL: pbpeerstream.TypeURLPeeringServerAddresses, + ResourceURL: pbpeerstream.TypeURLPeeringTrustBundle, // The PeerID field is only set for the messages coming FROM // the establishing side and are going to be empty from the // other side. diff --git a/proto/prototest/testing.go b/proto/prototest/testing.go index bf25fb0a10..f128e50747 100644 --- a/proto/prototest/testing.go +++ b/proto/prototest/testing.go @@ -26,14 +26,25 @@ func AssertDeepEqual(t testing.TB, x, y interface{}, opts ...cmp.Option) { func AssertElementsMatch[V any]( t testing.TB, listX, listY []V, opts ...cmp.Option, ) { - t.Helper() + diff := diffElements(listX, listY, opts...) + if diff != "" { + t.Fatalf("assertion failed: slices do not have matching elements\n--- expected\n+++ actual\n%v", diff) + } +} +func diffElements[V any]( + listX, listY []V, opts ...cmp.Option, +) string { if len(listX) == 0 && len(listY) == 0 { - return + return "" } opts = append(opts, protocmp.Transform()) + if len(listX) != len(listY) { + return cmp.Diff(listX, listY, opts...) + } + // dump into a map keyed by sliceID mapX := make(map[int]V) for i, val := range listX { @@ -57,8 +68,8 @@ func AssertElementsMatch[V any]( } } - if len(outX) == len(outY) && len(listX) == len(listY) { - return // matches + if len(outX) == len(listX) && len(outY) == len(listY) { + return "" // matches } // dump remainder into the slice so we can generate a useful error @@ -69,7 +80,5 @@ func AssertElementsMatch[V any]( outY = append(outY, itemY) } - if diff := cmp.Diff(outX, outY, opts...); diff != "" { - t.Fatalf("assertion failed: slices do not have matching elements\n--- expected\n+++ actual\n%v", diff) - } + return cmp.Diff(outX, outY, opts...) } diff --git a/proto/prototest/testing_test.go b/proto/prototest/testing_test.go new file mode 100644 index 0000000000..22c1848821 --- /dev/null +++ b/proto/prototest/testing_test.go @@ -0,0 +1,103 @@ +package prototest + +import ( + "strconv" + "testing" + + "github.com/stretchr/testify/require" +) + +type wrap struct { + V int + O string +} + +func (w *wrap) String() string { + return strconv.Itoa(w.V) +} + +func (w *wrap) GoString() string { + return w.String() +} + +func TestDiffElements_noProtobufs(t *testing.T) { + // NOTE: this test only tests non-protobuf slices initially + + type testcase struct { + a, b []*wrap + notSame bool + } + + run := func(t *testing.T, tc testcase) { + diff := diffElements(tc.a, tc.b) + if tc.notSame { + require.False(t, diff == "", "expected not to be the same") + } else { + require.True(t, diff == "", "expected to be the same") + } + } + + w := func(v int) *wrap { + return &wrap{V: v} + } + + cases := map[string]testcase{ + "nil": {}, + "empty": {a: []*wrap{}, b: []*wrap{}}, + "nil and empty": {a: []*wrap{}, b: nil}, + "ordered match": { + a: []*wrap{w(1), w(22), w(303), w(43004), w(-5)}, + b: []*wrap{w(1), w(22), w(303), w(43004), w(-5)}, + }, + "permuted match": { + a: []*wrap{w(1), w(22), w(303), w(43004), w(-5)}, + b: []*wrap{w(-5), w(43004), w(303), w(22), w(1)}, + }, + "duplicates": { + a: []*wrap{w(1), w(2), w(2), w(3)}, + b: []*wrap{w(2), w(1), w(3), w(2)}, + }, + // no match + "1 vs nil": { + a: []*wrap{w(1)}, + b: nil, + notSame: true, + }, + "1 vs 2": { + a: []*wrap{w(1)}, + b: []*wrap{w(2)}, + notSame: true, + }, + "1,2 vs 2,3": { + a: []*wrap{w(1), w(2)}, + b: []*wrap{w(2), w(3)}, + notSame: true, + }, + "1,2 vs 1,2,3": { + a: []*wrap{w(1), w(2)}, + b: []*wrap{w(1), w(2), w(3)}, + notSame: true, + }, + "duplicates omitted": { + a: []*wrap{w(1), w(2), w(2), w(3)}, + b: []*wrap{w(1), w(3), w(2)}, + notSame: true, + }, + } + + allCases := make(map[string]testcase) + for name, tc := range cases { + allCases[name] = tc + allCases[name+" (flipped)"] = testcase{ + a: tc.b, + b: tc.a, + notSame: tc.notSame, + } + } + + for name, tc := range allCases { + t.Run(name, func(t *testing.T) { + run(t, tc) + }) + } +}