metadata: fix validation issues #6001
Conversation
|
|
|
Thanks for your review! done. |
|
@zasweq can you take a second pass please? |
internal/metadata/metadata.go
Outdated
| @@ -118,3 +101,31 @@ func hasNotPrintable(msg string) bool { | |||
| } | |||
| return false | |||
| } | |||
|
|
|||
| // ValidatePair validate single pair in metadata | |||
There was a problem hiding this comment.
Nit: I know these comments are similar to ones on master here and throughout the PR, but can you please reword to have proper grammar (capitalization and periods). Also, some of these comments are not explaining why/reasoning a block of code is happening, simply stating exactly the operations taking place within the code block. Please reword to proper grammar/explaining why/delete those which you find appropriate.
| if _, err := stream.Recv(); status.Code(err) != status.Code(test.recv) || !strings.Contains(err.Error(), test.recv.Error()) { | ||
| t.Errorf("stream.Recv() = _, get err :%v, want err :%v", err, test.recv) | ||
| } | ||
| t.Run("streaming "+test.name, func(t *testing.T) { |
There was a problem hiding this comment.
Optional nit (not important for linter or correctness): I prefer spaces in between operators i.e. "streaming " + test.name). Here and elsewhere
| t.Errorf("call ss.Client stream Send(nil) will success but got err :%v", err) | ||
| } | ||
| if _, err := stream.Recv(); status.Code(err) != status.Code(test.recv) || !strings.Contains(err.Error(), test.recv.Error()) { | ||
| t.Errorf("stream.Recv() = _, get err :%v, want err :%v", err, test.recv) |
| defer cancel() | ||
| stream, err := ss.Client.FullDuplexCall(ctx) | ||
| if err != nil { | ||
| t.Errorf("call ss.Client.FullDuplexCall(context.Background()) will success but got err :%v", err) |
There was a problem hiding this comment.
I know you didn't write this, but this grammar is strange, and is incorrect (it's not being called with context.Background(), it's being called with context with a timeout). Please reword (see other examples in codebase). Perhaps something like "ss.Client.FullDuplexCall(ctx) want err: %v, got err : %v", nil, err). Here and elsewhere.
| return | ||
| } | ||
| if err := stream.Send(&testpb.StreamingOutputCallRequest{}); err != nil { | ||
| t.Errorf("call ss.Client stream Send(nil) will success but got err :%v", err) |
There was a problem hiding this comment.
Same here (see comment above). This isn't sending nil, but an allocated testpb.StreamingOutputCallRequest.
| md := metadata.Join(test.md, metadata.Pairs(test.appendMD...)) | ||
|
|
||
| if err := stream.SetHeader(md); !reflect.DeepEqual(test.want, err) { | ||
| return fmt.Errorf("call stream.SendHeader(md) validate metadata which is %v got err :%v, want err :%v", md, err, test.want) |
fix: #5913
RELEASE NOTES: