Skip to content

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

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

Open
wants to merge 36 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

@parthea
Copy link
Contributor

parthea commented Mar 31, 2025

I think it's better to close this PR since no one is actively working on it and this is blocked on googleapis/gapic-showcase#1377 which is not actively being worked on. Please feel free to file a new issue in this repository, if needed , in case googleapis/gapic-showcase#1377 does not capture the current state.

@parthea parthea closed this Mar 31, 2025
@daniel-sanche
Copy link
Contributor Author

@parthea One option would be to add a skip label to the known-failing test, and merge the others?

@parthea parthea reopened this Apr 1, 2025
@parthea
Copy link
Contributor

parthea commented Apr 1, 2025

Sounds good. @daniel-sanche, Please could you also add a TODO comment with a link to an issue in https://github.com/googleapis/gapic-generator-python/issues?

@daniel-sanche daniel-sanche marked this pull request as ready for review April 5, 2025 00:47
@daniel-sanche
Copy link
Contributor Author

daniel-sanche commented Apr 5, 2025

@parthea Ok, I skipped the test and added a new bug to track it: #2375 (linked to the existing bug in the showcase repo: googleapis/gapic-showcase#1377)

@daniel-sanche daniel-sanche removed the status: blocked Resolving the issue is dependent on other work. label Apr 7, 2025
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