[WPT Export] Delete remote branch, add label, additional refactoring

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

Index: third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_exporter.py
diff --git a/third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_exporter.py b/third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_exporter.py
index 599b48d83642e50753971ca43857e0c7bee360c5..e47ba9d5101905718d48028d42176c0b7cfe002b 100644
--- a/third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_exporter.py
+++ b/third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_exporter.py
@@ -8,47 +8,55 @@ from webkitpy.w3c.local_wpt import LocalWPT
 from webkitpy.w3c.chromium_commit import ChromiumCommit
 
 _log = logging.getLogger(__name__)
+
 CHROMIUM_WPT_DIR = 'third_party/WebKit/LayoutTests/imported/wpt/'
 
+# TODO(jeffcarp): have the script running this fetch Chromium origin/master
+# TODO(jeffcarp): move WPT fetch out of its constructor to match planned ChromiumWPT pattern
 
 class TestExporter(object):
 
     def __init__(self, host, wpt_github, dry_run=False):
         self.host = host
         self.wpt_github = wpt_github
-        self.local_wpt = LocalWPT(self.host)
         self.dry_run = dry_run
+        self.local_wpt = LocalWPT(self.host)
 
     def run(self):
         # First, poll for an in-flight pull request and merge if exists
         pull_requests = self.wpt_github.in_flight_pull_requests()
 
+        # Only do one action at a time
+        # The script assumes it will be run every 10 minutes or so

[jeffcarp, 2017/01/03 20:42:31]

I broke up the run() function into 2 separate functions that are called based on
whether an in-flight PR exists: `merge_in_flight_pull_request` and
`export_first_exportable_commit`.

[qyearsley, 2017/01/06 19:20:24]

Excellent, those names sound clearer than run(). The result of
export_first_exportable_commit is that it will create an in-flight PR but not
necessarily merge right?

Also, my personal preference is to end all full-sentence comments with periods.

[jeffcarp, 2017/01/06 23:51:59]

It will necessarily not merge in export_first_exportable_commit. Since it takes
time for the PR to be verified on Travis CI, the script will only ever do one of
(merge PR, create PR) one each invocation.
Got it, I wish there was a lint rule for this

         if len(pull_requests) == 1:
-            pull_request = pull_requests.pop()
-
-            _log.info('In-flight PR found: #%d', pull_request['number'])
-            _log.info(pull_request['title'])
-
-            # TODO(jeffcarp): Check the PR status here
-
-            if self.dry_run:
-                _log.info('[dry_run] Would have attempted to merge PR')
-            else:
-                _log.info('Merging...')
-                self.wpt_github.merge_pull_request(pull_request['number'])
-                _log.info('PR merged!')
+            self.merge_in_flight_pull_request(pull_requests.pop())
         elif len(pull_requests) > 1:
             _log.error(pull_requests)
             # TODO(jeffcarp): Print links to PRs
             raise Exception('More than two in-flight PRs!')
+        else:
+            self.export_first_exportable_commit()
 
-        # Second, look for exportable commits in Chromium
-        # At this point, no in-flight PRs should exist
-        # If there was an issue merging, it should have errored out
+    def merge_in_flight_pull_request(self, pull_request):

[qyearsley, 2017/01/06 19:20:24]

Optionally you could add a one-line docstring here.

+        _log.info('In-flight PR found: #%d', pull_request['number'])
+        _log.info(pull_request['title'])
 
-        # TODO(jeffcarp): have the script running this fetch Chromium origin/master
-        # TODO(jeffcarp): move WPT fetch out of its constructor to match planned ChromiumWPT pattern
+        # TODO(jeffcarp): Check the PR status here (for Travis CI, etc.)
 
+        if self.dry_run:
+            _log.info('[dry_run] Would have attempted to merge PR')
+        else:
+            _log.info('Merging...')
+            self.wpt_github.merge_pull_request(pull_request['number'])
+            _log.info('PR merged! Deleting branch.')
+            # TODO(jeffcarp): Make branch name more available

[qyearsley, 2017/01/06 19:20:24]

What does "more available" mean?

[jeffcarp, 2017/01/06 23:51:59]

I honestly have no idea what I meant by that, haha. I'll remove this.

+            self.wpt_github.delete_remote_branch('chromium-export-try')
+            _log.info('Branch deleted!')

[qyearsley, 2017/01/06 19:20:24]

The else clause could be removed if a return is added in the `if self.dry_run`
clause.

[jeffcarp, 2017/01/06 23:51:59]

Great!

+
+    def export_first_exportable_commit(self):
+        # Look for exportable commits in Chromium
+        # At this point, no in-flight PRs should exist
+        # If there was an issue merging, it should have errored out

[qyearsley, 2017/01/06 19:20:24]

For consistency, this should be made into a docstring with a one-line summary
and more detailed notes below.

         wpt_commit, chromium_commit = self.local_wpt.most_recent_chromium_commit()
         assert chromium_commit, 'No Chromium commit found, this is impossible'
 
@@ -79,8 +87,6 @@ class TestExporter(object):
         patch = outbound_commit.format_patch()
         message = outbound_commit.message()
 
-        # TODO: now do a test comparison of patch against local WPT
-
         if self.dry_run:
             _log.info('[dry_run] Stopping before creating PR')
             _log.info('\n\n[dry_run] message:')
@@ -89,10 +95,10 @@ class TestExporter(object):
             _log.info(patch)
             return
 
-        local_branch_name = self.local_wpt.create_branch_with_patch(message, patch)
+        remote_branch_name = self.local_wpt.create_branch_with_patch(message, patch)
 
         response_data = self.wpt_github.create_pr(
-            local_branch_name=local_branch_name,
+            remote_branch_name=remote_branch_name,
             desc_title=outbound_commit.subject(),
             body=outbound_commit.body())