Apply Chromium patches to local wpt and continue

Previously, the importer would check for exportable Chromium commits and
abort if any existed. This CL would make it so that instead of aborting,
the importer would apply the patch to the local WPT before copying
files over.

This should make it so importer is no longer blocked on exporter in
general, but it should still be the case that exportable Chromium changes
aren't clobbered.

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

third_party/WebKit/Tools/Scripts/webkitpy/common/system/executive_mock.py

@@ -61,7 +61,8 @@ class MockProcess(object):
         return
 
 
-MockCall = collections.namedtuple('MockCall', ('args', 'env'))
+MockCall = collections.namedtuple(
+    'MockCall', ('args', 'kwargs'))
 
 
 class MockExecutive(object):
@@ -89,13 +90,8 @@ class MockExecutive(object):
         self._proc = proc
         self.full_calls = []
 
-    def _append_call(self, args, env=None):
-        if env:
-            env = env.copy()
-        self.full_calls.append(MockCall(
-            args=args,
-            env=env.copy() if env is not None else None,
-        ))
+    def _append_call(self, args, **kwargs):
+        self.full_calls.append(MockCall(args=args, kwargs=kwargs))
 
     def check_running_pid(self, pid):
         return pid in self._running_pids.values()
@@ -126,7 +122,7 @@ class MockExecutive(object):
                     decode_output=False,
                     env=None,
                     debug_logging=False):
-        self._append_call(args, env=env)
+        self._append_call(args, cwd=cwd, input=input, env=env)
 
         assert isinstance(args, list) or isinstance(args, tuple)
 
@@ -170,7 +166,7 @@ class MockExecutive(object):
 
     def popen(self, args, cwd=None, env=None, **_):
         assert all(isinstance(arg, basestring) for arg in args)
-        self._append_call(args, env=env)
+        self._append_call(args, cwd=cwd, env=env)
         if self._should_log:
             cwd_string = ''
             if cwd:

third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/linux_unittest.py

@@ -147,7 +147,7 @@ class LinuxPortTest(port_testcase.PortTestCase, LoggingTestCase):
                 ['Xvfb', ':99', '-screen', '0', '1280x800x24', '-ac', '-dpi', '96'],
                 ['xdpyinfo'],
             ])
-        self.assertEqual(port.host.executive.full_calls[1].env.get('TMPDIR'), '/tmp')
+        self.assertEqual(port.host.executive.full_calls[1].kwargs['env'].get('TMPDIR'), '/tmp')
         env = port.setup_environ_for_server()
         self.assertEqual(env['DISPLAY'], ':99')
 

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

@@ -39,6 +39,9 @@ class ChromiumCommit(object):
         self.sha = sha
         self.position = position
 
+    def __str__(self):
+        return '{} "{}"'.format(self.short_sha, self.subject())
+
     @property
     def short_sha(self):
         return self.sha[0:10]

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

@@ -125,29 +125,37 @@ class LocalWPT(object):
 
         Args:
             patch: The patch to test against.
+            chromium_commit: The ChromiumCommit, for logging extra info.
 
         Returns:
             A string containing the diff the patch produced.
         """
         self.clean()
+        applied = self.apply_patch(patch)
+        output = self.run(['git', 'diff', 'origin/master'])
+        self.clean()
+        if applied:
+            return output
+        if chromium_commit:
+            _log.info('Commit: %s', chromium_commit.url())
+            _log.info('Commit subject: "%s"', chromium_commit.subject())
+        return ''
+
+    def apply_patch(self, patch):
+        """Applies a Chromium patch to the local WPT repo and stages.
 
