From d72d4516e9176d7d23abf8f678b94b58ab9e1b9d Mon Sep 17 00:00:00 2001 From: Venelin Date: Mon, 30 Oct 2023 10:18:18 +0000 Subject: [PATCH 1/7] dedup go errors --- .gitignore | 2 ++ x/muxer/muxer.go | 15 +++++++++++---- 2 files changed, 13 insertions(+), 4 deletions(-) diff --git a/.gitignore b/.gitignore index ee34b1bdb..1d1c206b2 100644 --- a/.gitignore +++ b/.gitignore @@ -7,3 +7,5 @@ coverage.txt pkg/tests/coverage.txt testing/coverage.txt +go.work +go.work.sum \ No newline at end of file diff --git a/x/muxer/muxer.go b/x/muxer/muxer.go index 31a93f3ba..2111df7e7 100644 --- a/x/muxer/muxer.go +++ b/x/muxer/muxer.go @@ -79,8 +79,10 @@ type GetMappingResponse struct { Data []byte } -type getMappingHandler = map[string]MultiMappingHandler -type MultiMappingHandler = func(GetMappingArgs) (GetMappingResponse, error) +type ( + getMappingHandler = map[string]MultiMappingHandler + MultiMappingHandler = func(GetMappingArgs) (GetMappingResponse, error) +) func (m *muxer) getFunction(token string) server { i, ok := m.dispatchTable.Functions[token] @@ -120,8 +122,15 @@ func (m *muxer) CheckConfig(ctx context.Context, req *rpc.CheckRequest) (*rpc.Ch failures := []*rpc.CheckFailure{} uniqueFailures := map[string]struct{}{} var errs multierror.Error + uniqueErrors := map[string]struct{}{} for i, r := range asyncJoin(subs) { if err := r.B; err != nil { + errString := err.Error() + // De-duplicate errors + if _, has := uniqueErrors[errString]; has { + continue + } + uniqueErrors[errString] = struct{}{} errs.Errors = append(errs.Errors, err) continue } @@ -384,7 +393,6 @@ func (m *muxer) Cancel(ctx context.Context, e *emptypb.Empty) (*emptypb.Empty, e } } return e, m.muxedErrors(errs) - } func (m *muxer) GetPluginInfo(ctx context.Context, e *emptypb.Empty) (*rpc.PluginInfo, error) { @@ -424,7 +432,6 @@ func (a *getMappingArgs) Fetch() []GetMappingResponse { } func (m *muxer) GetMapping(ctx context.Context, req *rpc.GetMappingRequest) (*rpc.GetMappingResponse, error) { - // We need to merge multiple mappings combineMapping, found := m.getMappingByKey[req.Key] if !found { From 88814db6096bee5d804a4c1dc8aa7b22cc09f31b Mon Sep 17 00:00:00 2001 From: Venelin Date: Wed, 15 Nov 2023 11:30:49 +0000 Subject: [PATCH 2/7] tests --- testing/x/replay.go | 11 ++++++-- x/muxer/muxer.go | 2 +- x/muxer/tests/muxer_test.go | 53 +++++++++++++++++++++++++++---------- 3 files changed, 49 insertions(+), 17 deletions(-) diff --git a/testing/x/replay.go b/testing/x/replay.go index 54188e3fd..e73f08ab1 100644 --- a/testing/x/replay.go +++ b/testing/x/replay.go @@ -181,8 +181,14 @@ func replay[Req protoreflect.ProtoMessage, Resp protoreflect.ProtoMessage]( assert.NoError(t, err) resp, err := serve(ctx, req) - require.NoError(t, err) - + if err != nil && entry.Errors != nil { + errList := make([]string, 0) + unmarshalErr := json.Unmarshal(entry.Errors, &errList) + assert.NoError(t, unmarshalErr) + assert.Len(t, errList, 1) // we only expect one error + assert.Equal(t, errList[0], err.Error()) + return + } bytes, err := jsonpb.Marshal(resp) assert.NoError(t, err) @@ -236,4 +242,5 @@ type jsonLogEntry struct { Method string `json:"method"` Request json.RawMessage `json:"request,omitempty"` Response json.RawMessage `json:"response,omitempty"` + Errors json.RawMessage `json:"errors,omitempty"` } diff --git a/x/muxer/muxer.go b/x/muxer/muxer.go index 2111df7e7..6503a59f4 100644 --- a/x/muxer/muxer.go +++ b/x/muxer/muxer.go @@ -150,7 +150,7 @@ func (m *muxer) CheckConfig(ctx context.Context, req *rpc.CheckRequest) (*rpc.Ch } } - // Here we de-duplicate errors. + // Here we de-duplicate rpc failures. for _, e := range r.A.GetFailures() { s := e.GetProperty() + ":" + e.GetReason() if _, has := uniqueFailures[s]; has { diff --git a/x/muxer/tests/muxer_test.go b/x/muxer/tests/muxer_test.go index fa0c196e7..99e1b28a3 100644 --- a/x/muxer/tests/muxer_test.go +++ b/x/muxer/tests/muxer_test.go @@ -17,6 +17,7 @@ package muxer_test import ( "context" "encoding/json" + "errors" "fmt" "reflect" "testing" @@ -68,6 +69,20 @@ func TestSimpleDispatch(t *testing.T) { ) } +func TestCheckConfigErrorNotDuplicated(t *testing.T) { + var m muxer.DispatchTable + m.Resources = map[string]int{ + "test:mod:A": 0, + "test:mod:B": 1, + } + req := `{}` + mux(t, m).replay( + exchange("/pulumirpc.ResourceProvider/CheckConfig", req, "{}", `["[\"myerr\"]"]`, + part(0, req, "{}", `["myerr"]`), + part(1, req, "{}", `["myerr"]`), + )) +} + func TestConfigure(t *testing.T) { var m muxer.DispatchTable m.Resources = map[string]int{ @@ -84,7 +99,7 @@ func TestConfigure(t *testing.T) { } }`, `{ "supportsPreview": true - }`, + }`, "", part(0, `{ "args": { "a": "1", @@ -94,7 +109,7 @@ func TestConfigure(t *testing.T) { }`, `{ "acceptSecrets": true, "supportsPreview": true -}`), +}`, ""), part(1, `{ "args": { "a": "1", @@ -104,7 +119,7 @@ func TestConfigure(t *testing.T) { }`, `{ "supportsPreview": true, "acceptResources": true -}`), +}`, ""), )) } @@ -139,9 +154,9 @@ func TestDivergentCheckConfig(t *testing.T) { } }` muxedResp := resp0 - e := exchange("/pulumirpc.ResourceProvider/CheckConfig", req, muxedResp, - part(0, req, resp0), - part(1, req, resp1)) + e := exchange("/pulumirpc.ResourceProvider/CheckConfig", req, muxedResp, "", + part(0, req, resp0, ""), + part(1, req, resp1, "")) m := muxer.DispatchTable{} m.Resources = map[string]int{} @@ -162,17 +177,17 @@ func TestGetMapping(t *testing.T) { }`, `{ "provider": "p1", "data": "dw=="`+ /* the base64 encoding of d1 */ ` -}`, part(0, `{ +}`, "", part(0, `{ "key": "k1" }`, `{ "provider": "p1", "data": "d1" -}`), part(1, `{ +}`, ""), part(1, `{ "key": "k1" }`, `{ "provider": "", "data": "" -}`))) +}`, ""))) }) t.Run("merged-responding-server", func(t *testing.T) { var m muxer.DispatchTable @@ -199,17 +214,17 @@ func TestGetMapping(t *testing.T) { }`, `{ "provider": "p1", "data": "cjE="`+ /* the base64 encoding of r1 */ ` -}`, part(0, `{ +}`, "", part(0, `{ "key": "k" }`, `{ "provider": "p1", "data": "ZDE="`+ /* the base64 encoding of d1*/ ` -}`), part(1, `{ +}`, ""), part(1, `{ "key": "k" }`, `{ "provider": "p1", "data": "ZDI="`+ /* the base64 encoding of d2*/ ` -}`))) +}`, ""))) }) } @@ -242,6 +257,7 @@ func (m testMuxer) replay(exchanges ...Exchange) { call{ incoming: part.Request, response: part.Response, + errors: part.Errors, }) } } @@ -260,6 +276,7 @@ type Exchange struct { Method string `json:"method"` Request json.RawMessage `json:"request"` Response json.RawMessage `json:"response"` + Errors json.RawMessage `json:"errors,omitempty"` Parts []ExchangePart `json:"-"` } @@ -267,6 +284,7 @@ type ExchangePart struct { Provider int Request string `json:"request"` Response string `json:"response"` + Errors string `json:"errors,omitempty"` } // A simple exchange is one where only one sub-server is used @@ -285,20 +303,22 @@ func simpleExchange(provider int, method, request, response string) Exchange { } } -func exchange(method, request, response string, parts ...ExchangePart) Exchange { +func exchange(method, request, response, errors string, parts ...ExchangePart) Exchange { return Exchange{ Method: method, Request: json.RawMessage(request), Response: json.RawMessage(response), + Errors: json.RawMessage(errors), Parts: parts, } } -func part(provider int, request, response string) ExchangePart { +func part(provider int, request, response, errors string) ExchangePart { return ExchangePart{ provider, request, response, + errors, } } @@ -340,6 +360,7 @@ type server struct { type call struct { incoming string response string + errors string } // Assert that a gRPC call matches the next expected call, then rehydrate and return the @@ -353,6 +374,10 @@ func handleMethod[T proto.Message, R proto.Message](m *server, req T) (R, error) var r R reflect.ValueOf(&r).Elem().Set(reflect.New(reflect.TypeOf(r).Elem())) + if next.errors != "" { + return r, errors.New(next.errors) + } + marshalled, err := protojson.MarshalOptions{Multiline: true}.Marshal(req) require.NoError(m.t, err) From d04d1908a411777dfbf662bcac0ba25bdf7d6b99 Mon Sep 17 00:00:00 2001 From: Venelin Date: Wed, 15 Nov 2023 11:54:06 +0000 Subject: [PATCH 3/7] use local testutils --- x/muxer/tests/go.mod | 2 ++ x/muxer/tests/go.sum | 2 -- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/x/muxer/tests/go.mod b/x/muxer/tests/go.mod index 632d2dded..1e2260235 100644 --- a/x/muxer/tests/go.mod +++ b/x/muxer/tests/go.mod @@ -4,6 +4,8 @@ go 1.20 replace github.com/pulumi/pulumi-terraform-bridge/x/muxer => ../ +replace github.com/pulumi/pulumi-terraform-bridge/testing => ../../../testing/ + require ( github.com/pulumi/pulumi-terraform-bridge/testing v0.0.0-20230406212415-0b560771908d github.com/pulumi/pulumi-terraform-bridge/x/muxer v0.0.4 diff --git a/x/muxer/tests/go.sum b/x/muxer/tests/go.sum index 8e48a1938..98e03fca5 100644 --- a/x/muxer/tests/go.sum +++ b/x/muxer/tests/go.sum @@ -148,8 +148,6 @@ github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZb github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4= github.com/pulumi/esc v0.5.6 h1:4WV3X7OEVcChIwbSG+JxhZDdmq/q7lFPaSjHRYlPwmI= github.com/pulumi/esc v0.5.6/go.mod h1:wpwNfVS5fV7Kd51j4dJ6FWYlKfxdqyppgp0gtkzqH04= -github.com/pulumi/pulumi-terraform-bridge/testing v0.0.0-20230406212415-0b560771908d h1:OjHMocIBmWuY13luNE7okcyRu1bxNdXP6kPSW0p6x84= -github.com/pulumi/pulumi-terraform-bridge/testing v0.0.0-20230406212415-0b560771908d/go.mod h1:slt4v0erbPg9FCxmbQFc6SA6toGtqfdSsQPihRMktBU= github.com/pulumi/pulumi/pkg/v3 v3.91.1 h1:xHnyEwJO9we2zCiM9gHTkJxjZ6a6yi5vYCwWHCYRj9Y= github.com/pulumi/pulumi/pkg/v3 v3.91.1/go.mod h1:dzBQDJyCOEhtBVN5INA5/i9yG9DZlsStl/mAkrhs9II= github.com/pulumi/pulumi/sdk/v3 v3.91.1 h1:6I9GMmHv23X+G6hoduU1XE6hBWSNtB+zcb1MX17YvlA= From 8e63541214ba75ca6c52e7cd63dfe32012b44432 Mon Sep 17 00:00:00 2001 From: Venelin Date: Wed, 15 Nov 2023 11:59:05 +0000 Subject: [PATCH 4/7] missed err check --- testing/x/replay.go | 1 + 1 file changed, 1 insertion(+) diff --git a/testing/x/replay.go b/testing/x/replay.go index e73f08ab1..f6a01d3c1 100644 --- a/testing/x/replay.go +++ b/testing/x/replay.go @@ -189,6 +189,7 @@ func replay[Req protoreflect.ProtoMessage, Resp protoreflect.ProtoMessage]( assert.Equal(t, errList[0], err.Error()) return } + require.NoError(t, err) bytes, err := jsonpb.Marshal(resp) assert.NoError(t, err) From fd25ed0bf13a3dfb259b15fe02d4e8197aadab2a Mon Sep 17 00:00:00 2001 From: Venelin Date: Thu, 16 Nov 2023 14:25:37 +0000 Subject: [PATCH 5/7] two more error tests --- x/muxer/tests/muxer_test.go | 33 +++++++++++++++++++++++++++++---- 1 file changed, 29 insertions(+), 4 deletions(-) diff --git a/x/muxer/tests/muxer_test.go b/x/muxer/tests/muxer_test.go index 99e1b28a3..fce1644f1 100644 --- a/x/muxer/tests/muxer_test.go +++ b/x/muxer/tests/muxer_test.go @@ -75,11 +75,36 @@ func TestCheckConfigErrorNotDuplicated(t *testing.T) { "test:mod:A": 0, "test:mod:B": 1, } - req := `{}` mux(t, m).replay( - exchange("/pulumirpc.ResourceProvider/CheckConfig", req, "{}", `["[\"myerr\"]"]`, - part(0, req, "{}", `["myerr"]`), - part(1, req, "{}", `["myerr"]`), + exchange("/pulumirpc.ResourceProvider/CheckConfig", "{}", "{}", `["[\"myerr\"]"]`, + part(0, "{}", "{}", `["myerr"]`), + part(1, "{}", "{}", `["myerr"]`), + )) +} + +func TestCheckConfigDifferentErrorsNotDropped(t *testing.T) { + var m muxer.DispatchTable + m.Resources = map[string]int{ + "test:mod:A": 0, + "test:mod:B": 1, + } + mux(t, m).replay( + exchange("/pulumirpc.ResourceProvider/CheckConfig", "{}", "{}", `["2 errors occurred:\n\t* [\"myerr\"]\n\t* [\"othererr\"]\n\n"]`, + part(0, "{}", "{}", `["myerr"]`), + part(1, "{}", "{}", `["othererr"]`), + )) +} + +func TestCheckConfigOneErrorReturned(t *testing.T) { + var m muxer.DispatchTable + m.Resources = map[string]int{ + "test:mod:A": 0, + "test:mod:B": 1, + } + mux(t, m).replay( + exchange("/pulumirpc.ResourceProvider/CheckConfig", "{}", "{}", `["[\"myerr\"]"]`, + part(0, "{}", `{"inputs": {"myurn":"urn"}}`, ""), + part(1, "{}", "{}", `["myerr"]`), )) } From e17ea50d95a8d591cbf5f66b85869bcdf914f811 Mon Sep 17 00:00:00 2001 From: Venelin Date: Thu, 16 Nov 2023 16:51:38 +0000 Subject: [PATCH 6/7] lint --- x/muxer/tests/muxer_test.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/x/muxer/tests/muxer_test.go b/x/muxer/tests/muxer_test.go index fce1644f1..ad55fc435 100644 --- a/x/muxer/tests/muxer_test.go +++ b/x/muxer/tests/muxer_test.go @@ -89,7 +89,11 @@ func TestCheckConfigDifferentErrorsNotDropped(t *testing.T) { "test:mod:B": 1, } mux(t, m).replay( - exchange("/pulumirpc.ResourceProvider/CheckConfig", "{}", "{}", `["2 errors occurred:\n\t* [\"myerr\"]\n\t* [\"othererr\"]\n\n"]`, + exchange( + "/pulumirpc.ResourceProvider/CheckConfig", + "{}", + "{}", + `["2 errors occurred:\n\t* [\"myerr\"]\n\t* [\"othererr\"]\n\n"]`, part(0, "{}", "{}", `["myerr"]`), part(1, "{}", "{}", `["othererr"]`), )) From ff6ec62f854544205cd6a30e3f7a623423f2831a Mon Sep 17 00:00:00 2001 From: Venelin Date: Thu, 16 Nov 2023 18:50:40 +0000 Subject: [PATCH 7/7] clean up error passing in tests --- testing/x/replay.go | 8 ++--- x/muxer/tests/muxer_test.go | 65 ++++++++++++++++++++----------------- 2 files changed, 37 insertions(+), 36 deletions(-) diff --git a/testing/x/replay.go b/testing/x/replay.go index f6a01d3c1..ca029d45f 100644 --- a/testing/x/replay.go +++ b/testing/x/replay.go @@ -182,11 +182,7 @@ func replay[Req protoreflect.ProtoMessage, Resp protoreflect.ProtoMessage]( resp, err := serve(ctx, req) if err != nil && entry.Errors != nil { - errList := make([]string, 0) - unmarshalErr := json.Unmarshal(entry.Errors, &errList) - assert.NoError(t, unmarshalErr) - assert.Len(t, errList, 1) // we only expect one error - assert.Equal(t, errList[0], err.Error()) + assert.Equal(t, *entry.Errors, err.Error()) return } require.NoError(t, err) @@ -243,5 +239,5 @@ type jsonLogEntry struct { Method string `json:"method"` Request json.RawMessage `json:"request,omitempty"` Response json.RawMessage `json:"response,omitempty"` - Errors json.RawMessage `json:"errors,omitempty"` + Errors *string `json:"errors,omitempty"` } diff --git a/x/muxer/tests/muxer_test.go b/x/muxer/tests/muxer_test.go index ad55fc435..b8d4f8690 100644 --- a/x/muxer/tests/muxer_test.go +++ b/x/muxer/tests/muxer_test.go @@ -75,10 +75,11 @@ func TestCheckConfigErrorNotDuplicated(t *testing.T) { "test:mod:A": 0, "test:mod:B": 1, } + errString := "myerr" mux(t, m).replay( - exchange("/pulumirpc.ResourceProvider/CheckConfig", "{}", "{}", `["[\"myerr\"]"]`, - part(0, "{}", "{}", `["myerr"]`), - part(1, "{}", "{}", `["myerr"]`), + exchange("/pulumirpc.ResourceProvider/CheckConfig", "{}", "{}", &errString, + part(0, "{}", "{}", &errString), + part(1, "{}", "{}", &errString), )) } @@ -88,14 +89,17 @@ func TestCheckConfigDifferentErrorsNotDropped(t *testing.T) { "test:mod:A": 0, "test:mod:B": 1, } + firstErr := "myerr" + secondErr := "othererr" + expectedErr := "2 errors occurred:\n\t* myerr\n\t* othererr\n\n" mux(t, m).replay( exchange( "/pulumirpc.ResourceProvider/CheckConfig", "{}", "{}", - `["2 errors occurred:\n\t* [\"myerr\"]\n\t* [\"othererr\"]\n\n"]`, - part(0, "{}", "{}", `["myerr"]`), - part(1, "{}", "{}", `["othererr"]`), + &expectedErr, + part(0, "{}", "{}", &firstErr), + part(1, "{}", "{}", &secondErr), )) } @@ -105,10 +109,11 @@ func TestCheckConfigOneErrorReturned(t *testing.T) { "test:mod:A": 0, "test:mod:B": 1, } + err := "myerr" mux(t, m).replay( - exchange("/pulumirpc.ResourceProvider/CheckConfig", "{}", "{}", `["[\"myerr\"]"]`, - part(0, "{}", `{"inputs": {"myurn":"urn"}}`, ""), - part(1, "{}", "{}", `["myerr"]`), + exchange("/pulumirpc.ResourceProvider/CheckConfig", "{}", "{}", &err, + part(0, "{}", `{"inputs": {"myurn":"urn"}}`, nil), + part(1, "{}", "{}", &err), )) } @@ -128,7 +133,7 @@ func TestConfigure(t *testing.T) { } }`, `{ "supportsPreview": true - }`, "", + }`, nil, part(0, `{ "args": { "a": "1", @@ -138,7 +143,7 @@ func TestConfigure(t *testing.T) { }`, `{ "acceptSecrets": true, "supportsPreview": true -}`, ""), +}`, nil), part(1, `{ "args": { "a": "1", @@ -148,7 +153,7 @@ func TestConfigure(t *testing.T) { }`, `{ "supportsPreview": true, "acceptResources": true -}`, ""), +}`, nil), )) } @@ -183,9 +188,9 @@ func TestDivergentCheckConfig(t *testing.T) { } }` muxedResp := resp0 - e := exchange("/pulumirpc.ResourceProvider/CheckConfig", req, muxedResp, "", - part(0, req, resp0, ""), - part(1, req, resp1, "")) + e := exchange("/pulumirpc.ResourceProvider/CheckConfig", req, muxedResp, nil, + part(0, req, resp0, nil), + part(1, req, resp1, nil)) m := muxer.DispatchTable{} m.Resources = map[string]int{} @@ -206,17 +211,17 @@ func TestGetMapping(t *testing.T) { }`, `{ "provider": "p1", "data": "dw=="`+ /* the base64 encoding of d1 */ ` -}`, "", part(0, `{ +}`, nil, part(0, `{ "key": "k1" }`, `{ "provider": "p1", "data": "d1" -}`, ""), part(1, `{ +}`, nil), part(1, `{ "key": "k1" }`, `{ "provider": "", "data": "" -}`, ""))) +}`, nil))) }) t.Run("merged-responding-server", func(t *testing.T) { var m muxer.DispatchTable @@ -243,17 +248,17 @@ func TestGetMapping(t *testing.T) { }`, `{ "provider": "p1", "data": "cjE="`+ /* the base64 encoding of r1 */ ` -}`, "", part(0, `{ +}`, nil, part(0, `{ "key": "k" }`, `{ "provider": "p1", "data": "ZDE="`+ /* the base64 encoding of d1*/ ` -}`, ""), part(1, `{ +}`, nil), part(1, `{ "key": "k" }`, `{ "provider": "p1", "data": "ZDI="`+ /* the base64 encoding of d2*/ ` -}`, ""))) +}`, nil))) }) } @@ -305,15 +310,15 @@ type Exchange struct { Method string `json:"method"` Request json.RawMessage `json:"request"` Response json.RawMessage `json:"response"` - Errors json.RawMessage `json:"errors,omitempty"` - Parts []ExchangePart `json:"-"` + Errors *string + Parts []ExchangePart `json:"-"` } type ExchangePart struct { Provider int Request string `json:"request"` Response string `json:"response"` - Errors string `json:"errors,omitempty"` + Errors *string } // A simple exchange is one where only one sub-server is used @@ -332,17 +337,17 @@ func simpleExchange(provider int, method, request, response string) Exchange { } } -func exchange(method, request, response, errors string, parts ...ExchangePart) Exchange { +func exchange(method, request, response string, errors *string, parts ...ExchangePart) Exchange { return Exchange{ Method: method, Request: json.RawMessage(request), Response: json.RawMessage(response), - Errors: json.RawMessage(errors), + Errors: errors, Parts: parts, } } -func part(provider int, request, response, errors string) ExchangePart { +func part(provider int, request, response string, errors *string) ExchangePart { return ExchangePart{ provider, request, @@ -389,7 +394,7 @@ type server struct { type call struct { incoming string response string - errors string + errors *string } // Assert that a gRPC call matches the next expected call, then rehydrate and return the @@ -403,8 +408,8 @@ func handleMethod[T proto.Message, R proto.Message](m *server, req T) (R, error) var r R reflect.ValueOf(&r).Elem().Set(reflect.New(reflect.TypeOf(r).Elem())) - if next.errors != "" { - return r, errors.New(next.errors) + if next.errors != nil { + return r, errors.New(*next.errors) } marshalled, err := protojson.MarshalOptions{Multiline: true}.Marshal(req)