- Notifications
You must be signed in to change notification settings - Fork322
test: make_AsyncJob tests mock at a lower layer#340
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
This is intented to make the `_AsyncJob` tests more robustto changes in retry behavior. It also more explicitly teststhe retry behavior by observing API calls rather than callsto 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.
| raiseNotImplementedError("Abstract") | ||
| returncopy.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 thestatistics property from theirto_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 bereported as "done" without having the results of a `getQueryResults` APIcall.Follow-up to#340
This is intented to make the
_AsyncJobtests 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 🦕