Validate bpo issue number by lysnikolaou · Pull Request #159 · python/bedevere

lysnikolaou · 2019-03-23T00:48:39Z

Because #72 was closed I thought I could move the work of Lev forward. Taking some of his work and following all the suggestions in the initial PR, I have formulated a new one.

I used the requests library, but did not updated the requirements.txt file, because I'm not sure that the correct way to go. urllib might be a better alternative, because it's in the stdlib.

Relevant issue: #15
Co-authored-by: Lev Lazinskiy

Update: I updated requirements.txt so that the tests can pass on Travis. In case we use some other library, it can always be updated again.

codecov · 2019-03-23T01:10:30Z

Codecov Report

Merging #159 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff          @@
##           master   #159   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          20     20           
  Lines        1577   1650   +73     
  Branches       87     88    +1     
=====================================
+ Hits         1577   1650   +73

Impacted Files	Coverage Δ
bedevere/__main__.py	`100% <100%> (ø)`	⬆️
bedevere/bpo.py	`100% <100%> (ø)`	⬆️
tests/test_bpo.py	`100% <100%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e80b3bc...d7d0a7e. Read the comment docs.

brettcannon · 2019-03-25T19:17:06Z

Just an FYI I'm not comfortable accepting this with the use of requests as that's synchronous and the design of the bot is purposefully async. Please use aiohttp for any HTTP calls.

lysnikolaou · 2019-03-26T15:24:33Z

Sorry @brettcannon, hadn't thought of that. I updated my PR to use aiohttp.

brettcannon · 2019-03-26T21:23:09Z

                              target_url=url)


+def create_failure_status(issue_not_found_on_bpo, found_issue=None):


There's no need to overload creating a failure case for the missing issue number in the PR versus the issue not existing. Just make them separate functions.

brettcannon · 2019-03-26T21:25:32Z

+    url = f"https://bugs.python.org/issue{issue_number}"
+    async with aiohttp.ClientSession() as session:
+        async with session.head(url) as res:
+            return False if res.status == 404 else True


Suggested change

return False if res.status == 404 else True

return res.status != 404

Haha, that's kind of embarrassing...

brettcannon · 2019-03-26T21:25:52Z


+@pytest.mark.asyncio
+@pytest.mark.parametrize("action", ["opened", "synchronize", "reopened"])
+async def test_set_status_failure_via_issue_non_found_on_bpo(action):


Suggested change

async def test_set_status_failure_via_issue_non_found_on_bpo(action):

async def test_set_status_failure_via_issue_not_found_on_bpo(action):

lysnikolaou · 2019-03-26T23:05:49Z

Thanks for the review @brettcannon, I have updated my PR.

lysnikolaou · 2019-03-30T00:44:15Z

@brettcannon Thanks again for the review. I have now pushed one more commit which adds the necessary docstrings, so the PR might be ready to be merged.

brettcannon · 2019-04-01T19:43:05Z

I don't quite have time to review again, but I did notice your docstrings weren't complete sentences (i.e. missing periods). We follow PEP 8 as a style so please update the docstrings (I know this can seem very nit-picky, but it helps readers know you actually meant for the sentence to end and didn't accidentally leave something out).

lysnikolaou · 2019-04-02T20:35:24Z

I added the missing periods, @brettcannon. Please let me know in case there is any way I can help out in making the PR easier to review.

brettcannon · 2019-04-11T21:12:03Z

@lysnikolaou unless you can magically find me more time, it's just a matter of work easing up a bit (I'm hoping I will get to this either this week or next).

brettcannon

Some minor structural things to help keep the code a little easier to maintain.

brettcannon · 2019-04-18T23:40:32Z

-                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)


The patching of the first message seems to occur regardless of whether the issue # was validated. Do you think it would be better to not create a URL if we can check that it won't return a 404?

Yeah, that sounds like a good improvement.

brettcannon · 2019-04-18T23:43:20Z

-        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)


Any reason to not pass in issue_number since you already have that pulled out?

When I created the two functions create_failure_status_issue_not_on_bpo and create_failure_status_no_issue, I wanted them to be consistent with how create_success_status was already doing things. That's why I passed issue_number_found as the parameter and not issue_number. I agree that it would be better to pass issue_number though, since that is already there.

brettcannon · 2019-04-18T23:43:48Z


+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")


What is the benefit of not passing in the issue number directly instead of the re.Match object?

lysnikolaou · 2019-04-19T12:01:09Z

@brettcannon we should maybe also consider that validating issues in the tests has really slowed down test_bpo, because it has to wait for the responses. Do you think that's an issue or is it okay?

2. Refactor, so that the first message does not get patched even if the issue is not on bpo

brettcannon · 2019-04-22T23:00:13Z

@lysnikolaou Stubbing/mocking/patching out the b.p.o calls for all tests but the one(s) that are specifically testing this functionality would be good.

When you're ready for me to re-review, let me know (I think you can click on my review status to ask for a new review now in GitHub).

lysnikolaou · 2019-05-04T15:18:45Z

@brettcannon Sorry for my delayed response, I had no time at all in the few last days. I am not sure, how I could do that, since there is no direct calling of the functions from the tests. Should we maybe add another field to the event parameter and the check for that in set_status? That doesn't seem like the best way to do it, so a little bit of guidance would be needed.

brettcannon · 2019-05-06T17:27:45Z

@lysnikolaou you can use https://docs.pytest.org/en/latest/monkeypatch.html. And don't worry about the test mocking out functions that are not publicly exposed as we're fine with glass box testing.

lysnikolaou · 2019-05-06T19:55:16Z

@brettcannon I have patched out the bpo calls from all the tests that would make the call and do not test this specifically. I am not sure, if that's the best way to do it, so your review is needed once more.

brettcannon

I think a tweak to your mocking approach might be needed as I think you're avoiding the costly function a bit too aggressively 😉

brettcannon · 2019-05-31T22:18:09Z

+        issue_number = issue_number_found.group("issue")
+        try:
+            issue_number_on_bpo = await _validate_issue_number(issue_number)
+        except NameError:


What is triggering a NameError? That shouldn't be occurring and so that makes this try block suspicious. If it's because of how aiohttp functions then it should be contained within _validate_issue_number() so it effectively can't fail unless there's a critical exception that should bubble up to get logged.

brettcannon · 2019-05-31T22:20:17Z

+                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}


Is "maintainer_can_modify" required for this to work, or are you just sneaking that in here? If it's the latter then I would pull it out as it's easy to lose track of. Better to open an issue and do a separate check for it as this isn't related to issue numbers from bugs.python.org which is what this module is aimed at.

brettcannon · 2019-05-31T22:25:32Z

+@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')


The problem with the deletion of the function is now you don't know if the function was called with the appropriate issue number. I would set up a mock and verify that it was called with the expected issue number. That way you can also return false as expected to make sure everything is calling that function the way you expect it to.

brettcannon · 2019-05-31T22:27:48Z

+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:


Instead of creating a new session for aiohttp, you could pass it in through the __main__ method where dispatch occurs to try and maximize connection pooling. All routes should be acceptings *args, **kwargs so passing in a session keyword argument should work fine.

lysnikolaou · 2019-06-12T01:14:33Z

Hey @brettcannon, sorry for the delay again. Life gets in the way. For the last few days, I've been trying to implement all the proposed changes and I have an implementation that uses pytest-mock. First of all would that be ok? Second of all, if I have understood correctly, one cannot mock an async function. I have found the asynctest library, which provides a CoroutineMock. Should I use that or is there maybe another, easier way?

brettcannon · 2019-06-12T23:59:54Z

It isn't like I'm very fast on my reviews so no need to apologize on taking your time. 😄

As for mocking, I have no specific guidance beyond you can use the hand-coded mocks as necessary, but otherwise I have no opinion on what 3rd-party packages to use.

lysnikolaou · 2019-07-13T01:44:10Z

@brettcannon I used asynctest to mock out _validate_issue_number and it works alright up to a single method, which is successful on my local machine and when I debug it. Would you have an idea, what's wrong here?

Moreover, sending a kwarg in __main__.py does not work for me? Should it just work out of the box, without any additional considerations?

brettcannon · 2019-07-26T20:23:32Z

@lysnikolaou I think the mocking problem is that you are using a fixture to blindly assign the mock, which means you never actually put the object you mocked out back. I would drop your custom fixture and instead use http://doc.pytest.org/en/latest/monkeypatch.html and set a mock as needed in each test. That way monkeypatch can clean up the mock afterwards and put everything back to the way it was.

As for your __main__.py question, I realize my comment was way too vague. 😄 What I meant to say if you could tweak https://github.com/python/bedevere/blob/master/bedevere/__main__.py#L40 to pass in session=session. That way you are using the same session object and getting better connection pooling.

lysnikolaou · 2019-08-06T18:17:32Z

@brettcannon Both monkeypatching and the passing of session from main are now implemented. Because _validate_issue_number is being monkeypatched out during most of the tests, I do not create new session objects for those, which means that session gets the default value "" during those and then that gets passed to the mocked function. Do you think that's okay like that?

brettcannon

This is looking great and I think it's really close to being ready to merge. Just a few minor things to tweak.

brettcannon · 2019-08-09T21:49:01Z

-                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")
+        session = kwargs.get('session', '')


Instead of doing this I would add session as keyword argument to this function with a default value of None.

brettcannon · 2019-08-09T21:52:46Z

+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"


What do you think of changing this to be the issue URL to show that it's a 404? If they specified an issue number they know that's what they should be doing so I don't think linking back to the docs is going to be the most helpful option for us.

I totally agree. I will change the url to link to the (non-existing) issue.

brettcannon · 2019-08-09T21:54:34Z

+    }
+    event = sansio.Event(data, event="pull_request", delivery_id="12345")
+    gh = FakeGH()
+    session = aiohttp.ClientSession()


Any reason not to use the context manager to make sure it closes?

brettcannon · 2019-08-09T21:57:06Z

    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':


If this is necessary then it might be better to have a helper function for the tests which does everything that's the same and then break this into two separate tests that do the appropriate mock check that's unique to each.

2. Change failure url to point to bpo

…losed

brettcannon

One last minor change to make the tests slightly easier to read.

brettcannon · 2019-09-10T11:02:19Z

    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)
