From 144354bc048ed22afe1320b9e9211bfc861df755 Mon Sep 17 00:00:00 2001 From: Mikhail Goncharov Date: Thu, 6 Feb 2020 15:49:14 +0100 Subject: [PATCH] Generate HTML summary instead of adding comment + reworked phab-talk, now it's a class instead of free functions --- scripts/phabtalk/phabtalk.py | 472 ++++++++++++++++++----------------- 1 file changed, 243 insertions(+), 229 deletions(-) diff --git a/scripts/phabtalk/phabtalk.py b/scripts/phabtalk/phabtalk.py index 29ae23e..ef25397 100755 --- a/scripts/phabtalk/phabtalk.py +++ b/scripts/phabtalk/phabtalk.py @@ -31,27 +31,6 @@ from lxml import etree from phabricator import Phabricator -class BuildReport: - - def __init__(self): - self.comments = [] - self.success = True - self.working = False - self.unit = [] # type: List - self.lint = {} - self.test_stats = { - 'pass': 0, - 'fail': 0, - 'skip': 0 - } # type: Dict[str, int] - - def addLint(self, m): - key = '{}:{}'.format(m['path'], m['line']) - if key not in self.lint: - self.lint[key] = [] - self.lint[key].append(m) - - class PhabTalk: """Talk to Phabricator to upload build results. See https://secure.phabricator.com/conduit/method/harbormaster.sendmessage/ @@ -75,7 +54,7 @@ class PhabTalk: result = self._phab.differential.querydiffs(ids=[diff]) return 'D' + result[diff]['revisionID'] - def _comment_on_diff(self, diff_id: str, text: str): + def comment_on_diff(self, diff_id: str, text: str): """Add a comment to a differential based on the diff_id""" print('Sending comment to diff {}:'.format(diff_id)) print(text) @@ -101,19 +80,19 @@ class PhabTalk: self._phab.differential.revision.edit(objectIdentifier=revision, transactions=transactions) - def submit_report(self, diff_id: str, phid: str, report: BuildReport, build_result: str): + def update_build_status(self, diff_id: str, phid: str, working: bool, success: bool, lint: {}, unit: []): """Submit collected report to Phabricator. """ result_type = 'pass' - if report.working: + if working: result_type = 'working' - elif not report.success: + elif not success: result_type = 'fail' # Group lint messages by file and line. lint_messages = [] - for v in report.lint.values(): + for v in lint.values(): path = '' line = 0 descriptions = [] @@ -134,18 +113,15 @@ class PhabTalk: if self.dryrun: print('harbormaster.sendmessage =================') print('type: {}'.format(result_type)) - print('unit: {}'.format(report.unit)) + print('unit: {}'.format(unit)) print('lint: {}'.format(lint_messages)) else: _try_call(lambda: self._phab.harbormaster.sendmessage( buildTargetPHID=phid, type=result_type, - unit=report.unit, + unit=unit, lint=lint_messages)) - if len(report.comments) > 0: - _try_call(lambda: self._comment_on_diff(diff_id, '\n\n'.join(report.comments))) - def add_artifact(self, phid: str, file: str, name: str, results_url: str): _try_call(lambda: self._phab.harbormaster.createartifact( buildTargetPHID=phid, @@ -156,7 +132,7 @@ class PhabTalk: 'name': name})) -def _parse_patch(patch) -> List[Dict[str,str]]: +def _parse_patch(patch) -> List[Dict[str, str]]: """Extract the changed lines from `patch` file. The return value is a list of dictionaries {filename, line, diff}. Diff must be generated with -U0 (no context lines). @@ -199,98 +175,241 @@ def _parse_patch(patch) -> List[Dict[str,str]]: return entries -def _add_clang_format(report: BuildReport, results_dir: str, - results_url: str, clang_format_patch: str, pt: PhabTalk, ph_id: str): - """Populates results from diff produced by clang format.""" - present = (clang_format_patch is not None) and os.path.exists(os.path.join(results_dir, clang_format_patch)) - if not present: - print('clang-format result {} is not found'.format(clang_format_patch)) - report.comments.append(section_title('clang-format', False, False)) - return - p = os.path.join(results_dir, clang_format_patch) - if os.stat(p).st_size != 0: - pt.add_artifact(ph_id, clang_format_patch, "clang-format", results_url) - diffs = _parse_patch(open(p, 'r')) - success = len(diffs) == 0 - for d in diffs: - report.addLint({ - 'name': 'clang-format', - 'severity': 'autofix', - 'code': 'clang-format', - 'path': d['filename'], - 'line': d['line'], - 'char': 1, - 'description': 'please reformat the code\n```\n' + d['diff'] + '\n```', - }) - comment = section_title('clang-format', success, present) - if not success: - comment += 'Please format your changes with clang-format by running `git-clang-format HEAD^` or applying ' \ - 'this [[ {}/{} | patch ]].'.format(results_url, clang_format_patch) - report.comments.append(comment) - report.success = success and report.success +class BuildReport: + + def __init__(self, args): + # self.args = args + self.ph_id = args.ph_id # type: str + self.diff_id = args.diff_id # type: str + self.test_result_file = args.test_result_file # type: str + self.conduit_token = args.conduit_token # type: str + self.dryrun = args.dryrun # type: bool + self.buildresult = args.buildresult # type: str + self.clang_format_patch = args.clang_format_patch # type: str + self.clang_tidy_result = args.clang_tidy_result # type: str + self.clang_tidy_ignore = args.clang_tidy_ignore # type: str + self.results_dir = args.results_dir # type: str + self.results_url = args.results_url # type: str + self.workspace = args.workspace # type: str + + self.api = PhabTalk(args.conduit_token, args.host, args.dryrun) + + self.comments = [] + self.success = True + self.working = False + self.unit = [] # type: List + self.lint = {} + self.test_stats = { + 'pass': 0, + 'fail': 0, + 'skip': 0 + } # type: Dict[str, int] + + def add_lint(self, m): + key = '{}:{}'.format(m['path'], m['line']) + if key not in self.lint: + self.lint[key] = [] + self.lint[key].append(m) + + def final_report(self): + if self.buildresult is not None: + print('Jenkins result: {}'.format(self.buildresult)) + if self.buildresult.lower() == 'success': + pass + elif self.buildresult.lower() == 'null': + self.working = True + else: + self.success = False + + self.add_test_results() + self.add_clang_tidy() + self.add_clang_format() + self.api.update_build_status(self.diff_id, self.ph_id, self.working, self.success, self.lint, self.unit) + + self.add_links_to_artifacts() + + title = 'Issue with build for {} ({})'.format(self.api.get_revision_id(self.diff_id), self.diff_id) + self.comments.append( + 'Pre-merge checks is in beta. report issue.
' + 'Please join beta or ' + 'enable it for your project'.format( + urllib.parse.quote(title))) + with open(os.path.join(self.results_dir, 'summary.html'), 'w') as f: + f.write('') + f.write('

