Fixing retry bug revealed by unit test

apps/cli/executables/pexable/productfetcher/productfetcher/exceptions.py

@@ -27,7 +27,7 @@ import traceback
 from pathlib import Path
 
 
-class FetchError(Exception):
+class FetchError(BaseException):
     """
     Some sort of problem has occurred, and the program must end.
     """
@@ -54,7 +54,7 @@ class RetryableFetchError(FetchError):
         super().terminate_fetch()
 
 
-class FileValidationFault(FetchError):
+class FileValidationFault(RetryableFetchError):
     """
     A file has failed to validate for some reason. This is not a retryable error.
     """

apps/cli/executables/pexable/productfetcher/productfetcher/fetchers.py

@@ -247,7 +247,7 @@ class RetryableFileFetcher(FileFetcherDecorator):
         self.retries = retries
 
     def do_fetch(self) -> Path:
-        return retry(self.underlying.do_fetch, [2**attempt for attempt in range(1, 4)])
+        return retry(self.underlying.do_fetch, [2**attempt for attempt in range(1, self.retries)])
 
     def __str__(self):
         return f"{self.underlying} (with up to {self.retries} retries)"

apps/cli/executables/pexable/productfetcher/productfetcher/retry.py

@@ -18,6 +18,8 @@
 import time
 from typing import Callable, List, TypeVar
 
+from .exceptions import RetryableFetchError
+
 T = TypeVar("T")
 
 
@@ -38,7 +40,7 @@ def retry(f: Callable[[], T], backoff_times: List[int] = None, retry_number=1) -
     exception = None
     try:
         return f()
-    except Exception as ex:
+    except RetryableFetchError as ex:
         if backoff_times:
             # if we have, say, [1, 4, 9] in our list of backoff times,
             # we will delay for 1 second and then have [4, 9] remaining backoff times
@@ -54,4 +56,4 @@ def retry(f: Callable[[], T], backoff_times: List[int] = None, retry_number=1) -
                 ex.retries = retry_number
 
             # now raise the exception
-            raise exception
+            raise ex

apps/cli/executables/pexable/productfetcher/tests/test_fetchers.py

@@ -187,8 +187,8 @@ def test_retrying_succeeds(capsys):
 
 def test_retrying_fails(capsys):
     # three failures is too many
-    fail_twice = MagicMock(wraps=FailNTimesFileFetcher(3))
+    fail_thrice = MagicMock(wraps=FailNTimesFileFetcher(3))
     with patch("time.sleep"):
         with pytest.raises(RetryableFetchError):
-            RetryableFileFetcher(fail_twice).do_fetch()
+            RetryableFileFetcher(fail_thrice).do_fetch()
     capsys.readouterr()

apps/cli/executables/pexable/productfetcher/tests/test_retry.py

+#
+# Copyright (C) 2021 Associated Universities, Inc. Washington DC, USA.
+#
+# This file is part of NRAO Workspaces.
+#
+# Workspaces is free software: you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation, either version 3 of the License, or
+# (at your option) any later version.
+#
+# Workspaces is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with Workspaces.  If not, see <https://www.gnu.org/licenses/>.
+from unittest.mock import MagicMock, call, patch
+
+import pytest
+from productfetcher.exceptions import RetryableFetchError
+from productfetcher.retry import retry
+
+
+class FailNTimes:
+    def __init__(self, n):
+        self.total = n
+        self.current = 1
+
+    def __call__(self, *args, **kwargs):
+        if self.current <= self.total:
+            self.current += 1
+            raise RetryableFetchError(f"Failure #{self.current-1}")
+
+
+def test_fail_n_times():
+    x = FailNTimes(2)
+    with pytest.raises(RetryableFetchError):
+        x()
+    with pytest.raises(RetryableFetchError):
+        x()
+    x()
+
+
+def test_retrying_succeeds(capsys):
+    # two failures is OK
+    with patch("time.sleep") as sleep:
+        fail_twice = MagicMock(wraps=FailNTimes(2))
+        retry(fail_twice, [3, 7])
+        assert sleep.call_count == 2
+        assert sleep.call_args_list == [call(3), call(7)]
+    capsys.readouterr()
+
+
+def test_retrying_fails(capsys):
+    # three failures is too many
+    with patch("time.sleep"):
+        fail_thrice = MagicMock(wraps=FailNTimes(3))
+        with pytest.raises(RetryableFetchError):
+            retry(fail_thrice, [3, 7])
+    capsys.readouterr()