[wpt-importer] Add GitHub diff link to CL description
Also, wrap the commit message and CL description as close to 72
characters as possible.
Bug: 40631540
Change-Id: I495aec7a281b58f8715902019144842a53d50d24
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5406820
Reviewed-by: Weizhong Xia <weizhong@google.com>
Commit-Queue: Jonathan Lee <jonathanjlee@google.com>
Cr-Commit-Position: refs/heads/main@{#1280767}
diff --git a/third_party/blink/tools/blinkpy/w3c/chromium_commit_mock.py b/third_party/blink/tools/blinkpy/w3c/chromium_commit_mock.py
index 3e5fa2d..a093003 100644
--- a/third_party/blink/tools/blinkpy/w3c/chromium_commit_mock.py
+++ b/third_party/blink/tools/blinkpy/w3c/chromium_commit_mock.py
@@ -5,7 +5,8 @@
import hashlib
-class MockChromiumCommit(object):
+class MockChromiumCommit:
+
def __init__(self,
host,
position='refs/heads/master@{#123}',
@@ -23,6 +24,9 @@
self._body = body
self._patch = patch
+ def __str__(self):
+ return f'{self.short_sha} "{self.subject()}"'
+
@property
def short_sha(self):
return self.sha[0:10]
diff --git a/third_party/blink/tools/blinkpy/w3c/import_notifier.py b/third_party/blink/tools/blinkpy/w3c/import_notifier.py
index 78b9887..2bffe5a 100644
--- a/third_party/blink/tools/blinkpy/w3c/import_notifier.py
+++ b/third_party/blink/tools/blinkpy/w3c/import_notifier.py
@@ -26,6 +26,7 @@
from blinkpy.common import path_finder
from blinkpy.common.checkout.git import CommitRange
+from blinkpy.common.memoized import memoized
from blinkpy.common.net.git_cl import CLRevisionID
from blinkpy.common.system.executive import ScriptError
from blinkpy.web_tests.models import typ_types
@@ -105,19 +106,14 @@
Note: "test names" are paths of the tests relative to web_tests.
"""
- # TODO(crbug.com/40631540): Add the GitHub comparison link to each
- # import commit message. Then, the notifier can parse out the WPT
- # revision range here without needing to look at the second most recent
- # commit.
- wpt_end_rev, import_rev = self._latest_wpt_revision_in_range()
+ wpt_end_rev, import_rev = self.latest_wpt_import()
cl = self._cl_for_wpt_revision(wpt_end_rev)
repo = self.host.project_config.gerrit_project
_log.info(f'Identifying failures for {repo}@{import_rev} ({cl.url})')
if self._bugs_already_filed(cl):
_log.info(f'Bugs have already been filed.')
return {}
- wpt_start_rev, _ = self._latest_wpt_revision_in_range(
- f'{import_rev}~1')
+ wpt_start_rev, _ = self.latest_wpt_import(f'{import_rev}~1')
self.examine_baseline_changes(import_rev, cl.current_revision_id)
self.examine_new_test_expectations(import_rev)
@@ -131,11 +127,16 @@
', '.join(sorted(bug.link for bug in filed_bugs.values())))
return filed_bugs
- def _latest_wpt_revision_in_range(
+ @memoized
+ def latest_wpt_import(
self,
commits: Union[None, str, CommitRange] = None) -> Tuple[str, str]:
"""Get commit hashes for the last WPT import.
+ Arguments:
+ commits: The range to search. See `Git.most_recent_log_matching()`
+ docstring for usage.
+
Returns:
A pair of SHA-1 hex digests (40 hex digits each):
* A valid commit in the WPT repo denoting how far WPT was rolled
diff --git a/third_party/blink/tools/blinkpy/w3c/test_importer.py b/third_party/blink/tools/blinkpy/w3c/test_importer.py
index 5fb394a..123883c 100644
--- a/third_party/blink/tools/blinkpy/w3c/test_importer.py
+++ b/third_party/blink/tools/blinkpy/w3c/test_importer.py
@@ -22,11 +22,19 @@
from blinkpy.common.net.network_transaction import NetworkTimeout
from blinkpy.common.path_finder import PathFinder
from blinkpy.common.system.log_utils import configure_logging
-from blinkpy.w3c.buganizer import BuganizerClient, BuganizerIssue
+from blinkpy.w3c.buganizer import BuganizerIssue
+from blinkpy.w3c.chromium_commit import ChromiumCommit
from blinkpy.w3c.chromium_exportable_commits import exportable_commits_over_last_n_commits
-from blinkpy.w3c.common import read_credentials, is_testharness_baseline, is_file_exportable, WPT_GH_URL
+from blinkpy.w3c.common import (
+ read_credentials,
+ is_testharness_baseline,
+ is_file_exportable,
+ WPT_GH_URL,
+ WPT_GH_RANGE_URL_TEMPLATE,
+)
from blinkpy.w3c.directory_owners_extractor import DirectoryOwnersExtractor
from blinkpy.w3c.gerrit import GerritAPI
+from blinkpy.w3c.import_notifier import ImportNotifier
from blinkpy.w3c.local_wpt import LocalWPT
from blinkpy.w3c.test_copier import TestCopier
from blinkpy.w3c.wpt_expectations_updater import WPTExpectationsUpdater
@@ -122,33 +130,32 @@
# File bugs for the previous imported CL. This is done at the start so
# that manually revived CLs still receive bugs.
gerrit_api = GerritAPI.from_credentials(self.host, credentials)
- self.file_and_record_bugs(local_wpt,
- gerrit_api,
+ notifier = ImportNotifier(self.host, self.project_git, local_wpt,
+ gerrit_api)
+ self.file_and_record_bugs(notifier,
auto_file_bugs=options.auto_file_bugs)
if options.revision is not None:
_log.info('Checking out %s', options.revision)
self.wpt_git.run(['checkout', options.revision])
- _log.debug('Noting the revision we are importing.')
- import_commit = f'wpt@{self.wpt_git.latest_git_commit()}'
-
- _log.info('Importing %s to Chromium %s', import_commit,
+ new_wpt_revision = self.wpt_git.latest_git_commit()
+ _log.info('Importing wpt@%s to Chromium %s', new_wpt_revision,
chromium_revision)
if options.ignore_exportable_commits:
- commit_message = self._commit_message(chromium_revision,
- import_commit)
+ commits = []
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_revision,
- import_commit,
- locally_applied_commits=commits)
+ last_wpt_revision, _ = notifier.latest_wpt_import()
+ wpt_range = CommitRange(last_wpt_revision, new_wpt_revision)
+ commit_message = self.commit_message(chromium_revision,
+ wpt_range,
+ locally_applied_commits=commits)
self._clear_out_dest_path()
@@ -188,7 +195,7 @@
return 0
directory_owners = self.get_directory_owners()
- description = self._cl_description(directory_owners)
+ description = self.cl_description(directory_owners)
self._upload_cl(description)
_log.info('Issue: %s', self.git_cl.run(['issue']).strip())
@@ -490,18 +497,24 @@
return True
return False
- 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)
+ def commit_message(
+ self,
+ chromium_commit_sha: str,
+ wpt_range: CommitRange,
+ locally_applied_commits: Optional[List[ChromiumCommit]] = None,
+ ) -> str:
+ wpt_short_range = CommitRange(wpt_range.start[:10], wpt_range.end[:10])
+ message = textwrap.dedent(f"""\
+ {ImportNotifier.IMPORT_SUBJECT_PREFIX}{wpt_range.end}
+
+ {WPT_GH_RANGE_URL_TEMPLATE.format(*wpt_short_range)}
+
+ Using wpt-import in Chromium {chromium_commit_sha}.
+ """)
if locally_applied_commits:
message += 'With Chromium commits locally applied on WPT:\n'
- message += '\n'.join(
- str(commit) for commit in locally_applied_commits)
- message += '\nNo-Export: true'
- message += '\nValidate-Test-Flakiness: skip'
+ message += '\n'.join(f' {textwrap.shorten(str(commit), 70)}'
+ for commit in locally_applied_commits) + '\n'
return message
def _delete_orphaned_baselines(self):
@@ -571,7 +584,7 @@
extractor = DirectoryOwnersExtractor(self.host)
return extractor.list_owners(changed_files)
- def _cl_description(self, directory_owners):
+ def cl_description(self, directory_owners):
"""Returns a CL description string.
Args:
@@ -579,13 +592,18 @@
"""
# TODO(robertma): Add a method in Git for getting the commit body.
description = self.project_git.run(['log', '-1', '--format=%B'])
- description += (
- 'Note to sheriffs: This CL imports external tests and adds\n'
- 'expectations for those tests; if this CL is large and causes\n'
- 'a few new failures, please fix the failures by adding new\n'
- 'lines to TestExpectations rather than reverting. See:\n'
- 'https://chromium.googlesource.com'
- '/chromium/src/+/main/docs/testing/web_platform_tests.md\n\n')
+ gardener_instructions = textwrap.dedent("""\
+ Note to gardeners: This CL imports external tests and adds
+ expectations for those tests; if this CL is large and causes a few
+ new failures, please fix the failures by adding new lines to
+ TestExpectations rather than reverting. See:
+ """)
+ description += '\n'.join([
+ '',
+ *textwrap.wrap(gardener_instructions, width=72),
+ 'https://chromium.googlesource.com/chromium/src/+/'
+ 'main/docs/testing/web_platform_tests.md',
+ ]) + '\n\n'
if directory_owners:
description += self._format_directory_owners(
@@ -593,11 +611,6 @@
# Prevent FindIt from auto-reverting import CLs.
description += 'NOAUTOREVERT=true\n'
-
- # Move any No-Export tag and flakiness footers to the end of the description.
- description = description.replace('No-Export: true', '')
- description = description.replace('Validate-Test-Flakiness: skip', '')
- description = description.replace('\n\n\n\n\n', '\n\n')
description += 'No-Export: true\n'
description += 'Validate-Test-Flakiness: skip\n'
@@ -662,9 +675,7 @@
def file_and_record_bugs(
self,
- local_wpt: LocalWPT,
- gerrit_api: GerritAPI,
- buganizer_client: Optional[BuganizerClient] = None,
+ notifier: ImportNotifier,
auto_file_bugs: bool = True,
):
"""File bugs for the last imported CL and add them to TestExpectations.
@@ -680,10 +691,6 @@
(origin/main) <tip-of-tree>
"""
_log.info('Filing bugs for the last WPT import')
- from blinkpy.w3c.import_notifier import ImportNotifier
- # Construct the notifier here so that any errors won't affect the import.
- notifier = ImportNotifier(self.host, self.project_git, local_wpt,
- gerrit_api, buganizer_client)
bugs_by_dir = notifier.main(dry_run=(not auto_file_bugs))
referenced_bugs = self._update_bugs_in_expectations(
notifier, bugs_by_dir)
@@ -714,7 +721,7 @@
def _update_bugs_in_expectations(
self,
- notifier,
+ notifier: ImportNotifier,
bugs_by_dir: Mapping[str, BuganizerIssue],
) -> Set[int]:
expectations = TestExpectations(
diff --git a/third_party/blink/tools/blinkpy/w3c/test_importer_unittest.py b/third_party/blink/tools/blinkpy/w3c/test_importer_unittest.py
index 6d2c5091..f9bf96c 100644
--- a/third_party/blink/tools/blinkpy/w3c/test_importer_unittest.py
+++ b/third_party/blink/tools/blinkpy/w3c/test_importer_unittest.py
@@ -8,6 +8,7 @@
import unittest
from unittest import mock
+from blinkpy.common.checkout.git import CommitRange
from blinkpy.common.checkout.git_mock import MockGit
from blinkpy.common.host_mock import MockHost
from blinkpy.common.net.git_cl import TryJobStatus
@@ -21,6 +22,7 @@
from blinkpy.w3c.buganizer import BuganizerIssue
from blinkpy.w3c.chromium_commit_mock import MockChromiumCommit
from blinkpy.w3c.directory_owners_extractor import WPTDirMetadata
+from blinkpy.w3c.import_notifier import ImportNotifier
from blinkpy.w3c.local_wpt import LocalWPT
from blinkpy.w3c.local_wpt_mock import MockLocalWPT
from blinkpy.w3c.test_importer import TestImporter, ROTATIONS_URL, SHERIFF_EMAIL_FALLBACK, RUBBER_STAMPER_BOT
@@ -450,44 +452,68 @@
def test_commit_message(self):
importer = self._get_test_importer(self.mock_host())
self.assertEqual(
- importer._commit_message('aaaa', '1111'), 'Import 1111\n\n'
- 'Using wpt-import in Chromium aaaa.\n\n'
- 'No-Export: true\n'
- 'Validate-Test-Flakiness: skip')
+ importer.commit_message('aaaa', CommitRange('0000', '1111')),
+ textwrap.dedent("""\
+ Import wpt@1111
+
+ https://github.com/web-platform-tests/wpt/compare/0000...1111
+
+ Using wpt-import in Chromium aaaa.
+ """))
+
+ def test_commit_message_with_pending_exportable_changes(self):
+ host = self.mock_host()
+ importer = self._get_test_importer(host)
+ locally_applied_commits = [
+ MockChromiumCommit(host, subject='Pending export 1'),
+ MockChromiumCommit(
+ host,
+ 'refs/heads/main@{#222)',
+ subject=f'Pending export 2 with very long subject {"a" * 80}'),
+ ]
+ self.assertEqual(
+ importer.commit_message('aaaa', CommitRange('0000', '1111'),
+ locally_applied_commits),
+ textwrap.dedent("""\
+ Import wpt@1111
+
+ https://github.com/web-platform-tests/wpt/compare/0000...1111
+
+ Using wpt-import in Chromium aaaa.
+ With Chromium commits locally applied on WPT:
+ 14fd77e88e "Pending export 1"
+ 3e977a7ce6 "Pending export 2 with very long subject [...]
+ """)),
def test_cl_description_with_empty_environ(self):
host = self.mock_host()
- host.executive = MockExecutive(output='Last commit message\n\n')
+ host.executive = MockExecutive(output='Last commit message\n')
importer = self._get_test_importer(host)
- description = importer._cl_description(directory_owners={})
+ description = importer.cl_description(directory_owners={})
self.assertEqual(
- description, 'Last commit message\n\n'
- 'Note to sheriffs: This CL imports external tests and adds\n'
- 'expectations for those tests; if this CL is large and causes\n'
- 'a few new failures, please fix the failures by adding new\n'
- 'lines to TestExpectations rather than reverting. See:\n'
- 'https://chromium.googlesource.com'
- '/chromium/src/+/main/docs/testing/web_platform_tests.md\n\n'
- 'NOAUTOREVERT=true\n'
- 'No-Export: true\n'
- 'Validate-Test-Flakiness: skip\n'
- 'Cq-Include-Trybots: luci.chromium.try:linux-blink-rel\n')
- self.assertEqual(host.executive.calls,
- [MANIFEST_INSTALL_CMD] +
- [['git', 'log', '-1', '--format=%B']])
+ description,
+ textwrap.dedent("""\
+ Last commit message
- def test_cl_description_moves_noexport_tag(self):
- host = self.mock_host()
- host.executive = MockExecutive(output='Summary\n\nNo-Export: true\n\n')
- importer = self._get_test_importer(host)
- description = importer._cl_description(directory_owners={})
- self.assertIn('No-Export: true', description)
+ Note to gardeners: This CL imports external tests and adds expectations
+ for those tests; if this CL is large and causes a few new failures,
+ please fix the failures by adding new lines to TestExpectations rather
+ than reverting. See:
+ https://chromium.googlesource.com/chromium/src/+/main/docs/testing/web_platform_tests.md
+
+ NOAUTOREVERT=true
+ No-Export: true
+ Validate-Test-Flakiness: skip
+ Cq-Include-Trybots: luci.chromium.try:linux-blink-rel
+ """))
+ self.assertEqual(host.executive.calls, [MANIFEST_INSTALL_CMD] +
+ [['git', 'log', '-1', '--format=%B']])
def test_cl_description_with_directory_owners(self):
host = self.mock_host()
- host.executive = MockExecutive(output='Last commit message\n\n')
+ host.executive = MockExecutive(output='Last commit message\n')
importer = self._get_test_importer(host)
- description = importer._cl_description(
+ description = importer.cl_description(
directory_owners={
('someone@chromium.org', ):
['external/wpt/foo', 'external/wpt/bar'],
@@ -631,12 +657,13 @@
**dataclasses.asdict(issue),
'issue_id': 111,
})
+ notifier = ImportNotifier(host, git, local_wpt, gerrit_api,
+ buganizer_client)
with mock.patch(
'blinkpy.w3c.import_notifier.'
'DirectoryOwnersExtractor.read_dir_metadata',
return_value=WPTDirMetadata(should_notify=True)):
- importer.file_and_record_bugs(local_wpt, gerrit_api,
- buganizer_client)
+ importer.file_and_record_bugs(notifier)
gerrit_cl.post_comment.assert_called_once_with(
'Filed bugs for failures introduced by this CL: '
@@ -697,12 +724,13 @@
local_wpt = MockLocalWPT()
gerrit_api, buganizer_client = mock.Mock(), mock.Mock()
gerrit_api.query_cls.return_value = [mock.Mock(messages=[])]
+ notifier = ImportNotifier(host, git, local_wpt, gerrit_api,
+ buganizer_client)
with mock.patch(
'blinkpy.w3c.import_notifier.'
'DirectoryOwnersExtractor.read_dir_metadata',
return_value=WPTDirMetadata(should_notify=True)):
- importer.file_and_record_bugs(local_wpt, gerrit_api,
- buganizer_client)
+ importer.file_and_record_bugs(notifier)
self.assertEqual(
git.show_blob(RELATIVE_WEB_TESTS + 'TestExpectations',
