From 775fd0b08c9ed6c2f79dcf0446df477c2c83e047 Mon Sep 17 00:00:00 2001 From: Lory Cloutier Date: Wed, 30 Nov 2022 11:53:46 -0600 Subject: [PATCH] Don't use reflect.DeepEqual to compare errors. FB-1771 In api_test.go, it was being used to make sure that incorrect input produced the right errors; this is now handled by making sure the error isn't nil and then checking its string against the expected error string. In executor_test.go and internal_client_test.go, it was being used to compare QueryResponse structures, which contain an error. handler.go now has a function specifically for comparing them, which can provide additional detail if necessary. Added a test for SameAs to handler_test.go. --- api_test.go | 16 ++++++++-------- executor_test.go | 2 +- handler.go | 24 ++++++++++++++++++++++++ handler_test.go | 37 +++++++++++++++++++++++++++++++++++++ internal_client_test.go | 4 ++-- 5 files changed, 72 insertions(+), 11 deletions(-) diff --git a/api_test.go b/api_test.go index 1458c1130..37fb0c974 100644 --- a/api_test.go +++ b/api_test.go @@ -1264,8 +1264,8 @@ func TestVariousApiTranslateCalls(t *testing.T) { t.Run("translateIndexDbOnNilIndex", func(t *testing.T) { err := api.TranslateIndexDB(context.Background(), "nonExistentIndex", 0, r) - expected := fmt.Errorf("index %q not found", "nonExistentIndex") - if !reflect.DeepEqual(err, expected) { + expected := fmt.Sprintf("index %q not found", "nonExistentIndex") + if err == nil || err.Error() != expected { t.Fatalf("expected '%#v', got '%#v'", expected, err) } }) @@ -1273,8 +1273,8 @@ func TestVariousApiTranslateCalls(t *testing.T) { t.Run("translateIndexDbOnNilTranslateStore", func(t *testing.T) { err := api.TranslateIndexDB(context.Background(), c.Idx(), 0, r) - expected := fmt.Errorf("index %q has no translate store", c.Idx()) - if !reflect.DeepEqual(err, expected) { + expected := fmt.Sprintf("index %q has no translate store", c.Idx()) + if err == nil || err.Error() != expected { t.Fatalf("expected '%#v', got '%#v'", expected, err) } }) @@ -1282,8 +1282,8 @@ func TestVariousApiTranslateCalls(t *testing.T) { t.Run("translateFieldDbOnNilIndex", func(t *testing.T) { err := api.TranslateFieldDB(context.Background(), "nonExistentIndex", "field", r) - expected := fmt.Errorf("index %q not found", "nonExistentIndex") - if !reflect.DeepEqual(err, expected) { + expected := fmt.Sprintf("index %q not found", "nonExistentIndex") + if err == nil || err.Error() != expected { t.Fatalf("expected '%#v', got '%#v'", expected, err) } }) @@ -1291,8 +1291,8 @@ func TestVariousApiTranslateCalls(t *testing.T) { t.Run("translateFieldDbOnNilField", func(t *testing.T) { err := api.TranslateFieldDB(context.Background(), c.Idx(), "nonExistentField", r) - expected := fmt.Errorf("field %q/%q not found", c.Idx(), "nonExistentField") - if !reflect.DeepEqual(err, expected) { + expected := fmt.Sprintf("field %q/%q not found", c.Idx(), "nonExistentField") + if err == nil || err.Error() != expected { t.Fatalf("expected '%#v', got '%#v'", expected, err) } }) diff --git a/executor_test.go b/executor_test.go index 3daa2fe07..ba6e5a85a 100644 --- a/executor_test.go +++ b/executor_test.go @@ -9325,7 +9325,7 @@ func TestExternalLookup(t *testing.T) { t.Parallel() result := c.Query(t, c.Idx(), tc.query) - if !reflect.DeepEqual(result, tc.expect) { + if tc.expect.SameAs(&result) != nil { t.Errorf("expected %v but got %v", tc.expect, result) } }) diff --git a/handler.go b/handler.go index bc5e35212..e08197982 100644 --- a/handler.go +++ b/handler.go @@ -3,7 +3,9 @@ package pilosa import ( "encoding/json" + "fmt" "math/bits" + "reflect" "time" "github.com/molecula/featurebase/v3/shardwidth" @@ -77,6 +79,28 @@ func (resp *QueryResponse) MarshalJSON() ([]byte, error) { }) } +// SameAs compares one QueryResponse to another and returns nil if the Results +// and Err are both identical, or a descriptive error if they differ. +// This function replaces using reflect.DeepEqual directly on the +// QueryResponse, since QueryResponse contains an error field and reflect.DeepEqual +// should not be used on errors. +func (qr *QueryResponse) SameAs(other *QueryResponse) error { + switch { + case !reflect.DeepEqual(qr.Results, other.Results): + return fmt.Errorf("responses contained different results") + case qr.Err == nil && other.Err == nil: + return nil + case qr.Err == nil && other.Err != nil: + return fmt.Errorf("unexpected error: %w", other.Err) + case qr.Err != nil && other.Err == nil: + return fmt.Errorf("missing error: expected %v, got no error", qr.Err) + case qr.Err.Error() != other.Err.Error(): + return fmt.Errorf("wrong error: expected %v, got %w", qr.Err, other.Err) + default: + return nil + } +} + // HandlerI is the interface for the data handler, a wrapper around // Pilosa's data store. type HandlerI interface { diff --git a/handler_test.go b/handler_test.go index 7e102d303..612b7c38d 100644 --- a/handler_test.go +++ b/handler_test.go @@ -2,7 +2,9 @@ package pilosa_test import ( + "fmt" "math/rand" + "strings" "testing" pilosa "github.com/molecula/featurebase/v3" @@ -26,3 +28,38 @@ func TestSortToShards(t *testing.T) { } } } + +func TestQueryResponseSameAs(t *testing.T) { + var qr1, qr2 pilosa.QueryResponse + qr1.Results = []any{"testing different results"} + qr2.Results = []any{"with SCIENCE!"} + qr1.Err = nil + qr2.Err = nil + if err := qr1.SameAs(&qr2); err.Error() != "responses contained different results" { + t.Fatalf("expected responses to contain different results") + } + qr1.Results = []any{"this time, for sure!"} + qr2.Results = []any{"this time, for sure!"} + if err := qr1.SameAs(&qr2); err != nil { + t.Fatalf("expected responses to be the same") + } + err1 := fmt.Errorf("out of cheese") + err2 := fmt.Errorf("out of dryd frorg pills") + qr1.Err = err1 + if err := qr1.SameAs(&qr2); !strings.Contains(err.Error(), "missing error") { + t.Fatalf("should have had a missing error") + } + qr1.Err = nil + qr2.Err = err2 + if err := qr1.SameAs(&qr2); !strings.Contains(err.Error(), "unexpected error") { + t.Fatalf("should have had an unexpected error") + } + qr1.Err = err1 + if err := qr1.SameAs(&qr2); !strings.Contains(err.Error(), "wrong error") { + t.Fatalf("should have had a wrong error") + } + qr2.Err = err1 + if err := qr1.SameAs(&qr2); err != nil { + t.Fatalf("expected responses top be the same") + } +} diff --git a/internal_client_test.go b/internal_client_test.go index bbd474b54..f96579b24 100644 --- a/internal_client_test.go +++ b/internal_client_test.go @@ -160,11 +160,11 @@ func TestClient_MultiNode(t *testing.T) { } // Compare TopN results across all nodes in the cluster. - if !reflect.DeepEqual(result, result1) { + if result.SameAs(result1) != nil { t.Fatalf("TopN result should be the same on node0 and node1: %s", spew.Sdump(result1)) } - if !reflect.DeepEqual(result, result2) { + if result.SameAs(result2) != nil { t.Fatalf("TopN result should be the same on node0 and node2: %s", spew.Sdump(result2)) } }