diff --git a/.changelog/5faed85326334a93a9b0a96284389287.json b/.changelog/5faed85326334a93a9b0a96284389287.json new file mode 100644 index 000000000..6da094278 --- /dev/null +++ b/.changelog/5faed85326334a93a9b0a96284389287.json @@ -0,0 +1,8 @@ +{ + "id": "5faed853-2633-4a93-a9b0-a96284389287", + "type": "bugfix", + "description": "Remove runtime dependency on go-cmp.", + "modules": [ + "." + ] +} \ No newline at end of file diff --git a/codegen/smithy-go-codegen/src/main/java/software/amazon/smithy/go/codegen/integration/HttpProtocolUnitTestGenerator.java b/codegen/smithy-go-codegen/src/main/java/software/amazon/smithy/go/codegen/integration/HttpProtocolUnitTestGenerator.java index 25e0304e4..c8d35dc66 100644 --- a/codegen/smithy-go-codegen/src/main/java/software/amazon/smithy/go/codegen/integration/HttpProtocolUnitTestGenerator.java +++ b/codegen/smithy-go-codegen/src/main/java/software/amazon/smithy/go/codegen/integration/HttpProtocolUnitTestGenerator.java @@ -445,29 +445,7 @@ protected void writeAssertComplexEqual( GoWriter writer, String expect, String actual, String[] ignoreTypes ) { writer.addUseImports(SmithyGoDependency.SMITHY_TESTING); - writer.addUseImports(SmithyGoDependency.GO_CMP); - writer.addUseImports(SmithyGoDependency.GO_CMP_OPTIONS); - writer.addUseImports(SmithyGoDependency.SMITHY_DOCUMENT); - writer.addUseImports(SmithyGoDependency.MATH); - - writer.openBlock("opts := cmp.Options{", "}", () -> { - writer.openBlock("cmpopts.IgnoreUnexported(", "),", () -> { - for (String ignoreType : ignoreTypes) { - writer.write("$L,", ignoreType); - } - }); - writer.write("cmp.FilterValues(func(x, y float64) bool {\n" - + "\treturn math.IsNaN(x) && math.IsNaN(y)\n" - + "}," - + "cmp.Comparer(func(_, _ interface{}) bool { return true })),"); - writer.write("cmp.FilterValues(func(x, y float32) bool {\n" - + "\treturn math.IsNaN(float64(x)) && math.IsNaN(float64(y))\n" - + "}," - + "cmp.Comparer(func(_, _ interface{}) bool { return true })),"); - writer.write("cmpopts.IgnoreTypes(smithydocument.NoSerde{}),"); - }); - - writer.openBlock("if err := smithytesting.CompareValues($L, $L, opts...); err != nil {", "}", + writer.openBlock("if err := smithytesting.CompareValues($L, $L); err != nil {", "}", expect, actual, () -> { writer.write("t.Errorf(\"expect $L value match:\\n%v\", err)", expect); }); diff --git a/go.mod b/go.mod index c1ef3249c..1139d913e 100644 --- a/go.mod +++ b/go.mod @@ -1,5 +1,3 @@ module github.com/aws/smithy-go go 1.20 - -require github.com/google/go-cmp v0.5.8 diff --git a/go.sum b/go.sum index e9b099ce2..e69de29bb 100644 --- a/go.sum +++ b/go.sum @@ -1,2 +0,0 @@ -github.com/google/go-cmp v0.5.8 h1:e6P7q2lk1O+qJJb4BtCQXlK8vWEO8V1ZeuEdJNOqZyg= -github.com/google/go-cmp v0.5.8/go.mod h1:17dUlkBOakJ0+DkrSSNjCkIjxS6bF9zb3elmeNGIjoY= diff --git a/modman.toml b/modman.toml index 20295cdd2..9d94b7cbd 100644 --- a/modman.toml +++ b/modman.toml @@ -1,5 +1,4 @@ [dependencies] - "github.com/google/go-cmp" = "v0.5.8" "github.com/jmespath/go-jmespath" = "v0.4.0" [modules] diff --git a/testing/document.go b/testing/document.go index 0cf158c04..a9b32fdca 100644 --- a/testing/document.go +++ b/testing/document.go @@ -4,12 +4,11 @@ import ( "bytes" "encoding/json" "fmt" + "reflect" "sort" "strings" "github.com/aws/smithy-go/testing/xml" - - "github.com/google/go-cmp/cmp" ) // JSONEqual compares two JSON documents and identifies if the documents contain @@ -25,8 +24,8 @@ func JSONEqual(expectBytes, actualBytes []byte) error { return fmt.Errorf("failed to unmarshal actual bytes, %v", err) } - if diff := cmp.Diff(expect, actual); len(diff) != 0 { - return fmt.Errorf("JSON mismatch (-expect +actual):\n%s", diff) + if !reflect.DeepEqual(expect, actual) { + return fmt.Errorf("JSON mismatch: %v != %v", expect, actual) } return nil @@ -60,8 +59,8 @@ func XMLEqual(expectBytes, actualBytes []byte) error { return err } - if diff := cmp.Diff(actualString, expectedString); len(diff) != 0 { - return fmt.Errorf("XML mismatch (-expect +actual):\n%s", diff) + if expectedString != actualString { + return fmt.Errorf("XML mismatch: %v != %v", expectedString, actualString) } return nil @@ -85,8 +84,10 @@ func AssertXMLEqual(t T, expect, actual []byte) bool { // contain the same values. Returns an error if the two documents are not // equal. func URLFormEqual(expectBytes, actualBytes []byte) error { - if diff := cmp.Diff(parseFormBody(expectBytes), parseFormBody(actualBytes)); len(diff) != 0 { - return fmt.Errorf("Query mismatch (-expect +actual):\n%s", diff) + expect := parseFormBody(expectBytes) + actual := parseFormBody(actualBytes) + if !reflect.DeepEqual(expect, actual) { + return fmt.Errorf("Query mismatch: %v != %v", expect, actual) } return nil } diff --git a/testing/struct.go b/testing/struct.go index 080ee397d..3b08f9c2b 100644 --- a/testing/struct.go +++ b/testing/struct.go @@ -4,41 +4,148 @@ import ( "bytes" "encoding/hex" "fmt" - "github.com/aws/smithy-go/document" "io" - "io/ioutil" - "net/http" + "math" + "reflect" - "github.com/google/go-cmp/cmp" + "github.com/aws/smithy-go/document" + "github.com/aws/smithy-go/middleware" ) -// CompareValues compares two values to determine if they are equal. -func CompareValues(expect, actual interface{}, opts ...cmp.Option) error { - opts = append(make([]cmp.Option, 0, len(opts)+1), opts...) - - var skippedReaders filterSkipDifferentIoReader - - opts = append(opts, - cmp.Transformer("http.NoBody", transformHTTPNoBodyToNil), - cmp.FilterValues(skippedReaders.filter, cmp.Ignore()), - cmp.Comparer(compareDocumentTypes), - ) +// CompareValues compares two values to determine if they are equal, +// specialized for comparison of SDK operation output types. +// +// CompareValues expects the two values to be of the same underlying type. +// Doing otherwise will result in undefined behavior. +// +// The third variadic argument is vestigial from a previous implementation that +// depended on go-cmp. Values passed therein have no effect. +func CompareValues(expect, actual interface{}, _ ...interface{}) error { + return deepEqual(reflect.ValueOf(expect), reflect.ValueOf(actual), "") +} - if diff := cmp.Diff(expect, actual, opts...); len(diff) != 0 { - return fmt.Errorf("values do not match\n%s", diff) +func deepEqual(expect, actual reflect.Value, path string) error { + if et, at := expect.Kind(), actual.Kind(); et != at { + return fmt.Errorf("%s: kind %s != %s", path, et, at) } - var errs []error - for _, s := range skippedReaders { - if err := CompareReaders(s.A, s.B); err != nil { - errs = append(errs, err) + // there are a handful of short-circuit cases here within the context of + // operation responses: + // - ResultMetadata (we don't care) + // - document.Interface (check for marshaled []byte equality) + // - io.Reader (check for Read() []byte equality) + ei, ai := expect.Interface(), actual.Interface() + if _, _, ok := asMetadatas(ei, ai); ok { + return nil + } + if e, a, ok := asDocuments(ei, ai); ok { + if !compareDocumentTypes(e, a) { + return fmt.Errorf("%s: document values unequal", path) } + return nil } - if len(errs) != 0 { - return fmt.Errorf("io.Readers have different values\n%v", errs) + if e, a, ok := asReaders(ei, ai); ok { + if err := CompareReaders(e, a); err != nil { + return fmt.Errorf("%s: %w", path, err) + } + return nil } - return nil + switch expect.Kind() { + case reflect.Pointer: + if expect.Type() != actual.Type() { + return fmt.Errorf("%s: type mismatch", path) + } + + expect = deref(expect) + actual = deref(actual) + ek, ak := expect.Kind(), actual.Kind() + if ek == reflect.Invalid || ak == reflect.Invalid { + // one was a nil pointer, so they both must be nil + if ek == ak { + return nil + } + return fmt.Errorf("%s: %s != %s", path, fmtNil(ek), fmtNil(ak)) + } + if err := deepEqual(expect, actual, path); err != nil { + return err + } + return nil + case reflect.Slice: + if expect.Len() != actual.Len() { + return fmt.Errorf("%s: slice length unequal", path) + } + for i := 0; i < expect.Len(); i++ { + ipath := fmt.Sprintf("%s[%d]", path, i) + if err := deepEqual(expect.Index(i), actual.Index(i), ipath); err != nil { + return err + } + } + return nil + case reflect.Map: + if expect.Len() != actual.Len() { + return fmt.Errorf("%s: map length unequal", path) + } + for _, k := range expect.MapKeys() { + kpath := fmt.Sprintf("%s[%q]", path, k.String()) + if err := deepEqual(expect.MapIndex(k), actual.MapIndex(k), kpath); err != nil { + return err + } + } + return nil + case reflect.Struct: + for i := 0; i < expect.NumField(); i++ { + if !expect.Field(i).CanInterface() { + continue // unexported + } + fpath := fmt.Sprintf("%s.%s", path, expect.Type().Field(i).Name) + if err := deepEqual(expect.Field(i), actual.Field(i), fpath); err != nil { + return err + } + } + return nil + case reflect.Float32, reflect.Float64: + // NaN != NaN by definition but we just care about bitwise equality + ef, af := math.Float64bits(expect.Float()), math.Float64bits(actual.Float()) + if ef != af { + return fmt.Errorf("%s: float 0x%x != 0x%x", path, ef, af) + } + return nil + default: + // everything else is just scalars and can be delegated + if !reflect.DeepEqual(ei, ai) { + return fmt.Errorf("%s: %v != %v", path, ei, ai) + } + return nil + } +} + +func asMetadatas(i, j interface{}) (ii, jj middleware.Metadata, ok bool) { + ii, iok := i.(middleware.Metadata) + jj, jok := j.(middleware.Metadata) + return ii, jj, iok || jok +} + +func asDocuments(i, j interface{}) (ii, jj documentInterface, ok bool) { + ii, iok := i.(documentInterface) + jj, jok := j.(documentInterface) + return ii, jj, iok || jok +} + +func asReaders(i, j interface{}) (ii, jj io.Reader, ok bool) { + ii, iok := i.(io.Reader) + jj, jok := j.(io.Reader) + return ii, jj, iok || jok +} + +func deref(v reflect.Value) reflect.Value { + switch v.Kind() { + case reflect.Interface, reflect.Ptr: + for v.Kind() == reflect.Interface || v.Kind() == reflect.Ptr { + v = v.Elem() + } + } + return v } type documentInterface interface { @@ -47,6 +154,13 @@ type documentInterface interface { } func compareDocumentTypes(x documentInterface, y documentInterface) bool { + if x == nil { + x = nopMarshaler{} + } + if y == nil { + y = nopMarshaler{} + } + xBytes, err := x.MarshalSmithyDocument() if err != nil { panic(fmt.Sprintf("MarshalSmithyDocument error: %v", err)) @@ -58,49 +172,22 @@ func compareDocumentTypes(x documentInterface, y documentInterface) bool { return JSONEqual(xBytes, yBytes) == nil } -func transformHTTPNoBodyToNil(v io.Reader) io.Reader { - if v == http.NoBody { - return nil - } - return v -} - -type filterSkipDifferentIoReader []skippedReaders - -func (f *filterSkipDifferentIoReader) filter(a, b io.Reader) bool { - if a == nil || b == nil { - return false - } - //at, bt := reflect.TypeOf(a), reflect.TypeOf(b) - //for at.Kind() == reflect.Ptr { - // at = at.Elem() - //} - //for bt.Kind() == reflect.Ptr { - // bt = bt.Elem() - //} - - //// The underlying reader types are the same they can be compared directly. - //if at == bt { - // return false - //} - - *f = append(*f, skippedReaders{A: a, B: b}) - return true -} - -type skippedReaders struct { - A, B io.Reader -} - // CompareReaders two io.Reader values together to determine if they are equal. // Will read the contents of the readers until they are empty. func CompareReaders(expect, actual io.Reader) error { - e, err := ioutil.ReadAll(expect) + if expect == nil { + expect = nopReader{} + } + if actual == nil { + actual = nopReader{} + } + + e, err := io.ReadAll(expect) if err != nil { return fmt.Errorf("failed to read expect body, %w", err) } - a, err := ioutil.ReadAll(actual) + a, err := io.ReadAll(actual) if err != nil { return fmt.Errorf("failed to read actual body, %w", err) } @@ -112,3 +199,19 @@ func CompareReaders(expect, actual io.Reader) error { return nil } + +func fmtNil(k reflect.Kind) string { + if k == reflect.Invalid { + return "nil" + } + return "non-nil" +} + +type nopReader struct{} + +func (nopReader) Read(p []byte) (int, error) { return 0, io.EOF } + +type nopMarshaler struct{} + +func (nopMarshaler) MarshalSmithyDocument() ([]byte, error) { return nil, nil } +func (nopMarshaler) UnmarshalSmithyDocument(v interface{}) error { return nil } diff --git a/testing/struct_test.go b/testing/struct_test.go index be92c76de..2ec400365 100644 --- a/testing/struct_test.go +++ b/testing/struct_test.go @@ -4,29 +4,54 @@ import ( "bytes" "io" "io/ioutil" + "math" "strings" "testing" + + "github.com/aws/smithy-go/middleware" + "github.com/aws/smithy-go/ptr" ) -func TestCompareStructEqual(t *testing.T) { +func TestCompareValues(t *testing.T) { + const float64NaN = 0x7fffffff_ffffffff // mantissa flipped all the way on + cases := map[string]struct { A, B interface{} ExpectErr string }{ - "simple match": { - A: struct { + "totally different types": { + A: 1, + B: struct { Foo string Bar int }{ Foo: "abc", Bar: 123, }, - B: struct { - Foo string - Bar int + ExpectErr: ": kind int != struct", + }, + "simple match": { + A: struct { + Foo string + Bar int + Metadata middleware.Metadata }{ Foo: "abc", Bar: 123, + Metadata: func() middleware.Metadata { + var md middleware.Metadata + md.Set(1, 1) + return md + }(), + }, + B: struct { + Foo string + Bar int + Metadata middleware.Metadata + }{ + Foo: "abc", + Bar: 123, + Metadata: middleware.Metadata{}, // different, shouldn't matter }, }, "simple diff": { @@ -44,7 +69,7 @@ func TestCompareStructEqual(t *testing.T) { Foo: "abc", Bar: 456, }, - ExpectErr: "values do not match", + ExpectErr: ".Bar: 123 != 456", }, "reader match": { A: struct { @@ -77,7 +102,161 @@ func TestCompareStructEqual(t *testing.T) { Foo: ioutil.NopCloser(strings.NewReader("123abc")), Bar: 123, }, - ExpectErr: "bytes do not match", + ExpectErr: ".Foo: bytes do not match", + }, + "float match": { + A: struct { + Foo float64 + Bar int + }{ + Foo: math.Float64frombits(float64NaN), + Bar: 123, + }, + B: struct { + Foo float64 + Bar int + }{ + Foo: math.Float64frombits(float64NaN), + Bar: 123, + }, + }, + "float diff": { + A: struct { + Foo float64 + Bar int + }{ + Foo: math.Float64frombits(float64NaN), + Bar: 123, + }, + B: struct { + Foo float64 + Bar int + }{ + Foo: math.Float64frombits(float64NaN - 1), + Bar: 123, + }, + ExpectErr: ".Foo: float 0x7fffffffffffffff != 0x7ffffffffffffffe", + }, + "document equal": { + A: &mockDocumentMarshaler{[]byte("123"), nil}, + B: &mockDocumentMarshaler{[]byte("123"), nil}, + }, + "document unequal": { + A: &mockDocumentMarshaler{[]byte("123"), nil}, + B: &mockDocumentMarshaler{[]byte("124"), nil}, + ExpectErr: ": document values unequal", + }, + "slice equal": { + A: []struct { + Bar int + }{{0}, {1}}, + B: []struct { + Bar int + }{{0}, {1}}, + }, + "slice length unequal": { + A: []struct { + Bar int + }{{0}}, + B: []struct { + Bar int + }{{0}, {1}}, + ExpectErr: "slice length unequal", + }, + "slice value unequal": { + A: []struct { + Bar int + }{{2}, {1}, {0}}, + B: []struct { + Bar int + }{{2}, {0}, {1}}, + ExpectErr: "[1].Bar: 1 != 0", + }, + "map equal": { + A: map[string]struct { + Bar int + }{ + "foo": {0}, + "bar": {1}, + }, + B: map[string]struct { + Bar int + }{ + "bar": {1}, + "foo": {0}, + }, + }, + "map length unequal": { + A: map[string]struct { + Bar int + }{ + "foo": {0}, + "bar": {1}, + }, + B: map[string]struct { + Bar int + }{ + "foo": {0}, + }, + ExpectErr: "map length unequal", + }, + "map value unequal": { + A: map[string]struct { + IntField int + }{ + "foo": {0}, + "bar": {1}, + }, + B: map[string]struct { + IntField int + }{ + "bar": {1}, + "foo": {1}, + }, + ExpectErr: `["foo"].IntField: 0 != 1`, + }, + "handles deref, nil equal": { + A: struct { + Int *int + }{nil}, + B: struct { + Int *int + }{nil}, + }, + "handles deref, value equal": { + A: struct { + Int *int + }{ptr.Int(12)}, + B: struct { + Int *int + }{ptr.Int(12)}, + }, + "handles deref, different types are unequal": { + A: struct { + Int *int + }{nil}, + B: struct { + Int *string + }{nil}, + ExpectErr: ".Int: type mismatch", + }, + "handles deref, unequal": { + A: struct { + Int *int + }{ptr.Int(12)}, + B: struct { + Int *int + }{nil}, + ExpectErr: ".Int: non-nil != nil", + }, + "handles deref, unequal switched": { + A: struct { + Int *int + }{nil}, + B: struct { + Int *int + }{ptr.Int(12)}, + ExpectErr: ".Int: nil != non-nil", }, } @@ -100,3 +279,48 @@ func TestCompareStructEqual(t *testing.T) { }) } } + +func TestCompareValues_Document(t *testing.T) { + cases := map[string]struct { + A, B interface{} + ExpectErr string + }{ + "equal": { + A: &mockDocumentMarshaler{[]byte("123"), nil}, + B: &mockDocumentMarshaler{[]byte("123"), nil}, + }, + "unequal": { + A: &mockDocumentMarshaler{[]byte("123"), nil}, + B: &mockDocumentMarshaler{[]byte("124"), nil}, + ExpectErr: ": document values unequal", + }, + } + for name, c := range cases { + t.Run(name, func(t *testing.T) { + err := CompareValues(c.A, c.B) + + if len(c.ExpectErr) != 0 { + if err == nil { + t.Errorf("expect error, got none") + } + if e, a := c.ExpectErr, err.Error(); !strings.Contains(a, e) { + t.Errorf("expect error to contain %v, got %v", e, a) + } + return + } + if err != nil { + t.Errorf("expect no error, got %v", err) + } + }) + } +} + +type mockDocumentMarshaler struct { + p []byte + err error +} + +var _ documentInterface = (*mockDocumentMarshaler)(nil) + +func (m *mockDocumentMarshaler) MarshalSmithyDocument() ([]byte, error) { return m.p, m.err } +func (m *mockDocumentMarshaler) UnmarshalSmithyDocument(v interface{}) error { return nil }