Reland "Refactor wpt-import to use common.checkout.git"
This is a reland of cd0c516c9dbe22b47d7f4325baae0a3b411ed634
Compared with the original CL, this one instantiates self.wpt_git
after local_wpt.fetch() to ensure local_wpt.path exists so that
Git.__init__ does not throw.
Original change's description:
> Refactor wpt-import to use common.checkout.git
>
> Some methods are not currently supported by common.checkout.git, and
> Git.run is used in such cases for now. Later we would like to port some
> of them (the reusable ones) to common.checkout.git.
>
> Bug: 676399
> Change-Id: I42459221c684f8ffeabb9991a64dea3489663d19
> Reviewed-on: https://chromium-review.googlesource.com/793984
> Reviewed-by: Quinten Yearsley <qyearsley@chromium.org>
> Commit-Queue: Robert Ma <robertma@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#519777}
Bug: 676399
Change-Id: I919064f5599a1b503a9ce5db927cc6713d9f428f
Reviewed-on: https://chromium-review.googlesource.com/794610
Reviewed-by: Quinten Yearsley <qyearsley@chromium.org>
Commit-Queue: Robert Ma <robertma@chromium.org>
Cr-Commit-Position: refs/heads/master@{#520115}diff --git a/third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_importer.py b/third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_importer.py
index 25f8022d..173235c 100644
--- a/third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_importer.py
+++ b/third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_importer.py
@@ -45,15 +45,27 @@
def __init__(self, host, wpt_github=None):
self.host = host
+ self.wpt_github = wpt_github
+
self.executive = host.executive
self.fs = host.filesystem
self.finder = PathFinder(self.fs)
- self.verbose = False
- self.git_cl = None
- self.wpt_github = wpt_github
+ self.chromium_git = self.host.git(self.finder.chromium_base())
self.dest_path = self.finder.path_from_layout_tests('external', 'wpt')
+ # A common.net.git_cl.GitCL instance.
+ self.git_cl = None
+ # Another Git instance with local WPT as CWD, which can only be
+ # instantiated after the working directory is created.
+ self.wpt_git = None
+ # The WPT revision we are importing.
+ self.wpt_revision = None
+ self.verbose = False
+
def main(self, argv=None):
+ # TODO(robertma): Test this method! Split it to make it easier to test
+ # if necessary.
+
options = self.parse_args(argv)
self.verbose = options.verbose
@@ -70,36 +82,36 @@
gh_user = credentials.get('GH_USER')
gh_token = credentials.get('GH_TOKEN')
self.wpt_github = self.wpt_github or WPTGitHub(self.host, gh_user, gh_token)
- local_wpt = LocalWPT(self.host, gh_token=gh_token)
self.git_cl = GitCL(self.host, auth_refresh_token_json=options.auth_refresh_token_json)
- _log.debug('Noting the current Chromium commit.')
- # TODO(qyearsley): Use Git (self.host.git) to run git commands.
- _, show_ref_output = self.run(['git', 'show-ref', '--verify', '--head', '--hash', 'HEAD'])
- chromium_commit = show_ref_output.strip()
+ _log.debug('Noting the current Chromium revision.')
+ chromium_revision = self.chromium_git.latest_git_commit()
+ # Instantiate Git after local_wpt.fetch() to make sure the path exists.
+ local_wpt = LocalWPT(self.host, gh_token=gh_token)
local_wpt.fetch()
+ self.wpt_git = self.host.git(local_wpt.path)
if options.revision is not None:
_log.info('Checking out %s', options.revision)
- self.run(['git', 'checkout', options.revision], cwd=local_wpt.path)
+ self.wpt_git.run(['checkout', options.revision])
_log.debug('Noting the revision we are importing.')
- _, show_ref_output = self.run(['git', 'show-ref', 'origin/master'], cwd=local_wpt.path)
- import_commit = 'wpt@%s' % show_ref_output.split()[0]
+ self.wpt_revision = self.wpt_git.latest_git_commit()
+ import_commit = 'wpt@%s' % self.wpt_revision
- _log.info('Importing %s to Chromium %s', import_commit, chromium_commit)
+ _log.info('Importing %s to Chromium %s', import_commit, chromium_revision)
- commit_message = self._commit_message(chromium_commit, import_commit)
-
- if not options.ignore_exportable_commits:
+ if options.ignore_exportable_commits:
+ commit_message = self._commit_message(chromium_revision, import_commit)
+ else:
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)
+ chromium_revision, import_commit, locally_applied_commits=commits)
self._clear_out_dest_path()
@@ -107,7 +119,8 @@
test_copier = TestCopier(self.host, local_wpt.path)
test_copier.do_import()
- self.run(['git', 'add', '--all', 'external/wpt'])
+ # TODO(robertma): Implement `add --all` in Git (it is different from `commit --all`).
+ self.chromium_git.run(['add', '--all', self.dest_path])
self._generate_manifest()
@@ -120,8 +133,7 @@
_log.info('Updating TestExpectations for any removed or renamed tests.')
self.update_all_test_expectations_files(self._list_deleted_tests(), self._list_renamed_tests())
- has_changes = self._has_changes()
- if not has_changes:
+ if not self.chromium_git.has_working_directory_changes():
_log.info('Done: no changes to import.')
return 0
@@ -177,10 +189,10 @@
if try_results and self.git_cl.some_failed(try_results):
self.fetch_new_expectations_and_baselines()
- if self.host.git().has_working_directory_changes():
+ if self.chromium_git.has_working_directory_changes():
self._generate_manifest()
message = 'Update test expectations and baselines.'
- self.check_run(['git', 'commit', '-a', '-m', message])
+ self._commit_changes(message)
self._upload_patchset(message)
return True
@@ -259,13 +271,11 @@
return parser.parse_args(argv)
def checkout_is_okay(self):
- git_diff_retcode, _ = self.run(
- ['git', 'diff', '--quiet', 'HEAD'], exit_on_failure=False)
- if git_diff_retcode:
+ if self.chromium_git.has_working_directory_changes():
_log.warning('Checkout is dirty; aborting.')
return False
- _, local_commits = self.run(
- ['git', 'log', '--oneline', 'origin/master..HEAD'])
+ # TODO(robertma): Add a method in Git to query a range of commits.
+ local_commits = self.chromium_git.run(['log', '--oneline', 'origin/master..HEAD'])
if local_commits:
_log.warning('Checkout has local commits before import.')
return True
@@ -304,10 +314,7 @@
_log.error('Commit cannot be applied cleanly:')
_log.error(error)
return None
- self.run(
- ['git', 'commit', '--all', '-F', '-'],
- stdin='Applying patch %s' % commit.sha,
- cwd=local_wpt.path)
+ self.wpt_git.commit_locally_with_message('Applying patch %s' % commit.sha)
return commits
def exportable_but_not_exported_commits(self, local_wpt):
@@ -336,7 +343,7 @@
manifest_base_path = self.fs.normpath(
self.fs.join(self.dest_path, '..', 'WPT_BASE_MANIFEST.json'))
self.copyfile(manifest_path, manifest_base_path)
- self.run(['git', 'add', manifest_base_path])
+ self.chromium_git.add_list([manifest_base_path])
def _clear_out_dest_path(self):
"""Removes all files that are synced with upstream from Chromium WPT.
@@ -353,14 +360,10 @@
def _commit_changes(self, commit_message):
_log.info('Committing changes.')
- self.run(['git', 'commit', '--all', '-F', '-'], stdin=commit_message)
-
- def _has_changes(self):
- return_code, _ = self.run(['git', 'diff', '--quiet', 'HEAD'], exit_on_failure=False)
- return return_code == 1
+ self.chromium_git.commit_locally_with_message(commit_message)
def _only_wpt_manifest_changed(self):
- changed_files = self.host.git().changed_files()
+ changed_files = self.chromium_git.changed_files()
wpt_base_manifest = self.fs.relpath(
self.fs.join(self.dest_path, '..', 'WPT_BASE_MANIFEST.json'),
self.finder.chromium_base())
@@ -408,30 +411,6 @@
base = '/' + rel_path.replace('-expected.txt', '')
return any((base + ext) in wpt_urls for ext in Port.supported_file_extensions)
- def run(self, cmd, exit_on_failure=True, cwd=None, stdin=''):
- _log.debug('Running command: %s', ' '.join(cmd))
-
- cwd = cwd or self.finder.path_from_layout_tests()
- proc = self.executive.popen(cmd, stdout=self.executive.PIPE, stderr=self.executive.PIPE, stdin=self.executive.PIPE, cwd=cwd)
- out, err = proc.communicate(stdin)
- if proc.returncode or self.verbose:
- _log.info('# ret> %d', proc.returncode)
- if out:
- for line in out.splitlines():
- _log.info('# out> %s', line)
- if err:
- for line in err.splitlines():
- _log.info('# err> %s', line)
- if exit_on_failure and proc.returncode:
- self.host.exit(proc.returncode)
- return proc.returncode, out
-
- def check_run(self, command):
- return_code, out = self.run(command)
- if return_code:
- raise Exception('%s failed with exit code %d.' % ' '.join(command), return_code)
- return out
-
def copyfile(self, source, destination):
_log.debug('cp %s %s', source, destination)
self.fs.copyfile(source, destination)
@@ -469,7 +448,7 @@
def get_directory_owners(self):
"""Returns a mapping of email addresses to owners of changed tests."""
_log.info('Gathering directory owners emails to CC.')
- changed_files = self.host.git().changed_files()
+ changed_files = self.chromium_git.changed_files()
extractor = DirectoryOwnersExtractor(self.fs)
return extractor.list_owners(changed_files)
@@ -479,7 +458,8 @@
Args:
directory_owners: A dict of tuples of owner names to lists of directories.
"""
- description = self.check_run(['git', 'log', '-1', '--format=%B'])
+ # TODO(robertma): Add a method in Git for getting the commit body.
+ description = self.chromium_git.run(['log', '-1', '--format=%B'])
build_link = current_build_link(self.host)
if build_link:
description += 'Build: %s\n\n' % build_link
@@ -586,7 +566,8 @@
def _list_deleted_tests(self):
"""List of layout tests that have been deleted."""
- out = self.check_run(['git', 'diff', 'origin/master', '-M100%', '--diff-filter=D', '--name-only'])
+ # TODO(robertma): Improve Git.changed_files so that we can use it here.
+ out = self.chromium_git.run(['diff', 'origin/master', '-M100%', '--diff-filter=D', '--name-only'])
deleted_tests = []
for path in out.splitlines():
test = self._relative_to_layout_test_dir(path)
@@ -599,7 +580,7 @@
Returns a dict mapping source name to destination name.
"""
- out = self.check_run(['git', 'diff', 'origin/master', '-M100%', '--diff-filter=R', '--name-status'])
+ out = self.chromium_git.run(['diff', 'origin/master', '-M100%', '--diff-filter=R', '--name-status'])
renamed_tests = {}
for line in out.splitlines():
_, source_path, dest_path = line.split()
diff --git a/third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_importer_unittest.py b/third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_importer_unittest.py
index 140e173f..723f19b 100644
--- a/third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_importer_unittest.py
+++ b/third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_importer_unittest.py
@@ -222,6 +222,7 @@
# TODO(robertma): Consider using MockLocalWPT.
host = MockHost()
importer = TestImporter(host, wpt_github=MockWPTGitHub(pull_requests=[]))
+ importer.wpt_git = MockGit(cwd='/tmp/wpt', executive=host.executive)
fake_commit = MockChromiumCommit(
host, subject='My fake commit',
patch=(
@@ -233,6 +234,8 @@
importer.exportable_but_not_exported_commits = lambda _: [fake_commit]
applied = importer.apply_exportable_commits_locally(LocalWPT(host))
self.assertEqual(applied, [fake_commit])
+ # This assertion is implementation details of LocalWPT.apply_patch.
+ # TODO(robertma): Move this to local_wpt_unittest.py.
self.assertEqual(host.executive.full_calls, [
MockCall(
['git', 'apply', '-'],
@@ -248,11 +251,10 @@
}),
MockCall(
['git', 'add', '.'],
- kwargs={'input': None, 'cwd': '/tmp/wpt', 'env': None}),
- MockCall(
- ['git', 'commit', '--all', '-F', '-'],
- kwargs={'cwd': '/tmp/wpt', 'env': None})
+ kwargs={'input': None, 'cwd': '/tmp/wpt', 'env': None})
])
+ self.assertEqual(importer.wpt_git.local_commits(),
+ [['Applying patch 14fd77e88e42147c57935c49d9e3b2412b8491b7']])
def test_apply_exportable_commits_locally_returns_none_on_failure(self):
host = MockHost()
@@ -290,10 +292,8 @@
host.filesystem.write_text_file('/mock-checkout/third_party/WebKit/LayoutTests/W3CImportExpectations', '')
host.filesystem.write_text_file('/mock-checkout/third_party/WebKit/LayoutTests/external/wpt/foo/OWNERS',
'someone@chromium.org\n')
- git = MockGit(filesystem=host.filesystem, executive=host.executive, platform=host.platform)
- git.changed_files = lambda: ['third_party/WebKit/LayoutTests/external/wpt/foo/x.html']
- host.git = lambda: git
importer = TestImporter(host)
+ importer.chromium_git.changed_files = lambda: ['third_party/WebKit/LayoutTests/external/wpt/foo/x.html']
self.assertEqual(importer.get_directory_owners(), {('someone@chromium.org',): ['external/wpt/foo']})
def test_get_directory_owners_no_changed_files(self):
@@ -309,11 +309,8 @@
def test_commit_changes(self):
host = MockHost()
importer = TestImporter(host)
- importer._has_changes = lambda: True
importer._commit_changes('dummy message')
- self.assertEqual(
- host.executive.calls,
- [['git', 'commit', '--all', '-F', '-']])
+ self.assertEqual(importer.chromium_git.local_commits(), [['dummy message']])
def test_commit_message(self):
importer = TestImporter(MockHost())
@@ -472,23 +469,21 @@
'--work',
'--tests-root',
blink_path + '/LayoutTests/external/wpt',
- ],
- [
- 'git',
- 'add',
- blink_path + '/LayoutTests/external/WPT_BASE_MANIFEST.json',
]
])
+ self.assertEqual(importer.chromium_git.added_paths,
+ {blink_path + '/LayoutTests/external/WPT_BASE_MANIFEST.json'})
def test_only_wpt_manifest_changed(self):
host = MockHost()
- git = host.git()
- git.changed_files = lambda: ['third_party/WebKit/LayoutTests/external/WPT_BASE_MANIFEST.json',
- 'third_party/WebKit/LayoutTests/external/wpt/foo/x.html']
importer = TestImporter(host)
+ importer.chromium_git.changed_files = lambda: [
+ 'third_party/WebKit/LayoutTests/external/WPT_BASE_MANIFEST.json',
+ 'third_party/WebKit/LayoutTests/external/wpt/foo/x.html']
self.assertFalse(importer._only_wpt_manifest_changed())
- git.changed_files = lambda: ['third_party/WebKit/LayoutTests/external/WPT_BASE_MANIFEST.json']
+ importer.chromium_git.changed_files = lambda: [
+ 'third_party/WebKit/LayoutTests/external/WPT_BASE_MANIFEST.json']
self.assertTrue(importer._only_wpt_manifest_changed())
def test_delete_orphaned_baselines_basic(self):
