Merge ChromiumWPT functionality into TestExporter, use ChromiumCommits

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 d23f73b21972aa8b4ff4f8355fe30e1b5a497901..17004ca09e5a521e6030a94c08d9fec3a5deb0ab 100644
--- a/third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_exporter.py
+++ b/third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_exporter.py
@@ -4,10 +4,11 @@
 
 import logging
 from webkitpy.w3c.local_wpt import LocalWPT
-from webkitpy.w3c.chromium_wpt import ChromiumWPT
 from webkitpy.w3c.chromium_commit import ChromiumCommit
+from webkitpy.w3c.deps_updater import DepsUpdater
 
 _log = logging.getLogger(__name__)
+CHROMIUM_WPT_DIR = 'third_party/WebKit/LayoutTests/imported/wpt/'

[qyearsley, 2016/12/16 00:45:17]

Nit: Optional: Could add a blank line after `import logging` and after `_log =
...`.

 
 
 class TestExporter(object):
@@ -44,7 +45,6 @@ class TestExporter(object):
         # At this point, no in-flight PRs should exist
         # If there was an issue merging, it should have errored out
         local_wpt = LocalWPT(self.host, use_github=False)
-        chromium_wpt = ChromiumWPT(self.host)
 
         # 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
@@ -62,8 +62,7 @@ class TestExporter(object):
         _log.info('chromium@%s', chromium_commit.sha)
         _log.info('(%d behind chromium@origin/master)', chromium_commit.num_behind_master())
 
-        # TODO(jeffcarp): Have this function return ChromiumCommits
-        exportable_commits = chromium_wpt.exportable_commits_since(chromium_commit.sha)
+        exportable_commits = self.exportable_commits_since(chromium_commit.sha)
 
         if not exportable_commits:
             _log.info('No exportable commits found in Chromium, stopping.')
@@ -71,9 +70,9 @@ class TestExporter(object):
 
         _log.info('Found %d exportable commits in Chromium:', len(exportable_commits))
         for commit in exportable_commits:
-            _log.info('- %s %s', commit, chromium_wpt.subject(commit))
+            _log.info('- %s %s', commit, commit.subject())
 
-        outbound_commit = ChromiumCommit(self.host, sha=exportable_commits[0])
+        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())
 
@@ -98,3 +97,40 @@ class TestExporter(object):
             body=outbound_commit.body())
 
         _log.info('Create PR response: %s', response_data)
+
+    def exportable_commits_since(self, commit):
+        """Returns SHAs of exportable commits since `commit` in chronological order.
+
+        Args:
+            commit: The SHA of the Chromium commit from which this method will look.
+        """
+        toplevel = self.host.executive.run_command([

[qyearsley, 2016/12/16 00:45:17]

toplevel is the repo root, right? Is this necessary here? Is there a better
name?

[jeffcarp, 2016/12/16 20:07:10]

Yep, I can rename it to repo_root. Is there a cleaner way to achieve this?

+            'git', 'rev-parse', '--show-toplevel'
+        ]).strip()
+
+        commits = self.host.executive.run_command([
+            'git', 'rev-list', '{}..HEAD'.format(commit), '--reverse',
+            '--', toplevel + '/' + CHROMIUM_WPT_DIR
+        ]).splitlines()
+
+        chromium_commits = [ChromiumCommit(self.host, sha=c) for c in commits]
+
+        def is_exportable(chromium_commit):
+            message = chromium_commit.message()
+            return (
+                'NOEXPORT=true' not in message
+                and not message.startswith('Import ')
+                # TODO(jeffcarp): change this to allow any commit with
+                #   any non-expectation changes to be exportable
+                and not self._has_expectations(chromium_commit)
+            )

[qyearsley, 2016/12/16 00:45:17]

Doesn't make much of a difference here, but this could also be made a
class-level (non-nested) method _is_exportable. Optionally it could be a static
method.

[jeffcarp, 2016/12/16 20:07:10]

It seemed cleaner to have it in here because it's only used within this method,
but now that you bring it up a static is_exportable method might be useful to
other modules too.

This method will change a bit in later CLs, can we revisit this after those?

+
+        return filter(is_exportable, chromium_commits)
+
+    def _has_expectations(self, chromium_commit):
+        files = self.host.executive.run_command([
+            'git', 'diff-tree', '--no-commit-id',
+            '--name-only', '-r', chromium_commit.sha
+        ]).splitlines()
+
+        return any(DepsUpdater.is_baseline(f) for f in files)