[changes] Include list of failed tests on changes

Summary:
Same as title also removes a lot of redundant info.

Comment for a build with a lot of failures: https://tails.corp.dropbox.com/D95380#1447721
Comment for a build with < 10 failures: https://tails.corp.dropbox.com/D95380#1447726

Test Plan: unit tests, manually make the conduit calls view results

Reviewers: vishal, wwu, alexallain

Reviewed By: wwu

Subscribers: changesbot, wwu

Differential Revision: https://tails.corp.dropbox.com/D95495

changes/listeners/phabricator_listener.py

@@ -2,6 +2,8 @@
 import time
 import hashlib
 import json
+from changes.models.job import Job
+from changes.models.test import TestCase
 import requests
 
 from flask import current_app
@@ -10,6 +12,7 @@
 from changes.constants import Result
 from changes.models import Build, ProjectOption
 from changes.utils.http import build_uri
+from sqlalchemy.orm import joinedload
 
 logger = logging.getLogger('phabricator-listener')
 
@@ -97,21 +100,66 @@ def build_finished_handler(build_id, **kwargs):
     else:
         result_image = '{icon question, color=orange}'
 
-    message = u'Build {result} {image} - [{project} #{number}]({link}) ({target}).'.format(
+    message = u'{project} build {result} {image} - ([results]({link})).'.format(
+        project=build.project.name,
         image=result_image,
-        number='{0}'.format(build.number),
         result=unicode(build.result),
-        target=build.target or build.source.revision_sha or 'Unknown',
-        project=build.project.name,
         link=build_uri('/projects/{0}/builds/{1}/'.format(build.project.slug, build.id.hex))
     )
 
-    if build.author:
-        message += ' - {author}'.format(author=build.author.email,)
+    jobs = list(Job.query.filter(
+        Job.build_id == build_id,
+    ))
+
+    test_failures = TestCase.query.options(
+        joinedload('job', innerjoin=True),
+        ).filter(
+        TestCase.job_id.in_([j.id for j in jobs]),
+        TestCase.result == Result.failed,
+        ).order_by(TestCase.name.asc())
+    num_test_failures = test_failures.count()
+
+    if num_test_failures > 0:
+        message += ' There were [{num_failures} test failures]({link})'.format(
+            num_failures=num_test_failures,
+            link=build_uri('/projects/{0}/builds/{1}/tests/?result=failed'.format(build.project.slug, build.id.hex))
+        )
+
+        message += '\n'
+        message += get_remarkup_test_failure_table(build, test_failures)
 
     post_comment(target, message)
 
 
+def get_remarkup_test_failure_table(build, tests):
+    did_truncate = False
+    num_failures = tests.count()
+    if num_failures > 10:
+        tests = tests[:10]
+        did_truncate = True
+
+    table = ['|Test Name | Package|',
+             '|--|--|']
+    for test in tests:
+        pkg = test.package
+        name = test.name
+        if name.startswith(pkg):
+            name = name[len(pkg) + 1:]
+
+        test_link = build_uri('/projects/{0}/builds/{1}/jobs/{2}/tests/{3}/'.format(
+            build.project.slug,
+            build.id.hex,
+            test.job_id.hex,
+            test.id.hex
+        ))
+        table = table + ['|[%s](%s)|%s|' % (name, test_link, pkg)]
+
+    if did_truncate:
+        table += ['|...more...|...|']
+
+    return '\n'.join(table)
+
+
 def post_comment(target, message):
     try:
         logger.info("Posting build results to %s", target)

tests/changes/listeners/test_phabricator.py

@@ -5,12 +5,12 @@
 import mock
 
 from changes.constants import Result
-from changes.testutils import TestCase
+from changes.testutils import TestCase as UnitTestCase
 from changes.listeners.phabricator_listener import build_finished_handler
 from changes.utils.http import build_uri
 
 
-class PhabricatorListenerTest(TestCase):
+class PhabricatorListenerTest(UnitTestCase):
     def setUp(self):
         super(PhabricatorListenerTest, self).setUp()
         current_app.config['PHABRICATOR_POST_BUILD_RESULT'] = True
@@ -47,7 +47,48 @@ def test_whitelisted_project(self, get_options, phab):
         get_options.assert_called_once_with(project.id)
         build_link = build_uri('/projects/{0}/builds/{1}/'.format(
             build.project.slug, build.id.hex))
-        expected_msg = "Build Failed {{icon times, color=red}} - [test #1]({0}) (D1).".format(
-            build_link)
+        expected_msg = "test build Failed {{icon times, color=red}} - ([results]({0})).".format(
+            build_link
+        )
 
         phab.assert_called_once_with('1', expected_msg)
+
+    @mock.patch('changes.listeners.phabricator_listener.post_diff_comment')
+    @mock.patch('changes.listeners.phabricator_listener.get_options')
+    def test_build_failure_with_tests(self, get_options, phab):
+        get_options.return_value = {
+            'phabricator.notify': '1'
+        }
+        project = self.create_project(name='Server', slug='project-slug')
+        self.assertEquals(phab.call_count, 0)
+        build = self.create_build(project, result=Result.failed, target='D1')
+        job = self.create_job(build=build)
+        testcase = self.create_test(
+            package='test.group.ClassName',
+            name='test.group.ClassName.test_foo',
+            job=job,
+            duration=134,
+            result=Result.failed,
+            )
+
+        build_finished_handler(build_id=build.id.hex)
+
+        get_options.assert_called_once_with(project.id)
+        build_link = build_uri('/projects/{0}/builds/{1}/'.format(
+            build.project.slug, build.id.hex))
+        failure_link = build_uri('/projects/{0}/builds/{1}/tests/?result=failed'.format(
+            build.project.slug, build.id.hex))
+
+        test_link = build_uri('/projects/{0}/builds/{1}/jobs/{2}/tests/{3}/'.format(
+            build.project.slug,
+            build.id.hex,
+            testcase.job_id.hex,
+            testcase.id.hex
+        ))
+        test_desc = "[test_foo](%s)" % test_link
+        expected_msg = """Server build Failed {{icon times, color=red}} - ([results]({0})). There were [1 test failures]({1})
+|Test Name | Package|
+|--|--|
+|{2}|test.group.ClassName|"""
+
+        phab.assert_called_once_with('1', expected_msg.format(build_link, failure_link, test_desc))