+        Returns True if the patch could be applied, false otherwise.
+        """
         # Remove Chromium WPT directory prefix.
         patch = patch.replace(CHROMIUM_WPT_DIR, '')
-
         try:
             self.run(['git', 'apply', '-'], input=patch)
             self.run(['git', 'add', '.'])
-            output = self.run(['git', 'diff', 'origin/master'])
-        except ScriptError as e:
-            _log.info('Patch did not apply cleanly for the following commit:')
-            if chromium_commit:
-                _log.info('Commit: %s', chromium_commit.url())
-                _log.info('Commit subject: "%s"', chromium_commit.subject())
-                _log.info('Message: %s\n\n', e.message)
-            output = ''
-
-        self.clean()
-        return output
+        except ScriptError as error:
+            _log.warning('Patch did not apply cleanly for the following commit:')
+            _log.warning('Message: %s\n\n', error.message)
+            return False
+        return True
 
     def commits_behind_master(self, commit):
         """Returns the number of commits after the given commit on origin/master.

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

@@ -15,7 +15,7 @@ import argparse
 import logging
 
 from webkitpy.common.net.buildbot import current_build_link
-from webkitpy.common.net.git_cl import GitCL, TryJobStatus
+from webkitpy.common.net.git_cl import GitCL
 from webkitpy.common.path_finder import PathFinder
 from webkitpy.layout_tests.models.test_expectations import TestExpectations, TestExpectationParser
 from webkitpy.layout_tests.port.base import Port
@@ -69,24 +69,6 @@ class TestImporter(object):
 
         local_wpt.fetch()
 
-        if not options.ignore_exportable_commits:
-            commits = self.exportable_but_not_exported_commits(local_wpt)
-            if commits:
-                _log.info('There were exportable but not-yet-exported commits:')
-                for commit in commits:
-                    _log.info('Commit: %s', commit.url())
-                    _log.info('Subject: %s', commit.subject().strip())
-                    pull_request = self.wpt_github.pr_for_chromium_commit(commit)
-                    if pull_request:
-                        _log.info('PR: https://github.com/w3c/web-platform-tests/pull/%d', pull_request.number)
-                    else:
-                        _log.warning('No pull request found.')
-                    _log.info('Modified files in wpt directory in this commit:')
-                    for path in commit.filtered_changed_files():
-                        _log.info('  %s', path)
-                _log.info('Aborting import to prevent clobbering commits.')
-                return 0
-
         if options.revision is not None:
             _log.info('Checking out %s', options.revision)
             self.run(['git', 'checkout', options.revision], cwd=local_wpt.path)
@@ -95,6 +77,17 @@ class TestImporter(object):
         _, show_ref_output = self.run(['git', 'show-ref', 'origin/master'], cwd=local_wpt.path)
         import_commit = 'wpt@%s' % show_ref_output.split()[0]
 
+        commit_message = self._commit_message(chromium_commit, import_commit)
+
+        if not options.ignore_exportable_commits:
+            commits = self.apply_exportable_commits_locally(local_wpt)
+            if commits is None:
+                _log.error('Could not apply some exportable commits cleanly.')
+                _log.error('Aborting import to prevent clobbering commits.')
+                return 1
+            commit_message = self._commit_message(
+                chromium_commit, import_commit, locally_applied_commits=commits)
+
         dest_path = self.finder.path_from_layout_tests('external', 'wpt')
 
         self._clear_out_dest_path(dest_path)
@@ -122,7 +115,7 @@ class TestImporter(object):
             _log.info('Done: no changes to import.')
             return 0
 
-        commit_message = self._commit_message(chromium_commit, import_commit)
+        self.run(['git', 'commit', '--all', '-F', '-'], stdin=commit_message)
         self._commit_changes(commit_message)
         _log.info('Changes imported and committed.')
 
@@ -231,6 +224,44 @@ class TestImporter(object):
             _log.warning('Checkout has local commits before import.')
         return True
 
