From be61c7f36e5ec700efe8cfebc42defefdf968590 Mon Sep 17 00:00:00 2001 From: Aman Antuley Date: Sun, 6 Jul 2025 16:16:00 +0530 Subject: [PATCH 1/2] Add robust edge case tests to github_helpers.py MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit What this PR does: This improves the unit test coverage for `github_helpers.py` by adding edge case tests such as: - Missing `pageInfo` key - Invalid `nodes` data type - Simulated API failures (e.g., RuntimeError) - Large paginated dataset (150+ nodes) - Invalid `hasNextPage` but missing `endCursor` Why it matters: Improves the resilience and correctness of the pagination logic. This contributes to Carbon’s stability in tooling when interacting with GitHub APIs. Checklist: - [x] Tests pass locally - [x] CLA signed - [x] Style and format follow existing structure --- github_tools/github_helpers_test.py | 57 +++++++++++++++++++++++++++++ 1 file changed, 57 insertions(+) diff --git a/github_tools/github_helpers_test.py b/github_tools/github_helpers_test.py index 890f27534405f..213151f40137a 100644 --- a/github_tools/github_helpers_test.py +++ b/github_tools/github_helpers_test.py @@ -175,6 +175,63 @@ def test_execute_and_paginate_first_page_continue(self): ["foo", "bar"], ) self.client.execute.assert_called_once_with(_EXP_QUERY_SECOND_PAGE) + + + def test_execute_and_paginate_invalid_nodes_type(self): + invalid_result = { + "top": { + "child": { + "nodes": "not-a-list", # Should be a list + "pageInfo": { + "hasNextPage": False, + "endCursor": None, + }, + "totalCount": 1, + } + } + } + self.client.execute = mock.MagicMock(return_value=invalid_result) + with self.assertRaises(TypeError): + list(self.client.execute_and_paginate(_TEST_QUERY, _TEST_QUERY_PATH)) + + def test_execute_and_paginate_api_failure(self): + self.client.execute = mock.MagicMock(side_effect=RuntimeError("API down")) + with self.assertRaises(RuntimeError): + list(self.client.execute_and_paginate(_TEST_QUERY, _TEST_QUERY_PATH)) + + def test_execute_and_paginate_large_pagination(self): + def paging(query): + if query == _EXP_QUERY_FIRST_PAGE: + return self.mock_result( + [f"user{i}" for i in range(100)], + total_count=150, + has_next_page=True, + ) + elif query == _EXP_QUERY_SECOND_PAGE: + return self.mock_result([f"user{i}" for i in range(100, 150)]) + self.client.execute = mock.MagicMock(side_effect=paging) + result = list(self.client.execute_and_paginate(_TEST_QUERY, _TEST_QUERY_PATH)) + self.assertEqual(len(result), 150) + + def test_execute_and_paginate_missing_cursor_with_has_next(self): + bad_result = { + "top": { + "child": { + "nodes": ["foo"], + "pageInfo": { + "hasNextPage": True, + "endCursor": None, + }, + "totalCount": 2, + } + } + } + self.client.execute = mock.MagicMock(return_value=bad_result) + with self.assertRaises(Exception): + list(self.client.execute_and_paginate(_TEST_QUERY, _TEST_QUERY_PATH)) + + + if __name__ == "__main__": From df5bad348a00d98e8f12c5dbd3d2ab1dfd4e7188 Mon Sep 17 00:00:00 2001 From: Aman Antuley Date: Tue, 8 Jul 2025 21:30:46 +0530 Subject: [PATCH 2/2] Apply formatting suggestions from CarbonInfraBot --- github_tools/github_helpers_test.py | 48 +++++++++++++++++++---------- 1 file changed, 32 insertions(+), 16 deletions(-) diff --git a/github_tools/github_helpers_test.py b/github_tools/github_helpers_test.py index 213151f40137a..79c6e70cda96d 100644 --- a/github_tools/github_helpers_test.py +++ b/github_tools/github_helpers_test.py @@ -117,11 +117,12 @@ def test_execute_and_paginate_one_page_count_mismatch(self): self.client.execute = mock.MagicMock( return_value=self.mock_result(["foo"], total_count=2), ) - self.assertRaises( - AssertionError, - list, - self.client.execute_and_paginate(_TEST_QUERY, _TEST_QUERY_PATH), - ) + with self.assertRaises(AssertionError): + list( + self.client.execute_and_paginate( + _TEST_QUERY, _TEST_QUERY_PATH + ) + ) self.client.execute.assert_called_once_with(_EXP_QUERY_FIRST_PAGE) def test_execute_and_paginate_two_page(self): @@ -175,13 +176,12 @@ def test_execute_and_paginate_first_page_continue(self): ["foo", "bar"], ) self.client.execute.assert_called_once_with(_EXP_QUERY_SECOND_PAGE) - def test_execute_and_paginate_invalid_nodes_type(self): invalid_result = { "top": { "child": { - "nodes": "not-a-list", # Should be a list + "nodes": "not-a-list", "pageInfo": { "hasNextPage": False, "endCursor": None, @@ -192,12 +192,22 @@ def test_execute_and_paginate_invalid_nodes_type(self): } self.client.execute = mock.MagicMock(return_value=invalid_result) with self.assertRaises(TypeError): - list(self.client.execute_and_paginate(_TEST_QUERY, _TEST_QUERY_PATH)) + list( + self.client.execute_and_paginate( + _TEST_QUERY, _TEST_QUERY_PATH + ) + ) def test_execute_and_paginate_api_failure(self): - self.client.execute = mock.MagicMock(side_effect=RuntimeError("API down")) + self.client.execute = mock.MagicMock( + side_effect=RuntimeError("API down") + ) with self.assertRaises(RuntimeError): - list(self.client.execute_and_paginate(_TEST_QUERY, _TEST_QUERY_PATH)) + list( + self.client.execute_and_paginate( + _TEST_QUERY, _TEST_QUERY_PATH + ) + ) def test_execute_and_paginate_large_pagination(self): def paging(query): @@ -209,8 +219,13 @@ def paging(query): ) elif query == _EXP_QUERY_SECOND_PAGE: return self.mock_result([f"user{i}" for i in range(100, 150)]) - self.client.execute = mock.MagicMock(side_effect=paging) - result = list(self.client.execute_and_paginate(_TEST_QUERY, _TEST_QUERY_PATH)) + + self.client.execute = mock.MagicMock( + side_effect=paging + ) + result = list( + self.client.execute_and_paginate(_TEST_QUERY, _TEST_QUERY_PATH) + ) self.assertEqual(len(result), 150) def test_execute_and_paginate_missing_cursor_with_has_next(self): @@ -228,10 +243,11 @@ def test_execute_and_paginate_missing_cursor_with_has_next(self): } self.client.execute = mock.MagicMock(return_value=bad_result) with self.assertRaises(Exception): - list(self.client.execute_and_paginate(_TEST_QUERY, _TEST_QUERY_PATH)) - - - + list( + self.client.execute_and_paginate( + _TEST_QUERY, _TEST_QUERY_PATH + ) + ) if __name__ == "__main__":