You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2022/07/07 08:40:00 UTC
[GitHub] [airflow] kosteev commented on a diff in pull request #24682: Add test_connection method to GoogleBaseHook
kosteev commented on code in PR #24682:
URL: https://github.com/apache/airflow/pull/24682#discussion_r915600447
##########
airflow/providers/google/common/hooks/base_google.py:
##########
@@ -580,3 +586,19 @@ def download_content_from_request(file_handle, request: dict, chunk_size: int) -
while done is False:
_, done = downloader.next_chunk()
file_handle.flush()
+
+ def test_connection(self):
+ """Test the Google cloud connectivity from UI"""
+ status, message = False, ''
+ try:
+ token = self._get_access_token()
+ url = f"https://www.googleapis.com/oauth2/v3/tokeninfo?access_token={token}"
Review Comment:
It doesn't look safe, as we do not have url encoding here.
If token contains "?", "/" etc. this url will be broken.
##########
airflow/providers/google/common/hooks/base_google.py:
##########
@@ -580,3 +586,19 @@ def download_content_from_request(file_handle, request: dict, chunk_size: int) -
while done is False:
_, done = downloader.next_chunk()
file_handle.flush()
+
+ def test_connection(self):
+ """Test the Google cloud connectivity from UI"""
+ status, message = False, ''
+ try:
+ token = self._get_access_token()
+ url = f"https://www.googleapis.com/oauth2/v3/tokeninfo?access_token={token}"
+ response = requests.post(url)
+ if response.status_code == 200:
+ status = True
+ message = 'Connection successfully tested'
+ except Exception as e:
Review Comment:
AFAIK catching all type of exceptions is bad practice.
What specific type of exceptions we want to catch here? Probably exception on request post - there has to be a contract what exception will be raised there.
##########
airflow/providers/google/common/hooks/base_google.py:
##########
@@ -580,3 +586,19 @@ def download_content_from_request(file_handle, request: dict, chunk_size: int) -
while done is False:
_, done = downloader.next_chunk()
file_handle.flush()
+
+ def test_connection(self):
+ """Test the Google cloud connectivity from UI"""
Review Comment:
Nit: "Google **C**loud"
##########
tests/providers/google/common/hooks/test_base_google.py:
##########
@@ -341,6 +341,24 @@ def test_get_credentials_and_project_id_with_default_auth(self, mock_get_creds_a
)
assert ('CREDENTIALS', 'PROJECT_ID') == result
+ @mock.patch('requests.post')
+ @mock.patch(MODULE_NAME + '.get_credentials_and_project_id')
+ def test_connection_success(self, mock_get_creds_and_proj_id, requests_post):
+ requests_post.return_value.status_code = 200
+ credentials = mock.MagicMock()
+ type(credentials).token = mock.PropertyMock(return_value="TOKEN")
Review Comment:
It should work instead:
credentials = mock.MagicMock(token="TOKEN")
##########
tests/providers/google/common/hooks/test_base_google.py:
##########
@@ -341,6 +341,24 @@ def test_get_credentials_and_project_id_with_default_auth(self, mock_get_creds_a
)
assert ('CREDENTIALS', 'PROJECT_ID') == result
+ @mock.patch('requests.post')
Review Comment:
Good practice is to mock with autospec=True to catch passing incorrect parameters to method.
Unless there are real objections to use autospec, I would use it, as this actually the only way sometimes (and here specifically) we may spot misalignment in the contract of the function that is mocked and usage of it.
##########
tests/providers/google/common/hooks/test_base_google.py:
##########
@@ -341,6 +341,24 @@ def test_get_credentials_and_project_id_with_default_auth(self, mock_get_creds_a
)
assert ('CREDENTIALS', 'PROJECT_ID') == result
+ @mock.patch('requests.post')
+ @mock.patch(MODULE_NAME + '.get_credentials_and_project_id')
+ def test_connection_success(self, mock_get_creds_and_proj_id, requests_post):
Review Comment:
for the consistency sake I would name "requests_post" -> "mock_requests_post"
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org