Repository navigation
tests: add system tests for resumable uploads - #18539
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces comprehensive system tests for the resumable upload feature in the GAPIC generator. It adds conditional imports and setup for ResumableUploadServiceClient and its REST interceptor in conftest.py, along with helper functions to initiate and resume uploads. It also adds four new test files covering progress tracking, resumption scenarios, error recovery paths, and stall/deadline controls. There are no review comments, so I have no feedback to provide.
f0defed to
bae5b33
Compare
39c26aa to
f8fa453
Compare
bae5b33 to
ed209a5
Compare
| if not HAS_RESUMABLE_UPLOAD_CLIENT or not HAS_RESUMABLE_UPLOAD_INTERCEPTOR: | ||
| pytest.skip("ResumableUploadServiceClient not available.") | ||
|
|
||
| transport_name = "rest" |
There was a problem hiding this comment.
We need tests starting from clients created with grpc transports too, because those are the ones I'm more concerned about
I worry some of these fixtures are obscuring important details here Do we have any tests that go from creating a client to reading the result, exactly we expect end-users would?
There was a problem hiding this comment.
I created a live test which doesn't use the conftest.py fixtures: packages/gapic-generator/tests/system_live/test_google_ads_resumable_upload.py . This now runs as a system test
be137fe to
54952a9
Compare
a07a705 to
09cea6f
Compare
54952a9 to
7a2bc54
Compare
cdacfb6 to
d5a65d4
Compare
8630d4a to
c0f9005
Compare
9425277 to
dd724d6
Compare
d40a869 to
9f5d734
Compare
| To run the manual live Google Ads acceptance suite: | ||
| RUN_GOOGLE_ADS_ACCEPTANCE=true \\ | ||
| GOOGLE_ADS_DEVELOPER_TOKEN="<developer-token>" \\ | ||
| GOOGLE_ADS_LOGIN_CUSTOMER_ID="7568249731" \\ |
There was a problem hiding this comment.
This seems a little weird to see here. Are you sure this makes sense to check in? Do we have coverage gaps without TestGoogleAdsLiveAcceptance?
There was a problem hiding this comment.
I moved these to environment variables. The live acceptance test now runs as a system test
| ResumableUploadSession = None | ||
|
|
||
|
|
||
| def make_resumable_upload( |
There was a problem hiding this comment.
It looks like the tests still exercise this code in a way different than the user would. Is it possible to create a client, and start the stream using the client apis, instead of creating the session directly?
If this is the main system test, we should make sure we have good end-to-end coverage
There was a problem hiding this comment.
I created a live test which doesn't use conftest.py : packages/gapic-generator/tests/system_live/test_google_ads_resumable_upload.py
packages/gapic-generator/tests/system/conftest.py is used only for showcase integration tests
4847b7c to
9bcd175
Compare
9bcd175 to
2f01aca
Compare
2 participants
Fixes b/562515672