NotificationsYou must be signed in to change notification settings
Fork321
Star786

fix: broken create_job method#300

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

Jump to bottom

Merged

tswast merged 13 commits intogoogleapis:masterfromMaxxleLLC:bigquery_issue_297

Oct 20, 2020

Merged

fix: broken create_job method#300

tswast merged 13 commits intogoogleapis:masterfromMaxxleLLC:bigquery_issue_297

Oct 20, 2020

Conversation

Copy link

Contributor

HemangChothani commentedOct 6, 2020

Fixes#297

fix: broken create_job method

f305f04

HemangChothani requested review froma team andtswast

October 6, 2020 13:30

google-clabot added the cla: yesThis human has signed the Contributor License Agreement. label

Oct 6, 2020

bhtucker reviewed

Oct 6, 2020

View reviewed changes

Copy link

bhtucker left a comment•
edited
Loading

Choose a reason for hiding this comment

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

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 throughto_api_repr of theJob created by this method.

google/cloud/bigquery/client.py

		job_config
		)
		destination=_get_sub_prop(job_config, ["copy","destinationTable"])
		destination=TableReference.from_api_repr(destination)

Copy link

bhtuckerOct 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 withisinstance(TableReference, destination)?

Copy link

ContributorAuthor

HemangChothaniOct 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 coverto_api_repr. I am not sure about the guard to check instance becausejob_config is dict and as per the example it has values in the string format.

HemangChothani added3 commits

October 7, 2020 11:14

Merge branch 'master' ofhttps://github.com/googleapis/python-bigquery…

f98950a

…into bigquery_issue_297

fix: changes in unit tests

38ce75e

Merge branch 'master' ofhttps://github.com/googleapis/python-bigquery…

8a27c7c

…into bigquery_issue_297

tswast requested changes

Oct 9, 2020

View reviewed changes

google/cloud/bigquery/client.pyShow resolvedHide resolved

tests/unit/test_client.pyShow resolvedHide resolved

HemangChothani added2 commits

October 12, 2020 20:16

fix: fix sourceTable thing

14bbe01

Merge branch 'master' ofhttps://github.com/googleapis/python-bigquery…

64549c4

…into bigquery_issue_297

tswast requested changes

Oct 12, 2020

View reviewed changes

google/cloud/bigquery/job.py Outdated

		self._configuration._properties, ["copy","sourceTable"]
		)
		ifsource_table:
		_helpers._set_sub_prop(

Copy link

Contributor

tswastOct 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 thesourceTables with only a single item, but if given a job resource that has onlysourceTable we need to be able to handle that.