Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore(tests): add streaming retry showcase tests #1764

Draft
wants to merge 20 commits into
base: main
Choose a base branch
from

Conversation

daniel-sanche
Copy link
Contributor

@daniel-sanche daniel-sanche commented Sep 12, 2023

This PR adds tests for api-core streaming retries in the gapic showcase system tests, using Streaming Sequences

There is currently one expected failing test: when testing a stream that sends partial data before raising an exception, the gprc rest client doesn't yield any of the partial data, only the exception. I'm not sure if this is caused by a bug in the showcase server, a bug in the rest client, or expected behavior. Let me know if you have any information

Other tests will pass after streaming retries branch is merged

@daniel-sanche daniel-sanche requested a review from a team as a code owner September 12, 2023 18:57
@product-auto-label product-auto-label bot added the size: l Pull request size is large. label Sep 12, 2023
@vchudnov-g
Copy link
Contributor

Given the failure in googleapis/gapic-showcase#1377, we should ideally also do at least one manual check that the REST client's code handling server-streaming responses does indeed handle real server errors well.

@parthea
Copy link
Contributor

parthea commented Nov 27, 2023

Marking as draft as presubmits are failing

@parthea parthea marked this pull request as draft November 27, 2023 18:26
@parthea
Copy link
Contributor

parthea commented Jun 20, 2024

Closing as stale. Please re-open if this is still being worked on

@parthea parthea closed this Jun 20, 2024
@daniel-sanche daniel-sanche reopened this Jun 25, 2024
@daniel-sanche
Copy link
Contributor Author

This isn't something I'm currently working on, but I did put some work into this at one point, and I'd hate for it to get completely lost

The tests are failing because of a bug they turned up in the rest streaming code. I think originally we wanted to fix that so these tests would be green before merging, but the bug fix seems to have stalled. Maybe we should add a comment and skip those tests for now? Let me know how you want to proceed with this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size: l Pull request size is large.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants