Refactor WPT Export to ensure only one PR in flight at a time

third_party/WebKit/Tools/Scripts/webkitpy/w3c/github.py

Index: third_party/WebKit/Tools/Scripts/webkitpy/w3c/github.py
diff --git a/third_party/WebKit/Tools/Scripts/webkitpy/w3c/github.py b/third_party/WebKit/Tools/Scripts/webkitpy/w3c/github.py
index 49a4a7618225eafd34331f4da185497dca753d14..80e0e33f28232c88ea98d50d52e45e420a828adb 100644
--- a/third_party/WebKit/Tools/Scripts/webkitpy/w3c/github.py
+++ b/third_party/WebKit/Tools/Scripts/webkitpy/w3c/github.py
@@ -7,6 +7,8 @@ import json
 import httplib2
 import logging
 
+from types import IntType
+
 _log = logging.getLogger(__name__)
 
 
@@ -54,3 +56,52 @@ class GitHub(object):
         if resp["status"] != "201":
             return None
         return json.loads(content)
+
+    def in_flight_pull_requests():
+        response, content = self.request('/search/issues?q=repo:w3c/web-platform-tests%20is:open%20type:pr%20labels:Chromium%20Export')
+        return json.loads(content)
+
+    def request(self, path, body):
+        API_BASE = 'https://api.github.com'

[qyearsley, 2016/11/22 22:08:20]

Since this is a local variable, convention would have it still be lower case
even though it's meant to be constant.

+        assert path.startswith('/')

[qyearsley, 2016/11/22 22:08:20]

Note, I think that this is another good use of assert:
 * It's expressing a precondition in a readable way
 * It's checking input passed in from other code here, not validating user input

+
+        headers = {
+            "Accept": "application/vnd.github.v3+json",
+            "Authorization": "Basic " + self.auth_token()
+        }
+
+        if body:
+            json_body = json.JSONEncoder().encode(body)

[qyearsley, 2016/11/22 22:08:19]

Is there a difference between json.JSONEncoder().encode(x) and json.dumps(x)?

[jeffcarp, 2016/11/22 22:30:15]

No idea, in practice it doesn't look like it. I was just mirroring the previous
pattern.

+        else:
+            json_body = None
+
+        conn = httplib2.Http()
+        return conn.request(API_BASE + path, body=json_body)
+
+    def merge_pull_request(self, number):

[qyearsley, 2016/11/22 22:08:20]

What type of number? Would it make sense to call this pull_request_number?

+        assert number and number is IntType

[qyearsley, 2016/11/22 22:08:19]

Another way to express this would be `assert type(number) is int`; no need to
import types.IntType.

Also, it's pretty common practice to not explicitly assert types, and just rely
on a TypeError being raised below
if something goes wrong. I don't have strong opinions about this though.

+        url = '/repos/w3c/web-platform-tests/pulls/%s/merge' % number

[qyearsley, 2016/11/22 22:08:20]

%s could be changed to %d; also, this variable could be called path instead of
url.

+        body = {}
+        response, content = self.request(url, body, headers)
+
+        if response['status'] == '200':
+            return json.loads(content)
+        else:
+            raise Exception('PR could not be merged: %d' % number)
+
+
+class MockWPTGitHub(object):

[qyearsley, 2016/11/22 22:08:19]

In other places, I've seen fakes like this put into their own modules (e.g.
buildbot_mock, filesystem_mock, host_mock)

+
+    def __init__(self, pull_requests={}, unsuccessful_merge=False):
+        self.pull_requests = pull_requests
+        self.unsuccessful_merge = unsuccessful_merge
+        self.calls = []
+
+    def in_flight_pull_requests(self):
+        self.calls.append('in_flight_pull_requests')
+        return self.pull_requests
+
+    def merge_pull_request(self, number):
+        self.calls.append('merge_pull_request')
+        if self.unsuccessful_merge:
+            raise Exception('PR could not be merged: %d' % number)