From a7bb93dca7edf0b34adab16fbcd5c8859f85cdeb Mon Sep 17 00:00:00 2001 From: Danny Hermes Date: Tue, 25 Jul 2017 12:56:25 -0700 Subject: [PATCH 1/4] Simplifying Client constructor's for Bigtable and Spanner. --- bigtable/google/cloud/bigtable/client.py | 48 ++++++++++++------------ spanner/google/cloud/spanner/client.py | 28 ++++++-------- 2 files changed, 35 insertions(+), 41 deletions(-) diff --git a/bigtable/google/cloud/bigtable/client.py b/bigtable/google/cloud/bigtable/client.py index 62877371a945..e03ebe91113d 100644 --- a/bigtable/google/cloud/bigtable/client.py +++ b/bigtable/google/cloud/bigtable/client.py @@ -31,7 +31,6 @@ import os -import google.auth import google.auth.credentials from google.gax.utils import metrics from google.longrunning import operations_grpc @@ -39,8 +38,7 @@ from google.cloud._helpers import make_insecure_stub from google.cloud._helpers import make_secure_stub from google.cloud._http import DEFAULT_USER_AGENT -from google.cloud.client import _ClientFactoryMixin -from google.cloud.client import _ClientProjectMixin +from google.cloud.client import ClientWithProject from google.cloud.environment_vars import BIGTABLE_EMULATOR from google.cloud.bigtable import __version__ @@ -166,13 +164,13 @@ def _make_table_stub(client): client.emulator_host) -class Client(_ClientFactoryMixin, _ClientProjectMixin): +class Client(ClientWithProject): """Client for interacting with Google Cloud Bigtable API. .. note:: Since the Cloud Bigtable API requires the gRPC transport, no - ``http`` argument is accepted by this class. + ``_http`` argument is accepted by this class. :type project: :class:`str` or :func:`unicode ` :param project: (Optional) The ID of the project which owns the @@ -209,31 +207,17 @@ class Client(_ClientFactoryMixin, _ClientProjectMixin): def __init__(self, project=None, credentials=None, read_only=False, admin=False, user_agent=DEFAULT_USER_AGENT): - _ClientProjectMixin.__init__(self, project=project) - if credentials is None: - credentials, _ = google.auth.default() - if read_only and admin: raise ValueError('A read-only client cannot also perform' 'administrative actions.') - scopes = [] - if read_only: - scopes.append(READ_ONLY_SCOPE) - else: - scopes.append(DATA_SCOPE) - + # NOTE: This API has no use for the _http argument, but sending it + # will have no impact since the _http() @property only lazily + # creates a working HTTP object. + super(Client, self).__init__( + project=project, credentials=credentials, _http=None) self._read_only = bool(read_only) - - if admin: - scopes.append(ADMIN_SCOPE) - self._admin = bool(admin) - - credentials = google.auth.credentials.with_scopes_if_required( - credentials, scopes) - - self._credentials = credentials self.user_agent = user_agent self.emulator_host = os.getenv(BIGTABLE_EMULATOR) @@ -244,6 +228,22 @@ def __init__(self, project=None, credentials=None, self._operations_stub_internal = _make_operations_stub(self) self._table_stub_internal = _make_table_stub(self) + self._set_scopes() + + def _set_scopes(self): + """Set the scopes on the current credentials.""" + scopes = [] + if self._read_only: + scopes.append(READ_ONLY_SCOPE) + else: + scopes.append(DATA_SCOPE) + + if self._admin: + scopes.append(ADMIN_SCOPE) + + self._credentials = google.auth.credentials.with_scopes_if_required( + self._credentials, scopes) + def copy(self): """Make a copy of this client. diff --git a/spanner/google/cloud/spanner/client.py b/spanner/google/cloud/spanner/client.py index b701b017abb0..445d82851ccf 100644 --- a/spanner/google/cloud/spanner/client.py +++ b/spanner/google/cloud/spanner/client.py @@ -35,8 +35,7 @@ # pylint: enable=line-too-long from google.cloud._http import DEFAULT_USER_AGENT -from google.cloud.client import _ClientFactoryMixin -from google.cloud.client import _ClientProjectMixin +from google.cloud.client import ClientWithProject from google.cloud.iterator import GAXIterator from google.cloud.spanner import __version__ from google.cloud.spanner._helpers import _options_with_prefix @@ -73,13 +72,13 @@ def from_pb(cls, config_pb): return cls(config_pb.name, config_pb.display_name) -class Client(_ClientFactoryMixin, _ClientProjectMixin): +class Client(ClientWithProject): """Client for interacting with Cloud Spanner API. .. note:: Since the Cloud Spanner API requires the gRPC transport, no - ``http`` argument is accepted by this class. + ``_http`` argument is accepted by this class. :type project: :class:`str` or :func:`unicode ` :param project: (Optional) The ID of the project which owns the @@ -104,21 +103,16 @@ class Client(_ClientFactoryMixin, _ClientProjectMixin): _database_admin_api = None _SET_PROJECT = True # Used by from_service_account_json() + SCOPE = (SPANNER_ADMIN_SCOPE,) + """The scopes required for Google Cloud Spanner.""" + def __init__(self, project=None, credentials=None, user_agent=DEFAULT_USER_AGENT): - - _ClientProjectMixin.__init__(self, project=project) - if credentials is None: - credentials, _ = google.auth.default() - - scopes = [ - SPANNER_ADMIN_SCOPE, - ] - - credentials = google.auth.credentials.with_scopes_if_required( - credentials, scopes) - - self._credentials = credentials + # NOTE: This API has no use for the _http argument, but sending it + # will have no impact since the _http() @property only lazily + # creates a working HTTP object. + super(Client, self).__init__( + project=project, credentials=credentials, _http=None) self.user_agent = user_agent @property From cfcb455c4ed29912f914c0b9396c6820dc9da181 Mon Sep 17 00:00:00 2001 From: Danny Hermes Date: Wed, 26 Jul 2017 14:10:43 -0700 Subject: [PATCH 2/4] Fixing Bigtable unit tests after Client re-factor. Also slightly changing the Client constructor so that it only called `with_scopes()` one time on the credentials (was previously calling with `SCOPE=None` and then again with the custom scope for the instance) --- bigtable/google/cloud/bigtable/client.py | 26 +- bigtable/nox.py | 13 +- bigtable/tests/unit/test_client.py | 386 ++++++++++++----------- 3 files changed, 235 insertions(+), 190 deletions(-) diff --git a/bigtable/google/cloud/bigtable/client.py b/bigtable/google/cloud/bigtable/client.py index e03ebe91113d..bd604910ddc7 100644 --- a/bigtable/google/cloud/bigtable/client.py +++ b/bigtable/google/cloud/bigtable/client.py @@ -211,13 +211,17 @@ def __init__(self, project=None, credentials=None, raise ValueError('A read-only client cannot also perform' 'administrative actions.') + # NOTE: We set the scopes **before** calling the parent constructor. + # It **may** use those scopes in ``with_scopes_if_required``. + self._read_only = bool(read_only) + self._admin = bool(admin) + self.SCOPE = self._get_scopes() + # NOTE: This API has no use for the _http argument, but sending it # will have no impact since the _http() @property only lazily # creates a working HTTP object. super(Client, self).__init__( project=project, credentials=credentials, _http=None) - self._read_only = bool(read_only) - self._admin = bool(admin) self.user_agent = user_agent self.emulator_host = os.getenv(BIGTABLE_EMULATOR) @@ -228,21 +232,21 @@ def __init__(self, project=None, credentials=None, self._operations_stub_internal = _make_operations_stub(self) self._table_stub_internal = _make_table_stub(self) - self._set_scopes() + def _get_scopes(self): + """Get the scopes corresponding to admin / read-only state. - def _set_scopes(self): - """Set the scopes on the current credentials.""" - scopes = [] + Returns: + Tuple[str, ...]: The tuple of scopes. + """ if self._read_only: - scopes.append(READ_ONLY_SCOPE) + scopes = (READ_ONLY_SCOPE,) else: - scopes.append(DATA_SCOPE) + scopes = (DATA_SCOPE,) if self._admin: - scopes.append(ADMIN_SCOPE) + scopes += (ADMIN_SCOPE,) - self._credentials = google.auth.credentials.with_scopes_if_required( - self._credentials, scopes) + return scopes def copy(self): """Make a copy of this client. diff --git a/bigtable/nox.py b/bigtable/nox.py index b43e196a95ff..83b56e49d2df 100644 --- a/bigtable/nox.py +++ b/bigtable/nox.py @@ -37,10 +37,17 @@ def unit_tests(session, python_version): session.install('-e', '.') # Run py.test against the unit tests. - session.run('py.test', '--quiet', - '--cov=google.cloud.bigtable', '--cov=tests.unit', '--cov-append', - '--cov-config=.coveragerc', '--cov-report=', '--cov-fail-under=97', + session.run( + 'py.test', + '--quiet', + '--cov=google.cloud.bigtable', + '--cov=tests.unit', + '--cov-append', + '--cov-config=.coveragerc', + '--cov-report=', + '--cov-fail-under=97', 'tests/unit', + *session.posargs ) diff --git a/bigtable/tests/unit/test_client.py b/bigtable/tests/unit/test_client.py index c3ab8d1ed888..9e0485a41554 100644 --- a/bigtable/tests/unit/test_client.py +++ b/bigtable/tests/unit/test_client.py @@ -256,170 +256,215 @@ def _get_target_class(): def _make_one(self, *args, **kwargs): return self._get_target_class()(*args, **kwargs) - def _make_oneWithMocks(self, *args, **kwargs): - from google.cloud._testing import _Monkey - from google.cloud.bigtable import client as MUT - - mock_make_data_stub = _MakeStubMock() - mock_make_instance_stub = _MakeStubMock() - mock_make_operations_stub = _MakeStubMock() - mock_make_table_stub = _MakeStubMock() - with _Monkey(MUT, _make_data_stub=mock_make_data_stub, - _make_instance_stub=mock_make_instance_stub, - _make_operations_stub=mock_make_operations_stub, - _make_table_stub=mock_make_table_stub): - return self._make_one(*args, **kwargs) - - def _constructor_test_helper(self, expected_scopes, creds, - read_only=False, admin=False, - user_agent=None, expected_creds=None): - from google.cloud._testing import _Monkey - from google.cloud.bigtable import client as MUT - - user_agent = user_agent or MUT.DEFAULT_USER_AGENT - - mock_make_data_stub = _MakeStubMock() - mock_make_instance_stub = _MakeStubMock() - mock_make_operations_stub = _MakeStubMock() - mock_make_table_stub = _MakeStubMock() - with _Monkey(MUT, _make_data_stub=mock_make_data_stub, - _make_instance_stub=mock_make_instance_stub, - _make_operations_stub=mock_make_operations_stub, - _make_table_stub=mock_make_table_stub): - client = self._make_one(project=self.PROJECT, credentials=creds, - read_only=read_only, admin=admin, - user_agent=user_agent) - - # Verify the mocks. - self.assertEqual(mock_make_data_stub.calls, [client]) - if admin: - self.assertSequenceEqual(mock_make_instance_stub.calls, [client]) - self.assertSequenceEqual(mock_make_operations_stub.calls, [client]) - self.assertSequenceEqual(mock_make_table_stub.calls, [client]) - else: - self.assertSequenceEqual(mock_make_instance_stub.calls, []) - self.assertSequenceEqual(mock_make_operations_stub.calls, []) - self.assertSequenceEqual(mock_make_table_stub.calls, []) - - expected_creds = expected_creds or creds.with_scopes.return_value - self.assertIs(client._credentials, expected_creds) + @mock.patch('google.cloud.bigtable.client._make_table_stub') + @mock.patch('google.cloud.bigtable.client._make_operations_stub') + @mock.patch('google.cloud.bigtable.client._make_instance_stub') + @mock.patch('google.cloud.bigtable.client._make_data_stub') + def _make_one_with_mocks( + self, _make_data_stub, _make_instance_stub, + _make_operations_stub, _make_table_stub, + *args, **kwargs): + return self._make_one(*args, **kwargs) + + @mock.patch('google.cloud.bigtable.client._make_table_stub') + @mock.patch('google.cloud.bigtable.client._make_operations_stub') + @mock.patch('google.cloud.bigtable.client._make_instance_stub') + @mock.patch('google.cloud.bigtable.client._make_data_stub') + def test_constructor_default_scopes( + self, _make_data_stub, _make_instance_stub, + _make_operations_stub, _make_table_stub): + from google.cloud.bigtable.client import DATA_SCOPE - if expected_scopes is not None: - creds.with_scopes.assert_called_once_with(expected_scopes) + expected_scopes = (DATA_SCOPE,) + credentials = _make_credentials() + custom_user_agent = 'custom-application' + client = self._make_one( + project=self.PROJECT, credentials=credentials, + user_agent=custom_user_agent) self.assertEqual(client.project, self.PROJECT) - self.assertEqual(client.user_agent, user_agent) - # Check gRPC stubs (or mocks of them) are set - self.assertIs(client._data_stub, mock_make_data_stub.result) - if admin: - self.assertIs(client._instance_stub_internal, - mock_make_instance_stub.result) - self.assertIs(client._operations_stub_internal, - mock_make_operations_stub.result) - self.assertIs(client._table_stub_internal, - mock_make_table_stub.result) - else: - self.assertIsNone(client._instance_stub_internal) - self.assertIsNone(client._operations_stub_internal) - self.assertIsNone(client._table_stub_internal) - - def test_constructor_default_scopes(self): - from google.cloud.bigtable import client as MUT - - expected_scopes = [MUT.DATA_SCOPE] - creds = _make_credentials() - self._constructor_test_helper(expected_scopes, creds) - - def test_constructor_custom_user_agent(self): - from google.cloud.bigtable import client as MUT - - CUSTOM_USER_AGENT = 'custom-application' - expected_scopes = [MUT.DATA_SCOPE] - creds = _make_credentials() - self._constructor_test_helper(expected_scopes, creds, - user_agent=CUSTOM_USER_AGENT) - - def test_constructor_with_admin(self): - from google.cloud.bigtable import client as MUT - - expected_scopes = [MUT.DATA_SCOPE, MUT.ADMIN_SCOPE] - creds = _make_credentials() - self._constructor_test_helper(expected_scopes, creds, admin=True) + self.assertIs( + client._credentials, credentials.with_scopes.return_value) + self.assertIsNone(client._http_internal) + self.assertFalse(client._read_only) + self.assertFalse(client._admin) + self.assertEqual(client.SCOPE, expected_scopes) + self.assertEqual(client.user_agent, custom_user_agent) + self.assertIsNone(client.emulator_host) + self.assertIs(client._data_stub, _make_data_stub.return_value) + self.assertIsNone(client._instance_stub_internal) + self.assertIsNone(client._operations_stub_internal) + self.assertIsNone(client._table_stub_internal) + + # Check mocks. + credentials.with_scopes.assert_called_once_with(expected_scopes) + _make_data_stub.assert_called_once_with(client) + _make_instance_stub.assert_not_called() + _make_operations_stub.assert_not_called() + _make_table_stub.assert_not_called() + + @mock.patch('google.cloud.bigtable.client._make_table_stub') + @mock.patch('google.cloud.bigtable.client._make_operations_stub') + @mock.patch('google.cloud.bigtable.client._make_instance_stub') + @mock.patch('google.cloud.bigtable.client._make_data_stub') + def test_constructor_with_admin( + self, _make_data_stub, _make_instance_stub, + _make_operations_stub, _make_table_stub): + from google.cloud._http import DEFAULT_USER_AGENT + from google.cloud.bigtable.client import ADMIN_SCOPE + from google.cloud.bigtable.client import DATA_SCOPE - def test_constructor_with_read_only(self): - from google.cloud.bigtable import client as MUT + expected_scopes = (DATA_SCOPE, ADMIN_SCOPE) + credentials = _make_credentials() + client = self._make_one( + project=self.PROJECT, credentials=credentials, admin=True) - expected_scopes = [MUT.READ_ONLY_SCOPE] - creds = _make_credentials() - self._constructor_test_helper(expected_scopes, creds, read_only=True) + self.assertEqual(client.project, self.PROJECT) + self.assertIs( + client._credentials, credentials.with_scopes.return_value) + self.assertIsNone(client._http_internal) + self.assertFalse(client._read_only) + self.assertTrue(client._admin) + self.assertEqual(client.SCOPE, expected_scopes) + self.assertEqual(client.user_agent, DEFAULT_USER_AGENT) + self.assertIsNone(client.emulator_host) + self.assertIs(client._data_stub, _make_data_stub.return_value) + self.assertIs( + client._instance_stub_internal, _make_instance_stub.return_value) + self.assertIs( + client._operations_stub_internal, + _make_operations_stub.return_value) + self.assertIs( + client._table_stub_internal, _make_table_stub.return_value) + + # Check mocks. + credentials.with_scopes.assert_called_once_with(expected_scopes) + _make_data_stub.assert_called_once_with(client) + _make_instance_stub.assert_called_once_with(client) + _make_operations_stub.assert_called_once_with(client) + _make_table_stub.assert_called_once_with(client) def test_constructor_both_admin_and_read_only(self): - creds = _make_credentials() + credentials = _make_credentials() with self.assertRaises(ValueError): - self._constructor_test_helper([], creds, admin=True, - read_only=True) + self._make_one( + project=self.PROJECT, credentials=credentials, + admin=True, read_only=True) - def test_constructor_implicit_credentials(self): + def test__get_scopes_default(self): from google.cloud.bigtable.client import DATA_SCOPE - creds = _make_credentials() - expected_scopes = [DATA_SCOPE] + client = self._make_one( + project=self.PROJECT, credentials=_make_credentials()) + self.assertEqual(client._get_scopes(), (DATA_SCOPE,)) - patch = mock.patch( - 'google.auth.default', return_value=(creds, None)) - with patch as default: - self._constructor_test_helper( - None, None, - expected_creds=creds.with_scopes.return_value) + def test__get_scopes_admin(self): + from google.cloud.bigtable.client import ADMIN_SCOPE + from google.cloud.bigtable.client import DATA_SCOPE - default.assert_called_once_with() - creds.with_scopes.assert_called_once_with(expected_scopes) + client = self._make_one( + project=self.PROJECT, credentials=_make_credentials(), + admin=True) + expected_scopes = (DATA_SCOPE, ADMIN_SCOPE) + self.assertEqual(client._get_scopes(), expected_scopes) + + def test__get_scopes_read_only(self): + from google.cloud.bigtable.client import READ_ONLY_SCOPE + + client = self._make_one( + project=self.PROJECT, credentials=_make_credentials(), + read_only=True) + self.assertEqual(client._get_scopes(), (READ_ONLY_SCOPE,)) + + def _copy_helper_check_stubs(self, client, new_client): + if client._admin: + # Check the instance stub. + self.assertIs( + client._instance_stub_internal, mock.sentinel.inst_stub1) + self.assertIs( + new_client._instance_stub_internal, mock.sentinel.inst_stub2) + self.assertIsNot( + new_client._instance_stub_internal, + client._instance_stub_internal) + # Check the operations stub. + self.assertIs( + client._operations_stub_internal, mock.sentinel.ops_stub1) + self.assertIs( + new_client._operations_stub_internal, mock.sentinel.ops_stub2) + self.assertIsNot( + new_client._operations_stub_internal, + client._operations_stub_internal) + # Check the table stub. + self.assertIs( + client._table_stub_internal, mock.sentinel.table_stub1) + self.assertIs( + new_client._table_stub_internal, mock.sentinel.table_stub2) + self.assertIsNot( + new_client._table_stub_internal, client._table_stub_internal) + else: + # Check the instance stub. + self.assertIsNone(client._instance_stub_internal) + self.assertIsNone(new_client._instance_stub_internal) + # Check the operations stub. + self.assertIsNone(client._operations_stub_internal) + self.assertIsNone(new_client._operations_stub_internal) + # Check the table stub. + self.assertIsNone(client._table_stub_internal) + self.assertIsNone(new_client._table_stub_internal) + + @mock.patch( + 'google.cloud.bigtable.client._make_table_stub', + side_effect=[mock.sentinel.table_stub1, mock.sentinel.table_stub2], + ) + @mock.patch( + 'google.cloud.bigtable.client._make_operations_stub', + side_effect=[mock.sentinel.ops_stub1, mock.sentinel.ops_stub2], + ) + @mock.patch( + 'google.cloud.bigtable.client._make_instance_stub', + side_effect=[mock.sentinel.inst_stub1, mock.sentinel.inst_stub2], + ) + @mock.patch( + 'google.cloud.bigtable.client._make_data_stub', + side_effect=[mock.sentinel.data_stub1, mock.sentinel.data_stub2], + ) + def _copy_test_helper( + self, _make_data_stub, _make_instance_stub, + _make_operations_stub, _make_table_stub, **kwargs): + credentials = _make_credentials() + # Make sure it "already" is scoped. + credentials.requires_scopes = False - def test_constructor_credentials_wo_create_scoped(self): - creds = _make_credentials() - expected_scopes = None - self._constructor_test_helper(expected_scopes, creds) + client = self._make_one( + project=self.PROJECT, credentials=credentials, **kwargs) + self.assertIs(client._credentials, credentials) - def _copy_test_helper(self, read_only=False, admin=False): - from google.cloud._testing import _Monkey - from google.cloud.bigtable import client as MUT - - credentials = _make_credentials() - client = self._make_oneWithMocks( - project=self.PROJECT, - credentials=credentials, - read_only=read_only, - admin=admin, - user_agent=self.USER_AGENT) - # Put some fake stubs in place so that we can verify they don't - # get copied. In the admin=False case, only the data stub will - # not be None, so we over-ride all the internal values. - client._data_stub = object() - client._instance_stub_internal = object() - client._operations_stub_internal = object() - client._table_stub_internal = object() - - mock_make_data_stub = _MakeStubMock() - mock_make_instance_stub = _MakeStubMock() - mock_make_operations_stub = _MakeStubMock() - mock_make_table_stub = _MakeStubMock() - with _Monkey(MUT, _make_data_stub=mock_make_data_stub, - _make_instance_stub=mock_make_instance_stub, - _make_operations_stub=mock_make_operations_stub, - _make_table_stub=mock_make_table_stub): - new_client = client.copy() + new_client = client.copy() self.assertEqual(new_client._admin, client._admin) self.assertEqual(new_client._credentials, client._credentials) self.assertEqual(new_client.project, client.project) self.assertEqual(new_client.user_agent, client.user_agent) # Make sure stubs are not preserved. - self.assertNotEqual(new_client._data_stub, client._data_stub) - self.assertNotEqual(new_client._instance_stub_internal, - client._instance_stub_internal) - self.assertNotEqual(new_client._operations_stub_internal, - client._operations_stub_internal) - self.assertNotEqual(new_client._table_stub_internal, - client._table_stub_internal) + self.assertIs(client._data_stub, mock.sentinel.data_stub1) + self.assertIs(new_client._data_stub, mock.sentinel.data_stub2) + self.assertIsNot(new_client._data_stub, client._data_stub) + self._copy_helper_check_stubs(client, new_client) + + # Check mocks. + credentials.with_scopes.assert_not_called() + stub_calls = [ + mock.call(client), + mock.call(new_client), + ] + self.assertEqual(_make_data_stub.mock_calls, stub_calls) + if client._admin: + self.assertEqual(_make_instance_stub.mock_calls, stub_calls) + self.assertEqual(_make_operations_stub.mock_calls, stub_calls) + self.assertEqual(_make_table_stub.mock_calls, stub_calls) + else: + _make_instance_stub.assert_not_called() + _make_operations_stub.assert_not_called() + _make_table_stub.assert_not_called() def test_copy(self): self._copy_test_helper() @@ -433,61 +478,61 @@ def test_copy_read_only(self): def test_credentials_getter(self): credentials = _make_credentials() project = 'PROJECT' - client = self._make_oneWithMocks(project=project, - credentials=credentials) + client = self._make_one_with_mocks( + project=project, credentials=credentials) self.assertIs(client.credentials, credentials.with_scopes.return_value) def test_project_name_property(self): credentials = _make_credentials() project = 'PROJECT' - client = self._make_oneWithMocks(project=project, - credentials=credentials) + client = self._make_one_with_mocks( + project=project, credentials=credentials) project_name = 'projects/' + project self.assertEqual(client.project_name, project_name) def test_instance_stub_getter(self): credentials = _make_credentials() project = 'PROJECT' - client = self._make_oneWithMocks(project=project, - credentials=credentials, admin=True) + client = self._make_one_with_mocks( + project=project, credentials=credentials, admin=True) self.assertIs(client._instance_stub, client._instance_stub_internal) def test_instance_stub_non_admin_failure(self): credentials = _make_credentials() project = 'PROJECT' - client = self._make_oneWithMocks(project=project, - credentials=credentials, admin=False) + client = self._make_one_with_mocks( + project=project, credentials=credentials, admin=False) with self.assertRaises(ValueError): getattr(client, '_instance_stub') def test_operations_stub_getter(self): credentials = _make_credentials() project = 'PROJECT' - client = self._make_oneWithMocks(project=project, - credentials=credentials, admin=True) + client = self._make_one_with_mocks( + project=project, credentials=credentials, admin=True) self.assertIs(client._operations_stub, client._operations_stub_internal) def test_operations_stub_non_admin_failure(self): credentials = _make_credentials() project = 'PROJECT' - client = self._make_oneWithMocks(project=project, - credentials=credentials, admin=False) + client = self._make_one_with_mocks( + project=project, credentials=credentials, admin=False) with self.assertRaises(ValueError): getattr(client, '_operations_stub') def test_table_stub_getter(self): credentials = _make_credentials() project = 'PROJECT' - client = self._make_oneWithMocks(project=project, - credentials=credentials, admin=True) + client = self._make_one_with_mocks( + project=project, credentials=credentials, admin=True) self.assertIs(client._table_stub, client._table_stub_internal) def test_table_stub_non_admin_failure(self): credentials = _make_credentials() project = 'PROJECT' - client = self._make_oneWithMocks(project=project, - credentials=credentials, admin=False) + client = self._make_one_with_mocks( + project=project, credentials=credentials, admin=False) with self.assertRaises(ValueError): getattr(client, '_table_stub') @@ -501,8 +546,8 @@ def test_instance_factory_defaults(self): INSTANCE_ID = 'instance-id' DISPLAY_NAME = 'display-name' credentials = _make_credentials() - client = self._make_oneWithMocks(project=PROJECT, - credentials=credentials) + client = self._make_one_with_mocks( + project=PROJECT, credentials=credentials) instance = client.instance(INSTANCE_ID, display_name=DISPLAY_NAME) @@ -523,8 +568,8 @@ def test_instance_factory_w_explicit_serve_nodes(self): LOCATION_ID = 'locname' SERVE_NODES = 5 credentials = _make_credentials() - client = self._make_oneWithMocks(project=PROJECT, - credentials=credentials) + client = self._make_one_with_mocks( + project=PROJECT, credentials=credentials) instance = client.instance( INSTANCE_ID, display_name=DISPLAY_NAME, @@ -554,7 +599,7 @@ def test_list_instances(self): 'projects/' + self.PROJECT + '/instances/' + INSTANCE_ID2) credentials = _make_credentials() - client = self._make_oneWithMocks( + client = self._make_one_with_mocks( project=self.PROJECT, credentials=credentials, admin=True, @@ -609,14 +654,3 @@ def __init__(self, credentials, user_agent, emulator_host=None): self.credentials = credentials self.user_agent = user_agent self.emulator_host = emulator_host - - -class _MakeStubMock(object): - - def __init__(self): - self.result = object() - self.calls = [] - - def __call__(self, client): - self.calls.append(client) - return self.result From f57885ae7632d92ed0e6d4a35e954944bb513f20 Mon Sep 17 00:00:00 2001 From: Danny Hermes Date: Wed, 26 Jul 2017 14:23:13 -0700 Subject: [PATCH 3/4] Fixing Spanner unit tests after Client re-factor. Also slightly changing the `copy()` method so that it just passes the same credentials instance. Also updating `nox` config to allow session `posargs`. --- spanner/google/cloud/spanner/client.py | 15 +++---- spanner/nox.py | 13 ++++-- spanner/tests/unit/test_client.py | 56 ++++++++++---------------- 3 files changed, 39 insertions(+), 45 deletions(-) diff --git a/spanner/google/cloud/spanner/client.py b/spanner/google/cloud/spanner/client.py index 445d82851ccf..ba25bacf18fa 100644 --- a/spanner/google/cloud/spanner/client.py +++ b/spanner/google/cloud/spanner/client.py @@ -175,19 +175,20 @@ def copy(self): :rtype: :class:`.Client` :returns: A copy of the current client. """ - credentials = self._credentials - copied_creds = credentials.create_scoped(credentials.scopes) return self.__class__( - self.project, - copied_creds, - self.user_agent, + project=self.project, + credentials=self._credentials, + user_agent=self.user_agent, ) def list_instance_configs(self, page_size=None, page_token=None): """List available instance configurations for the client's project. - See - https://cloud.google.com/spanner/reference/rpc/google.spanner.admin.database.v1#google.spanner.admin.database.v1.InstanceAdmin.ListInstanceConfigs + .. _RPC docs: https://cloud.google.com/spanner/docs/reference/rpc/\ + google.spanner.admin.instance.v1#google.spanner.admin.\ + instance.v1.InstanceAdmin.ListInstanceConfigs + + See `RPC docs`_. :type page_size: int :param page_size: (Optional) Maximum number of results to return. diff --git a/spanner/nox.py b/spanner/nox.py index 980bff46c85d..bdb2b4e4cbb6 100644 --- a/spanner/nox.py +++ b/spanner/nox.py @@ -38,10 +38,17 @@ def unit_tests(session, python_version): session.install('-e', '.') # Run py.test against the unit tests. - session.run('py.test', '--quiet', - '--cov=google.cloud.spanner', '--cov=tests.unit', '--cov-append', - '--cov-config=.coveragerc', '--cov-report=', '--cov-fail-under=97', + session.run( + 'py.test', + '--quiet', + '--cov=google.cloud.spanner', + '--cov=tests.unit', + '--cov-append', + '--cov-config=.coveragerc', + '--cov-report=', + '--cov-fail-under=97', 'tests/unit', + *session.posargs ) diff --git a/spanner/tests/unit/test_client.py b/spanner/tests/unit/test_client.py index e5e90fd6b7ab..28eee9b78f56 100644 --- a/spanner/tests/unit/test_client.py +++ b/spanner/tests/unit/test_client.py @@ -15,6 +15,7 @@ import unittest import mock +import six def _make_credentials(): @@ -40,13 +41,13 @@ class TestClient(unittest.TestCase): TIMEOUT_SECONDS = 80 USER_AGENT = 'you-sir-age-int' - def _getTargetClass(self): + def _get_target_class(self): from google.cloud.spanner.client import Client return Client def _make_one(self, *args, **kwargs): - return self._getTargetClass()(*args, **kwargs) + return self._get_target_class()(*args, **kwargs) def _constructor_test_helper(self, expected_scopes, creds, user_agent=None, @@ -70,9 +71,9 @@ def _constructor_test_helper(self, expected_scopes, creds, def test_constructor_default_scopes(self): from google.cloud.spanner import client as MUT - expected_scopes = [ + expected_scopes = ( MUT.SPANNER_ADMIN_SCOPE, - ] + ) creds = _make_credentials() self._constructor_test_helper(expected_scopes, creds) @@ -80,9 +81,9 @@ def test_constructor_custom_user_agent_and_timeout(self): from google.cloud.spanner import client as MUT CUSTOM_USER_AGENT = 'custom-application' - expected_scopes = [ + expected_scopes = ( MUT.SPANNER_ADMIN_SCOPE, - ] + ) creds = _make_credentials() self._constructor_test_helper(expected_scopes, creds, user_agent=CUSTOM_USER_AGENT) @@ -186,24 +187,27 @@ def __init__(self, *args, **kwargs): self.assertIs(api.kwargs['credentials'], client.credentials) def test_copy(self): - credentials = _Credentials('value') + credentials = _make_credentials() + # Make sure it "already" is scoped. + credentials.requires_scopes = False + client = self._make_one( project=self.PROJECT, credentials=credentials, user_agent=self.USER_AGENT) new_client = client.copy() - self.assertEqual(new_client._credentials, client._credentials) + self.assertIs(new_client._credentials, client._credentials) self.assertEqual(new_client.project, client.project) self.assertEqual(new_client.user_agent, client.user_agent) def test_credentials_property(self): - credentials = _Credentials() + credentials = _make_credentials() client = self._make_one(project=self.PROJECT, credentials=credentials) - self.assertIs(client.credentials, credentials) + self.assertIs(client.credentials, credentials.with_scopes.return_value) def test_project_name_property(self): - credentials = _Credentials() + credentials = _make_credentials() client = self._make_one(project=self.PROJECT, credentials=credentials) project_name = 'projects/' + self.PROJECT self.assertEqual(client.project_name, project_name) @@ -213,7 +217,7 @@ def test_list_instance_configs_wo_paging(self): from google.gax import INITIAL_PAGE from google.cloud.spanner.client import InstanceConfig - credentials = _Credentials() + credentials = _make_credentials() client = self._make_one(project=self.PROJECT, credentials=credentials) client.connection = object() api = client._instance_admin_api = _FauxInstanceAdminAPI() @@ -240,14 +244,13 @@ def test_list_instance_configs_wo_paging(self): [('google-cloud-resource-prefix', client.project_name)]) def test_list_instance_configs_w_paging(self): - import six from google.cloud._testing import _GAXPageIterator from google.cloud.spanner.client import InstanceConfig SIZE = 15 TOKEN_RETURNED = 'TOKEN_RETURNED' TOKEN_PASSED = 'TOKEN_PASSED' - credentials = _Credentials() + credentials = _make_credentials() client = self._make_one(project=self.PROJECT, credentials=credentials) client.connection = object() api = client._instance_admin_api = _FauxInstanceAdminAPI() @@ -280,7 +283,7 @@ def test_instance_factory_defaults(self): from google.cloud.spanner.instance import DEFAULT_NODE_COUNT from google.cloud.spanner.instance import Instance - credentials = _Credentials() + credentials = _make_credentials() client = self._make_one(project=self.PROJECT, credentials=credentials) instance = client.instance(self.INSTANCE_ID) @@ -295,7 +298,7 @@ def test_instance_factory_defaults(self): def test_instance_factory_explicit(self): from google.cloud.spanner.instance import Instance - credentials = _Credentials() + credentials = _make_credentials() client = self._make_one(project=self.PROJECT, credentials=credentials) instance = client.instance(self.INSTANCE_ID, self.CONFIGURATION_NAME, @@ -314,7 +317,7 @@ def test_list_instances_wo_paging(self): from google.gax import INITIAL_PAGE from google.cloud.spanner.instance import Instance - credentials = _Credentials() + credentials = _make_credentials() client = self._make_one(project=self.PROJECT, credentials=credentials) client.connection = object() api = client._instance_admin_api = _FauxInstanceAdminAPI() @@ -346,14 +349,13 @@ def test_list_instances_wo_paging(self): [('google-cloud-resource-prefix', client.project_name)]) def test_list_instances_w_paging(self): - import six from google.cloud._testing import _GAXPageIterator from google.cloud.spanner.instance import Instance SIZE = 15 TOKEN_RETURNED = 'TOKEN_RETURNED' TOKEN_PASSED = 'TOKEN_PASSED' - credentials = _Credentials() + credentials = _make_credentials() client = self._make_one(project=self.PROJECT, credentials=credentials) client.connection = object() api = client._instance_admin_api = _FauxInstanceAdminAPI() @@ -389,22 +391,6 @@ def test_list_instances_w_paging(self): [('google-cloud-resource-prefix', client.project_name)]) -class _Credentials(object): - - scopes = None - - def __init__(self, access_token=None): - self._access_token = access_token - self._tokens = [] - - def create_scoped(self, scope): - self.scopes = scope - return self - - def __eq__(self, other): - return self._access_token == other._access_token - - class _FauxInstanceAdminAPI(object): def list_instance_configs(self, name, page_size, options): From 56b33e5ceb7de76657a988621f3ed53f752998be Mon Sep 17 00:00:00 2001 From: Danny Hermes Date: Wed, 26 Jul 2017 14:27:06 -0700 Subject: [PATCH 4/4] Removing unused imports after Bigtable/Spanner Client re-factor. --- bigtable/google/cloud/bigtable/client.py | 1 - spanner/google/cloud/spanner/client.py | 2 -- 2 files changed, 3 deletions(-) diff --git a/bigtable/google/cloud/bigtable/client.py b/bigtable/google/cloud/bigtable/client.py index bd604910ddc7..de6d0768266f 100644 --- a/bigtable/google/cloud/bigtable/client.py +++ b/bigtable/google/cloud/bigtable/client.py @@ -31,7 +31,6 @@ import os -import google.auth.credentials from google.gax.utils import metrics from google.longrunning import operations_grpc diff --git a/spanner/google/cloud/spanner/client.py b/spanner/google/cloud/spanner/client.py index ba25bacf18fa..6274d28d9e18 100644 --- a/spanner/google/cloud/spanner/client.py +++ b/spanner/google/cloud/spanner/client.py @@ -24,8 +24,6 @@ :class:`~google.cloud.spanner.database.Database` """ -import google.auth -import google.auth.credentials from google.gax import INITIAL_PAGE # pylint: disable=line-too-long from google.cloud.gapic.spanner_admin_database.v1.database_admin_client import ( # noqa