-
Notifications
You must be signed in to change notification settings - Fork 298
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
test: make _AsyncJob
tests mock at a lower layer
#340
Conversation
This is intented to make the `_AsyncJob` tests more robust to changes in retry behavior. It also more explicitly tests the retry behavior by observing API calls rather than calls to certain methods.
# policy passed to it, so we don't throw a non-retriable | ||
# exception here. See: | ||
# https://github.com/googleapis/python-bigquery/issues/24 | ||
_make_retriable_exception(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -588,7 +587,7 @@ def _check_resource_config(cls, resource): | |||
|
|||
def to_api_repr(self): | |||
"""Generate a resource for the job.""" | |||
raise NotImplementedError("Abstract") | |||
return copy.deepcopy(self._properties) | |||
|
|||
_build_resource = to_api_repr # backward-compatibility alias |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This all LGTM, but I'm curious what this backwards compatibility is for here. It's not clear to me why we were specifically testing to make sure this would raise NotImplementedError before.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not clear to me why we were specifically testing to make sure this would raise NotImplementedError before.
I think that was solely to make sure the coverage tests pass. We're still doing a bit of funny business in the *Job
classes, in that most (all?) subclasses omit the statistics
property from their to_api_repr
.
I'm curious what this backwards compatibility is for here.
I don't remember, actually. I see a few hits for _build_resource
in search. https://github.com/googleapis/python-bigquery/search?p=1&q=_build_resource I think _build_resource
is usually the name we use for "update" methods that need to take a field mask and only populate certain fields. Jobs don't support update, so not sure why we ever had one.
This enables checking the job status without making an API call. It also fixes an inconsistency in `QueryJob`, where a job can be reported as "done" without having the results of a `getQueryResults` API call. Follow-up to #340
This is intented to make the
_AsyncJob
tests more robustto changes in retry behavior. It also more explicitly tests
the retry behavior by observing API calls rather than calls
to certain methods.
Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:
Fixes #339 🦕