Update exporter to use WPTGitHub.pr_for_chromium_commit

This is a follow-up to https://crrev.com/c/558200,
which replaces corresponding_pull_request_for_commit with
WPTGitHub.pr_for_chromium_commit, so that change ID is used first,
and position is only used as a backup.

This CL also changes MockWPTGitHub and MockChromiumCommit, updates the
tests, and adds an ommitted `return` in pr_for_chromium_commit.

Change-Id: Iae2c3eaef91d6848deee7091d39ac8298e588c66
Reviewed-on: https://chromium-review.googlesource.com/565923
Commit-Queue: Quinten Yearsley <qyearsley@chromium.org>
Reviewed-by: Jeff Carpenter <jeffcarp@chromium.org>
Cr-Commit-Position: refs/heads/master@{#488082}

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

@@ -9,15 +9,19 @@ class MockChromiumCommit(object):
 
     def __init__(self, host,
                  position='refs/heads/master@{#123}',
-                 message='Fake commit message',
-                 patch='Fake patch contents',
-                 change_id='Iba5eba11'):
+                 change_id='Iba5eba11',
+                 author='Fake author',
+                 subject='Fake subject',
+                 body='Fake body',
+                 patch='Fake patch contents'):
         self.host = host
         self.position = position
         self.sha = hashlib.sha1(position).hexdigest()
-        self._message = message
-        self._patch = patch
         self._change_id = change_id
+        self._author = author
+        self._subject = subject
+        self._body = body
+        self._patch = patch
 
     @property
     def short_sha(self):
@@ -32,11 +36,17 @@ class MockChromiumCommit(object):
     def url(self):
         return 'https://fake-chromium-commit-viewer.org/+/%s' % self.short_sha
 
-    def message(self):
-        return self._message
+    def author(self):
+        return self._author
 
     def subject(self):
-        return self._message
+        return self._subject
+
+    def body(self):
+        return self._body + '\n\nChange-Id: ' + self.change_id()
+
+    def message(self):
+        return self.subject() + '\n\n' + self.body()
 
     def format_patch(self):
         return self._patch

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

@@ -52,36 +52,35 @@ class CommonTest(unittest.TestCase):
             ['git', 'diff-tree', '--name-only', '--no-commit-id', '-r', 'add087a97844f4b9e307d9a216940582d96db306', '--',
              '/mock-checkout/third_party/WebKit/LayoutTests/external/wpt'],
             ['git', 'format-patch', '-1', '--stdout', 'add087a97844f4b9e307d9a216940582d96db306', '--', 'some', 'files'],
-            ['git', 'footers', '--key', 'Change-Id', 'add087a97844f4b9e307d9a216940582d96db306'],
         ])
 
     def test_is_exportable(self):
-        commit = MockChromiumCommit(MockHost(), message='Message')
+        commit = MockChromiumCommit(MockHost())
         github = MockWPTGitHub(pull_requests=[])
         self.assertTrue(is_exportable(commit, MockLocalWPT(), github))
 
     def test_commit_with_noexport_is_not_exportable(self):
-        commit = MockChromiumCommit(MockHost(), message='Message\nNo-Export: true')
+        commit = MockChromiumCommit(MockHost(), body='Message\nNo-Export: true')
         github = MockWPTGitHub(pull_requests=[])
         self.assertFalse(is_exportable(commit, MockLocalWPT(), github))
 
         # The older NOEXPORT tag also makes it non-exportable.
-        old_commit = MockChromiumCommit(MockHost(), message='Message\nNOEXPORT=true')
+        old_commit = MockChromiumCommit(MockHost(), body='Message\nNOEXPORT=true')
         self.assertFalse(is_exportable(old_commit, MockLocalWPT(), github))
 
         # No-Export/NOEXPORT in a revert CL also makes it non-exportable.
-        revert = MockChromiumCommit(MockHost(), message='Revert of Message\n> No-Export: true')
+        revert = MockChromiumCommit(MockHost(), body='Revert of Message\n> No-Export: true')
         self.assertFalse(is_exportable(revert, MockLocalWPT(), github))
-        old_revert = MockChromiumCommit(MockHost(), message='Revert of Message\n> NOEXPORT=true')
+        old_revert = MockChromiumCommit(MockHost(), body='Revert of Message\n> NOEXPORT=true')
         self.assertFalse(is_exportable(old_revert, MockLocalWPT(), github))
 
     def test_commit_that_starts_with_import_is_not_exportable(self):
-        commit = MockChromiumCommit(MockHost(), message='Import message')
+        commit = MockChromiumCommit(MockHost(), subject='Import message')
         github = MockWPTGitHub(pull_requests=[])
         self.assertFalse(is_exportable(commit, MockLocalWPT(), github))
 
     def test_commit_that_has_open_pr_is_exportable(self):
-        commit = MockChromiumCommit(MockHost(), change_id='Change-Id: I00decade')
+        commit = MockChromiumCommit(MockHost(), change_id='I00decade')
         github = MockWPTGitHub(pull_requests=[
             PullRequest('PR1', 1, 'body\nChange-Id: I00c0ffee', 'closed', []),
             PullRequest('PR2', 2, 'body\nChange-Id: I00decade', 'open', []),
@@ -89,7 +88,7 @@ class CommonTest(unittest.TestCase):
         self.assertTrue(is_exportable(commit, MockLocalWPT(), github))
 
     def test_commit_that_has_closed_pr_is_not_exportable(self):
-        commit = MockChromiumCommit(MockHost(), change_id='Change-Id: I00decade')
+        commit = MockChromiumCommit(MockHost(), change_id='I00decade')
         github = MockWPTGitHub(pull_requests=[
             PullRequest('PR1', 1, 'body\nChange-Id: I00c0ffee', 'closed', []),
             PullRequest('PR2', 2, 'body\nChange-Id: I00decade', 'closed', []),

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

@@ -40,7 +40,7 @@ class TestExporter(object):
 
         exportable_commits = self.get_exportable_commits()
         for exportable_commit in exportable_commits:
-            pull_request = self.corresponding_pull_request_for_commit(exportable_commit)
+            pull_request = self.wpt_github.pr_for_chromium_commit(exportable_commit)
 
             if pull_request:
                 if pull_request.state == 'open':
@@ -247,20 +247,3 @@ class TestExporter(object):
             ))
 
         return response_data
-
-    def corresponding_pull_request_for_commit(self, exportable_commit):
-        """Search pull requests for one that corresponds to exportable_commit.
-
-        Returns the pull_request if found, else returns None.
-        """
-        # Check for PRs created by open Gerrit CLs.
-        change_id = exportable_commit.change_id()
-        if change_id:
-            return self.wpt_github.pr_with_change_id(change_id)
-
-        # Check for PRs created by commits on master.
-        pull_request = self.wpt_github.pr_with_position(exportable_commit.position)
-        if pull_request:
-            return pull_request
-
-        return None

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

@@ -8,11 +8,12 @@ import unittest
 from webkitpy.common.host_mock import MockHost
 from webkitpy.common.system.executive_mock import MockExecutive, mock_git_commands
 from webkitpy.w3c.chromium_commit import ChromiumCommit
+from webkitpy.w3c.chromium_commit_mock import MockChromiumCommit
+from webkitpy.w3c.gerrit import GerritCL
+from webkitpy.w3c.gerrit_mock import MockGerritAPI
 from webkitpy.w3c.test_exporter import TestExporter
 from webkitpy.w3c.wpt_github import PullRequest
 from webkitpy.w3c.wpt_github_mock import MockWPTGitHub
-from webkitpy.w3c.gerrit import GerritCL
-from webkitpy.w3c.gerrit_mock import MockGerritAPI
 
 
 class TestExporterTest(unittest.TestCase):
@@ -39,9 +40,9 @@ class TestExporterTest(unittest.TestCase):
         test_exporter.run()
 
         self.assertEqual(test_exporter.wpt_github.calls, [
-            'pr_with_position',
-            'pr_with_position',
-            'pr_with_position',
+            'pr_for_chromium_commit',
+            'pr_for_chromium_commit',
+            'pr_for_chromium_commit',
         ])
 
     def test_creates_pull_request_for_all_exportable_commits(self):
@@ -77,16 +78,16 @@ class TestExporterTest(unittest.TestCase):
         test_exporter.run()
 
         self.assertEqual(test_exporter.wpt_github.calls, [
-            'pr_with_change_id',
-            'pr_with_change_id',
-            'pr_with_change_id',
-            'pr_with_change_id',
+            'pr_for_chromium_commit',
+            'pr_for_chromium_commit',
+            'pr_for_chromium_commit',
+            'pr_for_chromium_commit',
             'create_pr',
             'add_label "chromium-export"',
-            'pr_with_change_id',
+            'pr_for_chromium_commit',
             'create_pr',
             'add_label "chromium-export"',
-            'pr_with_change_id',
+            'pr_for_chromium_commit',
             'create_pr',
             'add_label "chromium-export"',
         ])
@@ -103,51 +104,52 @@ class TestExporterTest(unittest.TestCase):
         # 4. #458478 has an in-flight PR associated with it and should be merged successfully.
         host = MockHost()
         host.executive = mock_git_commands({
-            'show': 'git show text\nCr-Commit-Position: refs/heads/master@{#458476}',
+            'show': 'git show text\nCr-Commit-Position: refs/heads/master@{#458476}\nChange-Id: I0476',
             'crrev-parse': 'c2087acb00eee7960339a0be34ea27d6b20e1131',
+
         })
         test_exporter = TestExporter(host, 'gh-username', 'gh-token', gerrit_user=None, gerrit_token=None)
         test_exporter.wpt_github = MockWPTGitHub(pull_requests=[
             PullRequest(
                 title='Open PR',
                 number=1234,
-                body='rutabaga\nCr-Commit-Position: refs/heads/master@{#458475}',
+                body='rutabaga\nCr-Commit-Position: refs/heads/master@{#458475}\nChange-Id: I0005',
                 state='open',
                 labels=['do not merge yet']
             ),
             PullRequest(
                 title='Merged PR',
                 number=2345,
-                body='rutabaga\nCr-Commit-Position: refs/heads/master@{#458477}',
+                body='rutabaga\nCr-Commit-Position: refs/heads/master@{#458477}\nChange-Id: Idead',
                 state='closed',
                 labels=[]
             ),
             PullRequest(
                 title='Open PR',
                 number=3456,
-                body='rutabaga\nCr-Commit-Position: refs/heads/master@{#458478}',
+                body='rutabaga\nCr-Commit-Position: refs/heads/master@{#458478}\nChange-Id: I0118',
                 state='open',
                 labels=[]  # It's important that this is empty.
             ),
         ], unsuccessful_merge_index=0)
         test_exporter.gerrit = MockGerritAPI(host, 'gerrit-username', 'gerrit-token')
         test_exporter.get_exportable_commits = lambda: [
-            ChromiumCommit(host, position='refs/heads/master@{#458475}'),
-            ChromiumCommit(host, position='refs/heads/master@{#458476}'),
-            ChromiumCommit(host, position='refs/heads/master@{#458477}'),
-            ChromiumCommit(host, position='refs/heads/master@{#458478}'),
+            MockChromiumCommit(host, position='refs/heads/master@{#458475}', change_id='I0005'),
+            MockChromiumCommit(host, position='refs/heads/master@{#458476}', change_id='I0476'),
+            MockChromiumCommit(host, position='refs/heads/master@{#458477}', change_id='Idead'),
+            MockChromiumCommit(host, position='refs/heads/master@{#458478}', change_id='I0118'),
         ]
         test_exporter.run()
         self.assertEqual(test_exporter.wpt_github.calls, [
-            'pr_with_position',
+            'pr_for_chromium_commit',
             'remove_label "do not merge yet"',
             'get_pr_branch',
             'merge_pull_request',
-            'pr_with_position',
+            'pr_for_chromium_commit',
             'create_pr',
             'add_label "chromium-export"',
-            'pr_with_position',
-            'pr_with_position',
+            'pr_for_chromium_commit',
+            'pr_for_chromium_commit',
             # Testing the lack of remove_label here. The exporter should not
             # try to remove the provisional label from PRs it has already
             # removed it from.
@@ -156,9 +158,7 @@ class TestExporterTest(unittest.TestCase):
             'delete_remote_branch',
         ])
         self.assertEqual(test_exporter.wpt_github.pull_requests_created, [
-            ('chromium-export-c2087acb00',
-             'git show text\nCr-Commit-Position: refs/heads/master@{#458476}',
-             'git show text\nCr-Commit-Position: refs/heads/master@{#458476}'),
+            ('chromium-export-52c3178508', 'Fake subject', 'Fake body\n\nChange-Id: I0476'),
         ])
 
     def test_new_gerrit_cl(self):
@@ -291,7 +291,7 @@ class TestExporterTest(unittest.TestCase):
         test_exporter.run()
 
         self.assertEqual(test_exporter.wpt_github.calls, [
-            'pr_with_change_id',
+            'pr_for_chromium_commit',
             'remove_label "do not merge yet"',
             'get_pr_branch',
             'merge_pull_request',

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

@@ -26,7 +26,7 @@ class TestImporterTest(LoggingTestCase):
         ])
         importer = TestImporter(host, wpt_github=wpt_github)
         importer.exportable_but_not_exported_commits = lambda _: [
-            MockChromiumCommit(host, change_id='Iba5eba11')
+            MockChromiumCommit(host, subject='Fake PR subject', change_id='Iba5eba11')
         ]
         importer.checkout_is_okay = lambda: True
         return_code = importer.main(['--credentials-json=/tmp/creds.json'])
@@ -35,7 +35,7 @@ class TestImporterTest(LoggingTestCase):
             'INFO: Cloning GitHub w3c/web-platform-tests into /tmp/wpt\n',
             'INFO: There were exportable but not-yet-exported commits:\n',
             'INFO: Commit: https://fake-chromium-commit-viewer.org/+/14fd77e88e\n',
-            'INFO: Subject: Fake commit message\n',
+            'INFO: Subject: Fake PR subject\n',
             'INFO: PR: https://github.com/w3c/web-platform-tests/pull/5\n',
             'INFO: Modified files in wpt directory in this commit:\n',
             'INFO:   third_party/WebKit/LayoutTests/external/wpt/one.html\n',
@@ -50,7 +50,7 @@ class TestImporterTest(LoggingTestCase):
         wpt_github = MockWPTGitHub(pull_requests=[])
         importer = TestImporter(host, wpt_github=wpt_github)
         importer.exportable_but_not_exported_commits = lambda _: [
-            MockChromiumCommit(host, position='refs/heads/master@{#431}')
+            MockChromiumCommit(host, subject='Fake PR subject', position='refs/heads/master@{#431}')
         ]
         importer.checkout_is_okay = lambda: True
         return_code = importer.main(['--credentials-json=/tmp/creds.json'])
@@ -59,7 +59,7 @@ class TestImporterTest(LoggingTestCase):
             'INFO: Cloning GitHub w3c/web-platform-tests into /tmp/wpt\n',
             'INFO: There were exportable but not-yet-exported commits:\n',
             'INFO: Commit: https://fake-chromium-commit-viewer.org/+/fa2de685c0\n',
-            'INFO: Subject: Fake commit message\n',
+            'INFO: Subject: Fake PR subject\n',
             'WARNING: No pull request found.\n',
             'INFO: Modified files in wpt directory in this commit:\n',
             'INFO:   third_party/WebKit/LayoutTests/external/wpt/one.html\n',

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

@@ -63,19 +63,25 @@ class MockWPTGitHub(object):
         return 'fake branch for PR {}'.format(number)
 
     def pr_for_chromium_commit(self, commit):
-        return self.pr_with_change_id(commit.change_id())
+        self.calls.append('pr_for_chromium_commit')
+        for pr in self.pull_requests:
+            if commit.change_id() in pr.body:
+                return pr
+        return None
 
     def pr_with_position(self, position):
         self.calls.append('pr_with_position')
         for pr in self.pull_requests:
             if position in pr.body:
                 return pr
+        return None
 
     def pr_with_change_id(self, change_id):
         self.calls.append('pr_with_change_id')
         for pr in self.pull_requests:
             if change_id in pr.body:
                 return pr
+        return None
 
     def extract_metadata(self, tag, commit_body):
         for line in commit_body.splitlines():

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

@@ -6,7 +6,8 @@ import base64
 import unittest
 
 from webkitpy.common.host_mock import MockHost
-from webkitpy.w3c.wpt_github import WPTGitHub, MergeError, GitHubError
+from webkitpy.w3c.chromium_commit_mock import MockChromiumCommit
+from webkitpy.w3c.wpt_github import GitHubError, MergeError, PullRequest, WPTGitHub
 
 
 class WPTGitHubTest(unittest.TestCase):
@@ -49,3 +50,23 @@ class WPTGitHubTest(unittest.TestCase):
 
         with self.assertRaises(GitHubError):
             self.wpt_github.delete_remote_branch('rutabaga')
+
+    def test_pr_for_chromium_commit_prefers_change_id(self):
+        self.wpt_github.all_pull_requests = lambda: [
+            PullRequest('PR1', 1, 'body\nChange-Id: I00c0ffee\nCr-Commit-Position: refs/heads/master@{#10}', 'open', []),
+            PullRequest('PR2', 2, 'body\nChange-Id: I00decade\nCr-Commit-Position: refs/heads/master@{#33}', 'open', []),
+        ]
+        chromium_commit = MockChromiumCommit(
+            MockHost(), change_id='I00decade', position='refs/heads/master@{#10}')
+        pull_request = self.wpt_github.pr_for_chromium_commit(chromium_commit)
+        self.assertEqual(pull_request.number, 2)
+
+    def test_pr_for_chromium_commit_falls_back_to_commit_position(self):
+        self.wpt_github.all_pull_requests = lambda: [
+            PullRequest('PR1', 1, 'body\nChange-Id: I00c0ffee\nCr-Commit-Position: refs/heads/master@{#10}', 'open', []),
+            PullRequest('PR2', 2, 'body\nChange-Id: I00decade\nCr-Commit-Position: refs/heads/master@{#33}', 'open', []),
+        ]
+        chromium_commit = MockChromiumCommit(
+            MockHost(), position='refs/heads/master@{#10}')
+        pull_request = self.wpt_github.pr_for_chromium_commit(chromium_commit)
+        self.assertEqual(pull_request.number, 1)