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

third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_exporter.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 logging
 
 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.dry_run = dry_run
         self.local_wpt = LocalWPT(self.host)
-        self.dry_run = dry_run
 
     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
+    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): 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.

-
-        # 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
-
         wpt_commit, chromium_commit = self.local_wpt.most_recent_chromium_commit()
         assert chromium_commit, 'No Chromium commit found, this is impossible'
 
         wpt_behind_master = self.local_wpt.commits_behind_master(wpt_commit)
 
         _log.info('\nLast Chromium export commit in web-platform-tests:')
         _log.info('web-platform-tests@%s', wpt_commit)
         _log.info('(%d behind web-platform-tests@origin/master)', wpt_behind_master)
 
         _log.info('\nThe above WPT commit points to the following Chromium commit:')
@@ skipping 10 matching lines @@
         for commit in exportable_commits:
             _log.info('- %s %s', commit, commit.subject())
 
         outbound_commit = exportable_commits[0]
         _log.info('Picking the earliest commit and creating a PR')
         _log.info('- %s %s', outbound_commit.sha, outbound_commit.subject())
 
         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:')
             _log.info(message)
             _log.info('\n\n[dry_run] patch:')
             _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())
 
         _log.info('Create PR response: %s', response_data)
 
         if response_data:
             data, status_code = self.wpt_github.add_label(response_data['number'])
             _log.info('Add label response (status %s): %s', status_code, data)
 
     def exportable_commits_since(self, commit):
@@ skipping 16 matching lines @@
         def is_exportable(chromium_commit):
             patch = chromium_commit.format_patch()
             return (
                 patch
                 and self.local_wpt.test_patch(patch)
                 and 'NOEXPORT=true' not in chromium_commit.message()
                 and not chromium_commit.message().startswith('Import ')
             )
 
         return [c for c in chromium_commits if is_exportable(c)]