-
Notifications
You must be signed in to change notification settings - Fork 6.5k
BigQuery: user credentials to run a query. #925
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
Conversation
I'll fix up the lint errors in a change I'm making that adds some tests. |
@tswast SG, just ping when you're ready for review. |
@jonparrott Okay, ready for review. I added tests that mock out the flow using a service account like we discussed in chat. |
@@ -0,0 +1 @@ | |||
{"installed":{"client_id":"1071053816718-pk867pkh36eb1ijd1lembvf54gqbces9.apps.googleusercontent.com","project_id":"bigquery-sample-166321","auth_uri":"https://accounts.google.com/o/oauth2/auth","token_uri":"https://accounts.google.com/o/oauth2/token","auth_provider_x509_cert_url":"https://www.googleapis.com/oauth2/v1/certs","client_secret":"skisPURyX9AunQJ9Bd_sW7al","redirect_uris":["urn:ietf:wg:oauth:2.0:oob","http://localhost"]}} |
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.
I'd be great to not have this in the repository, is there any reason for it?
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.
Removed.
Not needed after I changed the tests to mock out user credentials. I'll make sure the docs say to put such a file here.
break | ||
|
||
|
||
def auth_query(project, query, launch_browser=True): |
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.
auth_query
doesn't ring well to me as a good name. Maybe authenticate_and_query
?
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.
Renamed.
|
||
def test_auth_query_console(capsys): | ||
# Arrange | ||
google_auth_oauthlib.flow.InstalledAppFlow = MockFlow |
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.
you can use py.test's monkeypatch fixture for this or decorate with @mock.patch('google_auth_oauthlib.flow.InstalledAppFlow', new=MockFlow)
.
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.
Better yet, I'd actually prefer you to write a new fixture for this using Mock's autospec
. This will ensure that even if google-auth-oauthlib's API changes, our test will catch the interface change despite the mocking:
@pytest.fixture
def mock_flow():
flow_patch = mock.patch('google_oauthlib_flow.InstalledAppFlow', autospec=True)
with flow_patch as flow_mock:
flow_mock.credentials = google.auth.default()[0]
yield flow_mock
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.
Done, autospec
is quite cool.
|
||
|
||
def test_auth_query_console(capsys): | ||
# Arrange |
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.
No need for the comments. :)
assert '2' in out | ||
|
||
|
||
def test_auth_query_server(capsys): |
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.
No need for this test. We're really not after exhaustive coverage.
Mocks out user credentials using the Application Default Credentials, but uses the same scopes.
Thanks for doing this, @tswast! |
…loudPlatform/python-docs-samples#925) * BigQuery: user credentials to run a query. * BigQuery user creds sample: add tests. Mocks out user credentials using the Application Default Credentials, but uses the same scopes.
…loudPlatform/python-docs-samples#925) * BigQuery: user credentials to run a query. * BigQuery user creds sample: add tests. Mocks out user credentials using the Application Default Credentials, but uses the same scopes.
…rm/python-docs-samples#925) * BigQuery: user credentials to run a query. * BigQuery user creds sample: add tests. Mocks out user credentials using the Application Default Credentials, but uses the same scopes.
I'm not sure how to test this one.
Is it possible to add tests that only run locally not on Jenkins, Travis, or Circle?