ansible-test - Improve AZP commit API error handling (#86197)

test/lib/ansible_test/_internal/ci/azp.py

@@ -2,6 +2,7 @@
 
 from __future__ import annotations
 
+import json
 import os
 import tempfile
 import uuid
@@ -27,6 +28,7 @@ from ..http import (
 
 from ..util import (
     display,
+    ApplicationError,
     MissingEnvironmentVariable,
 )
 
@@ -220,6 +222,19 @@ class AzurePipelinesChanges:
             self.diff = []
 
     def get_successful_merge_run_commits(self) -> set[str]:
+        """
+        Return a set of recent successful merge commits from Azure Pipelines.
+        A warning will be displayed and no commits returned if an error occurs.
+        """
+        try:
+            commits = self._get_successful_merge_run_commits()
+        except ApplicationError as ex:
+            commits = set()
+            display.warning(f'Cannot determine changes. All tests will be executed. Reason: {ex}')
+
+        return commits
+
+    def _get_successful_merge_run_commits(self) -> set[str]:
         """Return a set of recent successful merge commits from Azure Pipelines."""
         parameters = dict(
             maxBuildsPerDefinition=100,  # max 5000
@@ -230,20 +245,29 @@ class AzurePipelinesChanges:
             repositoryId='%s/%s' % (self.org, self.project),
         )
 
-        url = '%s%s/_apis/build/builds?api-version=6.0&%s' % (self.org_uri, self.project, urllib.parse.urlencode(parameters))
+        url = '%s%s/_apis/build/builds?api-version=7.1&%s' % (self.org_uri, self.project, urllib.parse.urlencode(parameters))
 
         http = HttpClient(self.args, always=True)
         response = http.get(url)
 
-        # noinspection PyBroadException
         try:
-            result = response.json()
-        except Exception:  # pylint: disable=broad-except
-            # most likely due to a private project, which returns an HTTP 203 response with HTML
-            display.warning('Unable to find project. Cannot determine changes. All tests will be executed.')
-            return set()
+            result = json.loads(response.response)
+            result_type = 'JSON'
+        except json.JSONDecodeError:
+            result = ...
+            result_type = 'Non-JSON'
 
-        commits = set(build['sourceVersion'] for build in result['value'])
+        result_description = f'HTTP {response.status_code} {result_type} result'
+
+        if response.status_code != 200 or result is ...:
+            raise ApplicationError(f'Unable to find project due to {result_description}.')
+
+        try:
+            commits = {build['sourceVersion'] for build in result['value']}
+        except KeyError as ex:
+            raise ApplicationError(f'Missing {ex.args[0]!r} key in response from {result_description}.') from ex
+        except (ValueError, TypeError) as ex:
+            raise ApplicationError(f'Unexpected response format from {result_description}: {ex}') from ex
 
         return commits
 

test/units/ansible_test/_internal/ci/test_azp.py

@@ -0,0 +1,96 @@
+from __future__ import annotations
+
+import argparse
+import json
+import os
+import typing as t
+
+import pytest
+import pytest_mock
+
+if t.TYPE_CHECKING:
+    from ansible_test._internal.ci.azp import AzurePipelinesChanges
+
+
+def create_azure_pipelines_changes(mocker: pytest_mock.MockerFixture) -> AzurePipelinesChanges:
+    """Prepare an AzurePipelinesChanges instance for testing."""
+    from ansible_test._internal.ci.azp import AzurePipelinesChanges
+    from ansible_test._internal.config import CommonConfig
+
+    namespace = argparse.Namespace()
+    namespace.color = False
+    namespace.explain = False
+    namespace.verbosity = False
+    namespace.debug = False
+    namespace.truncate = False
+    namespace.redact = False
+    namespace.display_traceback = False
+
+    config = CommonConfig(namespace, 'sanity')
+
+    env = dict(
+        HOME=os.environ['HOME'],
+        SYSTEM_COLLECTIONURI='https://dev.azure.com/ansible/',
+        SYSTEM_TEAMPROJECT='ansible',
+        BUILD_REPOSITORY_PROVIDER='GitHub',
+        BUILD_SOURCEBRANCH='devel',
+        BUILD_SOURCEBRANCHNAME='devel',
+    )
+
+    mocker.patch.dict(os.environ, env, clear=True)
+
+    return AzurePipelinesChanges(config)
+
+
+@pytest.mark.parametrize("status_code,response,expected_commits,expected_warning", (
+    # valid 200 responses
+    (200, dict(value=[]), None, None),
+    (200, dict(value=[dict(sourceVersion='abc')]), {'abc'}, None),
+    # invalid 200 responses
+    (200, 'not-json', None, "Unable to find project due to HTTP 200 Non-JSON result."),
+    (200, '"not-a-dict"', None, "Unexpected response format from HTTP 200 JSON result: string indices must be integers, not 'str'"),
+    (200, dict(value='not-a-list'), None, "Unexpected response format from HTTP 200 JSON result: string indices must be integers, not 'str'"),
+    (200, dict(value=['not-a-dict']), None, "Unexpected response format from HTTP 200 JSON result: string indices must be integers, not 'str'"),
+    (200, dict(), None, "Missing 'value' key in response from HTTP 200 JSON result."),
+    (200, dict(value=[{}]), None, "Missing 'sourceVersion' key in response from HTTP 200 JSON result."),
+    # non-200 responses
+    (404, '', None, "Unable to find project due to HTTP 404 Non-JSON result."),
+    (404, '""', None, "Unable to find project due to HTTP 404 JSON result."),
+    (404, dict(value=[]), None, "Unable to find project due to HTTP 404 JSON result."),
+))
+def test_get_successful_merge_run_commits(
+    status_code: int,
+    response: object,
+    expected_commits: set[str] | None,
+    expected_warning: str | None,
+    mocker: pytest_mock.MockerFixture,
+) -> None:
+    """Verify AZP commit retrieval handles invalid responses gracefully."""
+    from ansible_test._internal.ci.azp import AzurePipelinesChanges
+    from ansible_test._internal.git import Git
+    from ansible_test._internal.http import HttpClient, HttpResponse
+    from ansible_test._internal.util import display
+
+    if not isinstance(response, str):
+        response = json.dumps(response)
+
+    if expected_warning:
+        expected_warning = f'Cannot determine changes. All tests will be executed. Reason: {expected_warning}'
+
+    patched_get = mocker.patch.object(HttpClient, 'get', return_value=HttpResponse('GET', 'URL', status_code, response))
+    patched_warning = mocker.patch.object(display, 'warning')
+
+    mocker.patch.object(Git, 'run_git', return_value='')  # avoid git
+
+    spy_get_successful_merge_run_commits = mocker.spy(AzurePipelinesChanges, 'get_successful_merge_run_commits')
+
+    create_azure_pipelines_changes(mocker)
+
+    assert patched_get.call_count == 1
+
+    if expected_warning:
+        patched_warning.assert_called_once_with(expected_warning)
+    else:
+        patched_warning.assert_not_called()
+
+    assert spy_get_successful_merge_run_commits.spy_return == (expected_commits or set())