+    def apply_exportable_commits_locally(self, local_wpt):
+        """Applies exportable Chromium changes to the local WPT repo.
+
+        The purpose of this is to avoid clobbering changes that were made in
+        Chromium but not yet merged upstream. By applying these changes to the
+        local copy of web-platform-tests before copying files over, we make
+        it so that the resulting change in Chromium doesn't undo the
+        previous Chromium change.
+
+        Args:
+            A LocalWPT instance for our local copy of WPT.
+
+        Returns:
+            A list of commits applied (could be empty), or None if any
+            of the patches could not be applied cleanly.
+        """
+        commits = self.exportable_but_not_exported_commits(local_wpt)
+        for commit in commits:
+            _log.info('Applying exportable commit locally:')
+            _log.info(commit.url())
+            _log.info('Subject: %s', commit.subject().strip())
+            # TODO(qyearsley): We probably don't need to know about
+            # corresponding PRs at all anymore, although this information
+            # could still be useful for reference.
+            pull_request = self.wpt_github.pr_for_chromium_commit(commit)
+            if pull_request:
+                _log.info('PR: https://github.com/w3c/web-platform-tests/pull/%d', pull_request.number)
+            else:
+                _log.warning('No pull request found.')
+            applied = local_wpt.apply_patch(commit.format_patch())
+            if not applied:
+                return None
+            self.run(
+                ['git', 'commit', '--all', '-F', '-'],
+                stdin='Applying patch %s' % commit.sha,
+                cwd=local_wpt.path)
+        return commits
+
     def exportable_but_not_exported_commits(self, local_wpt):
         """Returns a list of commits that would be clobbered by importer."""
         return exportable_commits_over_last_n_commits(self.host, local_wpt, self.wpt_github)
@@ -271,11 +302,15 @@ class TestImporter(object):
         return_code, _ = self.run(['git', 'diff', '--quiet', 'HEAD'], exit_on_failure=False)
         return return_code == 1
 
-    def _commit_message(self, chromium_commit, import_commit):
-        return ('Import %s\n\n'
-                'Using wpt-import in Chromium %s.\n\n'
-                'No-Export: true' %
-                (import_commit, chromium_commit))
+    def _commit_message(self, chromium_commit_sha, import_commit_sha,
+                        locally_applied_commits=None):
+        message = 'Import {}\n\nUsing wpt-import in Chromium {}.\n'.format(
+            import_commit_sha, chromium_commit_sha)
+        if locally_applied_commits is not None:
+            message += 'With Chromium commits locally applied on WPT:\n'
+            message += '\n'.join(str(commit) for commit in locally_applied_commits)
+        message += '\nNo-Export: true'
+        return message
 
     def _delete_orphaned_baselines(self, dest_path):
         _log.info('Deleting any orphaned baselines.')

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

@@ -7,66 +7,17 @@ from webkitpy.common.host_mock import MockHost
 from webkitpy.common.net.buildbot import Build
 from webkitpy.common.net.git_cl import TryJobStatus
 from webkitpy.common.net.git_cl_mock import MockGitCL
+from webkitpy.common.system.executive_mock import MockCall
 from webkitpy.common.system.executive_mock import MockExecutive
 from webkitpy.common.system.log_testing import LoggingTestCase
 from webkitpy.w3c.chromium_commit_mock import MockChromiumCommit
+from webkitpy.w3c.local_wpt import LocalWPT
 from webkitpy.w3c.test_importer import TestImporter
-from webkitpy.w3c.wpt_github import PullRequest
 from webkitpy.w3c.wpt_github_mock import MockWPTGitHub
 
 
 class TestImporterTest(LoggingTestCase):
 
