pickfirst: add tests for resolver error scenarios #6484
Conversation
test/pickfirst_test.go
Outdated
| // RPCs may fail on the client side in two ways, once the fake server closes | ||
| // the accepted connection: | ||
| // - writing the client preface succeeds, but not reading the server preface | ||
| // - writing the client preface fails | ||
| const readErr = "error reading server preface" | ||
| const writeErr = "write: broken pipe" |
There was a problem hiding this comment.
I'm a little worried that this will be too brittle, because the broken pipe comes from the standard libraries IIRC. Can we settle for checking status.Code(err) == codes.Unavailable?
There was a problem hiding this comment.
When I started writing this, it always was failing with error reading server preface locally. But then, when I pushed my code to GA, it was always failing with broken pipe error. I thought about switching to codes.Unavailable at that point, but then told myself that this should work. But I take your point, never know what it will fail with on google3. Switched it to codes.Unavailable. Thanks.
test/pickfirst_test.go
Outdated
| const readErr = "error reading server preface" | ||
| const writeErr = "write: broken pipe" | ||
| client := testgrpc.NewTestServiceClient(cc) | ||
| if _, err := client.EmptyCall(ctx, &testpb.Empty{}); !strings.Contains(err.Error(), readErr) && !strings.Contains(err.Error(), writeErr) { |
There was a problem hiding this comment.
FWIW if err is nil for any reason, then this will panic.. If we're keeping string checks, then checking err == nil || (these && strings) would be a simple fix.
There was a problem hiding this comment.
Switched to using status code instead. So, this is not required.
Fixes #6468
RELEASE NOTES: none