chore: Add Showcase Callables IT #1483
Conversation
…e-plugin to v3.0.0 (#1488) Co-authored-by: Lawrence Qiu <[email protected]>
Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com> Co-authored-by: Lawrence Qiu <[email protected]>
Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com>
product-auto-label
bot
added
size: l
product-auto-label
bot
added
size: m
product-auto-label
bot
added
size: s
|
Updating this PR following Mridula's PR: #1501 |
| */ | ||
| @Test | ||
| public void testHttpJson_serverResponseError_throwsException() { | ||
| StatusCode.Code notFoundStatusCode = StatusCode.Code.NOT_FOUND; |
There was a problem hiding this comment.
You may have already covered this in the javadoc but is there an advantage of using NOT_FOUND over CANCELLED? Mostly wondering if the grpc tests should also be switched to use this.
There was a problem hiding this comment.
I just picked NOT_FOUND instead of CANCELLED since the docs seem to suggest that CANCELLED status is from a user action (https://chromium.googlesource.com/external/github.com/grpc/grpc/+/refs/tags/v1.21.4-pre1/doc/statuscodes.md). Any error code should be fine since we just need the server to respond back with the error.
There was a problem hiding this comment.
Whether we choose NOT_FOUND or CANCELLED, let's please make this and testGrpc_serverResponseError_throwsException consistent so we aren't implying a difference in behavior. If we want to test both behaviors, let's do so explicitly.
There was a problem hiding this comment.
I'll update to CANCELLED.
| @Test | ||
| public void testHttpJson_serverResponseError_throwsException() { | ||
| StatusCode.Code notFoundStatusCode = StatusCode.Code.NOT_FOUND; | ||
| ApiException exception = |
There was a problem hiding this comment.
Perhaps we can make the exception more specific here? To make sure that the test doesn't pass for a false positive case since ApiException can occur for a number of reasons.
| () -> | ||
| httpJsonClient.echo( | ||
| EchoRequest.newBuilder() | ||
| .setError(Status.newBuilder().setCode(notFoundStatusCode.ordinal()).build()) |
There was a problem hiding this comment.
nit: this may not work but can the inner build() call be omitted when we nest builders? i.e. does EchoRequest.newBuilder().setError(Status.newBuilder().setCode(notFoundStatusCode.ordinal())).build() work?
There was a problem hiding this comment.
Moved it out to a variable
| assertThat(exception.getStatusCode().getCode()).isEqualTo(cancelledStatusCode); | ||
| } |
There was a problem hiding this comment.
nit: How about we use HttpJsonStatusCode.Code.CANCELLED here?
There was a problem hiding this comment.
Sounds good. Updating.
|
[gapic-generator-java-root] Kudos, SonarCloud Quality Gate passed!
|
|
[java_showcase_integration_tests] Kudos, SonarCloud Quality Gate passed!
|
|
[java_showcase_unit_tests] Kudos, SonarCloud Quality Gate passed!
|
Thank you for opening a Pull Request! For general contributing guidelines, please refer to contributing guide
Before submitting your PR, there are a few things you can do to make sure it goes smoothly:
Part of #1439 ☕️