Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP: Implement reporter for code issues in a PR #5929

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions common/pylintrc
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,7 @@ disable=
fixme,
global-variable-undefined,
unused-argument,
chained-comparison,
maybe-no-member,
locally-disabled,
bad-classmethod-argument,
Expand Down
154 changes: 154 additions & 0 deletions master/buildbot/reporters/generators/comment.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,154 @@
# This file is part of Buildbot. Buildbot is free software: you can
# redistribute it and/or modify it under the terms of the GNU General Public
# License as published by the Free Software Foundation, version 2.
#
# This program is distributed in the hope that it will be useful, but WITHOUT
# ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS
# FOR A PARTICULAR PURPOSE. See the GNU General Public License for more
# details.
#
# You should have received a copy of the GNU General Public License along with
# this program; if not, write to the Free Software Foundation, Inc., 51
# Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
#
# Copyright Buildbot Team Members

import bisect
import json

from twisted.internet import defer
from zope.interface import implementer

from buildbot import interfaces
from buildbot.reporters import utils
from buildbot.reporters.message import MessageFormatterFunction

from .utils import BuildStatusGeneratorMixin


@implementer(interfaces.IReportGenerator)
class BuildCodeIssueCommentsGenerator(BuildStatusGeneratorMixin):

wanted_event_keys = [
('builds', None, 'finished'),
]

compare_attrs = ['formatter']

def __init__(self, diffinfo_data_name=None, tags=None, builders=None, schedulers=None,
branches=None, message_formatter=None):
super().__init__('all', tags, builders, schedulers, branches, subject='', add_logs=False,
add_patch=False)
self.diffinfo_data_name = diffinfo_data_name
self.formatter = message_formatter
if self.formatter is None:
self.formatter = MessageFormatterFunction(lambda ctx: ctx['message'], 'plain')

@defer.inlineCallbacks
def generate(self, master, reporter, key, build):
_, _, event = key
if event != 'finished':
return None

yield utils.getDetailsForBuild(master, build,
wantProperties=self.formatter.wantProperties,
wantSteps=self.formatter.wantSteps,
wantPreviousBuild=False,
wantLogs=self.formatter.wantLogs)

if not self.is_message_needed_by_props(build):
return None
if not self.is_message_needed_by_results(build):
return None

report = yield self._build_report(self.formatter, master, reporter, build)
return report

def _get_target_changes_by_path(self, diff_info):
hunks_by_path = {}
for file_info in diff_info:
hunks = [(hunk['ts'], hunk['tl']) for hunk in file_info['hunks']]
if not hunks:
continue
hunks.sort()

hunks_by_path[file_info['target_file']] = hunks
return hunks_by_path

def _is_result_usable(self, result):
return result.get('test_code_path') is not None and result.get('line') is not None

def _is_result_in_target_diff(self, target_changes_by_path, result):
changes_in_path = target_changes_by_path.get(result['test_code_path'])
if not changes_in_path:
return False

result_line = result['line']

preceding_change_i = bisect.bisect(changes_in_path, (result_line + 1, -1))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if bisect is very well known module of python.

I wasn't aware of it.

Also, the fact that we are comparing tuple like this looks suspicious.

I think we need a proper helper function to compare code line intervals (with unit tests)

if preceding_change_i == 0:
return False # there's no change with start position earlier than the test result line
preceding_change_start, preceding_change_length = changes_in_path[preceding_change_i - 1]

return result_line >= preceding_change_start and \
result_line < preceding_change_start + preceding_change_length

@defer.inlineCallbacks
def _get_usable_results_in_changed_lines(self, master, target_changes_by_path, result_sets):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

an error could be in an untouched line (removal of used import for example).

gitlab allows you to comment on untouched lines.

So for this generic code, I believe we shouldn't filterout

maybe we could split two sets of results something like:

touched_line_issues
untouched_lines_issues

results_in_changed_lines = []

