Skip to content
This repository was archived by the owner on Mar 6, 2026. It is now read-only.
2 changes: 1 addition & 1 deletion google/cloud/bigquery/job/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -1044,7 +1044,7 @@ def result( # type: ignore # (incompatible with supertype)
if self.state is None:
self._begin(retry=retry, timeout=timeout)

kwargs = {} if retry is DEFAULT_RETRY else {"retry": retry}
kwargs = {"retry": retry}
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.

Do we need a kwargs dict here? Can't you just do return super(_AsyncJob, self).result(timeout=timeout, retry=retry)?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Removed that line and reworked things as you suggest.

return super(_AsyncJob, self).result(timeout=timeout, **kwargs)

def cancelled(self):
Expand Down
42 changes: 42 additions & 0 deletions tests/unit/test_job_retry.py
Original file line number Diff line number Diff line change
Expand Up @@ -615,3 +615,45 @@ def test_query_and_wait_retries_job_for_DDL_queries(global_time_lock):
_, kwargs = calls[3]
assert kwargs["method"] == "POST"
assert kwargs["path"] == query_request_path


@pytest.mark.parametrize(
"result_retry",
[
pytest.param(
{},
id="default retry",
Comment thread
chalmerlowe marked this conversation as resolved.
Outdated
),
pytest.param(
{"retry": google.cloud.bigquery.retry.DEFAULT_RETRY.with_timeout(timeout=10.0)},
id="custom retry object",
Comment thread
chalmerlowe marked this conversation as resolved.
Outdated
),
],
)
def test_retry_load_job_result(result_retry, PROJECT, DS_ID):
from google.cloud.bigquery.dataset import DatasetReference
from google.cloud.bigquery.job.load import LoadJob

client = make_client()
conn = client._connection = make_connection(
dict(
status=dict(state="RUNNING"),
jobReference={"jobId": "id_1"},
),
google.api_core.exceptions.ServiceUnavailable("retry me"),
dict(
status=dict(state="DONE"),
jobReference={"jobId": "id_1"},
statistics={"load": {"outputRows": 1}},
),
)

table_ref = DatasetReference(project=PROJECT, dataset_id=DS_ID).table("new_table")
job = LoadJob("id_1", source_uris=None, destination=table_ref, client=client)
result = job.result(**result_retry)

assert job.state == "DONE"
assert result.output_rows == 1

# We made all the calls we expected to.
assert conn.api_request.call_count == 3
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.

This test seems to ensure that the rpc is being retried, but not that it's using the retry configuration that was actually passed in.

If a regression made it fall back to DEFAULT_RETRY in the handwritten layer, or even the default retry configured in the gapic layer like before, wouldn't the tests still pass? Or is that tested elsewhere?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Spent some time thinking about this PR.

Began to doubt whether checking the client._connection.api_request() (e.g. conn.api_request) method was the right way to go.

Turns out, the client._call_api() method wraps the call to client._connection.api_request(). The retry object is used by _call_api to manage any needed retries, but the retry object itself is NOT passed as a direct argument to client._connection.api_request. Nor is it expected to be.

Here's what we did:

  1. Mock client._call_api: we wrapped the original client._call_api using mock.patch.object with wraps=client._call_api. This lets the real _call_api (including the retry decorator) execute, but we can still inspect the calls.
  2. Inspect retry argument: we added a loop through wrapped_call_api.mock_calls to check the retry object passed as the first argument to _call_api.
  3. Conditional Assertions: Inside the loop, we added assertions to verify that the properties of the called_retry object match the expected_retry (which is either DEFAULT_RETRY or the "custom retry" object). we've included a specific check for the _timeout == 10.0 in the "custom retry" case.
  4. Kept Existing Assertions: The original assertions on job.state, result.output_rows, and conn.api_request.call_count are still in place.