-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
interop: add test case for "pick_first" #2762
Conversation
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.
LGTM with some nits. Thanks for doing this
interop/client/client.go
Outdated
@@ -270,6 +271,9 @@ func main() { | |||
case "unimplemented_service": | |||
interop.DoUnimplementedService(testpb.NewUnimplementedServiceClient(conn)) | |||
grpclog.Infoln("UnimplementedService done") | |||
case "pick_first_unary": | |||
interop.DoPickFirstUnary(tc, 100) |
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.
small nit: might be better to unparameterize rpcCount
and hardcode 100 directly into the body of DoPickFirstUnary
- that, or make have it fully configurable via command arg.
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 will go with hardcode now. Adding a flag only for one test case also seems bad...
interop/test_utils.go
Outdated
} | ||
|
||
// NewTestServer creates a test server for test service. | ||
func NewTestServer() testpb.TestServiceServer { | ||
return &testServer{} | ||
return &testServer{ |
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's not strictly necessary that the Go server implement this test case just yet, since there is a special interop server that's configured with the TXT record etc. which the client will run against. But no problem having it - up to you
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 will revert it for now. It adds a uuid
dependency to gRPC.
interop/test_utils.go
Outdated
continue | ||
} | ||
if serverID != id { | ||
grpclog.Fatalf("got different server ids: %q vs %q", serverID, id) |
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.
nit: also log the iteration that it failed on?
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.
done
417d1e2
to
f49aa0f
Compare
Verify that all requests are sent to the same server.