From f52007412162dd5e3347653daba9ef5eea531012 Mon Sep 17 00:00:00 2001
From: "Emmanuel I. Obi" <withtwoemms@gmail.com>
Date: Mon, 31 Oct 2022 22:23:52 -0500
Subject: [PATCH] actionpack#148: "address RetryPolicy ergonomic issues" (#149)

* Action reaction guaranteed to execute

* uses generic TypeVar in partialaction type hinting

* updates repr; adds typecheck for RetryPolicy max_retries
---
 actionpack/action.py                          |  3 ++-
 actionpack/actions/retry_policy.py            |  7 +++--
 tests/actionpack/actions/test_retry_policy.py | 18 +++++++++++--
 tests/actionpack/test_action.py               | 26 +++++++++++++++++++
 4 files changed, 49 insertions(+), 5 deletions(-)

diff --git a/actionpack/action.py b/actionpack/action.py
index 2b80d2e..83b757e 100644
--- a/actionpack/action.py
+++ b/actionpack/action.py
@@ -87,7 +87,7 @@ def instruction():
         return instance
 
 
-def partialaction(name, parent: ActionType, **kwargs) -> ActionType:
+def partialaction(name, parent: T, **kwargs) -> T:
     partial__init__ = partialmethod(parent.__init__, **kwargs)
     return type(name, (parent,), {'__init__': partial__init__})
 
@@ -150,6 +150,7 @@ def _perform(
                 if should_raise:
                     raise e
                 outcome = Left(e)
+            finally:
                 if self._ActionType__reaction:
                     self._ActionType__reaction.perform(should_raise=should_raise)
 
diff --git a/actionpack/actions/retry_policy.py b/actionpack/actions/retry_policy.py
index 31a5a61..2f8cd84 100644
--- a/actionpack/actions/retry_policy.py
+++ b/actionpack/actions/retry_policy.py
@@ -17,6 +17,9 @@ def __init__(
         delay_between_attempts: int = 0,
         should_record: bool = False
     ):
+        if not isinstance(max_retries, int) or max_retries < 0:
+            raise self.Invalid(f'The number of max_retries must be greater than zero. Given max_retries={max_retries}.')
+
         self.action = action
         self.max_retries = max_retries
         self.delay_between_attempts = delay_between_attempts
@@ -70,10 +73,10 @@ def enact(self, with_delay: int = 0, counter: int = -1) -> Outcome:
         raise RetryPolicy.Expired(f'Max retries exceeded: {self.max_retries}.')
 
     def __repr__(self):
-        tmpl = Template('<$class_name($max_retries x $action_name$delay)>')
+        tmpl = Template('<$class_name($total_attempts x $action_name$delay)>')
         return tmpl.substitute(
             class_name=self.__class__.__name__,
-            max_retries=self.max_retries,
+            total_attempts=self.max_retries + 1,
             action_name=str(self.action),
             delay='' if not self.delay_between_attempts else f' | {self.delay_between_attempts}s delay'
         )
diff --git a/tests/actionpack/actions/test_retry_policy.py b/tests/actionpack/actions/test_retry_policy.py
index c53e40d..86f5af4 100644
--- a/tests/actionpack/actions/test_retry_policy.py
+++ b/tests/actionpack/actions/test_retry_policy.py
@@ -5,6 +5,7 @@
 from unittest.mock import ANY
 from unittest.mock import patch
 
+from actionpack import Action
 from actionpack.action import Result
 from actionpack.actions import MakeRequest
 from actionpack.actions import RetryPolicy
@@ -132,8 +133,21 @@ def test_delay_is_bypassed_after_expiration(self, mock_session_send):
         result = action.perform(timestamp_provider=timestamp_provider)
         assert result.produced_at < timestamp_provider() + delay
 
-    def test_can_serialize(self):
-        self.assertEqual(repr(self.action), '<RetryPolicy(2 x <MakeRequest>)>')
+    def test_instantiation_fails_given_invalid_max_retries(self):
+        action = RetryPolicy[str, str](action=MakeRequest('GET', 'http://localhost'), max_retries=-1)
+        self.assertIsInstance(action, Action.Construct)
+        result = action.perform()
+        self.assertFalse(result.successful)
+        self.assertIsInstance(result.value, RetryPolicy.Invalid)
+
+    @patch('requests.Session.send')
+    def test_can_serialize(self, mock_session_send):
+        action = RetryPolicy[str, str](
+            action=MakeRequest('GET', 'http://localhost'),
+            max_retries=0,
+        )
+        self.assertEqual(repr(action), '<RetryPolicy(1 x <MakeRequest>)>')
+        self.assertEqual(repr(self.action), '<RetryPolicy(3 x <MakeRequest>)>')
 
     @patch('requests.Session.send')
     def test_can_pickle(self, mock_session_send):
diff --git a/tests/actionpack/test_action.py b/tests/actionpack/test_action.py
index 9416120..5147d10 100644
--- a/tests/actionpack/test_action.py
+++ b/tests/actionpack/test_action.py
@@ -5,6 +5,7 @@
 from oslash import Right
 from threading import Thread
 from unittest import TestCase
+from unittest.mock import patch
 
 from actionpack import Action
 from actionpack import partialaction
@@ -85,6 +86,31 @@ def fill():
         self.assertFalse(result.successful)
         self.assertIn(contents, vessel)
 
+    @patch('oslash.Left.__init__')
+    def test_can_react_to_failure_during_catastrophe(self, mock_wrapper):
+        def raise_another_failure(e):
+            raise e
+
+        mock_wrapper.side_effect = raise_another_failure
+
+        vessel = []
+        contents = 'contents'
+
+        def fill():
+            vessel.append(contents)
+
+        reaction = FakeAction(instruction_provider=fill)
+        action = FakeAction(
+            instruction_provider=self.raise_failure,
+            reaction=reaction
+        )
+
+        with self.assertRaises(Exception):
+            result = action.perform()
+            self.assertFalse(result.successful)
+
+        self.assertIn(contents, vessel)
+
     def test_Action_Construct(self):
         construct = FakeAction(typecheck='Action instantiation fails.')
         result = construct.perform()