test(showcase): Add gRPC and HttpJson Showcase ITs to verify Error Details - #13928
test(showcase): Add gRPC and HttpJson Showcase ITs to verify Error Details#13928nnicolee wants to merge 9 commits into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new integration test class, ITErrorDetails.java, to verify that standard and custom error details are correctly parsed and unpacked from ApiException for both gRPC and HTTP/JSON clients. The review feedback highlights two main areas for improvement: ensuring exception-safe resource cleanup in destroyClients() to prevent potential resource leaks if one of the clients fails to close, and avoiding the use of fully qualified class names when there are no naming conflicts to improve code readability.
f2ca894 to
dbe16db
Compare
|
Lint issue looks external: Hmm, maybe we try and update the branch. Maybe got fixed in main |
lqiu96
changed the title
…est/showcase-errordetails
…ogle-cloud-java into test/showcase-errordetails
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new integration test class, ITErrorDetails.java, to verify that standard and custom error details are correctly propagated and deserialized over both gRPC and HTTP/JSON transports. The feedback suggests improving the @AfterAll teardown method to ensure exception-safe cleanup of the clients in reverse order of creation (LIFO) and to include null checks to prevent potential NullPointerExceptions.
| // Parse the AIP-193 "error" JSON object into a status builder | ||
| JsonObject root = JsonParser.parseString(errorJson).getAsJsonObject(); | ||
| JsonObject errorObj = root.getAsJsonObject("error"); | ||
| Status.Builder statusBuilder = Status.newBuilder(); |
There was a problem hiding this comment.
Actually I think this is supposed to map to https://github.com/googleapis/gapic-showcase/blob/b6c247f153369044d599969f6929ecdeb066c4c6/schema/google/showcase/v1beta1/rest_error.proto#L29
HttpJson 1.1 has a slightly different mapping than the Status used for gRPC. https://google.aip.dev/193#http11json-representation
| String customMessage = "this is a custom message to echo back"; | ||
| FailEchoWithDetailsRequest request = | ||
| FailEchoWithDetailsRequest.newBuilder().setMessage(customMessage).build(); | ||
| ApiException exception = | ||
| assertThrows(ApiException.class, () -> grpcClient.failEchoWithDetails(request)); | ||
|
|
||
| assertThat(exception.getStatusCode().getCode()).isEqualTo(StatusCode.Code.ABORTED); | ||
| assertThat(exception.getErrorDetails()).isNotNull(); | ||
|
|
||
| // Reuse Transport-neutral Validation | ||
| verifyErrorDetailsContent(exception.getErrorDetails(), customMessage); |
There was a problem hiding this comment.
qq, since the old difference in here is the PoetryError's poem field, could we just test for that in this test case? Just assert that a customMessage is able to be parsed? We can add an HttpJson variant as well
lqiu96
left a comment
lqiu96
left a comment
There was a problem hiding this comment.
I added a few more comments, but I think the PR looks good. Could you address them and the PR LGTM
3 participants
Description
This PR introduces E2E integration test coverage for standard and custom error details using the
FailEchoWithDetailsRPC in Showcase, validating the exception propagation and deserialization pipeline across both gRPC and HTTP/JSON transports.Key Changes
ErrorInfo,RetryInfo,DebugInfo,QuotaFailure,PreconditionFailure,BadRequest,RequestInfo,ResourceInfo,Help,LocalizedMessage).PoetryError) are successfully unpacked and mapped into the client-sideErrorDetails.ABORTED) propagates correctly.PoetryError) in JSON status payloads.