refactor: Move the duplicated retry classifier out of the cogs into a shared utility #50

Author: rippyboii

bot/exts/filtering/filtering.py

@@ -56,6 +56,7 @@
 from bot.utils.channel import is_mod_channel
 from bot.utils.lock import lock_arg
 from bot.utils.message_cache import MessageCache
+from bot.utils.retry import is_retryable_api_error
 
 log = get_logger(__name__)
 
@@ -114,7 +115,7 @@ async def cog_load(self) -> None:
                 raw_filter_lists = await self.bot.api_client.get("bot/filter/filter_lists")
                 break
             except Exception as error:
-                is_retryable = self._retryable_filter_load_error(error)
+                is_retryable = is_retryable_api_error(error)
                 is_last_attempt = attempt == URLs.connect_max_retries
 
                 if not is_retryable:
@@ -147,14 +148,6 @@ async def cog_load(self) -> None:
         await self.schedule_offending_messages_deletion()
         self.weekly_auto_infraction_report_task.start()
 
-    @staticmethod
-    def _retryable_filter_load_error(error: Exception) -> bool:
-        """Return whether loading filter lists failed due to some temporary error, thus retrying could help."""
-        if isinstance(error, ResponseCodeError):
-            return error.status in (408, 429) or error.status >= 500
-
-        return isinstance(error, (TimeoutError, OSError))
-
     def subscribe(self, filter_list: FilterList, *events: Event) -> None:
         """
         Subscribe a filter list to the given events.

bot/exts/info/python_news.py

@@ -15,6 +15,7 @@
 from bot.bot import Bot
 from bot.constants import URLs
 from bot.log import get_logger
+from bot.utils.retry import is_retryable_api_error
 from bot.utils.webhooks import send_webhook
 
 PEPS_RSS_URL = "https://peps.python.org/peps.rss"
@@ -46,12 +47,6 @@ def __init__(self, bot: Bot):
         self.webhook: discord.Webhook | None = None
         self.seen_items: dict[str, set[str]] = {}
 
-    @staticmethod
-    def _retryable_site_load_error(error: Exception) -> bool:
-        if isinstance(error, ResponseCodeError):
-            return error.status in (408, 429) or error.status >= 500
-        return isinstance(error, (TimeoutError, OSError))
-
     async def cog_load(self) -> None:
         """Load all existing seen items from db and create any missing mailing lists."""
         for attempt in range(1, URLs.connect_max_retries + 1):
@@ -74,7 +69,7 @@ async def cog_load(self) -> None:
                 return
 
             except Exception as error:
-                if not self._retryable_site_load_error(error):
+                if not is_retryable_api_error(error):
                     raise
 
                 if attempt == URLs.connect_max_retries:

bot/utils/retry.py

@@ -0,0 +1,9 @@
+from pydis_core.site_api import ResponseCodeError
+
+
+def is_retryable_api_error(error: Exception) -> bool:
+    """Return whether an API error is temporary and worth retrying."""
+    if isinstance(error, ResponseCodeError):
+        return error.status in (408, 429) or error.status >= 500
+
+    return isinstance(error, (TimeoutError, OSError))

tests/bot/exts/filtering/test_filtering_cog.py

@@ -1,8 +1,6 @@
 import unittest
 from unittest.mock import AsyncMock, MagicMock, patch
 
-from pydis_core.site_api import ResponseCodeError
-
 from bot.exts.filtering.filtering import Filtering
 
 
@@ -77,22 +75,3 @@ async def test_retries_three_times_fails_and_reraises(self):
         self.cog._fetch_or_generate_filtering_webhook.assert_not_awaited()
         self.cog.schedule_offending_messages_deletion.assert_not_awaited()
         self.mock_weekly_task_start.assert_not_called()
-
-    def test_retryable_filter_load_error(self):
-        """`_retryable_filter_load_error` should classify temporary failures as retryable."""
-        test_cases = (
-            (ResponseCodeError(MagicMock(status=408)), True),
-            (ResponseCodeError(MagicMock(status=429)), True),
-            (ResponseCodeError(MagicMock(status=500)), True),
-            (ResponseCodeError(MagicMock(status=503)), True),
-            (ResponseCodeError(MagicMock(status=400)), False),
-            (ResponseCodeError(MagicMock(status=404)), False),
-            (TimeoutError("timeout"), True),
-            (OSError("os error"), True),
-            (AttributeError("attr"), False),
-            (ValueError("value"), False),
-        )
-
-        for error, expected_retryable in test_cases:
-            with self.subTest(error=error):
-                self.assertEqual(self.cog._retryable_filter_load_error(error), expected_retryable)

tests/bot/exts/info/test_python_news.py

@@ -84,25 +84,6 @@ async def test_retries_max_times_fails_and_reraises(self):
         # Task should never start if load fails.
         self.mock_fetch_new_media_start.assert_not_called()
 
-    def test_retryable_python_news_load_error(self):
-        """`_retryable_site_load_error` should classify temporary failures as retryable."""
-        test_cases = (
-            (ResponseCodeError(MagicMock(status=408)), True),
-            (ResponseCodeError(MagicMock(status=429)), True),
-            (ResponseCodeError(MagicMock(status=500)), True),
-            (ResponseCodeError(MagicMock(status=503)), True),
-            (ResponseCodeError(MagicMock(status=400)), False),
-            (ResponseCodeError(MagicMock(status=404)), False),
-            (TimeoutError("timeout"), True),
-            (OSError("os error"), True),
-            (AttributeError("attr"), False),
-            (ValueError("value"), False),
-        )
-
-        for error, expected_retryable in test_cases:
-            with self.subTest(error=error):
-                self.assertEqual(self.cog._retryable_site_load_error(error), expected_retryable)
-
     async def test_cog_load_does_not_retry_non_retryable_error(self):
         """`cog_load` should not retry when the error is non-retryable."""
         # 404 should be considered non-retryable by your predicate.

tests/bot/utils/test_retry.py

@@ -0,0 +1,29 @@
+import unittest
+from unittest.mock import MagicMock
+
+from pydis_core.site_api import ResponseCodeError
+
+from bot.utils.retry import is_retryable_api_error
+
+
+class RetryTests(unittest.TestCase):
+    """Tests for retry classification helpers."""
+
+    def test_is_retryable_api_error(self):
+        """`is_retryable_api_error` should classify temporary failures as retryable."""
+        test_cases = (
+            (ResponseCodeError(MagicMock(status=408)), True),
+            (ResponseCodeError(MagicMock(status=429)), True),
+            (ResponseCodeError(MagicMock(status=500)), True),
+            (ResponseCodeError(MagicMock(status=503)), True),
+            (ResponseCodeError(MagicMock(status=400)), False),
+            (ResponseCodeError(MagicMock(status=404)), False),
+            (TimeoutError("timeout"), True),
+            (OSError("os error"), True),
+            (AttributeError("attr"), False),
+            (ValueError("value"), False),
+        )
+
+        for error, expected_retryable in test_cases:
+            with self.subTest(error=error):
+                self.assertEqual(is_retryable_api_error(error), expected_retryable)