diff --git a/bedevere/__main__.py b/bedevere/__main__.py index 2ea53e49..2ea16b17 100644 --- a/bedevere/__main__.py +++ b/bedevere/__main__.py @@ -37,7 +37,7 @@ async def main(request): cache=cache) # Give GitHub some time to reach internal consistency. await asyncio.sleep(1) - await router.dispatch(event, gh) + await router.dispatch(event, gh, session=session) try: print('GH requests remaining:', gh.rate_limit.remaining) except AttributeError: diff --git a/bedevere/bpo.py b/bedevere/bpo.py index 51a7afba..e1014a36 100644 --- a/bedevere/bpo.py +++ b/bedevere/bpo.py @@ -21,13 +21,6 @@ SKIP_ISSUE_LABEL = util.skip_label("issue") STATUS_CONTEXT = "bedevere/issue-number" # Try to keep descriptions at or below 50 characters, else GitHub's CSS will truncate it. -_FAILURE_DESCRIPTION = 'No issue # in title or "skip issue" label found' -_FAILURE_URL = "https://devguide.python.org/pullrequest/#submitting" -FAILURE_STATUS = util.create_status(STATUS_CONTEXT, util.StatusState.FAILURE, - description=_FAILURE_DESCRIPTION, - target_url=_FAILURE_URL) -del _FAILURE_DESCRIPTION -del _FAILURE_URL SKIP_ISSUE_STATUS = util.create_status(STATUS_CONTEXT, util.StatusState.SUCCESS, description="Issue report skipped") @@ -35,21 +28,26 @@ @router.register("pull_request", action="opened") @router.register("pull_request", action="synchronize") @router.register("pull_request", action="reopened") -async def set_status(event, gh, *args, **kwargs): +async def set_status(event, gh, session=None, *args, **kwargs): """Set the issue number status on the pull request.""" issue_number_found = ISSUE_RE.search(event.data["pull_request"]["title"]) if not issue_number_found: issue = await util.issue_for_PR(gh, event.data["pull_request"]) - status = SKIP_ISSUE_STATUS if util.skip("issue", issue) else FAILURE_STATUS + status = (SKIP_ISSUE_STATUS if util.skip("issue", issue) + else create_failure_status_no_issue()) else: - if "body" in event.data["pull_request"]: - body = event.data["pull_request"]["body"] or "" - if not body or CLOSING_TAG not in body: - issue_number = issue_number_found.group("issue") - new_body = BODY.format(body=body, issue_number=issue_number) - body_data = {"body": new_body} - await gh.patch(event.data["pull_request"]["url"], data=body_data) - status = create_success_status(issue_number_found) + issue_number = issue_number_found.group("issue") + issue_number_on_bpo = await _validate_issue_number(issue_number, session=session) + if issue_number_on_bpo: + if "body" in event.data["pull_request"]: + body = event.data["pull_request"]["body"] or "" + if not body or CLOSING_TAG not in body: + new_body = BODY.format(body=body, issue_number=issue_number) + body_data = {"body": new_body} + await gh.patch(event.data["pull_request"]["url"], data=body_data) + status = create_success_status(issue_number) + else: + status = create_failure_status_issue_not_on_bpo(issue_number) await util.post_status(gh, event, status) @@ -68,7 +66,7 @@ async def new_label(event, gh, *args, **kwargs): issue_number_found = ISSUE_RE.search( event.data["pull_request"]["title"]) if issue_number_found: - status = create_success_status(issue_number_found) + status = create_success_status(issue_number_found.group("issue")) else: status = SKIP_ISSUE_STATUS await util.post_status(gh, event, status) @@ -104,15 +102,32 @@ async def hyperlink_bpo_text(event, gh, *args, **kwargs): await gh.patch(event.data[event_name][body_location], data=body_data) -def create_success_status(found_issue): +def create_success_status(issue_number): """Create a success status for when an issue number was found in the title.""" - issue_number = found_issue.group("issue") url = f"https://bugs.python.org/issue{issue_number}" return util.create_status(STATUS_CONTEXT, util.StatusState.SUCCESS, description=f"Issue number {issue_number} found", target_url=url) +def create_failure_status_issue_not_on_bpo(issue_number): + """Create a failure status for when an issue does not exist on the bug tracker.""" + description = f"Issue #{issue_number} not found on bugs.python.org" + url = f"https://bugs.python.org/issue{issue_number}" + return util.create_status(STATUS_CONTEXT, util.StatusState.FAILURE, + description=description, + target_url=url) + + +def create_failure_status_no_issue(): + """Create a failure status for when no issue number was found in the title.""" + description = 'No issue # in title or "skip issue" label found' + url = "https://devguide.python.org/pullrequest/#submitting" + return util.create_status(STATUS_CONTEXT, util.StatusState.FAILURE, + description=description, + target_url=url) + + def check_hyperlink(match): """The span checking of regex matches takes care of cases like bpo-123 [bpo-123]…""" issue = match.group("issue") @@ -155,3 +170,10 @@ def create_hyperlink_in_comment_body(body): leftover_body = leftover_body[presence:] new_body = new_body + leftover_body return new_body + + +async def _validate_issue_number(issue_number, session): + """Make sure the issue number exists on bugs.python.org.""" + url = f"https://bugs.python.org/issue{issue_number}" + async with session.head(url) as res: + return res.status != 404 diff --git a/dev-requirements.txt b/dev-requirements.txt index e74e58d2..a1d13800 100644 --- a/dev-requirements.txt +++ b/dev-requirements.txt @@ -1,4 +1,5 @@ -r requirements.txt +asynctest==0.13.0 pytest==5.0.1 pytest-asyncio==0.10.0 pytest-aiohttp==0.3.0 diff --git a/tests/test_bpo.py b/tests/test_bpo.py index df8efe4b..1d3402a7 100644 --- a/tests/test_bpo.py +++ b/tests/test_bpo.py @@ -1,3 +1,5 @@ +import aiohttp +import asynctest import pytest from gidgethub import sansio @@ -27,7 +29,9 @@ async def patch(self, url, data): @pytest.mark.asyncio @pytest.mark.parametrize("action", ["opened", "synchronize", "reopened"]) -async def test_set_status_failure(action): +async def test_set_status_failure(action, monkeypatch): + monkeypatch.setattr(bpo, '_validate_issue_number', + asynctest.CoroutineMock(return_value=True)) data = { "action": action, "pull_request": { @@ -48,11 +52,35 @@ async def test_set_status_failure(action): assert status["state"] == "failure" assert status["target_url"].startswith("https://devguide.python.org") assert status["context"] == "bedevere/issue-number" + bpo._validate_issue_number.assert_not_awaited() @pytest.mark.asyncio @pytest.mark.parametrize("action", ["opened", "synchronize", "reopened"]) -async def test_set_status_success(action): +async def test_set_status_failure_via_issue_not_found_on_bpo(action): + data = { + "action": action, + "pull_request": { + "statuses_url": "https://api.github.com/blah/blah/git-sha", + "title": "bpo-123: Invalid issue number", + }, + } + event = sansio.Event(data, event="pull_request", delivery_id="12345") + gh = FakeGH() + async with aiohttp.ClientSession() as session: + await bpo.router.dispatch(event, gh, session=session) + status = gh.data + assert status["state"] == "failure" + assert status["target_url"].startswith("https://bugs.python.org") + assert status["context"] == "bedevere/issue-number" + assert status["description"] == "Issue #123 not found on bugs.python.org" + + +@pytest.mark.asyncio +@pytest.mark.parametrize("action", ["opened", "synchronize", "reopened"]) +async def test_set_status_success(action, monkeypatch): + monkeypatch.setattr(bpo, '_validate_issue_number', + asynctest.CoroutineMock(return_value=True)) data = { "action": action, "pull_request": { @@ -69,11 +97,36 @@ async def test_set_status_success(action): assert "1234" in status["description"] assert status["context"] == "bedevere/issue-number" assert "git-sha" in gh.url + bpo._validate_issue_number.assert_awaited_with("1234", session=None) + + +@pytest.mark.asyncio +@pytest.mark.parametrize("action", ["opened", "synchronize", "reopened"]) +async def test_set_status_success_issue_found_on_bpo(action): + data = { + "action": action, + "pull_request": { + "statuses_url": "https://api.github.com/blah/blah/git-sha", + "title": "bpo-12345: an issue!", + }, + } + event = sansio.Event(data, event="pull_request", delivery_id="12345") + gh = FakeGH() + async with aiohttp.ClientSession() as session: + await bpo.router.dispatch(event, gh, session=session) + status = gh.data + assert status["state"] == "success" + assert status["target_url"].endswith("issue12345") + assert "12345" in status["description"] + assert status["context"] == "bedevere/issue-number" + assert "git-sha" in gh.url @pytest.mark.asyncio @pytest.mark.parametrize("action", ["opened", "synchronize", "reopened"]) -async def test_set_status_success_via_skip_issue_label(action): +async def test_set_status_success_via_skip_issue_label(action, monkeypatch): + monkeypatch.setattr(bpo, '_validate_issue_number', + asynctest.CoroutineMock(return_value=True)) data = { "action": action, "pull_request": { @@ -94,10 +147,13 @@ async def test_set_status_success_via_skip_issue_label(action): assert status["state"] == "success" assert status["context"] == "bedevere/issue-number" assert "git-sha" in gh.url + bpo._validate_issue_number.assert_not_awaited() @pytest.mark.asyncio -async def test_edit_title(): +async def test_edit_title(monkeypatch): + monkeypatch.setattr(bpo, '_validate_issue_number', + asynctest.CoroutineMock(return_value=True)) data = { "pull_request": { "statuses_url": "https://api.github.com/blah/blah/git-sha", @@ -110,10 +166,13 @@ async def test_edit_title(): gh = FakeGH() await bpo.router.dispatch(event, gh) assert gh.data is not None + bpo._validate_issue_number.assert_awaited_with("1234", session=None) @pytest.mark.asyncio -async def test_no_body_when_edit_title(): +async def test_no_body_when_edit_title(monkeypatch): + monkeypatch.setattr(bpo, '_validate_issue_number', + asynctest.CoroutineMock(return_value=True)) data = { "action": "edited", "pull_request": { @@ -131,10 +190,13 @@ async def test_no_body_when_edit_title(): await bpo.router.dispatch(event, gh) assert gh.patch_data is not None assert gh.patch_data["body"] == "\n\n\nhttps://bugs.python.org/issue32636\n\n" + bpo._validate_issue_number.assert_awaited_with("32636", session=None) @pytest.mark.asyncio -async def test_edit_other_than_title(): +async def test_edit_other_than_title(monkeypatch): + monkeypatch.setattr(bpo, '_validate_issue_number', + asynctest.CoroutineMock(return_value=True)) data = { "pull_request": { "statuses_url": "https://api.github.com/blah/blah/git-sha", @@ -147,6 +209,7 @@ async def test_edit_other_than_title(): gh = FakeGH() await bpo.router.dispatch(event, gh) assert gh.data is None + bpo._validate_issue_number.assert_not_awaited() @pytest.mark.asyncio @@ -203,9 +266,11 @@ async def test_new_label_not_skip_issue(): @pytest.mark.asyncio -async def test_removed_label_from_label_deletion(): +async def test_removed_label_from_label_deletion(monkeypatch): """When a label is completely deleted from a repo, it triggers an 'unlabeled' event, but the payload has no details about the removed label.""" + monkeypatch.setattr(bpo, '_validate_issue_number', + asynctest.CoroutineMock(return_value=True)) data = { "action": "unlabeled", # No "label" key. @@ -218,10 +283,13 @@ async def test_removed_label_from_label_deletion(): gh = FakeGH() await bpo.router.dispatch(event, gh) assert gh.data is None + bpo._validate_issue_number.assert_not_awaited() @pytest.mark.asyncio -async def test_removed_label_skip_issue(): +async def test_removed_label_skip_issue(monkeypatch): + monkeypatch.setattr(bpo, '_validate_issue_number', + asynctest.CoroutineMock(return_value=True)) data = { "action": "unlabeled", "label": {"name": "skip issue"}, @@ -239,10 +307,13 @@ async def test_removed_label_skip_issue(): assert "1234" in status["description"] assert status["context"] == "bedevere/issue-number" assert "git-sha" in gh.url + bpo._validate_issue_number.assert_awaited_with("1234", session=None) @pytest.mark.asyncio -async def test_removed_label_non_skip_issue(): +async def test_removed_label_non_skip_issue(monkeypatch): + monkeypatch.setattr(bpo, '_validate_issue_number', + asynctest.CoroutineMock(return_value=True)) data = { "action": "unlabeled", "label": {"name": "non-trivial"}, @@ -254,10 +325,13 @@ async def test_removed_label_non_skip_issue(): gh = FakeGH() await bpo.router.dispatch(event, gh) assert gh.data is None + bpo._validate_issue_number.assert_not_awaited() @pytest.mark.asyncio -async def test_set_body_success(): +async def test_set_body_success(monkeypatch): + monkeypatch.setattr(bpo, '_validate_issue_number', + asynctest.CoroutineMock(return_value=True)) data = { "action": "opened", "pull_request": { @@ -273,10 +347,13 @@ async def test_set_body_success(): status = gh.patch_data assert "https://bugs.python.org/issue1234" in status["body"] assert "1347" in gh.patch_url + bpo._validate_issue_number.assert_awaited_with("1234", session=None) @pytest.mark.asyncio -async def test_set_body_failure(): +async def test_set_body_failure(monkeypatch): + monkeypatch.setattr(bpo, '_validate_issue_number', + asynctest.CoroutineMock(return_value=True)) data = { "action": "opened", "pull_request": { @@ -291,11 +368,24 @@ async def test_set_body_failure(): await bpo.router.dispatch(event, gh) assert gh.patch_data is None assert gh.patch_url is None + bpo._validate_issue_number.assert_awaited_with("1234", session=None) @pytest.mark.asyncio -@pytest.mark.parametrize("action", ["opened", "edited"]) -async def test_set_pull_request_body_success(action): +async def test_set_pull_request_body_success_opened(monkeypatch): + await set_pull_request_body_success_helper("opened", monkeypatch) + bpo._validate_issue_number.assert_awaited_with("12345", session=None) + + +@pytest.mark.asyncio +async def test_set_pull_request_body_success_edited(monkeypatch): + await set_pull_request_body_success_helper("edited", monkeypatch) + bpo._validate_issue_number.assert_not_awaited() + + +async def set_pull_request_body_success_helper(action, monkeypatch): + monkeypatch.setattr(bpo, '_validate_issue_number', + asynctest.CoroutineMock(return_value=True)) data = { "action": action, "pull_request": { @@ -315,6 +405,7 @@ async def test_set_pull_request_body_success(action): assert "[bpo-12345](https://bugs.python.org/issue12345)" in body_data["body"] assert "123456" in gh.patch_url + @pytest.mark.asyncio @pytest.mark.parametrize("event,action", [("issue_comment", "created"), ("issue_comment", "edited"), @@ -336,9 +427,12 @@ async def test_set_comment_body_success(event, action): assert "[bpo-12345](https://bugs.python.org/issue12345)" in body_data["body"] assert "123456" in gh.patch_url + @pytest.mark.asyncio @pytest.mark.parametrize("action", ["opened", "edited"]) -async def test_set_pull_request_body_without_bpo(action): +async def test_set_pull_request_body_without_bpo(action, monkeypatch): + monkeypatch.setattr(bpo, '_validate_issue_number', + asynctest.CoroutineMock(return_value=True)) data = { "action": action, "pull_request": { @@ -356,6 +450,7 @@ async def test_set_pull_request_body_without_bpo(action): await bpo.router.dispatch(event, gh) if gh.patch_data: assert "[bpo-123](https://bugs.python.org/issue123)" not in gh.patch_data + bpo._validate_issue_number.assert_awaited_with("12345", session=None) @pytest.mark.asyncio @@ -380,8 +475,26 @@ async def test_set_comment_body_without_bpo(event, action): @pytest.mark.asyncio -@pytest.mark.parametrize("action", ["opened", "edited"]) -async def test_set_pull_request_body_already_hyperlinked_bpo(action): +async def test_set_pull_request_body_already_hyperlinked_bpo_opened(monkeypatch): + await set_pull_request_body_already_hyperlinked_bpo_helper( + "opened", + monkeypatch + ) + bpo._validate_issue_number.assert_awaited_with("12345", session=None) + + +@pytest.mark.asyncio +async def test_set_pull_request_body_already_hyperlinked_bpo_edited(monkeypatch): + await set_pull_request_body_already_hyperlinked_bpo_helper( + "edited", + monkeypatch + ) + bpo._validate_issue_number.assert_not_awaited() + + +async def set_pull_request_body_already_hyperlinked_bpo_helper(action, monkeypatch): + monkeypatch.setattr(bpo, '_validate_issue_number', + asynctest.CoroutineMock(return_value=True)) data = { "action": action, "pull_request": {