diff --git a/algorithms_keeper/api.py b/algorithms_keeper/api.py index b17ab88..0a6b114 100644 --- a/algorithms_keeper/api.py +++ b/algorithms_keeper/api.py @@ -1,5 +1,8 @@ +import json import logging import os +import re +from http import HTTPStatus from typing import Any, Mapping, MutableMapping from aiohttp import ClientResponse @@ -80,11 +83,12 @@ async def _request( async with self._session.request( method, url, headers=headers, data=body ) as response: - self.log(response, body) - return response.status, response.headers, await response.read() + response_body = await response.read() + self.log(response, body, response_body) + return response.status, response.headers, response_body @staticmethod - def log(response: ClientResponse, body: bytes) -> None: # pragma: no cover + def log(response: ClientResponse, body: bytes, response_body: bytes) -> None: """Log the request-response cycle for the GitHub API calls made by the bot. The logger information will be useful to know what actions the bot made. @@ -102,6 +106,24 @@ def log(response: ClientResponse, body: bytes) -> None: # pragma: no cover else: loggerlevel = logger.error data = body.decode(UTF_8_CHARSET) + # Missing issue labels are expected when webhook handlers overlap. + # Keep other 404s and repository-label deletions visible as errors. + if ( + response.status == HTTPStatus.NOT_FOUND + and response.method == "DELETE" + and re.fullmatch( + r"/repos/[^/]+/[^/]+/issues/\d+/labels/[^/]+", response.url.raw_path + ) + ): + try: + response_data = json.loads(response_body) + except (json.JSONDecodeError, UnicodeDecodeError): + response_data = None + if ( + isinstance(response_data, dict) + and response_data.get("message") == "Label does not exist" + ): + loggerlevel = logger.info version = response.version if version is not None: version = f"{version.major}.{version.minor}" diff --git a/algorithms_keeper/utils.py b/algorithms_keeper/utils.py index 13141f8..cfe8ab3 100644 --- a/algorithms_keeper/utils.py +++ b/algorithms_keeper/utils.py @@ -15,9 +15,12 @@ import urllib.parse from base64 import b64decode from dataclasses import dataclass +from http import HTTPStatus from pathlib import Path from typing import Any, Mapping, Optional, Union +from gidgethub import BadRequest + from algorithms_keeper.api import GitHubAPI from algorithms_keeper.constants import PR_REVIEW_BODY @@ -121,10 +124,18 @@ async def remove_label_from_pr_or_issue( # or issue) at once. for label in label_list: parse_label = urllib.parse.quote(label) - await gh.delete( - f"{labels_url}/{parse_label}", - oauth_token=await gh.access_token, - ) + try: + await gh.delete( + f"{labels_url}/{parse_label}", + oauth_token=await gh.access_token, + ) + except BadRequest as exc: + # Another webhook may have removed the label since this snapshot. + if ( + exc.status_code != HTTPStatus.NOT_FOUND + or str(exc) != "Label does not exist" + ): + raise async def get_user_open_pr_numbers( diff --git a/tests/test_api.py b/tests/test_api.py index f56cbdc..d004bcf 100644 --- a/tests/test_api.py +++ b/tests/test_api.py @@ -1,3 +1,4 @@ +import logging from typing import Any, AsyncGenerator, Awaitable, Callable, Dict import aiohttp @@ -81,3 +82,75 @@ async def test_headers_and_log(github_api: GitHubAPI) -> None: ) data, rate_limit, _ = sansio.decipher_response(*resp) assert "rate" in data + + +@pytest.mark.asyncio() +@pytest.mark.parametrize( + "request_args, response_args, expected_level", + [ + ( + ("DELETE", "/repos/org/repo/issues/1/labels/tests%20are%20failing"), + (404, b'{"message": "Label does not exist"}'), + logging.INFO, + ), + ( + ("DELETE", "/repos/org/repo/issues/1/labels/tests%20are%20failing"), + (404, b'{"message": "Not Found"}'), + logging.ERROR, + ), + ( + ("DELETE", "/repos/org/repo/issues/1/labels/tests%20are%20failing"), + (403, b'{"message": "Label does not exist"}'), + logging.ERROR, + ), + ( + ("POST", "/repos/org/repo/issues/1/labels/tests%20are%20failing"), + (404, b'{"message": "Label does not exist"}'), + logging.ERROR, + ), + ( + ("DELETE", "/repos/org/repo/labels/tests%20are%20failing"), + (404, b'{"message": "Label does not exist"}'), + logging.ERROR, + ), + ( + ("DELETE", "/repos/org/repo/issues/1/labels/tests%20are%20failing"), + (404, b"not json"), + logging.ERROR, + ), + ( + ("DELETE", "/repos/org/repo/issues/1/labels/tests%20are%20failing"), + (404, b"[]"), + logging.ERROR, + ), + ], +) +async def test_label_removal_logging( + request_args: tuple[str, str], + response_args: tuple[int, bytes], + expected_level: int, + github_api: GitHubAPI, + aiohttp_server: Callable[..., Awaitable[TestServer]], + caplog: pytest.LogCaptureFixture, +) -> None: + response_status, response_body = response_args + + async def handler(request: web.Request) -> web.Response: + return web.Response(status=response_status, body=response_body) + + app = web.Application() + app.router.add_route("*", "/{path:.*}", handler) + server = await aiohttp_server(app) + method, path = request_args + with caplog.at_level(logging.INFO, logger="algorithms_keeper"): + status, _, body = await github_api._request( + method, str(server.make_url(path)), {} + ) + + assert status == response_status + assert body == response_body + records = [ + record for record in caplog.records if record.name == "algorithms_keeper" + ] + assert len(records) == 1 + assert records[0].levelno == expected_level diff --git a/tests/test_check_runs.py b/tests/test_check_runs.py index 70b6d35..1ea20af 100644 --- a/tests/test_check_runs.py +++ b/tests/test_check_runs.py @@ -1,6 +1,11 @@ +import asyncio +from http import HTTPStatus +from typing import Any +from unittest.mock import AsyncMock from urllib.parse import quote import pytest +from gidgethub import BadRequest from gidgethub.sansio import Event from algorithms_keeper.constants import Label @@ -226,3 +231,54 @@ async def test_check_run( ) -> None: await check_run_router.dispatch(event, gh) assert gh == expected + + +@pytest.mark.asyncio() +async def test_concurrent_check_runs_remove_same_label( + monkeypatch: pytest.MonkeyPatch, +) -> None: + gh = MockGitHubAPI( + getitem={ + search_url: { + "total_count": 1, + "items": [ + {"labels": [{"name": Label.FAILED_TEST}], "labels_url": labels_url} + ], + }, + check_run_url: { + "check_runs": [{"status": "completed", "conclusion": "success"}], + }, + } + ) + deleted = False + both_deleting = asyncio.Event() + + async def delete_label(url: str, **kwargs: Any) -> None: + nonlocal deleted + # Both handlers have read the same label snapshot before either deletes it. + if delete.await_count == 2: + both_deleting.set() + await asyncio.wait_for(both_deleting.wait(), timeout=5) + if deleted: + raise BadRequest(HTTPStatus.NOT_FOUND, "Label does not exist") + deleted = True + + delete = AsyncMock(side_effect=delete_label) + monkeypatch.setattr(gh, "delete", delete) + events = [ + Event( + data={ + "action": "completed", + "repository": {"full_name": repository}, + "check_run": {"id": check_id, "head_sha": sha}, + }, + event="check_run", + delivery_id=f"completed-check-{check_id}", + ) + for check_id in range(2) + ] + + await asyncio.gather(*(check_run_router.dispatch(event, gh) for event in events)) + + assert deleted + assert delete.await_count == 2 diff --git a/tests/test_utils.py b/tests/test_utils.py index ba9f0d7..6eca748 100644 --- a/tests/test_utils.py +++ b/tests/test_utils.py @@ -1,12 +1,16 @@ import urllib.parse +from http import HTTPStatus from pathlib import Path from typing import Dict, cast +from unittest.mock import AsyncMock import pytest +from gidgethub import BadRequest from algorithms_keeper import utils from algorithms_keeper.api import GitHubAPI from algorithms_keeper.constants import Label +from algorithms_keeper.event.pull_request import update_stage_label from .utils import ( MockGitHubAPI, @@ -145,6 +149,70 @@ async def test_remove_multiple_labels() -> None: assert f"{labels_url}/{parse_label2}" in gh.delete_url +@pytest.mark.asyncio() +async def test_remove_missing_label_continues(monkeypatch: pytest.MonkeyPatch) -> None: + gh = MockGitHubAPI() + delete = AsyncMock( + side_effect=[BadRequest(HTTPStatus.NOT_FOUND, "Label does not exist"), None] + ) + monkeypatch.setattr(gh, "delete", delete) + + await utils.remove_label_from_pr_or_issue( + cast(GitHubAPI, gh), + label=[Label.TYPE_HINT, Label.REQUIRE_TEST], + pr_or_issue={"issue_url": issue_url}, + ) + + assert [call.args[0] for call in delete.await_args_list] == [ + f"{labels_url}/{urllib.parse.quote(Label.TYPE_HINT)}", + f"{labels_url}/{urllib.parse.quote(Label.REQUIRE_TEST)}", + ] + + +@pytest.mark.asyncio() +async def test_stage_change_with_stale_labels(monkeypatch: pytest.MonkeyPatch) -> None: + gh = MockGitHubAPI() + monkeypatch.setattr( + gh, + "delete", + AsyncMock(side_effect=BadRequest(HTTPStatus.NOT_FOUND, "Label does not exist")), + ) + + await update_stage_label( + cast(GitHubAPI, gh), + pull_request={"issue_url": issue_url, "labels": [{"name": Label.CHANGE}]}, + next_label=Label.REVIEW, + ) + + assert gh.post_data == [{"labels": [Label.REVIEW]}] + + +@pytest.mark.asyncio() +@pytest.mark.parametrize( + "status, message", + [ + (HTTPStatus.NOT_FOUND, "Not Found"), + (HTTPStatus.FORBIDDEN, "Label does not exist"), + (HTTPStatus.FORBIDDEN, "Resource not accessible by integration"), + ], +) +async def test_remove_label_preserves_other_errors( + status: HTTPStatus, message: str, monkeypatch: pytest.MonkeyPatch +) -> None: + gh = MockGitHubAPI() + error = BadRequest(status, message) + monkeypatch.setattr(gh, "delete", AsyncMock(side_effect=error)) + + with pytest.raises(BadRequest) as exc_info: + await utils.remove_label_from_pr_or_issue( + cast(GitHubAPI, gh), + label=Label.FAILED_TEST, + pr_or_issue={"issue_url": issue_url}, + ) + + assert exc_info.value is error + + @pytest.mark.asyncio() async def test_get_user_open_pr_numbers() -> None: getiter = {