fix: broken create_job method #300
Conversation
google-cla
bot
added
the
cla: yes
There was a problem hiding this comment.
Since the root cause of this being broken for a while was the use of autospec mocks early in the test, I wonder if it's possible to mock something a bit further along so that the test still exercises the request marshalling. Seems like we should be able to at least have coverage through to_api_repr of the Job created by this method.
| @@ -1625,11 +1626,9 @@ def create_job(self, job_config, retry=DEFAULT_RETRY): | |||
| job_config | |||
| ) | |||
| destination = _get_sub_prop(job_config, ["copy", "destinationTable"]) | |||
| destination = TableReference.from_api_repr(destination) | |||
There was a problem hiding this comment.
Would you like to guard this check with isinstance(TableReference, destination)?
There was a problem hiding this comment.
I mock further in test so now it cover to_api_repr. I am not sure about the guard to check instance because job_config is dict and as per the example it has values in the string format.
…into bigquery_issue_297
…into bigquery_issue_297
…into bigquery_issue_297
google/cloud/bigquery/job.py
Outdated
| self._configuration._properties, ["copy", "sourceTable"] | ||
| ) | ||
| if source_table: | ||
| _helpers._set_sub_prop( |
There was a problem hiding this comment.
No need for this logic. We can always set the sourceTables with only a single item, but if given a job resource that has only sourceTable we need to be able to handle that.
tests/unit/test_client.py
Outdated
| with load_patch as client_method, get_config_patch: | ||
| client.create_job(job_config=job_config) | ||
| client_method.assert_called_once() | ||
| if "copy" in job_config: |
There was a problem hiding this comment.
Ditto. I'm sure confused by this logic, but I think whatever it was meant to do is unnecessary after #323
tests/unit/test_client.py
Outdated
| with load_patch as client_method, get_config_patch: | ||
| client.create_job(job_config=job_config) | ||
| client_method.assert_called_once() | ||
| # if "copy" in job_config: |
There was a problem hiding this comment.
Is this commented out code needed?
There was a problem hiding this comment.
Sorry, my bad.
google/cloud/bigquery/client.py
Outdated
| @@ -1654,7 +1657,6 @@ def create_job(self, job_config, retry=DEFAULT_RETRY): | |||
| ) | |||
| elif "query" in job_config: | |||
| copy_config = copy.deepcopy(job_config) | |||
| _del_sub_prop(copy_config, ["query", "destinationTable"]) | |||
There was a problem hiding this comment.
I didn't realize we were already removing this property in past releases. We should restore this, since I don't want to break existing clients.
Fixes #297