Generate HTML summary instead of adding comment

+ reworked phab-talk, now it's a class instead of free functions

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,21 +175,88 @@ 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):
+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. <a href="https://github.com/google/llvm-premerge-checks/issues/new?assignees'
+            '=&labels=bug&template=bug_report.md&title={}">report issue</a>.<br/>'
+            'Please <a href="https://reviews.llvm.org/project/update/78/join/">join beta</a> or '
+            '<a href="https://github.com/google/llvm-premerge-checks/issues/new?assignees=&labels=enhancement&template'
+            '=&title=enable%20checks%20for%20{{PATH}}">enable it for your project</a>'.format(
+                urllib.parse.quote(title)))
+        with open(os.path.join(self.results_dir, 'summary.html'), 'w') as f:
+            f.write('<html><head><style>body { font-family: monospace; margin: 16px; }</style></head><body>')
+            f.write('<p>' + '</p><p>'.join(self.comments) + '</p>')
+            f.write('</body></html>')
+            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 = (clang_format_patch is not None) and os.path.exists(os.path.join(results_dir, clang_format_patch))
+        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(clang_format_patch))
-        report.comments.append(section_title('clang-format', False, False))
+            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(results_dir, clang_format_patch)
+        p = os.path.join(self.results_dir, self.clang_format_patch)
         if os.stat(p).st_size != 0:
-        pt.add_artifact(ph_id, clang_format_patch, "clang-format", results_url)
+            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:
-        report.addLint({
+            self.add_lint({
                 'name': 'clang-format',
                 'severity': 'autofix',
                 'code': 'clang-format',
@@ -225,34 +268,33 @@ def _add_clang_format(report: BuildReport, results_dir: str,
         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
+                       'this <a href="{}">patch</a>.'.format(self.clang_format_patch)
+        self.comments.append(comment)
+        self.success = success and self.success
 
-
-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):
+    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(workspace)
+        pattern = '^{}/([^:]*):(\\d+):(\\d+): (.*): (.*)'.format(self.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))
+        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(clang_tidy_file))
-        report.comments.append(section_title('clang-tidy', False, False))
+            print('clang-tidy result {} is not found'.format(self.clang_tidy_result))
+            self.comments.append(section_title('clang-tidy', False, False))
             return
-    present = (clang_tidy_ignore is not None) and os.path.exists(clang_tidy_ignore)
+        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(clang_tidy_ignore))
-        report.comments.append(section_title('clang-tidy', False, False))
+            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(results_dir, clang_tidy_file)
+        p = os.path.join(self.results_dir, self.clang_tidy_result)
         if os.stat(p).st_size != 0:
-        pt.add_artifact(ph_id, clang_tidy_file, "clang-tidy", results_url)
+            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(clang_tidy_ignore, 'r').readlines())
+                                              open(self.clang_tidy_ignore, 'r').readlines())
         for line in open(p, 'r'):
             match = re.search(pattern, line)
             if match:
@@ -261,8 +303,9 @@ def _add_clang_tidy(report: BuildReport, results_dir: str, results_url: str, wor
                 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]] '
+                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
@@ -272,7 +315,7 @@ def _add_clang_tidy(report: BuildReport, results_dir: str, results_url: str, wor
                         print('{} is ignored by pattern and no comment will be added'.format(file_name))
                     else:
                         inline_comments += 1
-                    report.addLint({
+                        self.add_lint({
                             'name': 'clang-tidy',
                             'severity': 'warning',
                             'code': 'clang-tidy',
@@ -284,13 +327,89 @@ def _add_clang_tidy(report: BuildReport, results_dir: str, results_url: str, wor
         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)
+            comment += 'clang-tidy found <a href="{}">{} errors and {} warnings</a>. ' \
+                       '{} of them are added as review comments <a href="{}">why?</a>.'.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')
 
-    report.comments.append(comment)
-    report.success = success and report.success
+        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('<a href="{0}">{0}</a>'.format(f))
+        if len(file_links) > 0:
+            self.comments.append('<a href="./">Build artifacts</a>:<br/>' + '<br/>'.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 = '[?]'
+    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('<html><body>Build summary:<br/><a href="clang-tidy.txt">clang-tidy</a></body></html>')
-    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__':