From 599fc17dd32d9e2ffda0309d31be9e0b565d988e Mon Sep 17 00:00:00 2001 From: emil Date: Mon, 22 Jun 2026 01:22:45 +0000 Subject: [PATCH] GRM-51: fix: API resilience with retry, idempotent releases, and graceful Vikunja errors --- scripts/ci/auto_merge.py | 49 +++++- scripts/ci/post_merge.py | 12 +- scripts/ci/publish.py | 2 +- src/gitea_runner_manager/api_clients.py | 139 +++++++++++++++-- tests/unit/test_api_clients.py | 193 +++++++++++++++++++++++- tests/unit/test_auto_merge.py | 64 ++++++++ tests/unit/test_post_merge.py | 16 +- tests/unit/test_publish.py | 12 +- 8 files changed, 447 insertions(+), 40 deletions(-) diff --git a/scripts/ci/auto_merge.py b/scripts/ci/auto_merge.py index 9e4c17d..8c14f4e 100644 --- a/scripts/ci/auto_merge.py +++ b/scripts/ci/auto_merge.py @@ -16,6 +16,7 @@ Usage: import os import re +import subprocess # nosec B404 import time from typing import Any @@ -38,6 +39,21 @@ READY_TO_MERGE = "ready-to-merge" MAX_WAIT_SECONDS = 180 # 3 minutes max — CI should already be running POLL_INTERVAL_SECONDS = 15 # Poll every 15 seconds + +def run_cmd(args: list[str], check: bool = True) -> subprocess.CompletedProcess[str]: + """Run a command and return the completed process.""" + result = subprocess.run(args, capture_output=True, text=True, check=False) # nosec B603 + if check and result.returncode != 0: + raise click.ClickException( + _( + "Command failed ({cmd}): {stderr}", + cmd=" ".join(args), + stderr=result.stderr.strip() or result.stdout.strip(), + ) + ) + return result + + # PR title: GRM-N: PR_TITLE_RE = re.compile(r"^GRM-\d+:\s+.+") @@ -297,13 +313,32 @@ def main(branch: str, pr_title: str, repo: str, pr_number: str, label_name: str) try: client.merge_pr(pr_number, merge_title) except APIError as e: - raise click.ClickException( - _( - "Merge failed with HTTP {status}: {message}\nPlease check the PR is ready and you have merge rights.", - status=e.status, - message=e.message, - ) - ) from None + if e.status == 405 and "behind" in e.message.lower(): + # Head branch is behind master — pull master and rebase, then retry + click.echo(_("Head branch is behind master. Pulling and rebasing...")) + try: + run_cmd(["git", "fetch", "origin", "master"]) + run_cmd(["git", "rebase", "origin/master"]) + run_cmd(["git", "push", "--force-with-lease"]) + click.echo(_("Rebased and pushed. Retrying merge...")) + client.merge_pr(pr_number, merge_title) + except (APIError, Exception) as retry_err: + raise click.ClickException( + _( + "Merge failed after rebase retry: {error}\n" + "Please rebase the PR manually and re-add the ready-to-merge label.", + error=str(retry_err), + ) + ) from None + else: + raise click.ClickException( + _( + "Merge failed with HTTP {status}: {message}\n" + "Please check the PR is ready and you have merge rights.", + status=e.status, + message=e.message, + ) + ) from None click.echo( _( diff --git a/scripts/ci/post_merge.py b/scripts/ci/post_merge.py index 6b186c9..043e186 100644 --- a/scripts/ci/post_merge.py +++ b/scripts/ci/post_merge.py @@ -116,13 +116,19 @@ def main(commit_msg: str, commit_sha: str) -> None: client.post_comment(vikunja_task_id, html) client.update_task(vikunja_task_id, done=True) except APIError as e: - raise click.ClickException( + # Vikunja is a project management tool — if it's down, the merge + # still succeeded. Warn but don't fail the post-merge workflow. + click.echo( _( - "Vikunja API error: HTTP {status} — {message}", + "Warning: Vikunja API error (HTTP {status}): {message}. " + "Task {task_id} was NOT updated. The merge succeeded — " + "please update the Vikunja task manually.", status=e.status, message=e.message, + task_id=task_id, ) - ) from None + ) + return click.echo( _( diff --git a/scripts/ci/publish.py b/scripts/ci/publish.py index 98803e2..0804478 100644 --- a/scripts/ci/publish.py +++ b/scripts/ci/publish.py @@ -115,7 +115,7 @@ def main(tag: str, repo: str) -> None: release_body = generate_release_notes(tag) try: - client.create_release( + client.create_release_idempotent( tag=tag, body=release_body, ) diff --git a/src/gitea_runner_manager/api_clients.py b/src/gitea_runner_manager/api_clients.py index 7f704d0..57b1e9a 100644 --- a/src/gitea_runner_manager/api_clients.py +++ b/src/gitea_runner_manager/api_clients.py @@ -3,6 +3,7 @@ from __future__ import annotations import logging +import time from typing import Any import requests @@ -12,6 +13,11 @@ from .exceptions import APIError logger = logging.getLogger("grm") +# Retry configuration for transient errors (429, 5xx, connection errors) +MAX_RETRIES = 3 +RETRY_BACKOFF_BASE = 2 # seconds: 2, 4, 8 +RETRY_STATUS_CODES = {429, 500, 502, 503, 504} + def _parse_error(e: requests.HTTPError) -> tuple[int, str]: """Extract status code and message from an HTTPError response.""" @@ -25,6 +31,16 @@ def _parse_error(e: requests.HTTPError) -> tuple[int, str]: return status, message +def _is_retryable(e: Exception) -> bool: + """Check if an exception is a transient error worth retrying.""" + if isinstance(e, requests.ConnectionError): + return True + if isinstance(e, requests.HTTPError): + status, _ = _parse_error(e) + return status in RETRY_STATUS_CODES + return isinstance(e, requests.Timeout) + + class GiteaClient: """Low-level Gitea REST API client with connection pooling.""" @@ -45,13 +61,48 @@ class GiteaClient: def _request(self, method: str, path: str, **kwargs: Any) -> requests.Response: url = self._url(path) - try: - response = self._session.request(method, url, timeout=DEFAULT_TIMEOUT, **kwargs) - response.raise_for_status() - except requests.HTTPError as e: - status, message = _parse_error(e) - raise APIError(status, message) from e - return response + last_exc: Exception | None = None + for attempt in range(MAX_RETRIES): + try: + response = self._session.request(method, url, timeout=DEFAULT_TIMEOUT, **kwargs) + response.raise_for_status() + return response + except requests.HTTPError as e: + status, message = _parse_error(e) + if _is_retryable(e) and attempt < MAX_RETRIES - 1: + wait = RETRY_BACKOFF_BASE ** (attempt + 1) + logger.warning( + "Transient HTTP %d on %s %s, retrying in %ds (attempt %d/%d)", + status, + method, + path, + wait, + attempt + 1, + MAX_RETRIES, + ) + time.sleep(wait) + last_exc = e + continue + raise APIError(status, message) from e + except (requests.ConnectionError, requests.Timeout) as e: + if attempt < MAX_RETRIES - 1: + wait = RETRY_BACKOFF_BASE ** (attempt + 1) + logger.warning( + "Connection error on %s %s, retrying in %ds (attempt %d/%d)", + method, + path, + wait, + attempt + 1, + MAX_RETRIES, + ) + time.sleep(wait) + last_exc = e + continue + raise APIError(0, str(e)) from e + # Should not reach here, but just in case + if last_exc: # pragma: no cover + raise APIError(0, str(last_exc)) from last_exc + raise APIError(0, "Max retries exceeded") # pragma: no cover # -- repo settings -- @@ -202,6 +253,32 @@ class GiteaClient: r = self._request("POST", "/releases", json=payload) return r.json() + def get_release_by_tag(self, tag: str) -> dict[str, Any] | None: + """Fetch a release by its tag name. Returns None if not found.""" + try: + r = self._request("GET", f"/releases/tags/{tag}") + return r.json() + except APIError: + return None + + def create_release_idempotent( + self, + tag: str, + name: str = "", + body: str = "", + draft: bool = False, + prerelease: bool = False, + ) -> dict[str, Any]: + """Create a release, or return the existing one if it already exists. + + This is idempotent — safe to call multiple times for the same tag. + """ + existing = self.get_release_by_tag(tag) + if existing: + logger.info("Release for tag %s already exists (ID %s), skipping creation.", tag, existing.get("id")) + return existing + return self.create_release(tag=tag, name=name, body=body, draft=draft, prerelease=prerelease) + class VikunjaClient: """Low-level Vikunja REST API client with connection pooling.""" @@ -213,13 +290,47 @@ class VikunjaClient: def _request(self, method: str, path: str, **kwargs: Any) -> requests.Response: url = f"{self._base_url}{path}" - try: - response = self._session.request(method, url, timeout=DEFAULT_TIMEOUT, **kwargs) - response.raise_for_status() - except requests.HTTPError as e: - status, message = _parse_error(e) - raise APIError(status, message) from e - return response + last_exc: Exception | None = None + for attempt in range(MAX_RETRIES): + try: + response = self._session.request(method, url, timeout=DEFAULT_TIMEOUT, **kwargs) + response.raise_for_status() + return response + except requests.HTTPError as e: + status, message = _parse_error(e) + if _is_retryable(e) and attempt < MAX_RETRIES - 1: + wait = RETRY_BACKOFF_BASE ** (attempt + 1) + logger.warning( + "Transient HTTP %d on %s %s, retrying in %ds (attempt %d/%d)", + status, + method, + path, + wait, + attempt + 1, + MAX_RETRIES, + ) + time.sleep(wait) + last_exc = e + continue + raise APIError(status, message) from e + except (requests.ConnectionError, requests.Timeout) as e: + if attempt < MAX_RETRIES - 1: + wait = RETRY_BACKOFF_BASE ** (attempt + 1) + logger.warning( + "Connection error on %s %s, retrying in %ds (attempt %d/%d)", + method, + path, + wait, + attempt + 1, + MAX_RETRIES, + ) + time.sleep(wait) + last_exc = e + continue + raise APIError(0, str(e)) from e + if last_exc: # pragma: no cover + raise APIError(0, str(last_exc)) from last_exc + raise APIError(0, "Max retries exceeded") # pragma: no cover def list_tasks(self, **params: Any) -> list[dict[str, Any]]: r = self._request("GET", "/tasks", params=params) diff --git a/tests/unit/test_api_clients.py b/tests/unit/test_api_clients.py index e4656a7..0d1a777 100644 --- a/tests/unit/test_api_clients.py +++ b/tests/unit/test_api_clients.py @@ -1,12 +1,12 @@ """Unit tests for api_clients module.""" import http -from unittest.mock import MagicMock +from unittest.mock import MagicMock, patch import pytest import requests -from gitea_runner_manager.api_clients import GiteaClient, VikunjaClient, _parse_error +from gitea_runner_manager.api_clients import GiteaClient, VikunjaClient, _is_retryable, _parse_error from gitea_runner_manager.config import ( BRANCH_PROTECTION_CONFIG, DEFAULT_PER_PAGE, @@ -25,6 +25,15 @@ def _mock_response(json_data: object | None = None, raise_on_status: bool = Fals return mock +def _mock_http_error(status_code: int, message: str = "") -> requests.HTTPError: + """Create an HTTPError with a proper response attached (for _parse_error).""" + resp = MagicMock() + resp.status_code = status_code + resp.json.return_value = {"message": message or str(status_code)} + err = requests.HTTPError(f"{status_code} {message}", response=resp) + return err + + class TestParseError: def test_json_parse_fallback(self) -> None: mock_response = MagicMock() @@ -364,6 +373,115 @@ class TestGiteaClient: json={"tag_name": "v1.0.0", "name": "v1.0.0", "body": "", "draft": False, "prerelease": False}, ) + def test_get_release_by_tag_found(self) -> None: + client = GiteaClient("https://git.example.com", "tok", "owner", "repo") + client._session.request = MagicMock(return_value=_mock_response({"id": 1, "tag_name": "v1.0.0"})) + result = client.get_release_by_tag("v1.0.0") + assert result is not None + assert result["id"] == 1 + + def test_get_release_by_tag_not_found(self) -> None: + client = GiteaClient("https://git.example.com", "tok", "owner", "repo") + mock_resp = MagicMock() + mock_resp.raise_for_status.side_effect = requests.HTTPError("404") + mock_resp.status_code = 404 + client._session.request = MagicMock(return_value=mock_resp) + result = client.get_release_by_tag("v9.9.9") + assert result is None + + def test_create_release_idempotent_existing(self) -> None: + """If release already exists, should return it without creating a new one.""" + client = GiteaClient("https://git.example.com", "tok", "owner", "repo") + existing_response = _mock_response({"id": 42, "tag_name": "v1.0.0"}) + client._session.request = MagicMock(return_value=existing_response) + result = client.create_release_idempotent("v1.0.0") + assert result["id"] == 42 + # Should only call GET (check), not POST (create) + assert client._session.request.call_count == 1 + assert client._session.request.call_args[0][0] == "GET" + + def test_create_release_idempotent_new(self) -> None: + """If release doesn't exist, should create it.""" + client = GiteaClient("https://git.example.com", "tok", "owner", "repo") + not_found_resp = MagicMock() + not_found_resp.raise_for_status.side_effect = requests.HTTPError("404") + not_found_resp.status_code = 404 + create_resp = _mock_response({"id": 1, "tag_name": "v1.0.0"}) + client._session.request = MagicMock(side_effect=[not_found_resp, create_resp]) + result = client.create_release_idempotent("v1.0.0") + assert result["id"] == 1 + assert client._session.request.call_count == 2 + + @patch("gitea_runner_manager.api_clients.time.sleep") + def test_request_retries_on_429(self, mock_sleep: MagicMock) -> None: + """Should retry on 429 rate limit with exponential backoff.""" + client = GiteaClient("https://git.example.com", "tok", "owner", "repo") + rate_limited = MagicMock() + rate_limited.raise_for_status.side_effect = _mock_http_error(429, "rate limited") + success = _mock_response({"ok": True}) + client._session.request = MagicMock(side_effect=[rate_limited, rate_limited, success]) + result = client._request("GET", "/test") + assert result.json() == {"ok": True} + assert client._session.request.call_count == 3 + assert mock_sleep.call_count == 2 + + @patch("gitea_runner_manager.api_clients.time.sleep") + def test_request_retries_on_503(self, mock_sleep: MagicMock) -> None: + """Should retry on 503 service unavailable.""" + client = GiteaClient("https://git.example.com", "tok", "owner", "repo") + unavailable = MagicMock() + unavailable.raise_for_status.side_effect = _mock_http_error(503, "unavailable") + success = _mock_response({"ok": True}) + client._session.request = MagicMock(side_effect=[unavailable, success]) + result = client._request("GET", "/test") + assert result.json() == {"ok": True} + assert client._session.request.call_count == 2 + + @patch("gitea_runner_manager.api_clients.time.sleep") + def test_request_no_retry_on_404(self, mock_sleep: MagicMock) -> None: + """Should NOT retry on 404 — it's not a transient error.""" + client = GiteaClient("https://git.example.com", "tok", "owner", "repo") + not_found = MagicMock() + not_found.raise_for_status.side_effect = _mock_http_error(404, "not found") + client._session.request = MagicMock(return_value=not_found) + with pytest.raises(APIError) as exc_info: + client._request("GET", "/test") + assert exc_info.value.status == 404 + assert client._session.request.call_count == 1 + mock_sleep.assert_not_called() + + @patch("gitea_runner_manager.api_clients.time.sleep") + def test_request_retries_on_connection_error(self, mock_sleep: MagicMock) -> None: + """Should retry on connection errors.""" + client = GiteaClient("https://git.example.com", "tok", "owner", "repo") + success = _mock_response({"ok": True}) + client._session.request = MagicMock(side_effect=[requests.ConnectionError("refused"), success]) + result = client._request("GET", "/test") + assert result.json() == {"ok": True} + assert client._session.request.call_count == 2 + + @patch("gitea_runner_manager.api_clients.time.sleep") + def test_request_max_retries_exhausted(self, mock_sleep: MagicMock) -> None: + """Should raise APIError after max retries on persistent 503.""" + client = GiteaClient("https://git.example.com", "tok", "owner", "repo") + unavailable = MagicMock() + unavailable.raise_for_status.side_effect = _mock_http_error(503, "unavailable") + client._session.request = MagicMock(return_value=unavailable) + with pytest.raises(APIError) as exc_info: + client._request("GET", "/test") + assert exc_info.value.status == 503 + assert client._session.request.call_count == 3 # MAX_RETRIES + + @patch("gitea_runner_manager.api_clients.time.sleep") + def test_request_connection_error_exhausted(self, mock_sleep: MagicMock) -> None: + """Should raise APIError after max retries on persistent connection errors.""" + client = GiteaClient("https://git.example.com", "tok", "owner", "repo") + client._session.request = MagicMock(side_effect=requests.ConnectionError("refused")) + with pytest.raises(APIError) as exc_info: + client._request("GET", "/test") + assert exc_info.value.status == 0 + assert client._session.request.call_count == 3 # MAX_RETRIES + class TestVikunjaClient: def test_init_sets_headers(self) -> None: @@ -440,7 +558,9 @@ class TestVikunjaClient: def test_http_error_raises_api_error(self) -> None: client = VikunjaClient("https://work.example.com", "tok") - client._session.request = MagicMock(return_value=_mock_response(raise_on_status=True)) + mock_resp = MagicMock() + mock_resp.raise_for_status.side_effect = _mock_http_error(http.HTTPStatus.INTERNAL_SERVER_ERROR, "server error") + client._session.request = MagicMock(return_value=mock_resp) with pytest.raises(APIError): client.list_tasks() @@ -449,8 +569,73 @@ class TestVikunjaClient: client = VikunjaClient("https://work.example.com", "tok") err = requests.HTTPError("connection failed") err.response = None # type: ignore[assignment] - client._session.request = MagicMock(side_effect=err) + mock_resp = MagicMock() + mock_resp.raise_for_status.side_effect = err + client._session.request = MagicMock(return_value=mock_resp) with pytest.raises(APIError) as exc_info: client.list_tasks() assert "connection failed" in str(exc_info.value) + + @patch("gitea_runner_manager.api_clients.time.sleep") + def test_vikunja_retries_on_503(self, mock_sleep: MagicMock) -> None: + """VikunjaClient should also retry on 503.""" + client = VikunjaClient("https://work.example.com", "tok") + unavailable = MagicMock() + unavailable.raise_for_status.side_effect = _mock_http_error(503, "unavailable") + success = _mock_response([{"id": 1}]) + client._session.request = MagicMock(side_effect=[unavailable, success]) + result = client.list_tasks() + assert len(result) == 1 + assert client._session.request.call_count == 2 + + @patch("gitea_runner_manager.api_clients.time.sleep") + def test_vikunja_retries_on_connection_error(self, mock_sleep: MagicMock) -> None: + """VikunjaClient should retry on connection errors.""" + client = VikunjaClient("https://work.example.com", "tok") + success = _mock_response([{"id": 1}]) + client._session.request = MagicMock(side_effect=[requests.ConnectionError("refused"), success]) + result = client.list_tasks() + assert len(result) == 1 + assert client._session.request.call_count == 2 + + @patch("gitea_runner_manager.api_clients.time.sleep") + def test_vikunja_max_retries_exhausted(self, mock_sleep: MagicMock) -> None: + """VikunjaClient should raise APIError after max retries on persistent 503.""" + client = VikunjaClient("https://work.example.com", "tok") + unavailable = MagicMock() + unavailable.raise_for_status.side_effect = _mock_http_error(503, "unavailable") + client._session.request = MagicMock(return_value=unavailable) + with pytest.raises(APIError) as exc_info: + client.list_tasks() + assert exc_info.value.status == 503 + assert client._session.request.call_count == 3 # MAX_RETRIES + + @patch("gitea_runner_manager.api_clients.time.sleep") + def test_vikunja_connection_error_exhausted(self, mock_sleep: MagicMock) -> None: + """VikunjaClient should raise APIError after max retries on persistent connection errors.""" + client = VikunjaClient("https://work.example.com", "tok") + client._session.request = MagicMock(side_effect=requests.ConnectionError("refused")) + with pytest.raises(APIError) as exc_info: + client.list_tasks() + assert exc_info.value.status == 0 + assert client._session.request.call_count == 3 # MAX_RETRIES + + +class TestIsRetryable: + def test_connection_error_is_retryable(self) -> None: + assert _is_retryable(requests.ConnectionError("refused")) is True + + def test_timeout_is_retryable(self) -> None: + assert _is_retryable(requests.Timeout("timed out")) is True + + def test_429_is_retryable(self) -> None: + err = _mock_http_error(429, "rate limited") + assert _is_retryable(err) is True + + def test_404_is_not_retryable(self) -> None: + err = _mock_http_error(404, "not found") + assert _is_retryable(err) is False + + def test_generic_exception_is_not_retryable(self) -> None: + assert _is_retryable(ValueError("oops")) is False diff --git a/tests/unit/test_auto_merge.py b/tests/unit/test_auto_merge.py index df68675..589333c 100644 --- a/tests/unit/test_auto_merge.py +++ b/tests/unit/test_auto_merge.py @@ -16,6 +16,7 @@ from scripts.ci.auto_merge import ( has_approval_review, has_ready_to_merge_label, main, + run_cmd, validate_pr_title, validate_pr_title_matches_vikunja, wait_for_ci, @@ -604,3 +605,66 @@ class TestMain: result = runner.invoke(main, ["GRM-19-fix", "GRM-19: Some task title", "owner/repo", "1"]) assert result.exit_code == 0 assert "squash-merged" in result.output + + @patch.dict("os.environ", {"REPO_TOKEN": "tok"}) + @patch("scripts.ci.auto_merge.validate_pr_title_matches_vikunja") + @patch("scripts.ci.auto_merge.has_approval_review", return_value=True) + @patch("scripts.ci.auto_merge.GiteaClient") + def test_merge_405_behind_retries( + self, mock_client_cls: MagicMock, mock_approval: MagicMock, mock_vikunja: MagicMock + ) -> None: + """405 'behind' error should trigger rebase and retry.""" + mock_client = MagicMock() + mock_client.get_pr_labels.return_value = [{"name": "ready-to-merge"}] + mock_client.get_pr.return_value = _mock_pr() + mock_client.get_commit_status.return_value = _mock_ci_passing() + mock_client.get_pr_commits.return_value = _mock_commits() + # First merge_pr raises 405 "behind", second succeeds + mock_client.merge_pr.side_effect = [ + APIError(http.HTTPStatus.METHOD_NOT_ALLOWED, "head branch is behind base"), + None, + ] + mock_client_cls.return_value = mock_client + with patch("scripts.ci.auto_merge.run_cmd") as mock_run_cmd: + mock_run_cmd.return_value = MagicMock(returncode=0, stdout="", stderr="") + runner = CliRunner() + result = runner.invoke(main, ["GRM-19-fix", "GRM-19: Some task title", "owner/repo", "1"]) + assert result.exit_code == 0 + assert "Rebased" in result.output or "rebase" in result.output.lower() + assert mock_client.merge_pr.call_count == 2 + + @patch.dict("os.environ", {"REPO_TOKEN": "tok"}) + @patch("scripts.ci.auto_merge.validate_pr_title_matches_vikunja") + @patch("scripts.ci.auto_merge.has_approval_review", return_value=True) + @patch("scripts.ci.auto_merge.GiteaClient") + def test_merge_405_behind_rebase_fails( + self, mock_client_cls: MagicMock, mock_approval: MagicMock, mock_vikunja: MagicMock + ) -> None: + """405 'behind' with rebase failure should raise ClickException.""" + mock_client = MagicMock() + mock_client.get_pr_labels.return_value = [{"name": "ready-to-merge"}] + mock_client.get_pr.return_value = _mock_pr() + mock_client.get_commit_status.return_value = _mock_ci_passing() + mock_client.get_pr_commits.return_value = _mock_commits() + mock_client.merge_pr.side_effect = APIError(http.HTTPStatus.METHOD_NOT_ALLOWED, "head branch is behind base") + mock_client_cls.return_value = mock_client + with patch("scripts.ci.auto_merge.run_cmd") as mock_run_cmd: + mock_run_cmd.side_effect = click.ClickException("rebase failed") + runner = CliRunner() + result = runner.invoke(main, ["GRM-19-fix", "GRM-19: Some task title", "owner/repo", "1"]) + assert result.exit_code == 1 + assert "rebase" in result.output.lower() or "retry" in result.output.lower() + + def test_run_cmd_success(self) -> None: + """run_cmd should return CompletedProcess on success.""" + with patch("scripts.ci.auto_merge.subprocess.run") as mock_run: + mock_run.return_value = MagicMock(returncode=0, stdout="ok", stderr="") + result = run_cmd(["echo", "ok"]) + assert result.returncode == 0 + + def test_run_cmd_failure_raises(self) -> None: + """run_cmd should raise ClickException on non-zero exit.""" + with patch("scripts.ci.auto_merge.subprocess.run") as mock_run: + mock_run.return_value = MagicMock(returncode=1, stdout="", stderr="error") + with pytest.raises(click.ClickException): + run_cmd(["false"]) diff --git a/tests/unit/test_post_merge.py b/tests/unit/test_post_merge.py index af0ca0e..b384d96 100644 --- a/tests/unit/test_post_merge.py +++ b/tests/unit/test_post_merge.py @@ -176,7 +176,8 @@ class TestMain: @patch.dict("os.environ", {"VIKUNJA_TOKEN": "tok"}) @patch("scripts.ci.post_merge.VikunjaClient") - def test_post_comment_failure_raises_click(self, mock_client_cls: MagicMock) -> None: + def test_post_comment_failure_warns(self, mock_client_cls: MagicMock) -> None: + """Vikunja API errors should warn, not fail — the merge already succeeded.""" mock_client = MagicMock() mock_client.list_project_tasks.return_value = [ {"id": 267, "identifier": "GRM-20"}, @@ -185,12 +186,14 @@ class TestMain: mock_client_cls.return_value = mock_client runner = CliRunner() result = runner.invoke(main, ["GRM-20: fix: bug"]) - assert result.exit_code == 1 - assert "HTTP" in result.output + assert result.exit_code == 0 + assert "Warning" in result.output + assert "not updated" in result.output.lower() @patch.dict("os.environ", {"VIKUNJA_TOKEN": "tok"}) @patch("scripts.ci.post_merge.VikunjaClient") - def test_mark_done_failure_raises_click(self, mock_client_cls: MagicMock) -> None: + def test_mark_done_failure_warns(self, mock_client_cls: MagicMock) -> None: + """Vikunja API errors should warn, not fail — the merge already succeeded.""" mock_client = MagicMock() mock_client.list_project_tasks.return_value = [ {"id": 267, "identifier": "GRM-20"}, @@ -200,5 +203,6 @@ class TestMain: mock_client_cls.return_value = mock_client runner = CliRunner() result = runner.invoke(main, ["GRM-20: fix: bug"]) - assert result.exit_code == 1 - assert "HTTP" in result.output + assert result.exit_code == 0 + assert "Warning" in result.output + assert "not updated" in result.output.lower() diff --git a/tests/unit/test_publish.py b/tests/unit/test_publish.py index 5c331f2..ed6e72b 100644 --- a/tests/unit/test_publish.py +++ b/tests/unit/test_publish.py @@ -113,9 +113,9 @@ class TestMain: assert "Gitea release v1.0.0 created" in result.output mock_build.assert_called_once() mock_publish.assert_called_once_with("pypi-tok") - mock_client_cls.return_value.create_release.assert_called_once() + mock_client_cls.return_value.create_release_idempotent.assert_called_once() # Verify release body uses git-cliff notes - call_args = mock_client_cls.return_value.create_release.call_args + call_args = mock_client_cls.return_value.create_release_idempotent.call_args assert call_args.kwargs["body"] == "Release notes" @patch.dict("os.environ", {"REPO_TOKEN": "gitea-tok"}, clear=True) @@ -132,7 +132,7 @@ class TestMain: result = runner.invoke(main, ["v1.0.0", "owner/repo"]) assert result.exit_code == 0 mock_build.assert_called_once() - mock_client_cls.return_value.create_release.assert_called_once() + mock_client_cls.return_value.create_release_idempotent.assert_called_once() assert "PYPI_TOKEN not set" in result.output @patch.dict("os.environ", {"REPO_TOKEN": ""}, clear=True) @@ -181,7 +181,9 @@ class TestMain: mock_client = MagicMock() from gitea_runner_manager.exceptions import APIError - mock_client.create_release.side_effect = APIError(http.HTTPStatus.INTERNAL_SERVER_ERROR, "server error") + mock_client.create_release_idempotent.side_effect = APIError( + http.HTTPStatus.INTERNAL_SERVER_ERROR, "server error" + ) mock_client_cls.return_value = mock_client runner = CliRunner() result = runner.invoke(main, ["v1.0.0", "owner/repo"]) @@ -199,7 +201,7 @@ class TestMain: mock_client = MagicMock() from gitea_runner_manager.exceptions import APIError - mock_client.create_release.side_effect = APIError(http.HTTPStatus.BAD_GATEWAY, "bad gateway") + mock_client.create_release_idempotent.side_effect = APIError(http.HTTPStatus.BAD_GATEWAY, "bad gateway") mock_client_cls.return_value = mock_client runner = CliRunner() result = runner.invoke(main, ["v1.0.0", "owner/repo"])