Skip to content
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

fix: broken create_job method #300

Merged
merged 13 commits into from Oct 20, 2020
Merged

Conversation

@HemangChothani
Copy link
Contributor

@HemangChothani HemangChothani commented Oct 6, 2020

Fixes #297

@HemangChothani HemangChothani requested a review from tswast Oct 6, 2020
@HemangChothani HemangChothani requested a review from as a code owner Oct 6, 2020
@google-cla google-cla bot added the cla: yes label Oct 6, 2020
Copy link

@bhtucker bhtucker left a 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.

Loading

@@ -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)
Copy link

@bhtucker bhtucker Oct 6, 2020

Choose a reason for hiding this comment

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

Would you like to guard this check with isinstance(TableReference, destination)?

Loading

Copy link
Contributor Author

@HemangChothani HemangChothani Oct 7, 2020

Choose a reason for hiding this comment

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

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.

Loading

google/cloud/bigquery/client.py Show resolved Hide resolved
Loading
tests/unit/test_client.py Show resolved Hide resolved
Loading
self._configuration._properties, ["copy", "sourceTable"]
)
if source_table:
_helpers._set_sub_prop(
Copy link
Contributor

@tswast tswast Oct 12, 2020

Choose a reason for hiding this comment

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

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.

Loading

google/cloud/bigquery/job.py Outdated Show resolved Hide resolved
Loading
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:
Copy link
Contributor

@tswast tswast Oct 13, 2020

Choose a reason for hiding this comment

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

Ditto. I'm sure confused by this logic, but I think whatever it was meant to do is unnecessary after #323

Loading

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:
Copy link
Contributor

@tswast tswast Oct 16, 2020

Choose a reason for hiding this comment

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

Is this commented out code needed?

Loading

Copy link
Contributor Author

@HemangChothani HemangChothani Oct 16, 2020

Choose a reason for hiding this comment

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

Sorry, my bad.

Loading

tests/unit/test_client.py Show resolved Hide resolved
Loading
@@ -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"])
Copy link
Contributor

@tswast tswast Oct 19, 2020

Choose a reason for hiding this comment

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

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.

Loading

google/cloud/bigquery/client.py Show resolved Hide resolved
Loading
tswast
tswast approved these changes Oct 20, 2020
@tswast tswast merged commit 155bacc into googleapis:master Oct 20, 2020
10 checks passed
Loading
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants