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

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

 # Copyright 2016 The Chromium Authors. All rights reserved.
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
 import base64
 import json
 import httplib2
 import logging
 
+from types import IntType
+
 _log = logging.getLogger(__name__)
 
 
 class GitHub(object):
 
     def __init__(self, host):
         self.host = host
         self.user = self.host.environ.get('GH_USER')
         self.token = self.host.environ.get('GH_TOKEN')
 
@@ skipping 27 matching lines @@
             "body": body,
             "head": pr_branch_name,
             "base": "master"
         }
         resp, content = conn.request("https://api.github.com/repos/w3c/web-platform-tests/pulls",
                                      "POST", body=json.JSONEncoder().encode(body), headers=headers)
         _log.info("GitHub response: %s", content)
         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)