Skip to content

C
Chromium_Depot_tools
Commit 0e617c02 authored by Quinten Yearsley's avatar Quinten Yearsley Committed by Commit Bot
Browse files
Options

Change git cl comments to list robot comments. 

In particular, only show robot comments from the latest patchset.

Bug: 924780
Change-Id: I12038ddd2d90a5cb561b248de3fd37908d5b927e
Reviewed-on: https://chromium-review.googlesource.com/c/1457282Reviewed-by: 's avatarAndrii Shyshkalov <tandrii@chromium.org>
Commit-Queue: Quinten Yearsley <qyearsley@chromium.org>
parent c2674678
Hide whitespace changes
Inline Side-by-side
Showing with 218 additions and 53 deletions
gerrit_util.py
View file @ 0e617c02
......@@ -684,6 +684,12 @@ def GetChangeComments(host, change):
return ReadHttpJsonResponse(CreateHttpConn(host, path))
def GetChangeRobotComments(host, change):
"""Get the line- and file-level robot comments on a change."""
path = 'changes/%s/robotcomments' % change
return ReadHttpJsonResponse(CreateHttpConn(host, path))
def AbandonChange(host, change, msg=''):
"""Abandon a gerrit change."""
path = 'changes/%s/abandon' % change
......
git_cl.py
View file @ 0e617c02
......@@ -1050,7 +1050,7 @@ class GerritChangeNotExists(Exception):
_CommentSummary = collections.namedtuple(
'_CommentSummary', ['date', 'message', 'sender',
'_CommentSummary', ['date', 'message', 'sender', 'autogenerated',
# TODO(tandrii): these two aren't known in Gerrit.
'approval', 'disapproval'])
......@@ -2112,10 +2112,23 @@ class _GerritChangelistImpl(_ChangelistCodereviewBase):
def GetCommentsSummary(self, readable=True):
# DETAILED_ACCOUNTS is to get emails in accounts.
# CURRENT_REVISION is included to get the latest patchset so that
# only the robot comments from the latest patchset can be shown.
messages = self._GetChangeDetail(
options=['MESSAGES', 'DETAILED_ACCOUNTS']).get('messages', [])
options=['MESSAGES', 'DETAILED_ACCOUNTS',
'CURRENT_REVISION']).get('messages', [])
file_comments = gerrit_util.GetChangeComments(
self._GetGerritHost(), self._GerritChangeIdentifier())
robot_file_comments = gerrit_util.GetChangeRobotComments(
self._GetGerritHost(), self._GerritChangeIdentifier())
# Add the robot comments onto the list of comments, but only
# keep those that are from the latest pachset.
latest_patch_set = self.GetMostRecentPatchset()
for path, robot_comments in robot_file_comments.iteritems():
line_comments = file_comments.setdefault(path, [])
line_comments.extend(
[c for c in robot_comments if c['patch_set'] == latest_patch_set])
# Build dictionary of file comments for easy access and sorting later.
# {author+date: {path: {patchset: {line: url+message}}}}
......@@ -2123,7 +2136,8 @@ class _GerritChangelistImpl(_ChangelistCodereviewBase):
lambda: collections.defaultdict(lambda: collections.defaultdict(dict)))
for path, line_comments in file_comments.iteritems():
for comment in line_comments:
if comment.get('tag', '').startswith('autogenerated'):
tag = comment.get('tag', '')
if tag.startswith('autogenerated') and 'robot_id' not in comment:
continue
key = (comment['author']['email'], comment['updated'])
if comment.get('side', 'REVISION') == 'PARENT':
......@@ -2137,48 +2151,58 @@ class _GerritChangelistImpl(_ChangelistCodereviewBase):
str(line) if line else ''))
comments[key][path][patchset][line] = (url, comment['message'])
summary = []
summaries = []
for msg in messages:
# Don't bother showing autogenerated messages.
if msg.get('tag') and msg.get('tag').startswith('autogenerated'):
continue
# Gerrit spits out nanoseconds.
assert len(msg['date'].split('.')[-1]) == 9
date = datetime.datetime.strptime(msg['date'][:-3],
'%Y-%m-%d %H:%M:%S.%f')
message = msg['message']
key = (msg['author']['email'], msg['date'])
if key in comments:
message += '\n'
for path, patchsets in sorted(comments.get(key, {}).items()):
if readable:
message += '\n%s' % path
for patchset, lines in sorted(patchsets.items()):
for line, (url, content) in sorted(lines.items()):
if line:
line_str = 'Line %d' % line
path_str = '%s:%d:' % (path, line)
else:
line_str = 'File comment'
path_str = '%s:0:' % path
if readable:
message += '\n %s, %s: %s' % (patchset, line_str, url)
message += '\n %s\n' % content
else:
message += '\n%s ' % path_str
message += '\n%s\n' % content
summary.append(_CommentSummary(
date=date,
message=message,
sender=msg['author']['email'],
# These could be inferred from the text messages and correlated with
# Code-Review label maximum, however this is not reliable.
# Leaving as is until the need arises.
approval=False,
disapproval=False,
))
return summary
summary = self._BuildCommentSummary(msg, comments, readable)
if summary:
summaries.append(summary)
return summaries
@staticmethod
def _BuildCommentSummary(msg, comments, readable):
key = (msg['author']['email'], msg['date'])
# Don't bother showing autogenerated messages that don't have associated
# file or line comments. this will filter out most autogenerated
# messages, but will keep robot comments like those from Tricium.
is_autogenerated = msg.get('tag', '').startswith('autogenerated')
if is_autogenerated and not comments.get(key):
return None
message = msg['message']
# Gerrit spits out nanoseconds.
assert len(msg['date'].split('.')[-1]) == 9
date = datetime.datetime.strptime(msg['date'][:-3],
'%Y-%m-%d %H:%M:%S.%f')
if key in comments:
message += '\n'
for path, patchsets in sorted(comments.get(key, {}).items()):
if readable:
message += '\n%s' % path
for patchset, lines in sorted(patchsets.items()):
for line, (url, content) in sorted(lines.items()):
if line:
line_str = 'Line %d' % line
path_str = '%s:%d:' % (path, line)
else:
line_str = 'File comment'
path_str = '%s:0:' % path
if readable:
message += '\n %s, %s: %s' % (patchset, line_str, url)
message += '\n %s\n' % content
else:
message += '\n%s ' % path_str
message += '\n%s\n' % content
return _CommentSummary(
date=date,
message=message,
sender=msg['author']['email'],
autogenerated=is_autogenerated,
# These could be inferred from the text messages and correlated with
# Code-Review label maximum, however this is not reliable.
# Leaving as is until the need arises.
approval=False,
disapproval=False,
)
def CloseIssue(self):
gerrit_util.AbandonChange(
......@@ -4156,6 +4180,8 @@ def CMDcomments(parser, args):
color = Fore.GREEN
elif comment.sender == cl.GetIssueOwner():
color = Fore.MAGENTA
elif comment.autogenerated:
color = Fore.CYAN
else:
color = Fore.BLUE
print('\n%s%s %s%s\n%s' % (
......
tests/git_cl_test.py
View file @ 0e617c02
......@@ -655,6 +655,9 @@ class TestGitCl(TestCase):
self.mock(git_cl.gerrit_util, 'GetChangeComments',
lambda *args, **kwargs: self._mocked_call(
'GetChangeComments', *args, **kwargs))
self.mock(git_cl.gerrit_util, 'GetChangeRobotComments',
lambda *args, **kwargs: self._mocked_call(
'GetChangeRobotComments', *args, **kwargs))
self.mock(git_cl.gerrit_util, 'AddReviewers',
lambda h, i, reviewers, ccs, notify: self._mocked_call(
'AddReviewers', h, i, reviewers, ccs, notify))
......@@ -1513,8 +1516,7 @@ class TestGitCl(TestCase):
expected,
'GetHashTags(%r) == %r, expected %r' % (desc, actual, expected))
def test_get_target_ref(self):
# Check remote or remote branch not present.
self.assertEqual(None, git_cl.GetTargetRef('origin', None, 'master'))
self.assertEqual(None, git_cl.GetTargetRef(None,
'refs/remotes/origin/master',
......@@ -2766,8 +2768,8 @@ class TestGitCl(TestCase):
def test_git_cl_comments_fetch_gerrit(self):
self.mock(sys, 'stdout', StringIO.StringIO())
self.calls = [
((['git', 'symbolic-ref', 'HEAD'],), CERR1),
((['git', 'symbolic-ref', 'HEAD'],), CERR1),
((['git', 'config', 'branch.foo.gerritserver'],), ''),
((['git', 'config', 'branch.foo.merge'],), ''),
((['git', 'config', 'rietveld.upstream-branch'],), CERR1),
((['git', 'branch', '-r'],), 'origin/HEAD -> origin/master\n'
'origin/master'),
......@@ -2775,8 +2777,18 @@ class TestGitCl(TestCase):
'https://chromium.googlesource.com/infra/infra'),
(('GetChangeDetail', 'chromium-review.googlesource.com',
'infra%2Finfra~1',
['MESSAGES', 'DETAILED_ACCOUNTS']), {
['MESSAGES', 'DETAILED_ACCOUNTS', 'CURRENT_REVISION',
'CURRENT_COMMIT']), {
'owner': {'email': 'owner@example.com'},
'current_revision': 'ba5eba11',
'revisions': {
'deadbeaf': {
'_number': 1,
},
'ba5eba11': {
'_number': 2,
},
},
'messages': [
{
u'_revision_number': 1,
......@@ -2835,7 +2847,10 @@ class TestGitCl(TestCase):
'message': 'I removed this because it is bad',
},
]
})
}),
(('GetChangeRobotComments', 'chromium-review.googlesource.com',
'infra%2Finfra~1'), {}),
((['git', 'config', 'branch.foo.gerritpatchset', '2'],), ''),
] * 2 + [
(('write_json', 'output.json', [
{
......@@ -2847,6 +2862,7 @@ class TestGitCl(TestCase):
u' Base, Line 42: https://chromium-review.googlesource.com/' +
u'c/1/2/codereview.settings#b42\n' +
u' I removed this because it is bad\n'),
u'autogenerated': False,
u'approval': False,
u'disapproval': False,
u'sender': u'owner@example.com'
......@@ -2859,6 +2875,7 @@ class TestGitCl(TestCase):
u' PS2, File comment: https://chromium-review.googlesource' +
u'.com/c/1/2//COMMIT_MSG#\n' +
u' Please include a bug link\n'),
u'autogenerated': False,
u'approval': False,
u'disapproval': False,
u'sender': u'reviewer@example.com'
......@@ -2875,6 +2892,7 @@ class TestGitCl(TestCase):
u'c/1/2/codereview.settings#b42\n' +
u' I removed this because it is bad\n'),
date=datetime.datetime(2017, 3, 16, 20, 0, 41, 0),
autogenerated=False,
disapproval=False, approval=False, sender=u'owner@example.com'),
git_cl._CommentSummary(
message=(
......@@ -2885,12 +2903,127 @@ class TestGitCl(TestCase):
u'c/1/2//COMMIT_MSG#\n' +
u' Please include a bug link\n'),
date=datetime.datetime(2017, 3, 17, 5, 19, 37, 500000),
autogenerated=False,
disapproval=False, approval=False, sender=u'reviewer@example.com'),
]
cl = git_cl.Changelist(codereview='gerrit', issue=1)
cl = git_cl.Changelist(
codereview='gerrit', issue=1, branchref='refs/heads/foo')
self.assertEqual(cl.GetCommentsSummary(), expected_comments_summary)
self.mock(git_cl.Changelist, 'GetBranch', lambda _: 'foo')
self.assertEqual(
0, git_cl.main(['comments', '-i', '1', '-j', 'output.json']))
def test_git_cl_comments_robot_comments(self):
# git cl comments also fetches robot comments (which are considered a type
# of autogenerated comment), and unlike other types of comments, only robot
# comments from the latest patchset are shown.
self.mock(sys, 'stdout', StringIO.StringIO())
self.calls = [
((['git', 'config', 'branch.foo.gerritserver'],), ''),
((['git', 'config', 'branch.foo.merge'],), ''),
((['git', 'config', 'rietveld.upstream-branch'],), CERR1),
((['git', 'branch', '-r'],), 'origin/HEAD -> origin/master\n'
'origin/master'),
((['git', 'config', 'remote.origin.url'],),
'https://chromium.googlesource.com/infra/infra'),
(('GetChangeDetail', 'chromium-review.googlesource.com',
'infra%2Finfra~1',
['MESSAGES', 'DETAILED_ACCOUNTS', 'CURRENT_REVISION',
'CURRENT_COMMIT']), {
'owner': {'email': 'owner@example.com'},
'current_revision': 'ba5eba11',
'revisions': {
'deadbeaf': {
'_number': 1,
},
'ba5eba11': {
'_number': 2,
},
},
'messages': [
{
u'_revision_number': 1,
u'author': {
u'_account_id': 1111084,
u'email': u'commit-bot@chromium.org',
u'name': u'Commit Bot'
},
u'date': u'2017-03-15 20:08:45.000000000',
u'id': u'f5a6c25ecbd3b3b54a43ae418ed97eff046dc50b',
u'message': u'Patch Set 1:\n\nDry run: CQ is trying the patch...',
u'tag': u'autogenerated:cq:dry-run'
},
{
u'_revision_number': 1,
u'author': {
u'_account_id': 123,
u'email': u'tricium@serviceaccount.com',
u'name': u'Tricium'
},
u'date': u'2017-03-16 20:00:41.000000000',
u'id': u'f5a6c25ecbd3b3b54a43ae418ed97eff046d1234',
u'message': u'(1 comment)',
u'tag': u'autogenerated:tricium',
},
{
u'_revision_number': 1,
u'author': {
u'_account_id': 123,
u'email': u'tricium@serviceaccount.com',
u'name': u'Tricium'
},
u'date': u'2017-03-16 20:00:41.000000000',
u'id': u'f5a6c25ecbd3b3b54a43ae418ed97eff046d1234',
u'message': u'(1 comment)',
u'tag': u'autogenerated:tricium',
},
{
u'_revision_number': 2,
u'author': {
u'_account_id': 123 ,
u'email': u'tricium@serviceaccount.com',
u'name': u'reviewer'
},
u'date': u'2017-03-17 05:30:37.000000000',
u'tag': u'autogenerated:tricium',
u'id': u'f5a6c25ecbd3b3b54a43ae418ed97eff046d4568',
u'message': u'(1 comment)',
},
]
}),
(('GetChangeComments', 'chromium-review.googlesource.com',
'infra%2Finfra~1'), {}),
(('GetChangeRobotComments', 'chromium-review.googlesource.com',
'infra%2Finfra~1'), {
'codereview.settings': [
{
u'author': {u'email': u'tricium@serviceaccount.com'},
u'updated': u'2017-03-17 05:30:37.000000000',
u'robot_run_id': u'5565031076855808',
u'robot_id': u'Linter/Category',
u'tag': u'autogenerated:tricium',
u'patch_set': 2,
u'side': u'REVISION',
u'message': u'Linter warning message text',
u'line': 32,
},
],
}),
((['git', 'config', 'branch.foo.gerritpatchset', '2'],), ''),
]
expected_comments_summary = [
git_cl._CommentSummary(date=datetime.datetime(2017, 3, 17, 5, 30, 37),
message=(
u'(1 comment)\n\ncodereview.settings\n'
u' PS2, Line 32: https://chromium-review.googlesource.com/'
u'c/1/2/codereview.settings#32\n'
u' Linter warning message text\n'),
sender=u'tricium@serviceaccount.com',
autogenerated=True, approval=False, disapproval=False)
]
cl = git_cl.Changelist(
codereview='gerrit', issue=1, branchref='refs/heads/foo')
self.assertEqual(cl.GetCommentsSummary(), expected_comments_summary)
self.assertEqual(0, git_cl.main(['comment', '-i', '1',
'-j', 'output.json']))
def test_get_remote_url_with_mirror(self):
original_os_path_isdir = os.path.isdir
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment