-
Notifications
You must be signed in to change notification settings - Fork 39
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
fix: support metadata in deferred calls execution #1724
fix: support metadata in deferred calls execution #1724
Conversation
| private <Req, Res> SingleResponseRequestBuilder<Req, Res> addHeaders(SingleResponseRequestBuilder<Req, Res> requestBuilder, Metadata metadata){ | ||
| var updatedBuilder = requestBuilder; | ||
| for (Metadata.MetadataEntry entry: metadata){ | ||
| if (entry.isText()) { |
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.
Actually, maybe for grpc based sdk, I should support binary headers as well?
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 think we can skip those for now.
@@ -47,7 +48,7 @@ public void tearDown() throws Exception { | |||
// tag::initialize[] | |||
@Test | |||
public void initializeCartTest() throws ExecutionException, InterruptedException, TimeoutException { | |||
when(shoppingCartService.create(notNull())) | |||
when(shoppingCartService.create().invoke(notNull())) |
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.
Under the hood we have:
(Metadata metadata) -> addHeaders(((com.example.shoppingcart.ShoppingCartServiceClient) getGrpcClient(com.example.shoppingcart.ShoppingCartService.class)).create(), metadata).invoke(createCart)
thanks to RETURNS_DEEP_STUBS
I was able to fix it in the java version, but with mockito scala I'm not sure if this is an option.
Maybe we shouldn't encourage users to write unit tests for side effects. Personally I would go with IT tests for such cases.
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
References #1702