From f5fb352b9b75ad5baefc8efe78654955297cbd2f Mon Sep 17 00:00:00 2001 From: Lysandros Nikolaou Date: Fri, 22 Mar 2019 18:02:48 +0100 Subject: [PATCH 01/11] Validate bpo issue number --- bedevere/bpo.py | 45 +++++++++++++++++++++++++++++++++++---------- tests/test_bpo.py | 26 ++++++++++++++++++++++++++ 2 files changed, 61 insertions(+), 10 deletions(-) diff --git a/bedevere/bpo.py b/bedevere/bpo.py index 328a354c..418a978f 100644 --- a/bedevere/bpo.py +++ b/bedevere/bpo.py @@ -1,6 +1,7 @@ """Check if a bugs.python.org issue number is specified in the pull request's title.""" import re +import aiohttp from gidgethub import routing from . import util @@ -21,13 +22,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") @@ -40,16 +34,21 @@ async def set_status(event, gh, *args, **kwargs): 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: + issue_number = issue_number_found.group("issue") 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, "maintainer_can_modify": True} await gh.patch(event.data["pull_request"]["url"], data=body_data) - status = create_success_status(issue_number_found) + issue_number_on_bpo = await _validate_issue_number(issue_number) + if issue_number_on_bpo: + status = create_success_status(issue_number_found) + else: + status = create_failure_status_issue_not_on_bpo(issue_number_found) await util.post_status(gh, event, status) @@ -111,6 +110,24 @@ def create_success_status(found_issue): target_url=url) +def create_failure_status_issue_not_on_bpo(issue_number_found): + """Create a failure status for when an issue does not exist on the bug tracker""" + issue_number = issue_number_found.group("issue") + description = f"Issue #{issue_number} not found on bugs.python.org" + url = "https://devguide.python.org/pullrequest/#submitting" + return util.create_status(STATUS_CONTEXT, util.StatusState.FAILURE, + description=description, + target_url=url) + + +def create_failure_status_no_issue(): + 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") @@ -154,3 +171,11 @@ 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): + url = f"https://bugs.python.org/issue{issue_number}" + async with aiohttp.ClientSession() as session: + async with session.head(url) as res: + return res.status != 404 + diff --git a/tests/test_bpo.py b/tests/test_bpo.py index 99399ffa..f2090de4 100644 --- a/tests/test_bpo.py +++ b/tests/test_bpo.py @@ -50,6 +50,32 @@ async def test_set_status_failure(action): assert status["context"] == "bedevere/issue-number" +@pytest.mark.asyncio +@pytest.mark.parametrize("action", ["opened", "synchronize", "reopened"]) +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", + "issue_url": "issue URL", + }, + } + issue_data = { + "labels": [ + {"name": "non-trivial"}, + ] + } + event = sansio.Event(data, event="pull_request", delivery_id="12345") + gh = FakeGH(getitem=issue_data) + await bpo.router.dispatch(event, gh) + status = gh.data + assert status["state"] == "failure" + assert status["target_url"].startswith("https://devguide.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): From 0bd9b320c5b6b2a19542a1d22fc1fd7ccc835da4 Mon Sep 17 00:00:00 2001 From: Lysandros Nikolaou Date: Sat, 30 Mar 2019 01:37:24 +0100 Subject: [PATCH 02/11] Add docstrings for new functions --- bedevere/bpo.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/bedevere/bpo.py b/bedevere/bpo.py index 418a978f..354ff531 100644 --- a/bedevere/bpo.py +++ b/bedevere/bpo.py @@ -111,7 +111,7 @@ def create_success_status(found_issue): def create_failure_status_issue_not_on_bpo(issue_number_found): - """Create a failure status for when an issue does not exist on the bug tracker""" + """Create a failure status for when an issue does not exist on the bug tracker.""" issue_number = issue_number_found.group("issue") description = f"Issue #{issue_number} not found on bugs.python.org" url = "https://devguide.python.org/pullrequest/#submitting" @@ -121,6 +121,7 @@ def create_failure_status_issue_not_on_bpo(issue_number_found): 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, @@ -174,6 +175,7 @@ def create_hyperlink_in_comment_body(body): async def _validate_issue_number(issue_number): + """Make sure the issue number exists on bugs.python.org.""" url = f"https://bugs.python.org/issue{issue_number}" async with aiohttp.ClientSession() as session: async with session.head(url) as res: From 3bd6a910e7a934d2e708d2fffa4c44707aa9365a Mon Sep 17 00:00:00 2001 From: Lysandros Nikolaou Date: Fri, 19 Apr 2019 14:11:56 +0200 Subject: [PATCH 03/11] 1. Pass issue_number to create_status functions 2. Refactor, so that the first message does not get patched even if the issue is not on bpo --- bedevere/bpo.py | 24 +++++++++++------------- 1 file changed, 11 insertions(+), 13 deletions(-) diff --git a/bedevere/bpo.py b/bedevere/bpo.py index 354ff531..eaf44720 100644 --- a/bedevere/bpo.py +++ b/bedevere/bpo.py @@ -38,17 +38,17 @@ async def set_status(event, gh, *args, **kwargs): else create_failure_status_no_issue()) else: issue_number = issue_number_found.group("issue") - 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, "maintainer_can_modify": True} - await gh.patch(event.data["pull_request"]["url"], data=body_data) issue_number_on_bpo = await _validate_issue_number(issue_number) if issue_number_on_bpo: - status = create_success_status(issue_number_found) + 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, "maintainer_can_modify": True} + 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_found) + status = create_failure_status_issue_not_on_bpo(issue_number) await util.post_status(gh, event, status) @@ -67,7 +67,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) @@ -101,18 +101,16 @@ 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_found): +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.""" - issue_number = issue_number_found.group("issue") description = f"Issue #{issue_number} not found on bugs.python.org" url = "https://devguide.python.org/pullrequest/#submitting" return util.create_status(STATUS_CONTEXT, util.StatusState.FAILURE, From 3ebde4fe458a9cc4242af6771dbe1bc9f51e1e8a Mon Sep 17 00:00:00 2001 From: Lysandros Nikolaou Date: Mon, 6 May 2019 20:38:02 +0200 Subject: [PATCH 04/11] Patch out bpo validation call for not relevant tests --- bedevere/bpo.py | 5 ++++- tests/test_bpo.py | 48 ++++++++++++++++++++++++++++++++++++++--------- 2 files changed, 43 insertions(+), 10 deletions(-) diff --git a/bedevere/bpo.py b/bedevere/bpo.py index 66c98633..eb718853 100644 --- a/bedevere/bpo.py +++ b/bedevere/bpo.py @@ -38,7 +38,10 @@ async def set_status(event, gh, *args, **kwargs): else create_failure_status_no_issue()) else: issue_number = issue_number_found.group("issue") - issue_number_on_bpo = await _validate_issue_number(issue_number) + try: + issue_number_on_bpo = await _validate_issue_number(issue_number) + except NameError: + issue_number_on_bpo = True if issue_number_on_bpo: if "body" in event.data["pull_request"]: body = event.data["pull_request"]["body"] or "" diff --git a/tests/test_bpo.py b/tests/test_bpo.py index f2090de4..efdbaae7 100644 --- a/tests/test_bpo.py +++ b/tests/test_bpo.py @@ -27,7 +27,8 @@ 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.delattr('bedevere.bpo._validate_issue_number') data = { "action": action, "pull_request": { @@ -78,7 +79,8 @@ async def test_set_status_failure_via_issue_not_found_on_bpo(action): @pytest.mark.asyncio @pytest.mark.parametrize("action", ["opened", "synchronize", "reopened"]) -async def test_set_status_success(action): +async def test_set_status_success(action, monkeypatch): + monkeypatch.delattr('bedevere.bpo._validate_issue_number') data = { "action": action, "pull_request": { @@ -97,6 +99,27 @@ async def test_set_status_success(action): assert "git-sha" in gh.url +@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() + await bpo.router.dispatch(event, gh) + 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): @@ -123,7 +146,8 @@ async def test_set_status_success_via_skip_issue_label(action): @pytest.mark.asyncio -async def test_edit_title(): +async def test_edit_title(monkeypatch): + monkeypatch.delattr('bedevere.bpo._validate_issue_number') data = { "pull_request": { "statuses_url": "https://api.github.com/blah/blah/git-sha", @@ -139,7 +163,8 @@ async def test_edit_title(): @pytest.mark.asyncio -async def test_no_body_when_edit_title(): +async def test_no_body_when_edit_title(monkeypatch): + monkeypatch.delattr('bedevere.bpo._validate_issue_number') data = { "action": "edited", "pull_request": { @@ -247,7 +272,8 @@ async def test_removed_label_from_label_deletion(): @pytest.mark.asyncio -async def test_removed_label_skip_issue(): +async def test_removed_label_skip_issue(monkeypatch): + monkeypatch.delattr('bedevere.bpo._validate_issue_number') data = { "action": "unlabeled", "label": {"name": "skip issue"}, @@ -283,7 +309,8 @@ async def test_removed_label_non_skip_issue(): @pytest.mark.asyncio -async def test_set_body_success(): +async def test_set_body_success(monkeypatch): + monkeypatch.delattr('bedevere.bpo._validate_issue_number') data = { "action": "opened", "pull_request": { @@ -302,7 +329,8 @@ async def test_set_body_success(): @pytest.mark.asyncio -async def test_set_body_failure(): +async def test_set_body_failure(monkeypatch): + monkeypatch.delattr('bedevere.bpo._validate_issue_number') data = { "action": "opened", "pull_request": { @@ -321,7 +349,8 @@ async def test_set_body_failure(): @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(action, monkeypatch): + monkeypatch.delattr('bedevere.bpo._validate_issue_number') data = { "action": action, "pull_request": { @@ -401,7 +430,8 @@ async def test_set_comment_body_without_bpo(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(action, monkeypatch): + monkeypatch.delattr('bedevere.bpo._validate_issue_number') data = { "action": action, "pull_request": { From b1c3942d1497b66aa20b4c5ae9637d19f36c4e5b Mon Sep 17 00:00:00 2001 From: Lysandros Nikolaou Date: Wed, 12 Jun 2019 01:15:19 +0000 Subject: [PATCH 05/11] First commit to be squashed later --- bedevere/bpo.py | 7 ++-- tests/test_bpo.py | 87 ++++++++++++++++++++++++++++++++++------------- 2 files changed, 66 insertions(+), 28 deletions(-) diff --git a/bedevere/bpo.py b/bedevere/bpo.py index eb718853..386cb93e 100644 --- a/bedevere/bpo.py +++ b/bedevere/bpo.py @@ -38,16 +38,13 @@ async def set_status(event, gh, *args, **kwargs): else create_failure_status_no_issue()) else: issue_number = issue_number_found.group("issue") - try: - issue_number_on_bpo = await _validate_issue_number(issue_number) - except NameError: - issue_number_on_bpo = True + issue_number_on_bpo = await _validate_issue_number(issue_number) 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, "maintainer_can_modify": True} + body_data = {"body": new_body} await gh.patch(event.data["pull_request"]["url"], data=body_data) status = create_success_status(issue_number) else: diff --git a/tests/test_bpo.py b/tests/test_bpo.py index efdbaae7..42c35d3b 100644 --- a/tests/test_bpo.py +++ b/tests/test_bpo.py @@ -1,3 +1,5 @@ +from unittest import mock + import pytest from gidgethub import sansio @@ -25,10 +27,17 @@ async def patch(self, url, data): self.patch_data = data +@pytest.fixture +def mock_issue_validation(mocker): + return mocker.patch( + 'bedevere.bpo._validate_issue_number', + return_value=True + ) + + @pytest.mark.asyncio @pytest.mark.parametrize("action", ["opened", "synchronize", "reopened"]) -async def test_set_status_failure(action, monkeypatch): - monkeypatch.delattr('bedevere.bpo._validate_issue_number') +async def test_set_status_failure(action, mock_issue_validation): data = { "action": action, "pull_request": { @@ -49,6 +58,7 @@ async def test_set_status_failure(action, monkeypatch): assert status["state"] == "failure" assert status["target_url"].startswith("https://devguide.python.org") assert status["context"] == "bedevere/issue-number" + mock_issue_validation.assert_not_called() @pytest.mark.asyncio @@ -79,8 +89,7 @@ async def test_set_status_failure_via_issue_not_found_on_bpo(action): @pytest.mark.asyncio @pytest.mark.parametrize("action", ["opened", "synchronize", "reopened"]) -async def test_set_status_success(action, monkeypatch): - monkeypatch.delattr('bedevere.bpo._validate_issue_number') +async def test_set_status_success(action, mock_issue_validation): data = { "action": action, "pull_request": { @@ -97,6 +106,7 @@ async def test_set_status_success(action, monkeypatch): assert "1234" in status["description"] assert status["context"] == "bedevere/issue-number" assert "git-sha" in gh.url + mock_issue_validation.assert_called_with("1234") @pytest.mark.asyncio @@ -120,9 +130,11 @@ async def test_set_status_success_issue_found_on_bpo(action): assert "git-sha" in gh.url +@mock.patch('bedevere.bpo._validate_issue_number', + mock.MagicMock(return_value=True)) @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(fake_validate, action): data = { "action": action, "pull_request": { @@ -143,11 +155,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 + fake_validate.assert_not_called() +@mock.patch('bedevere.bpo._validate_issue_number', + mock.MagicMock(return_value=True)) @pytest.mark.asyncio -async def test_edit_title(monkeypatch): - monkeypatch.delattr('bedevere.bpo._validate_issue_number') +async def test_edit_title(fake_validate): data = { "pull_request": { "statuses_url": "https://api.github.com/blah/blah/git-sha", @@ -160,11 +174,13 @@ async def test_edit_title(monkeypatch): gh = FakeGH() await bpo.router.dispatch(event, gh) assert gh.data is not None + fake_validate.assert_called_with("1234") +@mock.patch('bedevere.bpo._validate_issue_number', + mock.MagicMock(return_value=True)) @pytest.mark.asyncio -async def test_no_body_when_edit_title(monkeypatch): - monkeypatch.delattr('bedevere.bpo._validate_issue_number') +async def test_no_body_when_edit_title(fake_validate): data = { "action": "edited", "pull_request": { @@ -182,10 +198,13 @@ async def test_no_body_when_edit_title(monkeypatch): 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" + fake_validate.assert_called_with("32636") +@mock.patch('bedevere.bpo._validate_issue_number', + mock.MagicMock(return_value=True)) @pytest.mark.asyncio -async def test_edit_other_than_title(): +async def test_edit_other_than_title(fake_validate): data = { "pull_request": { "statuses_url": "https://api.github.com/blah/blah/git-sha", @@ -198,6 +217,7 @@ async def test_edit_other_than_title(): gh = FakeGH() await bpo.router.dispatch(event, gh) assert gh.data is None + fake_validate.assert_not_called() @pytest.mark.asyncio @@ -253,8 +273,10 @@ async def test_new_label_not_skip_issue(): assert gh.data is None +@mock.patch('bedevere.bpo._validate_issue_number', + mock.MagicMock(return_value=True)) @pytest.mark.asyncio -async def test_removed_label_from_label_deletion(): +async def test_removed_label_from_label_deletion(fake_validate): """When a label is completely deleted from a repo, it triggers an 'unlabeled' event, but the payload has no details about the removed label.""" data = { @@ -269,11 +291,13 @@ async def test_removed_label_from_label_deletion(): gh = FakeGH() await bpo.router.dispatch(event, gh) assert gh.data is None + fake_validate.assert_not_called() +@mock.patch('bedevere.bpo._validate_issue_number', + mock.MagicMock(return_value=True)) @pytest.mark.asyncio -async def test_removed_label_skip_issue(monkeypatch): - monkeypatch.delattr('bedevere.bpo._validate_issue_number') +async def test_removed_label_skip_issue(fake_validate): data = { "action": "unlabeled", "label": {"name": "skip issue"}, @@ -291,10 +315,13 @@ async def test_removed_label_skip_issue(monkeypatch): assert "1234" in status["description"] assert status["context"] == "bedevere/issue-number" assert "git-sha" in gh.url + fake_validate.assert_called_with("1234") +@mock.patch('bedevere.bpo._validate_issue_number', + mock.MagicMock(return_value=True)) @pytest.mark.asyncio -async def test_removed_label_non_skip_issue(): +async def test_removed_label_non_skip_issue(fake_validate): data = { "action": "unlabeled", "label": {"name": "non-trivial"}, @@ -306,11 +333,13 @@ async def test_removed_label_non_skip_issue(): gh = FakeGH() await bpo.router.dispatch(event, gh) assert gh.data is None + fake_validate.assert_not_called() +@mock.patch('bedevere.bpo._validate_issue_number', + mock.MagicMock(return_value=True)) @pytest.mark.asyncio -async def test_set_body_success(monkeypatch): - monkeypatch.delattr('bedevere.bpo._validate_issue_number') +async def test_set_body_success(fake_validate): data = { "action": "opened", "pull_request": { @@ -326,11 +355,13 @@ async def test_set_body_success(monkeypatch): status = gh.patch_data assert "https://bugs.python.org/issue1234" in status["body"] assert "1347" in gh.patch_url + fake_validate.assert_called_with("1234") +@mock.patch('bedevere.bpo._validate_issue_number', + mock.MagicMock(return_value=True)) @pytest.mark.asyncio -async def test_set_body_failure(monkeypatch): - monkeypatch.delattr('bedevere.bpo._validate_issue_number') +async def test_set_body_failure(fake_validate): data = { "action": "opened", "pull_request": { @@ -345,12 +376,14 @@ async def test_set_body_failure(monkeypatch): await bpo.router.dispatch(event, gh) assert gh.patch_data is None assert gh.patch_url is None + fake_validate.assert_called_with("1234") +@mock.patch('bedevere.bpo._validate_issue_number', + mock.MagicMock(return_value=True)) @pytest.mark.asyncio @pytest.mark.parametrize("action", ["opened", "edited"]) -async def test_set_pull_request_body_success(action, monkeypatch): - monkeypatch.delattr('bedevere.bpo._validate_issue_number') +async def test_set_pull_request_body_success(fake_validate, action): data = { "action": action, "pull_request": { @@ -369,6 +402,8 @@ async def test_set_pull_request_body_success(action, monkeypatch): body_data = gh.patch_data assert "[bpo-12345](https://bugs.python.org/issue12345)" in body_data["body"] assert "123456" in gh.patch_url + fake_validate.assert_called_with("12345") + @pytest.mark.asyncio @pytest.mark.parametrize("action", ["created", "edited"]) @@ -388,9 +423,12 @@ async def test_set_comment_body_success(action): assert "[bpo-12345](https://bugs.python.org/issue12345)" in body_data["body"] assert "123456" in gh.patch_url + +@mock.patch('bedevere.bpo._validate_issue_number', + mock.MagicMock(return_value=True)) @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(fake_validate, action): data = { "action": action, "pull_request": { @@ -408,6 +446,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 + fake_validate.assert_called_with("12345") @pytest.mark.asyncio @@ -428,10 +467,11 @@ async def test_set_comment_body_without_bpo(action): assert gh.patch_url is None +@mock.patch('bedevere.bpo._validate_issue_number', + mock.MagicMock(return_value=True)) @pytest.mark.asyncio @pytest.mark.parametrize("action", ["opened", "edited"]) -async def test_set_pull_request_body_already_hyperlinked_bpo(action, monkeypatch): - monkeypatch.delattr('bedevere.bpo._validate_issue_number') +async def test_set_pull_request_body_already_hyperlinked_bpo(fake_validate, action): data = { "action": action, "pull_request": { @@ -453,6 +493,7 @@ async def test_set_pull_request_body_already_hyperlinked_bpo(action, monkeypatch body_data = gh.patch_data assert body_data["body"].count("[bpo-123](https://bugs.python.org/issue123)") == 2 assert "123456" in gh.patch_url + fake_validate.assert_called_with("12345") @pytest.mark.asyncio From d5463c29bc7c56e7af9976078d14817da1c507ca Mon Sep 17 00:00:00 2001 From: Lysandros Nikolaou Date: Sat, 13 Jul 2019 01:38:19 +0000 Subject: [PATCH 06/11] Mock _validate_issue_number on irrelevant tests for performance --- bedevere/bpo.py | 1 - dev-requirements.txt | 1 + tests/test_bpo.py | 120 +++++++++++++++++++------------------------ 3 files changed, 55 insertions(+), 67 deletions(-) diff --git a/bedevere/bpo.py b/bedevere/bpo.py index 386cb93e..9ebc4a0e 100644 --- a/bedevere/bpo.py +++ b/bedevere/bpo.py @@ -178,4 +178,3 @@ async def _validate_issue_number(issue_number): async with aiohttp.ClientSession() as session: async with session.head(url) as res: return res.status != 404 - diff --git a/dev-requirements.txt b/dev-requirements.txt index 696c7dbc..a7103a45 100644 --- a/dev-requirements.txt +++ b/dev-requirements.txt @@ -1,4 +1,5 @@ -r requirements.txt +asynctest==0.13.0 pytest==4.4.0 pytest-asyncio==0.10.0 pytest-aiohttp==0.3.0 diff --git a/tests/test_bpo.py b/tests/test_bpo.py index 42c35d3b..545135e8 100644 --- a/tests/test_bpo.py +++ b/tests/test_bpo.py @@ -1,5 +1,6 @@ from unittest import mock +import asynctest import pytest from gidgethub import sansio @@ -27,17 +28,15 @@ async def patch(self, url, data): self.patch_data = data -@pytest.fixture -def mock_issue_validation(mocker): - return mocker.patch( - 'bedevere.bpo._validate_issue_number', - return_value=True - ) +@pytest.fixture(scope='function', autouse=False) +def mock_issue_validation(request): + bpo._validate_issue_number = asynctest.CoroutineMock(return_value=True) @pytest.mark.asyncio +@pytest.mark.usefixtures('mock_issue_validation') @pytest.mark.parametrize("action", ["opened", "synchronize", "reopened"]) -async def test_set_status_failure(action, mock_issue_validation): +async def test_set_status_failure(action): data = { "action": action, "pull_request": { @@ -58,7 +57,7 @@ async def test_set_status_failure(action, mock_issue_validation): assert status["state"] == "failure" assert status["target_url"].startswith("https://devguide.python.org") assert status["context"] == "bedevere/issue-number" - mock_issue_validation.assert_not_called() + bpo._validate_issue_number.assert_not_awaited() @pytest.mark.asyncio @@ -67,18 +66,12 @@ 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", + "statuses_url": "https://api.github.com/blah/blah/git-sha", "title": "bpo-123: Invalid issue number", - "issue_url": "issue URL", }, } - issue_data = { - "labels": [ - {"name": "non-trivial"}, - ] - } event = sansio.Event(data, event="pull_request", delivery_id="12345") - gh = FakeGH(getitem=issue_data) + gh = FakeGH() await bpo.router.dispatch(event, gh) status = gh.data assert status["state"] == "failure" @@ -88,8 +81,9 @@ async def test_set_status_failure_via_issue_not_found_on_bpo(action): @pytest.mark.asyncio +@pytest.mark.usefixtures('mock_issue_validation') @pytest.mark.parametrize("action", ["opened", "synchronize", "reopened"]) -async def test_set_status_success(action, mock_issue_validation): +async def test_set_status_success(action): data = { "action": action, "pull_request": { @@ -106,7 +100,7 @@ async def test_set_status_success(action, mock_issue_validation): assert "1234" in status["description"] assert status["context"] == "bedevere/issue-number" assert "git-sha" in gh.url - mock_issue_validation.assert_called_with("1234") + bpo._validate_issue_number.assert_awaited_with("1234") @pytest.mark.asyncio @@ -130,11 +124,10 @@ async def test_set_status_success_issue_found_on_bpo(action): assert "git-sha" in gh.url -@mock.patch('bedevere.bpo._validate_issue_number', - mock.MagicMock(return_value=True)) @pytest.mark.asyncio +@pytest.mark.usefixtures('mock_issue_validation') @pytest.mark.parametrize("action", ["opened", "synchronize", "reopened"]) -async def test_set_status_success_via_skip_issue_label(fake_validate, action): +async def test_set_status_success_via_skip_issue_label(action): data = { "action": action, "pull_request": { @@ -155,13 +148,12 @@ async def test_set_status_success_via_skip_issue_label(fake_validate, action): assert status["state"] == "success" assert status["context"] == "bedevere/issue-number" assert "git-sha" in gh.url - fake_validate.assert_not_called() + bpo._validate_issue_number.assert_not_awaited() -@mock.patch('bedevere.bpo._validate_issue_number', - mock.MagicMock(return_value=True)) @pytest.mark.asyncio -async def test_edit_title(fake_validate): +@pytest.mark.usefixtures('mock_issue_validation') +async def test_edit_title(): data = { "pull_request": { "statuses_url": "https://api.github.com/blah/blah/git-sha", @@ -174,13 +166,12 @@ async def test_edit_title(fake_validate): gh = FakeGH() await bpo.router.dispatch(event, gh) assert gh.data is not None - fake_validate.assert_called_with("1234") + bpo._validate_issue_number.assert_awaited_with('1234') -@mock.patch('bedevere.bpo._validate_issue_number', - mock.MagicMock(return_value=True)) @pytest.mark.asyncio -async def test_no_body_when_edit_title(fake_validate): +@pytest.mark.usefixtures('mock_issue_validation') +async def test_no_body_when_edit_title(): data = { "action": "edited", "pull_request": { @@ -198,13 +189,12 @@ async def test_no_body_when_edit_title(fake_validate): 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" - fake_validate.assert_called_with("32636") + bpo._validate_issue_number.assert_awaited_with('32636') -@mock.patch('bedevere.bpo._validate_issue_number', - mock.MagicMock(return_value=True)) @pytest.mark.asyncio -async def test_edit_other_than_title(fake_validate): +@pytest.mark.usefixtures('mock_issue_validation') +async def test_edit_other_than_title(): data = { "pull_request": { "statuses_url": "https://api.github.com/blah/blah/git-sha", @@ -217,7 +207,7 @@ async def test_edit_other_than_title(fake_validate): gh = FakeGH() await bpo.router.dispatch(event, gh) assert gh.data is None - fake_validate.assert_not_called() + bpo._validate_issue_number.assert_not_awaited() @pytest.mark.asyncio @@ -273,10 +263,9 @@ async def test_new_label_not_skip_issue(): assert gh.data is None -@mock.patch('bedevere.bpo._validate_issue_number', - mock.MagicMock(return_value=True)) @pytest.mark.asyncio -async def test_removed_label_from_label_deletion(fake_validate): +@pytest.mark.usefixtures('mock_issue_validation') +async def test_removed_label_from_label_deletion(): """When a label is completely deleted from a repo, it triggers an 'unlabeled' event, but the payload has no details about the removed label.""" data = { @@ -291,13 +280,12 @@ async def test_removed_label_from_label_deletion(fake_validate): gh = FakeGH() await bpo.router.dispatch(event, gh) assert gh.data is None - fake_validate.assert_not_called() + bpo._validate_issue_number.assert_not_awaited() -@mock.patch('bedevere.bpo._validate_issue_number', - mock.MagicMock(return_value=True)) @pytest.mark.asyncio -async def test_removed_label_skip_issue(fake_validate): +@pytest.mark.usefixtures('mock_issue_validation') +async def test_removed_label_skip_issue(): data = { "action": "unlabeled", "label": {"name": "skip issue"}, @@ -315,13 +303,12 @@ async def test_removed_label_skip_issue(fake_validate): assert "1234" in status["description"] assert status["context"] == "bedevere/issue-number" assert "git-sha" in gh.url - fake_validate.assert_called_with("1234") + bpo._validate_issue_number.assert_awaited_with("1234") -@mock.patch('bedevere.bpo._validate_issue_number', - mock.MagicMock(return_value=True)) @pytest.mark.asyncio -async def test_removed_label_non_skip_issue(fake_validate): +@pytest.mark.usefixtures('mock_issue_validation') +async def test_removed_label_non_skip_issue(): data = { "action": "unlabeled", "label": {"name": "non-trivial"}, @@ -333,13 +320,12 @@ async def test_removed_label_non_skip_issue(fake_validate): gh = FakeGH() await bpo.router.dispatch(event, gh) assert gh.data is None - fake_validate.assert_not_called() + bpo._validate_issue_number.assert_not_awaited() -@mock.patch('bedevere.bpo._validate_issue_number', - mock.MagicMock(return_value=True)) @pytest.mark.asyncio -async def test_set_body_success(fake_validate): +@pytest.mark.usefixtures('mock_issue_validation') +async def test_set_body_success(): data = { "action": "opened", "pull_request": { @@ -355,13 +341,12 @@ async def test_set_body_success(fake_validate): status = gh.patch_data assert "https://bugs.python.org/issue1234" in status["body"] assert "1347" in gh.patch_url - fake_validate.assert_called_with("1234") + bpo._validate_issue_number.assert_awaited_with("1234") -@mock.patch('bedevere.bpo._validate_issue_number', - mock.MagicMock(return_value=True)) @pytest.mark.asyncio -async def test_set_body_failure(fake_validate): +@pytest.mark.usefixtures('mock_issue_validation') +async def test_set_body_failure(): data = { "action": "opened", "pull_request": { @@ -376,14 +361,13 @@ async def test_set_body_failure(fake_validate): await bpo.router.dispatch(event, gh) assert gh.patch_data is None assert gh.patch_url is None - fake_validate.assert_called_with("1234") + bpo._validate_issue_number.assert_awaited_with("1234") -@mock.patch('bedevere.bpo._validate_issue_number', - mock.MagicMock(return_value=True)) @pytest.mark.asyncio +@pytest.mark.usefixtures('mock_issue_validation') @pytest.mark.parametrize("action", ["opened", "edited"]) -async def test_set_pull_request_body_success(fake_validate, action): +async def test_set_pull_request_body_success(action): data = { "action": action, "pull_request": { @@ -402,7 +386,10 @@ async def test_set_pull_request_body_success(fake_validate, action): body_data = gh.patch_data assert "[bpo-12345](https://bugs.python.org/issue12345)" in body_data["body"] assert "123456" in gh.patch_url - fake_validate.assert_called_with("12345") + if action == 'opened': + bpo._validate_issue_number.assert_awaited_with("12345") + else: + bpo._validate_issue_number.assert_not_awaited() @pytest.mark.asyncio @@ -424,11 +411,10 @@ async def test_set_comment_body_success(action): assert "123456" in gh.patch_url -@mock.patch('bedevere.bpo._validate_issue_number', - mock.MagicMock(return_value=True)) @pytest.mark.asyncio +@pytest.mark.usefixtures('mock_issue_validation') @pytest.mark.parametrize("action", ["opened", "edited"]) -async def test_set_pull_request_body_without_bpo(fake_validate, action): +async def test_set_pull_request_body_without_bpo(action): data = { "action": action, "pull_request": { @@ -446,7 +432,7 @@ async def test_set_pull_request_body_without_bpo(fake_validate, action): await bpo.router.dispatch(event, gh) if gh.patch_data: assert "[bpo-123](https://bugs.python.org/issue123)" not in gh.patch_data - fake_validate.assert_called_with("12345") + bpo._validate_issue_number.assert_awaited_with("12345") @pytest.mark.asyncio @@ -467,11 +453,10 @@ async def test_set_comment_body_without_bpo(action): assert gh.patch_url is None -@mock.patch('bedevere.bpo._validate_issue_number', - mock.MagicMock(return_value=True)) @pytest.mark.asyncio +@pytest.mark.usefixtures('mock_issue_validation') @pytest.mark.parametrize("action", ["opened", "edited"]) -async def test_set_pull_request_body_already_hyperlinked_bpo(fake_validate, action): +async def test_set_pull_request_body_already_hyperlinked_bpo(action): data = { "action": action, "pull_request": { @@ -493,7 +478,10 @@ async def test_set_pull_request_body_already_hyperlinked_bpo(fake_validate, acti body_data = gh.patch_data assert body_data["body"].count("[bpo-123](https://bugs.python.org/issue123)") == 2 assert "123456" in gh.patch_url - fake_validate.assert_called_with("12345") + if action == 'opened': + bpo._validate_issue_number.assert_awaited_with("12345") + else: + bpo._validate_issue_number.assert_not_awaited() @pytest.mark.asyncio From 2ebfa34f1121ef0dfef6fd8b3ef151cf16c92135 Mon Sep 17 00:00:00 2001 From: Lysandros Nikolaou Date: Tue, 6 Aug 2019 21:02:11 +0300 Subject: [PATCH 07/11] Use monkeypatch for mocking and pass session in kwargs --- bedevere/__main__.py | 2 +- bedevere/bpo.py | 11 +++-- tests/test_bpo.py | 104 ++++++++++++++++++++++++------------------- 3 files changed, 64 insertions(+), 53 deletions(-) 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 a15910e1..275b2eb7 100644 --- a/bedevere/bpo.py +++ b/bedevere/bpo.py @@ -1,7 +1,6 @@ """Check if a bugs.python.org issue number is specified in the pull request's title.""" import re -import aiohttp from gidgethub import routing from . import util @@ -38,7 +37,8 @@ async def set_status(event, gh, *args, **kwargs): else create_failure_status_no_issue()) else: issue_number = issue_number_found.group("issue") - issue_number_on_bpo = await _validate_issue_number(issue_number) + session = kwargs.get('session', '') + issue_number_on_bpo = await _validate_issue_number(issue_number, session) if issue_number_on_bpo: if "body" in event.data["pull_request"]: body = event.data["pull_request"]["body"] or "" @@ -173,9 +173,8 @@ def create_hyperlink_in_comment_body(body): return new_body -async def _validate_issue_number(issue_number): +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 aiohttp.ClientSession() as session: - async with session.head(url) as res: - return res.status != 404 + async with session.head(url) as res: + return res.status != 404 diff --git a/tests/test_bpo.py b/tests/test_bpo.py index d6edd799..cac7406f 100644 --- a/tests/test_bpo.py +++ b/tests/test_bpo.py @@ -1,5 +1,4 @@ -from unittest import mock - +import aiohttp import asynctest import pytest @@ -28,15 +27,11 @@ async def patch(self, url, data): self.patch_data = data -@pytest.fixture(scope='function', autouse=False) -def mock_issue_validation(request): - bpo._validate_issue_number = asynctest.CoroutineMock(return_value=True) - - @pytest.mark.asyncio -@pytest.mark.usefixtures('mock_issue_validation') @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": { @@ -72,7 +67,9 @@ async def test_set_status_failure_via_issue_not_found_on_bpo(action): } event = sansio.Event(data, event="pull_request", delivery_id="12345") gh = FakeGH() - await bpo.router.dispatch(event, gh) + session = aiohttp.ClientSession() + await bpo.router.dispatch(event, gh, session=session) + await session.close() status = gh.data assert status["state"] == "failure" assert status["target_url"].startswith("https://devguide.python.org") @@ -81,9 +78,10 @@ async def test_set_status_failure_via_issue_not_found_on_bpo(action): @pytest.mark.asyncio -@pytest.mark.usefixtures('mock_issue_validation') @pytest.mark.parametrize("action", ["opened", "synchronize", "reopened"]) -async def test_set_status_success(action): +async def test_set_status_success(action, monkeypatch): + monkeypatch.setattr(bpo, '_validate_issue_number', + asynctest.CoroutineMock(return_value=True)) data = { "action": action, "pull_request": { @@ -100,7 +98,7 @@ 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") + bpo._validate_issue_number.assert_awaited_with("1234", "") @pytest.mark.asyncio @@ -115,7 +113,9 @@ async def test_set_status_success_issue_found_on_bpo(action): } event = sansio.Event(data, event="pull_request", delivery_id="12345") gh = FakeGH() - await bpo.router.dispatch(event, gh) + session = aiohttp.ClientSession() + await bpo.router.dispatch(event, gh, session=session) + await session.close() status = gh.data assert status["state"] == "success" assert status["target_url"].endswith("issue12345") @@ -125,9 +125,10 @@ async def test_set_status_success_issue_found_on_bpo(action): @pytest.mark.asyncio -@pytest.mark.usefixtures('mock_issue_validation') @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": { @@ -152,8 +153,9 @@ async def test_set_status_success_via_skip_issue_label(action): @pytest.mark.asyncio -@pytest.mark.usefixtures('mock_issue_validation') -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", @@ -166,12 +168,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') + bpo._validate_issue_number.assert_awaited_with("1234", "") @pytest.mark.asyncio -@pytest.mark.usefixtures('mock_issue_validation') -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": { @@ -189,12 +192,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') + bpo._validate_issue_number.assert_awaited_with("32636", "") @pytest.mark.asyncio -@pytest.mark.usefixtures('mock_issue_validation') -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", @@ -264,10 +268,11 @@ async def test_new_label_not_skip_issue(): @pytest.mark.asyncio -@pytest.mark.usefixtures('mock_issue_validation') -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. @@ -284,8 +289,9 @@ async def test_removed_label_from_label_deletion(): @pytest.mark.asyncio -@pytest.mark.usefixtures('mock_issue_validation') -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"}, @@ -303,12 +309,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") + bpo._validate_issue_number.assert_awaited_with("1234", "") @pytest.mark.asyncio -@pytest.mark.usefixtures('mock_issue_validation') -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"}, @@ -324,8 +331,9 @@ async def test_removed_label_non_skip_issue(): @pytest.mark.asyncio -@pytest.mark.usefixtures('mock_issue_validation') -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": { @@ -341,12 +349,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") + bpo._validate_issue_number.assert_awaited_with("1234", "") @pytest.mark.asyncio -@pytest.mark.usefixtures('mock_issue_validation') -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": { @@ -361,13 +370,14 @@ 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") + bpo._validate_issue_number.assert_awaited_with("1234", "") @pytest.mark.asyncio -@pytest.mark.usefixtures('mock_issue_validation') @pytest.mark.parametrize("action", ["opened", "edited"]) -async def test_set_pull_request_body_success(action): +async def test_set_pull_request_body_success(action, monkeypatch): + monkeypatch.setattr(bpo, '_validate_issue_number', + asynctest.CoroutineMock(return_value=True)) data = { "action": action, "pull_request": { @@ -387,7 +397,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 if action == 'opened': - bpo._validate_issue_number.assert_awaited_with("12345") + bpo._validate_issue_number.assert_awaited_with("12345", "") else: bpo._validate_issue_number.assert_not_awaited() @@ -415,9 +425,10 @@ async def test_set_comment_body_success(event, action): @pytest.mark.asyncio -@pytest.mark.usefixtures('mock_issue_validation') @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": { @@ -435,7 +446,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") + bpo._validate_issue_number.assert_awaited_with("12345", "") @pytest.mark.asyncio @@ -460,9 +471,10 @@ async def test_set_comment_body_without_bpo(event, action): @pytest.mark.asyncio -@pytest.mark.usefixtures('mock_issue_validation') @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(action, monkeypatch): + monkeypatch.setattr(bpo, '_validate_issue_number', + asynctest.CoroutineMock(return_value=True)) data = { "action": action, "pull_request": { @@ -487,7 +499,7 @@ async def test_set_pull_request_body_already_hyperlinked_bpo(action): assert body_data["body"].count("[something about bpo-123](https://bugs.python.org/issue123)") == 1 assert "123456" in gh.patch_url if action == 'opened': - bpo._validate_issue_number.assert_awaited_with("12345") + bpo._validate_issue_number.assert_awaited_with("12345", "") else: bpo._validate_issue_number.assert_not_awaited() From 98ee2832ba11d61641d1759ae6d87a1a07ebcbdc Mon Sep 17 00:00:00 2001 From: Lysandros Nikolaou Date: Wed, 4 Sep 2019 18:50:58 +0300 Subject: [PATCH 08/11] 1. Remove action ifs from tests 2. Change failure url to point to bpo --- bedevere/bpo.py | 5 ++-- tests/test_bpo.py | 58 +++++++++++++++++++++++++++++++---------------- 2 files changed, 40 insertions(+), 23 deletions(-) diff --git a/bedevere/bpo.py b/bedevere/bpo.py index 275b2eb7..12ac7735 100644 --- a/bedevere/bpo.py +++ b/bedevere/bpo.py @@ -28,7 +28,7 @@ @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: @@ -37,7 +37,6 @@ async def set_status(event, gh, *args, **kwargs): else create_failure_status_no_issue()) else: issue_number = issue_number_found.group("issue") - session = kwargs.get('session', '') issue_number_on_bpo = await _validate_issue_number(issue_number, session) if issue_number_on_bpo: if "body" in event.data["pull_request"]: @@ -114,7 +113,7 @@ def create_success_status(issue_number): 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 = "https://devguide.python.org/pullrequest/#submitting" + url = f"https://bugs.python.org/issue{issue_number}" return util.create_status(STATUS_CONTEXT, util.StatusState.FAILURE, description=description, target_url=url) diff --git a/tests/test_bpo.py b/tests/test_bpo.py index cac7406f..39e34581 100644 --- a/tests/test_bpo.py +++ b/tests/test_bpo.py @@ -72,7 +72,7 @@ async def test_set_status_failure_via_issue_not_found_on_bpo(action): await session.close() status = gh.data assert status["state"] == "failure" - assert status["target_url"].startswith("https://devguide.python.org") + 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" @@ -98,7 +98,7 @@ async def test_set_status_success(action, monkeypatch): 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", "") + bpo._validate_issue_number.assert_awaited_with("1234", None) @pytest.mark.asyncio @@ -168,7 +168,7 @@ async def test_edit_title(monkeypatch): gh = FakeGH() await bpo.router.dispatch(event, gh) assert gh.data is not None - bpo._validate_issue_number.assert_awaited_with("1234", "") + bpo._validate_issue_number.assert_awaited_with("1234", None) @pytest.mark.asyncio @@ -192,7 +192,7 @@ async def test_no_body_when_edit_title(monkeypatch): 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", "") + bpo._validate_issue_number.assert_awaited_with("32636", None) @pytest.mark.asyncio @@ -309,7 +309,7 @@ async def test_removed_label_skip_issue(monkeypatch): 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", "") + bpo._validate_issue_number.assert_awaited_with("1234", None) @pytest.mark.asyncio @@ -349,7 +349,7 @@ async def test_set_body_success(monkeypatch): 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", "") + bpo._validate_issue_number.assert_awaited_with("1234", None) @pytest.mark.asyncio @@ -370,12 +370,22 @@ async def test_set_body_failure(monkeypatch): 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", "") + bpo._validate_issue_number.assert_awaited_with("1234", None) @pytest.mark.asyncio -@pytest.mark.parametrize("action", ["opened", "edited"]) -async def test_set_pull_request_body_success(action, monkeypatch): +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", 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 = { @@ -396,10 +406,6 @@ async def test_set_pull_request_body_success(action, monkeypatch): body_data = gh.patch_data assert "[bpo-12345](https://bugs.python.org/issue12345)" in body_data["body"] assert "123456" in gh.patch_url - if action == 'opened': - bpo._validate_issue_number.assert_awaited_with("12345", "") - else: - bpo._validate_issue_number.assert_not_awaited() @pytest.mark.asyncio @@ -446,7 +452,7 @@ async def test_set_pull_request_body_without_bpo(action, monkeypatch): 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", "") + bpo._validate_issue_number.assert_awaited_with("12345", None) @pytest.mark.asyncio @@ -471,8 +477,24 @@ 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, monkeypatch): +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", 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 = { @@ -498,10 +520,6 @@ async def test_set_pull_request_body_already_hyperlinked_bpo(action, monkeypatch assert body_data["body"].count("[bpo-123](https://bugs.python.org/issue123)") == 2 assert body_data["body"].count("[something about bpo-123](https://bugs.python.org/issue123)") == 1 assert "123456" in gh.patch_url - if action == 'opened': - bpo._validate_issue_number.assert_awaited_with("12345", "") - else: - bpo._validate_issue_number.assert_not_awaited() @pytest.mark.asyncio From 772f385fef4f111fef6b89b5aee1360bfeded186 Mon Sep 17 00:00:00 2001 From: Lysandros Nikolaou Date: Wed, 4 Sep 2019 18:55:16 +0300 Subject: [PATCH 09/11] Use context manager for ClientSession in tests to make sure it gets closed --- tests/test_bpo.py | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/tests/test_bpo.py b/tests/test_bpo.py index 39e34581..c3b7211b 100644 --- a/tests/test_bpo.py +++ b/tests/test_bpo.py @@ -67,9 +67,8 @@ async def test_set_status_failure_via_issue_not_found_on_bpo(action): } event = sansio.Event(data, event="pull_request", delivery_id="12345") gh = FakeGH() - session = aiohttp.ClientSession() - await bpo.router.dispatch(event, gh, session=session) - await session.close() + 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") @@ -113,9 +112,8 @@ async def test_set_status_success_issue_found_on_bpo(action): } event = sansio.Event(data, event="pull_request", delivery_id="12345") gh = FakeGH() - session = aiohttp.ClientSession() - await bpo.router.dispatch(event, gh, session=session) - await session.close() + 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") From 7f0c80ccc52f6e9685e6755115b6cda6a223622a Mon Sep 17 00:00:00 2001 From: Brett Cannon <54418+brettcannon@users.noreply.github.com> Date: Tue, 10 Sep 2019 12:08:22 +0100 Subject: [PATCH 10/11] Explicitly specify the keyword argument for 'session' --- tests/test_bpo.py | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/tests/test_bpo.py b/tests/test_bpo.py index c3b7211b..1d3402a7 100644 --- a/tests/test_bpo.py +++ b/tests/test_bpo.py @@ -97,7 +97,7 @@ async def test_set_status_success(action, monkeypatch): 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", None) + bpo._validate_issue_number.assert_awaited_with("1234", session=None) @pytest.mark.asyncio @@ -166,7 +166,7 @@ async def test_edit_title(monkeypatch): gh = FakeGH() await bpo.router.dispatch(event, gh) assert gh.data is not None - bpo._validate_issue_number.assert_awaited_with("1234", None) + bpo._validate_issue_number.assert_awaited_with("1234", session=None) @pytest.mark.asyncio @@ -190,7 +190,7 @@ async def test_no_body_when_edit_title(monkeypatch): 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", None) + bpo._validate_issue_number.assert_awaited_with("32636", session=None) @pytest.mark.asyncio @@ -307,7 +307,7 @@ async def test_removed_label_skip_issue(monkeypatch): 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", None) + bpo._validate_issue_number.assert_awaited_with("1234", session=None) @pytest.mark.asyncio @@ -347,7 +347,7 @@ async def test_set_body_success(monkeypatch): 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", None) + bpo._validate_issue_number.assert_awaited_with("1234", session=None) @pytest.mark.asyncio @@ -368,13 +368,13 @@ async def test_set_body_failure(monkeypatch): 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", None) + bpo._validate_issue_number.assert_awaited_with("1234", session=None) @pytest.mark.asyncio 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", None) + bpo._validate_issue_number.assert_awaited_with("12345", session=None) @pytest.mark.asyncio @@ -450,7 +450,7 @@ async def test_set_pull_request_body_without_bpo(action, monkeypatch): 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", None) + bpo._validate_issue_number.assert_awaited_with("12345", session=None) @pytest.mark.asyncio @@ -480,7 +480,7 @@ async def test_set_pull_request_body_already_hyperlinked_bpo_opened(monkeypatch) "opened", monkeypatch ) - bpo._validate_issue_number.assert_awaited_with("12345", None) + bpo._validate_issue_number.assert_awaited_with("12345", session=None) @pytest.mark.asyncio From d7d0a7e5774a74605d46158cdf74b9a5ecfc70db Mon Sep 17 00:00:00 2001 From: Brett Cannon <54418+brettcannon@users.noreply.github.com> Date: Tue, 10 Sep 2019 12:11:56 +0100 Subject: [PATCH 11/11] Pass `session` by keyword --- bedevere/bpo.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bedevere/bpo.py b/bedevere/bpo.py index 12ac7735..e1014a36 100644 --- a/bedevere/bpo.py +++ b/bedevere/bpo.py @@ -37,7 +37,7 @@ async def set_status(event, gh, session=None, *args, **kwargs): else create_failure_status_no_issue()) else: issue_number = issue_number_found.group("issue") - issue_number_on_bpo = await _validate_issue_number(issue_number, session) + 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 ""