for result_set in result_sets:
if result_set['category'] != 'code_issue':
continue
if result_set['value_unit'] != 'message':
continue

results = yield master.data.get(('test_result_sets', result_set['test_result_setid'],
'results'))

results = [tr for tr in results
if self._is_result_usable(tr) and
self._is_result_in_target_diff(target_changes_by_path, tr)]

results_in_changed_lines.extend(results)
return results_in_changed_lines

@defer.inlineCallbacks
def _build_report(self, formatter, master, reporter, build):
users = yield reporter.getResponsibleUsersForBuild(master, build['buildid'])

build_data = yield master.data.get(('builds', build['buildid'], 'data',
self.diffinfo_data_name, 'value'))

target_changes_by_path = self._get_target_changes_by_path(json.loads(build_data['raw']))

result_sets = yield master.data.get(('builds', build['buildid'], 'test_result_sets'))

results_in_changed_lines = \
yield self._get_usable_results_in_changed_lines(master, target_changes_by_path,
result_sets)

result_body = []
for result in results_in_changed_lines:

buildmsg = yield formatter.format_message_for_build(master, build, mode=self.mode,
users=users,
message=result['value'])

result_body.append({
'codebase': '',
'path': result['test_code_path'],
'line': result['line'],
'body': buildmsg['body']
})

return {
'body': result_body,
'subject': None,
'type': 'code_comments',
'results': build['results'],
'builds': [build],
'users': list(users),
'patches': None,
'logs': None
}
110 changes: 110 additions & 0 deletions master/buildbot/reporters/github.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
from twisted.internet import defer
from twisted.python import log

