From 9765c6aac4a37f69cdb2a4ad0078b722d2364b3d Mon Sep 17 00:00:00 2001 From: Sujay <89520981+SD-13@users.noreply.github.com> Date: Sun, 20 Aug 2023 19:13:45 +0530 Subject: [PATCH] Fix query, tests and ci checks (#3) --- .github/workflows/python_type_checks.yml | 2 +- src/github_services.py | 81 ++++++++++----- tests/github_services_test.py | 124 +++++++++++++---------- tests/main_test.py | 96 ++++++++++-------- 4 files changed, 177 insertions(+), 126 deletions(-) diff --git a/.github/workflows/python_type_checks.yml b/.github/workflows/python_type_checks.yml index 6e94389..9df3c3d 100644 --- a/.github/workflows/python_type_checks.yml +++ b/.github/workflows/python_type_checks.yml @@ -20,7 +20,7 @@ jobs: python-version: '3.8.15' architecture: 'x64' - name: Install dependencies - run: pip install -r requirements.txt + run: pip install -r requirements_dev.txt; pip install -r requirements.txt - name: Install Mypy run: pip install mypy - name: Run Mypy checks diff --git a/src/github_services.py b/src/github_services.py index b5928e4..59f2c27 100644 --- a/src/github_services.py +++ b/src/github_services.py @@ -215,9 +215,9 @@ def _get_discussion_data( discussion number. """ - # The following query is written in GraphQL and is being used to fetch data about the - # existing GitHub discussions. This helps to find out the discussion where we want - # to comment. To learn more, check this out https://docs.github.com/en/graphql. + # The following query is written in GraphQL and is being used to fetch the category + # ids and titles from the GitHub discussions. To learn more, check this out + # https://docs.github.com/en/graphql. query = """ query ($org_name: String!, $repository: String!) { repository(owner: $org_name, name: $repository) { @@ -225,18 +225,6 @@ def _get_discussion_data( nodes { id name - repository { - id - discussions(last: 10) { - edges { - node { - id - title - number - } - } - } - } } } } @@ -255,28 +243,67 @@ def _get_discussion_data( timeout=TIMEOUT_SECS ) data = response.json() - discussion_id = None + + category_id = None discussion_categories = ( data['data']['repository']['discussionCategories']['nodes']) for category in discussion_categories: if category['name'] == discussion_category: - discussions = category['repository']['discussions']['edges'] - for discussion in discussions: - if discussion['node']['title'] == discussion_title: - discussion_id = discussion['node']['id'] - discussion_number = discussion['node']['number'] - break - if discussion_id is None: - raise builtins.BaseException( - f'Discussion with title {discussion_title} not found, please create ' - 'a discussion with that title.') + category_id = category['id'] break - if discussion_id is None: + if category_id is None: raise builtins.BaseException( f'{discussion_category} category is missing in GitHub Discussion.') + # The following query is written in GraphQL and is being used to fetch discussions + # from a particular GitHub discussion category. This helps to find out the discussion + # where we want to comment. To learn more, check this out + # https://docs.github.com/en/graphql. + + query = """ + query ($org_name: String!, $repository: String!, $category_id: ID!) { + repository(owner: $org_name, name: $repository) { + discussions(categoryId: $category_id, last:10) { + nodes { + id + title + number + } + } + } + } + """ + + variables = { + 'org_name': org_name, + 'repository': repo_name, + 'category_id': category_id + } + + response = requests.post( + GITHUB_GRAPHQL_URL, + json={'query': query, 'variables': variables}, + headers=_get_request_headers(), + timeout=TIMEOUT_SECS + ) + data = response.json() + discussion_id = None + + discussions = data['data']['repository']['discussions']['nodes'] + + for discussion in discussions: + if discussion['title'] == discussion_title: + discussion_id = discussion['id'] + discussion_number = discussion['number'] + break + + if discussion_id is None: + raise builtins.BaseException( + f'Discussion with title {discussion_title} not found, please create a ' + 'discussion with that title.') + return discussion_id, discussion_number diff --git a/tests/github_services_test.py b/tests/github_services_test.py index e0ca298..ec7a26d 100644 --- a/tests/github_services_test.py +++ b/tests/github_services_test.py @@ -16,6 +16,7 @@ from __future__ import annotations +import builtins import datetime import json import unittest @@ -39,12 +40,12 @@ def test_init_service_with_token(self) -> None: def test_init_service_without_token(self) -> None: - with self.assertRaises(Exception): + with self.assertRaises(builtins.BaseException): github_services.init_service() def test_init_service_with_empty_token(self) -> None: - with self.assertRaises(Exception): + with self.assertRaises(builtins.BaseException): github_services.init_service('') @@ -63,45 +64,33 @@ def setUp(self) -> None: self.repo_name = 'repo' self.discussion_category = 'category' self.discussion_title = 'title' - # Here we use type Any because this response is hard to annotate in a typedDict. - self.response_for_get_discussion_data: Dict[str, Any] = { + self.response_for_get_categories = { + "data": { + "repository": { + "discussionCategories": { + "nodes": [ + { + "id": "test_category_id_1", + "name": "test_category_name_1" + }, + { + "id": "test_category_id_2", + "name": "test_category_name_2" + } + ] + } + } + } + } + self.response_for_get_discussion = { 'data': { 'repository': { - 'discussionCategories': { + 'discussions': { 'nodes': [ { - 'id': 'test_category_id_1', - 'name': 'test_category_name_1', - 'repository': { - 'discussions': { - 'edges': [ - { - 'node': { - 'id': 'test_discussion_id_1', - 'title': 'test_discussion_title_1', - 'number': 1 - } - } - ] - } - } - }, - { - 'id': 'test_category_id_2', - 'name': 'test_category_name_2', - 'repository': { - 'discussions': { - 'edges': [ - { - 'node': { - 'id': 'test_discussion_id_2', - 'title': 'test_discussion_title_2', - 'number': 2 - } - } - ] - } - } + 'id': 'test_discussion_id_1', + 'title': 'test_discussion_title_1', + 'number': 12345 } ] } @@ -231,6 +220,14 @@ def mock_all_get_requests(self, mock_request: requests_mock.Mocker) -> None: self.org_name, self.repo_name, 234) + param_page_2, text=json.dumps([])) + # Here we use type Any because this response is hard to annotate in a typedDict. + def mock_post_requests(self, response: Dict[str, Any]) -> mock.Mock: + """Mock post requests.""" + + mocked_response = mock.Mock() + mocked_response.json.return_value = response + return mocked_response + def test_get_pull_request_object_from_dict(self) -> None: token = 'my_github_token' @@ -297,15 +294,23 @@ def test_get_prs_assigned_to_reviewers(self) -> None: def test_get_discussion_data(self) -> None: """Test _get_discussion_data.""" - mock_response = mock.Mock() - mock_response.json.return_value = self.response_for_get_discussion_data - self.assertTrue(mock_response.assert_not_called) + mock_response_for_get_categories = self.mock_post_requests( + self.response_for_get_categories) + mock_response_for_get_discussion = self.mock_post_requests( + self.response_for_get_discussion) + + self.assertTrue(mock_response_for_get_categories.assert_not_called) + self.assertTrue(mock_response_for_get_discussion.assert_not_called) + mock_response = [ + mock_response_for_get_categories, + mock_response_for_get_discussion + ] with requests_mock.Mocker() as mock_requests: self.mock_all_get_requests(mock_requests) - with mock.patch('requests.post', side_effect=[mock_response]) as mock_post: + with mock.patch('requests.post', side_effect=mock_response) as mock_post: mocked_response = github_services._get_discussion_data( self.org_name, @@ -313,9 +318,10 @@ def test_get_discussion_data(self) -> None: 'test_category_name_1', 'test_discussion_title_1' ) - self.assertTrue(mock_response.assert_called_once) - self.assertEqual(mock_post.call_count, 1) - self.assertEqual(mocked_response, ('test_discussion_id_1', 1)) + self.assertTrue(mock_response_for_get_categories.assert_called_once) + self.assertTrue(mock_response_for_get_discussion.assert_called_once) + self.assertEqual(mock_post.call_count, 2) + self.assertEqual(mocked_response, ('test_discussion_id_1', 12345)) def test_get_old_comment_ids(self) -> None: """Test _get_old_comment_ids.""" @@ -333,7 +339,7 @@ def test_get_old_comment_ids(self) -> None: mocked_response = github_services._get_old_comment_ids( self.org_name, self.repo_name, - 1 + 12345 ) self.assertTrue(mock_response.assert_called_once) self.assertEqual(mock_post.call_count, 1) @@ -386,24 +392,28 @@ def test_delete_discussion_comments(self) -> None: github_services.init_service(token) mock_response_1 = mock.Mock() - mock_response_1.json.return_value = self.response_for_get_discussion_data + mock_response_1.json.return_value = self.response_for_get_categories mock_response_2 = mock.Mock() - mock_response_2.json.return_value = self.response_for_get_old_comment_ids + mock_response_2.json.return_value = self.response_for_get_discussion mock_response_3 = mock.Mock() - mock_response_3.json.return_value = self.response_for_delete_comment + mock_response_3.json.return_value = self.response_for_get_old_comment_ids + + mock_response_4 = mock.Mock() + mock_response_4.json.return_value = self.response_for_delete_comment self.assertTrue(mock_response_1.assert_not_called) self.assertTrue(mock_response_2.assert_not_called) self.assertTrue(mock_response_3.assert_not_called) + self.assertTrue(mock_response_4.assert_not_called) with requests_mock.Mocker() as mock_requests: self.mock_all_get_requests(mock_requests) with mock.patch('requests.post', side_effect=[ - mock_response_1, mock_response_2, mock_response_3]) as mock_post: + mock_response_1, mock_response_2, mock_response_3, mock_response_4]) as mock_post: github_services.delete_discussion_comments( self.org_name, @@ -414,7 +424,8 @@ def test_delete_discussion_comments(self) -> None: self.assertTrue(mock_response_1.assert_called) self.assertTrue(mock_response_2.assert_called) self.assertTrue(mock_response_3.assert_called) - self.assertEqual(mock_post.call_count, 3) + self.assertTrue(mock_response_4.assert_called) + self.assertEqual(mock_post.call_count, 4) def test_add_discussion_comments(self) -> None: """Test discussion comments.""" @@ -423,20 +434,24 @@ def test_add_discussion_comments(self) -> None: github_services.init_service(token) mock_response_1 = mock.Mock() - mock_response_1.json.return_value = self.response_for_get_discussion_data + mock_response_1.json.return_value = self.response_for_get_categories mock_response_2 = mock.Mock() - mock_response_2.json.return_value = self.response_for_post_comment + mock_response_2.json.return_value = self.response_for_get_discussion + + mock_response_3 = mock.Mock() + mock_response_3.json.return_value = self.response_for_post_comment self.assertTrue(mock_response_1.assert_not_called) self.assertTrue(mock_response_2.assert_not_called) + self.assertTrue(mock_response_3.assert_not_called) with requests_mock.Mocker() as mock_requests: self.mock_all_get_requests(mock_requests) with mock.patch('requests.post', side_effect=[ - mock_response_1, mock_response_2]) as mock_post: + mock_response_1, mock_response_2, mock_response_3]) as mock_post: github_services.add_discussion_comments( self.org_name, @@ -447,4 +462,5 @@ def test_add_discussion_comments(self) -> None: ) self.assertTrue(mock_response_1.assert_called) self.assertTrue(mock_response_2.assert_called) - self.assertEqual(mock_post.call_count, 2) + self.assertTrue(mock_response_3.assert_called) + self.assertEqual(mock_post.call_count, 3) diff --git a/tests/main_test.py b/tests/main_test.py index 67d3d21..fbdeb13 100644 --- a/tests/main_test.py +++ b/tests/main_test.py @@ -16,6 +16,7 @@ from __future__ import annotations +import builtins import datetime import json import unittest @@ -60,7 +61,7 @@ def test_generate_message_raises_template_not_found_error(self) -> None: '- [#123](https://githuburl.pull/123) [Waiting for the last 2 days, 8 ' 'hours]') with self.assertRaisesRegex( - Exception, f'Please add a template file at: {template_path}'): + builtins.BaseException, f'Please add a template file at: {template_path}'): main.generate_message('reviewerName1', pr_list, template_path) @@ -79,45 +80,33 @@ def setUp(self) -> None: self.repo_name = 'repo' self.discussion_category = 'category' self.discussion_title = 'title' - # Here we use type Any because this response is hard to annotate in a typedDict. - self.response_for_get_discussion_data: Dict[str, Any] = { + self.response_for_get_categories = { + "data": { + "repository": { + "discussionCategories": { + "nodes": [ + { + "id": "test_category_id_1", + "name": "test_category_name_1" + }, + { + "id": "test_category_id_2", + "name": "test_category_name_2" + } + ] + } + } + } + } + self.response_for_get_discussion = { 'data': { 'repository': { - 'discussionCategories': { + 'discussions': { 'nodes': [ { - 'id': 'test_category_id_1', - 'name': 'test_category_name_1', - 'repository': { - 'discussions': { - 'edges': [ - { - 'node': { - 'id': 'test_discussion_id_1', - 'title': 'test_discussion_title_1', - 'number': 1 - } - } - ] - } - } - }, - { - 'id': 'test_category_id_2', - 'name': 'test_category_name_2', - 'repository': { - 'discussions': { - 'edges': [ - { - 'node': { - 'id': 'test_discussion_id_2', - 'title': 'test_discussion_title_2', - 'number': 2 - } - } - ] - } - } + 'id': 'test_discussion_id_1', + 'title': 'test_discussion_title_1', + 'number': 12345 } ] } @@ -263,12 +252,15 @@ def test_executing_main_function_sends_notification(self) -> None: # Here we are mocking the POST requests that we will use in the test below. # and they are listed in the particular order they will be called. post_requests_side_effect: List[mock.Mock] = [ - self.mock_post_requests(self.response_for_get_discussion_data), + self.mock_post_requests(self.response_for_get_categories), + self.mock_post_requests(self.response_for_get_discussion), self.mock_post_requests(self.response_for_get_old_comment_ids), self.mock_post_requests(self.response_for_delete_comment), - self.mock_post_requests(self.response_for_get_discussion_data), + self.mock_post_requests(self.response_for_get_categories), + self.mock_post_requests(self.response_for_get_discussion), self.mock_post_requests(self.response_for_post_comment), - self.mock_post_requests(self.response_for_get_discussion_data), + self.mock_post_requests(self.response_for_get_categories), + self.mock_post_requests(self.response_for_get_discussion), self.mock_post_requests(self.response_for_post_comment) ] @@ -299,7 +291,10 @@ def test_executing_main_function_sends_notification(self) -> None: '--token', 'githubTokenForApiRequest' ]) - response_for_get_discussion_data = requests.post( + response_for_get_categories = requests.post( + github_services.GITHUB_GRAPHQL_URL, timeout=( + github_services.TIMEOUT_SECS)) + response_for_get_discussion = requests.post( github_services.GITHUB_GRAPHQL_URL, timeout=( github_services.TIMEOUT_SECS)) response_for_get_old_comment_ids = requests.post( @@ -308,21 +303,29 @@ def test_executing_main_function_sends_notification(self) -> None: response_for_delete_comment = requests.post( github_services.GITHUB_GRAPHQL_URL, timeout=( github_services.TIMEOUT_SECS)) - response_for_get_discussion_data = requests.post( + response_for_get_categories = requests.post( + github_services.GITHUB_GRAPHQL_URL, timeout=( + github_services.TIMEOUT_SECS)) + response_for_get_discussion = requests.post( github_services.GITHUB_GRAPHQL_URL, timeout=( github_services.TIMEOUT_SECS)) response_for_post_comment = requests.post( github_services.GITHUB_GRAPHQL_URL, timeout=( github_services.TIMEOUT_SECS)) - self.assertEqual(mock_post.call_count, 12) + self.assertEqual(mock_post.call_count, 17) self.assertEqual(mock_request.call_count, 6) # Here we use MyPy ignore because the response is of Mock type and # Mock does not contain return_value attribute, so because of this MyPy throws an # error. Thus to avoid the error, we used ignore here. self.assertEqual( - response_for_get_discussion_data.json.return_value, self.response_for_get_discussion_data) # type: ignore[attr-defined] + response_for_get_categories.json.return_value, self.response_for_get_categories) # type: ignore[attr-defined] + # Here we use MyPy ignore because the response is of Mock type and + # Mock does not contain return_value attribute, so because of this MyPy throws an + # error. Thus to avoid the error, we used ignore here. + self.assertEqual( + response_for_get_discussion.json.return_value, self.response_for_get_discussion) # type: ignore[attr-defined] # Here we use MyPy ignore because the response is of Mock type and # Mock does not contain return_value attribute, so because of this MyPy throws an # error. Thus to avoid the error, we used ignore here. @@ -337,7 +340,12 @@ def test_executing_main_function_sends_notification(self) -> None: # Mock does not contain return_value attribute, so because of this MyPy throws an # error. Thus to avoid the error, we used ignore here. self.assertEqual( - response_for_get_discussion_data.json.return_value, self.response_for_get_discussion_data) # type: ignore[attr-defined] + response_for_get_categories.json.return_value, self.response_for_get_categories) # type: ignore[attr-defined] + # Here we use MyPy ignore because the response is of Mock type and + # Mock does not contain return_value attribute, so because of this MyPy throws an + # error. Thus to avoid the error, we used ignore here. + self.assertEqual( + response_for_get_discussion.json.return_value, self.response_for_get_discussion) # type: ignore[attr-defined] # Here we use MyPy ignore because the response is of Mock type and # Mock does not contain return_value attribute, so because of this MyPy throws an # error. Thus to avoid the error, we used ignore here.