' + '

'.join(self.comments) + '

') + f.write('') + self.api.add_artifact(self.ph_id, 'summary.html', 'summary', self.results_url) + + def add_clang_format(self): + """Populates results from diff produced by clang format.""" + present = (self.clang_format_patch is not None) and os.path.exists( + os.path.join(self.results_dir, self.clang_format_patch)) + if not present: + print('clang-format result {} is not found'.format(self.clang_format_patch)) + self.comments.append(section_title('clang-format', False, False)) + return + p = os.path.join(self.results_dir, self.clang_format_patch) + if os.stat(p).st_size != 0: + self.api.add_artifact(self.ph_id, self.clang_format_patch, "clang-format", self.results_url) + diffs = _parse_patch(open(p, 'r')) + success = len(diffs) == 0 + for d in diffs: + self.add_lint({ + 'name': 'clang-format', + 'severity': 'autofix', + 'code': 'clang-format', + 'path': d['filename'], + 'line': d['line'], + 'char': 1, + 'description': 'please reformat the code\n```\n' + d['diff'] + '\n```', + }) + comment = section_title('clang-format', success, present) + if not success: + comment += 'Please format your changes with clang-format by running `git-clang-format HEAD^` or applying ' \ + 'this patch.'.format(self.clang_format_patch) + self.comments.append(comment) + self.success = success and self.success + + def add_clang_tidy(self): + # Typical message looks like + # [..]/clang/include/clang/AST/DeclCXX.h:3058:20: error: no member named 'LifetimeExtendedTemporary' in 'clang::Decl' [clang-diagnostic-error] + pattern = '^{}/([^:]*):(\\d+):(\\d+): (.*): (.*)'.format(self.workspace) + errors_count = 0 + warn_count = 0 + inline_comments = 0 + present = (self.clang_tidy_result is not None) and os.path.exists( + os.path.join(self.results_dir, self.clang_tidy_result)) + if not present: + print('clang-tidy result {} is not found'.format(self.clang_tidy_result)) + self.comments.append(section_title('clang-tidy', False, False)) + return + present = (self.clang_tidy_ignore is not None) and os.path.exists(self.clang_tidy_ignore) + if not present: + print('clang-tidy ignore file {} is not found'.format(self.clang_tidy_ignore)) + self.comments.append(section_title('clang-tidy', False, False)) + return + p = os.path.join(self.results_dir, self.clang_tidy_result) + if os.stat(p).st_size != 0: + self.api.add_artifact(self.ph_id, self.clang_tidy_result, "clang-tidy", self.results_url) + ignore = pathspec.PathSpec.from_lines(pathspec.patterns.GitWildMatchPattern, + open(self.clang_tidy_ignore, 'r').readlines()) + for line in open(p, 'r'): + match = re.search(pattern, line) + if match: + file_name = match.group(1) + line_pos = match.group(2) + char_pos = match.group(3) + severity = match.group(4) + text = match.group(5) + text += '\n[[{} | not useful]] '.format( + 'https://github.com/google/llvm-premerge-checks/blob/master/docs/clang_tidy.md#warning-is-not' + '-useful') + if severity in ['warning', 'error']: + if severity == 'warning': + warn_count += 1 + if severity == 'error': + errors_count += 1 + if ignore.match_file(file_name): + print('{} is ignored by pattern and no comment will be added'.format(file_name)) + else: + inline_comments += 1 + self.add_lint({ + 'name': 'clang-tidy', + 'severity': 'warning', + 'code': 'clang-tidy', + 'path': file_name, + 'line': int(line_pos), + 'char': int(char_pos), + 'description': '{}: {}'.format(severity, text), + }) + success = errors_count + warn_count == 0 + comment = section_title('clang-tidy', success, present) + if not success: + comment += 'clang-tidy found {} errors and {} warnings. ' \ + '{} of them are added as review comments why?.'.format( + self.clang_tidy_result, errors_count, warn_count, inline_comments, + 'https://github.com/google/llvm-premerge-checks/blob/master/docs/clang_tidy.md#review-comments') + + self.comments.append(comment) + self.success = success and self.success + + def add_test_results(self): + """Populates results from build test results XML. + + Only reporting failed tests as the full test suite is too large to upload. + """ + + success = True + present = (self.test_result_file is not None) and os.path.exists( + os.path.join(self.results_dir, self.test_result_file)) + if not present: + print('Warning: Could not find test results file: {}'.format(self.test_result_file)) + self.comments.append(section_title('Unit tests', False, present)) + return + + root_node = etree.parse(os.path.join(self.results_dir, self.test_result_file)) + for test_case in root_node.xpath('//testcase'): + test_result = _test_case_status(test_case) + self.test_stats[test_result] += 1 + + if test_result == 'fail': + success = False + failure = test_case.find('failure') + test_result = { + 'name': test_case.attrib['name'], + 'namespace': test_case.attrib['classname'], + 'result': test_result, + 'duration': float(test_case.attrib['time']), + 'details': failure.text + } + self.unit.append(test_result) + + comment = section_title('Unit tests', success, True) + comment += '{} tests passed, {} failed and {} were skipped.\n'.format( + self.test_stats['pass'], + self.test_stats['fail'], + self.test_stats['skip'], + ) + for test_case in self.unit: + if test_case['result'] == 'fail': + comment += ' failed: {}/{}\n'.format(test_case['namespace'], test_case['name']) + self.comments.append(comment) + self.success = success and self.success + + def add_links_to_artifacts(self): + """Comment on a diff, read text from file.""" + file_links = [] + for f in os.listdir(self.results_dir): + if f == 'summary.html': + continue + p = os.path.join(self.results_dir, f) + if not os.path.isfile(p): + continue + if os.stat(p).st_size == 0: + continue + file_links.append('{0}'.format(f)) + if len(file_links) > 0: + self.comments.append('Build artifacts:
' + '
'.join(file_links)) -def _add_clang_tidy(report: BuildReport, results_dir: str, results_url: str, workspace: str, clang_tidy_file: str, - clang_tidy_ignore: str, pt: PhabTalk, ph_id: str): - # Typical message looks like - # [..]/clang/include/clang/AST/DeclCXX.h:3058:20: error: no member named 'LifetimeExtendedTemporary' in 'clang::Decl' [clang-diagnostic-error] - pattern = '^{}/([^:]*):(\\d+):(\\d+): (.*): (.*)'.format(workspace) - errors_count = 0 - warn_count = 0 - inline_comments = 0 - present = (clang_tidy_file is not None) and os.path.exists(os.path.join(results_dir, clang_tidy_file)) - if not present: - print('clang-tidy result {} is not found'.format(clang_tidy_file)) - report.comments.append(section_title('clang-tidy', False, False)) - return - present = (clang_tidy_ignore is not None) and os.path.exists(clang_tidy_ignore) - if not present: - print('clang-tidy ignore file {} is not found'.format(clang_tidy_ignore)) - report.comments.append(section_title('clang-tidy', False, False)) - return - p = os.path.join(results_dir, clang_tidy_file) - if os.stat(p).st_size != 0: - pt.add_artifact(ph_id, clang_tidy_file, "clang-tidy", results_url) - ignore = pathspec.PathSpec.from_lines(pathspec.patterns.GitWildMatchPattern, - open(clang_tidy_ignore, 'r').readlines()) - for line in open(p, 'r'): - match = re.search(pattern, line) - if match: - file_name = match.group(1) - line_pos = match.group(2) - char_pos = match.group(3) - severity = match.group(4) - text = match.group(5) - text += '\n[[https://github.com/google/llvm-premerge-checks/blob/master/docs/clang_tidy.md#warning-is-not' \ - '-useful | not useful]] ' - if severity in ['warning', 'error']: - if severity == 'warning': - warn_count += 1 - if severity == 'error': - errors_count += 1 - if ignore.match_file(file_name): - print('{} is ignored by pattern and no comment will be added'.format(file_name)) - else: - inline_comments += 1 - report.addLint({ - 'name': 'clang-tidy', - 'severity': 'warning', - 'code': 'clang-tidy', - 'path': file_name, - 'line': int(line_pos), - 'char': int(char_pos), - 'description': '{}: {}'.format(severity, text), - }) - success = errors_count + warn_count == 0 - comment = section_title('clang-tidy', success, present) - if not success: - comment += "clang-tidy found [[ {}/{} | {} errors and {} warnings]]. {} of them are added as review comments " \ - "below ([[https://github.com/google/llvm-premerge-checks/blob/master/docs/clang_tidy.md#review" \ - "-comments | why?]])." \ - .format(results_url, clang_tidy_file, errors_count, warn_count, inline_comments) +def _test_case_status(test_case) -> str: + """Get the status of a test case based on an etree node.""" + if test_case.find('failure') is not None: + return 'fail' + if test_case.find('skipped') is not None: + return 'skip' + return 'pass' - report.comments.append(comment) - report.success = success and report.success + +def section_title(title: str, ok: bool, present: bool) -> str: + icon = '[?]' + result = 'unknown' + if present: + icon = '[V]' if ok else '[X]' + result = 'pass' if ok else 'fail' + return '{} {}: {}. '.format(icon, title, result) def _try_call(call): @@ -309,114 +428,7 @@ def _try_call(call): break -def _add_test_results(report: BuildReport, results_dir: str, build_result_file: str): - """Populates results from build test results XML. - - Only reporting failed tests as the full test suite is too large to upload. - """ - - success = True - present = (build_result_file is not None) and os.path.exists(os.path.join(results_dir, build_result_file)) - if not present: - print('Warning: Could not find test results file: {}'.format(build_result_file)) - report.comments.append(section_title('Unit tests', False, present)) - return - - root_node = etree.parse(os.path.join(results_dir, build_result_file)) - for test_case in root_node.xpath('//testcase'): - test_result = _test_case_status(test_case) - report.test_stats[test_result] += 1 - - if test_result == 'fail': - success = False - failure = test_case.find('failure') - test_result = { - 'name': test_case.attrib['name'], - 'namespace': test_case.attrib['classname'], - 'result': test_result, - 'duration': float(test_case.attrib['time']), - 'details': failure.text - } - report.unit.append(test_result) - - comment = section_title('Unit tests', success, True) - comment += '{} tests passed, {} failed and {} were skipped.\n'.format( - report.test_stats['pass'], - report.test_stats['fail'], - report.test_stats['skip'], - ) - for test_case in report.unit: - if test_case['result'] == 'fail': - comment += ' failed: {}/{}\n'.format(test_case['namespace'], test_case['name']) - report.comments.append(comment) - report.success = success and report.success - - -def _add_links_to_artifacts(report: BuildReport, results_dir: str, results_url: str): - """Comment on a diff, read text from file.""" - file_links = [] - for f in os.listdir(results_dir): - if not os.path.isfile(os.path.join(results_dir, f)): - continue - file_links.append('[[{0}/{1} | {1}]]'.format(results_url, f)) - if len(file_links) > 0: - report.comments.append('[[ {} | Build artifacts ]]: '.format(results_url) + ', '.join(file_links)) - - -def _test_case_status(test_case) -> str: - """Get the status of a test case based on an etree node.""" - if test_case.find('failure') is not None: - return 'fail' - if test_case.find('skipped') is not None: - return 'skip' - return 'pass' - - -def section_title(title: str, ok: bool, present: bool) -> str: - icon = '{icon question-circle color=gray}' - result = 'unknown' - if present: - icon = '{icon check-circle color=green}' if ok else '{icon times-circle color=red}' - result = 'pass' if ok else 'fail' - return '{} {}: {}. '.format(icon, title, result) - - def main(): - args = _parse_args() - report = BuildReport() - - if args.buildresult is not None: - print('Jenkins result: {}'.format(args.buildresult)) - if args.buildresult.lower() == 'success': - pass - elif args.buildresult.lower() == 'null': - report.working = True - else: - report.success = False - - p = PhabTalk(args.conduit_token, args.host, args.dryrun) - _add_test_results(report, args.results_dir, args.test_result_file) - _add_clang_tidy(report, args.results_dir, args.results_url, args.workspace, args.clang_tidy_result, - args.clang_tidy_ignore, p, args.ph_id) - _add_clang_format(report, args.results_dir, args.results_url, args.clang_format_patch, p, args.ph_id) - _add_links_to_artifacts(report, args.results_dir, args.results_url) - - with open(os.path.join(args.results_dir, 'summary.html'), 'w') as f: - f.write('Build summary:
clang-tidy') - p.add_artifact(args.ph_id, 'summary.html', 'summary', args.results_url) - - title = 'Issue with build for {} ({})'.format(p.get_revision_id(args.diff_id), args.diff_id) - report.comments.append( - '//Pre-merge checks is in beta. [[ https://github.com/google/llvm-premerge-checks/issues/new?assignees' - '=&labels=bug&template=bug_report.md&title={} | Report issue]]. ' - 'Please [[ https://reviews.llvm.org/project/update/78/join/ | join beta ]] or ' - '[[ https://github.com/google/llvm-premerge-checks/issues/new?assignees=&labels=enhancement&template' - '=&title=enable%20checks%20for%20{{PATH}} | enable it for your project ]].//'.format( - urllib.parse.quote(title))) - p.submit_report(args.diff_id, args.ph_id, report, args.buildresult) - - -def _parse_args(): parser = argparse.ArgumentParser( description='Write build status back to Phabricator.') parser.add_argument('ph_id', type=str) @@ -444,7 +456,9 @@ def _parse_args(): dest='results_url', help="public URL to access results directory") parser.add_argument('--workspace', type=str, required=True, help="path to workspace") - return parser.parse_args() + args = parser.parse_args() + reporter = BuildReport(args) + reporter.final_report() if __name__ == '__main__':