from buildbot import config
from buildbot.process.properties import Interpolate
from buildbot.process.properties import Properties
from buildbot.process.results import CANCELLED
Expand Down Expand Up @@ -290,3 +291,112 @@ def createStatus(self,
url = '/'.join(['/repos', repo_user, repo_name, 'issues', issue, 'comments'])
ret = yield self._http.post(url, json=payload)
return ret


class GitHubCodeCommentPush(GitHubStatusPush):
name = "GitHubCodeCommentPush"

def setup_context(self, context):
return ''

def _create_default_generators(self):
config.error("{}: generators argument is mandatory".format(self.__class__.__name__))

@defer.inlineCallbacks
def sendMessage(self, reports):
for report in reports:
if 'type' not in report or report['type'] != 'code_comments':
log.msg('{}: report contains unsupported data of type {}, ignoring'.format(
self, report['type'] if 'type' in report else '(None)'))
continue

build = report['builds'][0]

sourcestamps = build['buildset'].get('sourcestamps')
if not sourcestamps:
continue

comments = report['body']
comment_codebases = [c['codebase'] for c in comments]

props = Properties.fromDict(build['properties'])
props.master = self.master
issue = self._extract_issue(props)

if issue is None:
log.msg('{}: Skipped status update for build {} as issue is not specified'.format(
self, build['id']))
return

github_info_by_codebase = self._get_github_info_by_codebase(sourcestamps,
comment_codebases, issue)

for comment in comments:
codebase = comment['codebase']
if codebase not in github_info_by_codebase:
log.err('{}: Comment has unknown codebase {} (known: {})'.format(
self, codebase, list(github_info_by_codebase)))
continue

_, repo_owner, repo_name, sha = github_info_by_codebase[codebase]
path = comment['path']
line = comment['line']

response = None
try:
if self.verbose:
log.msg(("{}: Adding comment to github github status: {}/{}/{} at {}:{}"
).format(self, repo_owner, repo_name, issue, path, line))
response = yield self._post_comment(repo_owner, repo_name, issue, sha,
path, line, comment['body'])

if not self.is_status_2xx(response.code):
raise Exception()

except Exception as e:
if response:
content = yield response.content()
code = response.code
else:
content = code = "n/a"
log.err(e, ('{}: Failed to send comment to {}/{}/{} at {}:{}. HTTP {}, {}'
).format(self, repo_owner, repo_name, issue, path, line, code,
content))

def _get_github_info_by_codebase(self, sourcestamps, valid_codebases, issue):
info_by_codebase = {}
for sourcestamp in sourcestamps:
repo_owner, repo_name = self._extract_github_info(sourcestamp)
sha = sourcestamp['revision']

codebase = sourcestamp['codebase']
if codebase not in valid_codebases:
continue

if not repo_owner or not repo_name:
log.msg(('{}: Skipped comments because required repo ' +
'information is missing.').format(self))
continue

if not sha:
log.msg(('{}: Skipped status update for codebase {}, issue {}.'
).format(self, codebase, issue))
continue

info_by_codebase[codebase] = (sourcestamp, repo_owner, repo_name, sha)
return info_by_codebase

@defer.inlineCallbacks
def _post_comment(self, repo_user, repo_name, issue, sha, path, line, comment):

payload = {
'path': path,
'line': line,
'side': 'RIGHT',
'commit_id': sha,
'body': comment
}

url = '/'.join(['/repos', repo_user, repo_name, 'pulls', issue, 'comments'])
ret = yield self._http.post(url, json=payload)
return ret
4 changes: 2 additions & 2 deletions master/buildbot/reporters/message.py
Original file line number Diff line number Diff line change
Expand Up @@ -228,8 +228,8 @@ def __init__(self, function, template_type, **kwargs):
self._function = function

@defer.inlineCallbacks
def format_message_for_build(self, master, build, **kwargs):
msgdict = yield self.render_message_dict(master, {'build': build})
def format_message_for_build(self, master, build, message=None, **kwargs):
msgdict = yield self.render_message_dict(master, {'build': build, 'message': message})
return msgdict

def render_message_body(self, context):
Expand Down
6 changes: 4 additions & 2 deletions master/buildbot/test/unit/reporters/test_message.py
Original file line number Diff line number Diff line change
Expand Up @@ -267,7 +267,8 @@ def test_basic(self):
function.assert_called_with({
'build': BuildDictLookAlike(extra_keys=['prev_build'],
expected_missing_keys=['parentbuilder', 'buildrequest',
'parentbuild'])
'parentbuild']),
'message': None,
})
self.assertEqual(res, {
'body': {'key': 'value'},
Expand All @@ -289,7 +290,8 @@ def renderable_function(context):
function.assert_called_with({
'build': BuildDictLookAlike(extra_keys=['prev_build'],
expected_missing_keys=['parentbuilder', 'buildrequest',
'parentbuild'])
'parentbuild']),
'message': None,
})
self.assertEqual(res, {
'body': {'key': 'value'},
Expand Down
7 changes: 6 additions & 1 deletion master/setup.py
Original file line number Diff line number Diff line change
Expand Up @@ -332,6 +332,7 @@ def define_plugin_entries(groups):
'BuildStartEndStatusGenerator',
]),
('buildbot.reporters.generators.buildset', ['BuildSetStatusGenerator']),
('buildbot.reporters.generators.comment', ['BuildCodeIssueCommentsGenerator']),
('buildbot.reporters.generators.worker', ['WorkerMissingGenerator']),
('buildbot.reporters.mail', ['MailNotifier']),
('buildbot.reporters.pushjet', ['PushjetNotifier']),
Expand All @@ -347,7 +348,11 @@ def define_plugin_entries(groups):
('buildbot.reporters.gerrit_verify_status',
['GerritVerifyStatusPush']),
('buildbot.reporters.http', ['HttpStatusPush']),
('buildbot.reporters.github', ['GitHubStatusPush', 'GitHubCommentPush']),
('buildbot.reporters.github', [
'GitHubStatusPush',
'GitHubCommentPush',
'GitHubCodeCommentPush',
]),
('buildbot.reporters.gitlab', ['GitLabStatusPush']),
('buildbot.reporters.bitbucketserver', [
'BitbucketServerStatusPush',
Expand Down