From 2537157b478dbc10b728d19a8d7e59eabca91f40 Mon Sep 17 00:00:00 2001 From: per1234 Date: Tue, 23 Jan 2024 11:00:58 -0800 Subject: [PATCH 1/3] Restrict HTTP request error handling to appropriate exception The "arduino/report-size-deltas" action relies on the GitHub REST API. It makes HTTP requests to various API endpoints during each workflow run. It is expected that these requests might occasionally produce an exception due to transient network outages. For this reason, the action has special handling of exceptions. Previously, that exception handling code was configured to be triggered on any exception, even though it is specific to HTTP request errors. It is more appropriate to target it only to the specific urllib.error.HTTPError exception that results from an HTTP request error, leaving any other types of unexpected exceptions unhandled. --- reportsizedeltas/reportsizedeltas.py | 4 ++-- reportsizedeltas/tests/test_reportsizedeltas.py | 6 ++++-- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/reportsizedeltas/reportsizedeltas.py b/reportsizedeltas/reportsizedeltas.py index 0aca1c4..23bbc35 100644 --- a/reportsizedeltas/reportsizedeltas.py +++ b/reportsizedeltas/reportsizedeltas.py @@ -637,7 +637,7 @@ def raw_http_request(self, url: str, data: bytes | None = None): if url.startswith("https://api.github.com") and not url.startswith("https://api.github.com/rate_limit"): self.handle_rate_limiting() return urllib.request.urlopen(url=request) - except Exception as exception: + except urllib.error.HTTPError as exception: if not determine_urlopen_retry(exception=exception): raise exception @@ -664,7 +664,7 @@ def handle_rate_limiting(self) -> None: sys.exit(0) -def determine_urlopen_retry(exception) -> bool: +def determine_urlopen_retry(exception: urllib.error.HTTPError) -> bool: """Determine whether the exception warrants another attempt at opening the URL. If so, delay then return True. Otherwise, return False. diff --git a/reportsizedeltas/tests/test_reportsizedeltas.py b/reportsizedeltas/tests/test_reportsizedeltas.py index 3c33450..0cfde7f 100644 --- a/reportsizedeltas/tests/test_reportsizedeltas.py +++ b/reportsizedeltas/tests/test_reportsizedeltas.py @@ -901,9 +901,11 @@ def test_raw_http_request(mocker): urllib.request.urlopen.assert_called_once_with(url=request) # urllib.request.urlopen() has non-recoverable exception - urllib.request.urlopen.side_effect = Exception() + urllib.request.urlopen.side_effect = urllib.error.HTTPError( + url="http://example.com", code=404, msg="", hdrs=None, fp=None + ) mocker.patch("reportsizedeltas.determine_urlopen_retry", autospec=True, return_value=False) - with pytest.raises(expected_exception=Exception): + with pytest.raises(expected_exception=urllib.error.HTTPError): report_size_deltas.raw_http_request(url=url, data=data) # urllib.request.urlopen() has potentially recoverable exceptions, but exceeds retry count From c353809debd9b88d162c9d7c7688533f3e69a429 Mon Sep 17 00:00:00 2001 From: per1234 Date: Tue, 23 Jan 2024 11:12:12 -0800 Subject: [PATCH 2/3] Increase visibility of error message when action fails due to HTTP request error The "arduino/report-size-deltas" action relies on the GitHub REST API. It makes HTTP requests to various API endpoints during each workflow run. These requests might produce an exception due to transient network outages, misconfiguration of the workflow, or a problem with the action. This will cause the action's step in the workflow run to fail. The maintainer of the repository in which the action is used may then need to investigate the cause of the failure. Previously, the cause of the failure could only be determined by evaluating the workflow run logs, which are somewhat unfriendly due to containing the Python stack trace produced by the HTTP request exception. The significant error message is hereby made visible as an annotation on the workflow run summary page through the use of an "error" workflow command. --- reportsizedeltas/reportsizedeltas.py | 19 +++++++++++++------ .../tests/test_reportsizedeltas.py | 2 +- 2 files changed, 14 insertions(+), 7 deletions(-) diff --git a/reportsizedeltas/reportsizedeltas.py b/reportsizedeltas/reportsizedeltas.py index 23bbc35..948dfb3 100644 --- a/reportsizedeltas/reportsizedeltas.py +++ b/reportsizedeltas/reportsizedeltas.py @@ -630,19 +630,26 @@ def raw_http_request(self, url: str, data: bytes | None = None): request = urllib.request.Request(url=url, headers=headers, data=data) retry_count = 0 - while retry_count <= maximum_urlopen_retries: - retry_count += 1 + while True: try: # The rate limit API is not subject to rate limiting if url.startswith("https://api.github.com") and not url.startswith("https://api.github.com/rate_limit"): self.handle_rate_limiting() return urllib.request.urlopen(url=request) except urllib.error.HTTPError as exception: - if not determine_urlopen_retry(exception=exception): - raise exception + if determine_urlopen_retry(exception=exception): + if retry_count < maximum_urlopen_retries: + retry_count += 1 + continue + else: + # Maximum retries reached without successfully opening URL + print("Maximum number of URL load retries exceeded") + + print(f"::error::{exception.__class__.__name__}: {exception}") + for line in exception.fp: + print(line.decode(encoding="utf-8", errors="ignore")) - # Maximum retries reached without successfully opening URL - raise TimeoutError("Maximum number of URL load retries exceeded") + raise exception def handle_rate_limiting(self) -> None: """Check whether the GitHub API request limit has been reached. diff --git a/reportsizedeltas/tests/test_reportsizedeltas.py b/reportsizedeltas/tests/test_reportsizedeltas.py index 0cfde7f..f56a387 100644 --- a/reportsizedeltas/tests/test_reportsizedeltas.py +++ b/reportsizedeltas/tests/test_reportsizedeltas.py @@ -910,7 +910,7 @@ def test_raw_http_request(mocker): # urllib.request.urlopen() has potentially recoverable exceptions, but exceeds retry count reportsizedeltas.determine_urlopen_retry.return_value = True - with pytest.raises(expected_exception=TimeoutError): + with pytest.raises(expected_exception=urllib.error.HTTPError): report_size_deltas.raw_http_request(url=url, data=data) From 0149266499df03a3c9aa30435529167372b4ca1f Mon Sep 17 00:00:00 2001 From: per1234 Date: Tue, 23 Jan 2024 11:40:17 -0800 Subject: [PATCH 3/3] Configure pytest to capture test output By default, pytest captures all output from the code during test execution. For unknown reasons, the creator of the original pytest configuration file which was reused for this project decided to disable that behavior. This resulted in any output from the code under test being shown in the test logs, making them very difficult to read and interpret. That configuration is hereby removed. After this change, script output from successful test runs will not be shown in the test logs. Since the output might be useful for troubleshooting test failures, it will be shown when a test fails, in a special section of the logs. --- pytest.ini | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/pytest.ini b/pytest.ini index 3dccb16..e31ea24 100644 --- a/pytest.ini +++ b/pytest.ini @@ -5,7 +5,6 @@ filterwarnings = ignore::DeprecationWarning ignore::ResourceWarning -# --capture=no - disable per-test capture # --tb=long sets the length of the traceback in case of failures -addopts = --capture=no --tb=long --verbose +addopts = --tb=long --verbose pythonpath = reportsizedeltas