From 88c9948fbfd6fbfab5d9e769dbb90ce14c66e94a Mon Sep 17 00:00:00 2001
From: Daniel K Lyons <dlyons@nrao.edu>
Date: Wed, 14 Sep 2022 10:46:50 -0600
Subject: [PATCH] Fixing retry bug revealed by unit test
---
.../productfetcher/exceptions.py | 4 +-
.../productfetcher/productfetcher/fetchers.py | 2 +-
.../productfetcher/productfetcher/retry.py | 6 +-
.../productfetcher/tests/test_fetchers.py | 4 +-
.../productfetcher/tests/test_retry.py | 61 +++++++++++++++++++
5 files changed, 70 insertions(+), 7 deletions(-)
create mode 100644 apps/cli/executables/pexable/productfetcher/tests/test_retry.py
diff --git a/apps/cli/executables/pexable/productfetcher/productfetcher/exceptions.py b/apps/cli/executables/pexable/productfetcher/productfetcher/exceptions.py
index c3117a5dc..23f22f68e 100644
--- a/apps/cli/executables/pexable/productfetcher/productfetcher/exceptions.py
+++ b/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.
"""
diff --git a/apps/cli/executables/pexable/productfetcher/productfetcher/fetchers.py b/apps/cli/executables/pexable/productfetcher/productfetcher/fetchers.py
index 4f6d7af51..7389a1c9d 100644
--- a/apps/cli/executables/pexable/productfetcher/productfetcher/fetchers.py
+++ b/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)"
diff --git a/apps/cli/executables/pexable/productfetcher/productfetcher/retry.py b/apps/cli/executables/pexable/productfetcher/productfetcher/retry.py
index 97fc0c2ce..23b4113ea 100644
--- a/apps/cli/executables/pexable/productfetcher/productfetcher/retry.py
+++ b/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
diff --git a/apps/cli/executables/pexable/productfetcher/tests/test_fetchers.py b/apps/cli/executables/pexable/productfetcher/tests/test_fetchers.py
index 9d38ae00a..bcbb6ebe9 100644
--- a/apps/cli/executables/pexable/productfetcher/tests/test_fetchers.py
+++ b/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()
diff --git a/apps/cli/executables/pexable/productfetcher/tests/test_retry.py b/apps/cli/executables/pexable/productfetcher/tests/test_retry.py
new file mode 100644
index 000000000..86a7b82c3
--- /dev/null
+++ b/apps/cli/executables/pexable/productfetcher/tests/test_retry.py
@@ -0,0 +1,61 @@
+#
+# 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()
--
GitLab