-    def test_main_abort_on_exportable_commit_if_open_pr_found(self):
-        host = MockHost()
-        host.filesystem.write_text_file(
-            '/tmp/creds.json', '{"GH_USER": "x", "GH_TOKEN": "y"}')
-        wpt_github = MockWPTGitHub(pull_requests=[
-            PullRequest('Title', 5, 'Commit body\nChange-Id: Iba5eba11', 'open', []),
-        ])
-        importer = TestImporter(host, wpt_github=wpt_github)
-        importer.exportable_but_not_exported_commits = lambda _: [
-            MockChromiumCommit(host, subject='Fake PR subject', change_id='Iba5eba11')
-        ]
-        importer.checkout_is_okay = lambda: True
-        return_code = importer.main(['--credentials-json=/tmp/creds.json'])
-        self.assertEqual(return_code, 0)
-        self.assertLog([
-            '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 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',
-            'INFO:   third_party/WebKit/LayoutTests/external/wpt/two.html\n',
-            'INFO: Aborting import to prevent clobbering commits.\n',
-        ])
-
-    def test_main_abort_on_exportable_commit_if_no_pr_found(self):
-        host = MockHost()
-        host.filesystem.write_text_file(
-            '/tmp/creds.json', '{"GH_USER": "x", "GH_TOKEN": "y"}')
-        wpt_github = MockWPTGitHub(pull_requests=[])
-        importer = TestImporter(host, wpt_github=wpt_github)
-        importer.exportable_but_not_exported_commits = lambda _: [
-            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'])
-        self.assertEqual(return_code, 0)
-        self.assertLog([
-            '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 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',
-            'INFO:   third_party/WebKit/LayoutTests/external/wpt/two.html\n',
-            'INFO: Aborting import to prevent clobbering commits.\n',
-        ])
-
     def test_update_expectations_for_cl_no_results(self):
         host = MockHost()
         host.filesystem.write_text_file(
@@ -152,6 +103,52 @@ class TestImporterTest(LoggingTestCase):
             ['git', 'cl', 'set-close'],
         ])
 
+    def test_apply_exportable_commits_locally(self):
+        host = MockHost()
+        importer = TestImporter(host, wpt_github=MockWPTGitHub(pull_requests=[]))
+        fake_commit = MockChromiumCommit(
+            host, subject='My fake commit',
+            patch=(
+                'Fake patch contents...\n'
+                '--- a/third_party/WebKit/LayoutTests/external/wpt/css/css-ui-3/outline-004.html\n'
+                '+++ b/third_party/WebKit/LayoutTests/external/wpt/css/css-ui-3/outline-004.html\n'
+                '@@ -20,7 +20,7 @@\n'
+                '...'))
+        importer.exportable_but_not_exported_commits = lambda _: [fake_commit]
+        applied = importer.apply_exportable_commits_locally(LocalWPT(host))
+        self.assertEqual(applied, [fake_commit])
+        self.assertEqual(host.executive.full_calls, [
+            MockCall(
+                ['git', 'apply', '-'],
+                {
+                    'input': (
+                        'Fake patch contents...\n'
+                        '--- a/css/css-ui-3/outline-004.html\n'
+                        '+++ b/css/css-ui-3/outline-004.html\n'
+                        '@@ -20,7 +20,7 @@\n'
+                        '...'),
+                    'cwd': '/tmp/wpt',
+                    'env': None
+                }),
+            MockCall(
+                ['git', 'add', '.'],
+                kwargs={'input': None, 'cwd': '/tmp/wpt', 'env': None}),
+            MockCall(
+                ['git', 'commit', '--all', '-F', '-'],
+                kwargs={'cwd': '/tmp/wpt', 'env': None})
+        ])
+
+    def test_apply_exportable_commits_locally_returns_none_on_failure(self):
+        host = MockHost()
+        wpt_github = MockWPTGitHub(pull_requests=[])
+        importer = TestImporter(host, wpt_github=wpt_github)
+        commit = MockChromiumCommit(host, subject='My fake commit')
+        importer.exportable_but_not_exported_commits = lambda _: [commit]
+        local_wpt = LocalWPT(host)
+        local_wpt.apply_patch = lambda _: None  # Failure to apply patch.
+        applied = importer.apply_exportable_commits_locally(local_wpt)
+        self.assertIsNone(applied)
+
     def test_update_all_test_expectations_files(self):
         host = MockHost()
         host.filesystem.files['/mock-checkout/third_party/WebKit/LayoutTests/TestExpectations'] = (