From 3d26431e4f9b2bada9307b3943a5179fc41f074a Mon Sep 17 00:00:00 2001 From: Matt Clay Date: Thu, 13 Nov 2025 19:59:26 -0800 Subject: [PATCH] ansible-test - Improve AZP commit API error handling (#86197) --- test/lib/ansible_test/_internal/ci/azp.py | 40 ++++++-- .../ansible_test/_internal/ci/test_azp.py | 96 +++++++++++++++++++ 2 files changed, 128 insertions(+), 8 deletions(-) create mode 100644 test/units/ansible_test/_internal/ci/test_azp.py diff --git a/test/lib/ansible_test/_internal/ci/azp.py b/test/lib/ansible_test/_internal/ci/azp.py index 556859dc586..4d8274da285 100644 --- a/test/lib/ansible_test/_internal/ci/azp.py +++ b/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 diff --git a/test/units/ansible_test/_internal/ci/test_azp.py b/test/units/ansible_test/_internal/ci/test_azp.py new file mode 100644 index 00000000000..359f00fa8b0 --- /dev/null +++ b/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())