Skip to content

test(showcase): add integration tests showing LRO drops error details - #13984

Draft
nnicolee wants to merge 2 commits into
mainfrom
test/showcase-lro-error-details-bug
Draft

test(showcase): add integration tests showing LRO drops error details#13984
nnicolee wants to merge 2 commits into
mainfrom
test/showcase-lro-error-details-bug

Conversation

@nnicolee

@nnicolee nnicolee commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

This PR adds two new integration test cases to the gapic-showcase client targeting LRO error scenarios:

  • testGRPC_LROErrorResponse_dropsErrorDetails
  • testHttpJson_LROErrorResponse_dropsErrorDetails

These tests reproduce the defect described in #13369. They configure the showcase LRO Wait endpoint to fail with structured error details (a gRPC Status with packed PoetryError details) and assert that under the current code generator output, the thrown ApiException.getErrorDetails() returns null for both transports.

These test cases serve as a baseline showcase to demonstrate the issue. Once the planned generator changes are implemented, we will update these test cases to assert that the structured error details are correctly parsed and populated inside getErrorDetails().

@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 adds integration tests to verify that error details are currently dropped during LRO error parsing for both gRPC and HTTP/JSON clients. The reviewer recommends adding a timeout to the operationFuture.get() calls in both tests to prevent potential indefinite hangs in CI.

Comment on lines +221 to +222
ExecutionException exception =
assertThrows(ExecutionException.class, operationFuture::get);

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.

medium

Calling operationFuture.get() without a timeout can cause the test to hang indefinitely in CI if there is a regression or bug where the future never completes. It is safer to specify a timeout to ensure the test fails fast.

Suggested change
ExecutionException exception =
assertThrows(ExecutionException.class, operationFuture::get);
ExecutionException exception =
assertThrows(ExecutionException.class, () -> operationFuture.get(10, TimeUnit.SECONDS));

Comment on lines +253 to +254
ExecutionException exception =
assertThrows(ExecutionException.class, operationFuture::get);

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.

medium

Calling operationFuture.get() without a timeout can cause the test to hang indefinitely in CI if there is a regression or bug where the future never completes. It is safer to specify a timeout to ensure the test fails fast.

Suggested change
ExecutionException exception =
assertThrows(ExecutionException.class, operationFuture::get);
ExecutionException exception =
assertThrows(ExecutionException.class, () -> operationFuture.get(10, TimeUnit.SECONDS));

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.

1 participant