-
Notifications
You must be signed in to change notification settings - Fork 43
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Dedup go errors #1496
Dedup go errors #1496
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #1496 +/- ##
==========================================
- Coverage 62.02% 57.96% -4.06%
==========================================
Files 185 286 +101
Lines 32895 39695 +6800
==========================================
+ Hits 20402 23009 +2607
- Misses 11353 15344 +3991
- Partials 1140 1342 +202 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code looks good. There is code on L154 that does almost the same thing. I would probably change the comment on L154 to say "failures" instead of "errors" to help disambiguate.
Can you add a test for this?
testing/x/replay.go
Outdated
@@ -181,8 +181,15 @@ func replay[Req protoreflect.ProtoMessage, Resp protoreflect.ProtoMessage]( | |||
assert.NoError(t, err) | |||
|
|||
resp, err := serve(ctx, req) | |||
if err != nil && entry.Errors != nil { | |||
errList := make([]string, 0) | |||
unmarshalErr := json.Unmarshal(entry.Errors, &errList) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
entry.Errors
is a JSON list that is user provided, but we still assert that it should have a length of 1. What happens if we want to test that we have multiple errors?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't look like the provider can return multiple errors as far as I can see. It seems to combine all the errors into one. The CheckConfig
method can only return one error value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we can't have multiple errors, when why do we unmarshal entry.Errors
into a list. I think we can simplify this whole block into
if err != nil && entry.Error != nil {
assert.Equal(t, entry.Error, err.Error())
return
}
This would involve changing the field Errors json.RawMessage
to Error *string
. We would then represent the error from fmt.Errorf("my-error")
as &"my-error"
. No error would be represented as nil
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still don't understand the complexity for one of the tests, but otherwise this looks great! LGTM
testing/x/replay.go
Outdated
@@ -181,8 +181,15 @@ func replay[Req protoreflect.ProtoMessage, Resp protoreflect.ProtoMessage]( | |||
assert.NoError(t, err) | |||
|
|||
resp, err := serve(ctx, req) | |||
if err != nil && entry.Errors != nil { | |||
errList := make([]string, 0) | |||
unmarshalErr := json.Unmarshal(entry.Errors, &errList) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we can't have multiple errors, when why do we unmarshal entry.Errors
into a list. I think we can simplify this whole block into
if err != nil && entry.Error != nil {
assert.Equal(t, entry.Error, err.Error())
return
}
This would involve changing the field Errors json.RawMessage
to Error *string
. We would then represent the error from fmt.Errorf("my-error")
as &"my-error"
. No error would be represented as nil
.
The muxer previously de-duplicated
CheckFailure
s but not go errors. This PR should add that and address #1418 but not the original issue in pulumi/pulumi-aws#2285 (comment). See #1418 (comment)This fixes duplicated error messages in the aws-provider when no aws region is specified.
Before:
After: