Skip to content

test(showcase): Add gRPC and HttpJson Showcase ITs to verify Error Details - #13928

Merged
lqiu96 merged 13 commits into
mainfrom
test/showcase-errordetails
Aug 28, 2026
Merged

test(showcase): Add gRPC and HttpJson Showcase ITs to verify Error Details#13928
lqiu96 merged 13 commits into
mainfrom
test/showcase-errordetails

Conversation

@nnicolee

@nnicolee nnicolee commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

Description

This PR introduces E2E integration test coverage for standard and custom error details using the FailEchoWithDetails RPC in Showcase, validating the exception propagation and deserialization pipeline across both gRPC and HTTP/JSON transports.

Key Changes

  • gRPC Transport Verification:
    • Verifies unpacking and correctness of all standard error details (e.g. ErrorInfo, RetryInfo, DebugInfo, QuotaFailure, PreconditionFailure, BadRequest, RequestInfo, ResourceInfo, Help, LocalizedMessage).
    • Verifies that Showcase-specific custom trailers (e.g. PoetryError) are successfully unpacked and mapped into the client-side ErrorDetails.
    • Verifies dynamic message propagation inside custom trailers when parameters are passed to the request.
  • HTTP/JSON Transport Verification:
    • Verifies that error status code mapping (e.g. ABORTED) propagates correctly.
    • Handles GAX's HTTP/JSON parser boundaries when dealing with unregistered custom types (like PoetryError) in JSON status payloads.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@nnicolee
nnicolee marked this pull request as ready for review July 28, 2026 18:40
@nnicolee
nnicolee requested review from a team as code owners July 28, 2026 18:40
@nnicolee
nnicolee requested a review from lqiu96 July 28, 2026 18:40
@nnicolee
nnicolee force-pushed the test/showcase-errordetails branch from f2ca894 to dbe16db Compare July 28, 2026 20:53
@lqiu96

lqiu96 commented Jul 28, 2026

Copy link
Copy Markdown
Member

Lint issue looks external:

[ERROR] Failed to execute goal com.spotify.fmt:fmt-maven-plugin:2.25:check (default-cli) on project grpc-google-cloud-sql-v1: Found 6 non-complying files, failing build -> [Help 1]
[ERROR] Failed to execute goal com.spotify.fmt:fmt-maven-plugin:2.25:check (default-cli) on project grpc-google-cloud-sql-v1beta4: Found 1 non-complying files, failing build -> [Help 1]

Hmm, maybe we try and update the branch. Maybe got fixed in main

@lqiu96 lqiu96 changed the title test(showcase): add integration tests for error details test(showcase): Add gRPC and HttpJson Showcase ITs to verify Error Details Jul 28, 2026
@nnicolee

Copy link
Copy Markdown
Contributor Author

/gemini review

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@lqiu96 lqiu96 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a few more comments, but I think the PR looks good. Could you address them and the PR LGTM

lqiu96 added a commit that referenced this pull request Aug 26, 2026
Temp fix for lint issue and skip generate to avoid this overriden by
generation run.

This is a temporarily solution to unblock development (e.g.,
#13928), while we
figure out a longer term solution.

For googleapis/librarian#7146

---------

Co-authored-by: Lawrence Qiu <lawrenceqiu@google.com>
@lqiu96
lqiu96 enabled auto-merge (squash) August 26, 2026 18:00
@lqiu96
lqiu96 force-pushed the test/showcase-errordetails branch from c5ab0db to 334c90f Compare August 27, 2026 15:54
@lqiu96 lqiu96 added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Aug 27, 2026
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Aug 27, 2026
@lqiu96
lqiu96 merged commit 81003d2 into main Aug 28, 2026
360 of 368 checks passed
@lqiu96
lqiu96 deleted the test/showcase-errordetails branch August 28, 2026 20:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants