Improve type coverage for message generation.

Summary:
Introduces a new NamedTuple and a few annotations to better document
how contextual information for collections of builds is processed.

Test Plan: Unit, mypy

Reviewers: anupc

Reviewed By: anupc

Subscribers: changesbot, anupc

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

changes/buildsteps/default.py

@@ -6,7 +6,7 @@
 from copy import deepcopy
 from flask import current_app
 from itertools import chain
-from typing import Dict, List, Optional  # NOQA
+from typing import Dict, List, Optional, Type  # NOQA
 
 from changes.artifacts.analytics_json import AnalyticsJsonHandler
 from changes.artifacts.bazel_target import BazelTargetHandler
@@ -23,6 +23,7 @@
 from changes.models.jobphase import JobPhase
 from changes.models.jobstep import JobStep, FutureJobStep
 from changes.models.snapshot import SnapshotImage
+from changes.vcs.base import Vcs  # NOQA
 from changes.vcs.git import GitVcs
 from changes.vcs.hg import MercurialVcs
 
@@ -205,12 +206,12 @@ def custom_bin_path(cls):
         return ''
 
     def _other_repo_clone_commands(self, other_repos):
+        # type: (Optional[List[Dict[str, str]]]) -> List[FutureCommand]
         """
         Parses other_repos config and returns a list of FutureCommands
         that will clone said repos.
         """
-        # type: Optional[List[Dict[str, str]]] -> List[FutureCommand]
-        commands = []
+        commands = []  # type: List[FutureCommand]
         if other_repos is None:
             return commands
         if not isinstance(other_repos, list):
@@ -223,6 +224,8 @@ def _other_repo_clone_commands(self, other_repos):
             if not repo.get('path'):
                 raise ValueError("Each other_repo must specify a path")
 
+            repo_vcs = None  # type: Type[Vcs]
+
             if repo.get('backend') == 'hg':
                 repo_vcs = MercurialVcs
                 revision = repo.get('revision', 'default')

changes/lib/build_context_lib.py

@@ -1,4 +1,5 @@
 from itertools import chain, imap
+from datetime import datetime
 
 from changes.artifacts import xunit
 from flask import current_app
@@ -13,7 +14,7 @@
 from changes.utils.http import build_web_uri
 from sqlalchemy.orm import subqueryload_all
 
-from typing import Any, cast, Dict, List, Optional, Tuple  # NOQA
+from typing import Any, cast, Dict, List, NamedTuple, Optional, Tuple  # NOQA
 
 
 def _get_project_uri(build):
@@ -48,9 +49,21 @@ def _aggregate_count(items, key):
     # type: (List[Dict[str, Any]], str) -> int
     return sum(map(lambda item: cast(int, item[key]), items))
 
+CollectionContext = NamedTuple('CollectionContext',
+                               [('title', unicode),
+                                ('builds', List[Dict[str, Any]]),
+                                ('result', Result),
+                                ('target_uri', str),
+                                ('target', str),
+                                ('label', str),
+                                ('date_created', datetime),
+                                ('author', str),
+                                ('commit_message', str),
+                                ('failing_tests_count', int)])
+
 
 def get_collection_context(builds):
-    # type: (List[Build]) -> Dict[str, Any]
+    # type: (List[Build]) -> CollectionContext
     """
     Given a non-empty list of finished builds, returns a context for
     rendering the build results.
@@ -89,18 +102,18 @@ def sort_builds(builds_context):
 
     date_created = min([_build.date_created for _build in builds])
 
-    return {
-        'title': _get_title(target, build.label, result),
-        'builds': builds_context,
-        'result': result,
-        'target_uri': target_uri,
-        'target': target,
-        'label': build.label,
-        'date_created': date_created,
-        'author': build.author,
-        'commit_message': build.message or '',
-        'failing_tests_count': _aggregate_count(builds_context, 'failing_tests_count'),
-    }
+    return CollectionContext(
+        title=_get_title(target, build.label, result),
+        builds=builds_context,
+        result=result,
+        target_uri=target_uri,
+        target=target,
+        label=build.label,
+        date_created=date_created,
+        author=build.author,
+        commit_message=build.message or '',
+        failing_tests_count=_aggregate_count(builds_context, 'failing_tests_count'),
+    )
 
 
 def _get_title(target, label, result):

changes/listeners/mail.py

@@ -8,11 +8,13 @@
 from flask import current_app, render_template
 from flask_mail import Message, sanitize_address
 from jinja2 import Markup
+from typing import List  # NOQA
 
 from changes.config import db, mail
 from changes.constants import Result, Status
 from changes.db.utils import try_create
 from changes.lib import build_context_lib, build_type
+from changes.lib.build_context_lib import CollectionContext  # NOQA
 from changes.models.event import Event, EventType
 from changes.models.build import Build
 from changes.models.job import Job
@@ -66,39 +68,42 @@ def send(self, msg, build):
 
     def get_msg(self, builds):
         # type: (List[Build]) -> Message
-        context = build_context_lib.get_collection_context(builds)
-        if context['result'] == Result.passed:
+        context = build_context_lib.get_collection_context(builds)  # type: CollectionContext
+        if context.result == Result.passed:
             return None
         max_shown = current_app.config.get('MAX_SHOWN_ITEMS_PER_BUILD_MAIL', 3)
-        context.update({
+        context_dict = context._asdict()
+        context_dict.update({
             'MAX_SHOWN_ITEMS_PER_BUILD': max_shown,
             'showing_failing_tests_count':
-                sum([min(b['failing_tests_count'], max_shown) for b in context['builds']])
+                sum([min(b['failing_tests_count'], max_shown) for b in context.builds])
         })
         recipients = self.get_collection_recipients(context)
 
-        msg = Message(context['title'], recipients=recipients, extra_headers={
+        msg = Message(context.title, recipients=recipients, extra_headers={
             'Reply-To': ', '.join(sanitize_address(r) for r in recipients),
         })
-        msg.body = render_template('listeners/mail/notification.txt', **context)
+        msg.body = render_template('listeners/mail/notification.txt', **context_dict)
         msg.html = Markup(toronado.from_string(
-            render_template('listeners/mail/notification.html', **context)
+            render_template('listeners/mail/notification.html', **context_dict)
         ))
 
         return msg
 
     def get_collection_recipients(self, collection_context):
+        # type: (CollectionContext) -> List[unicode]
         """
         Returns a list of recipients for a collection context created by
         get_collection_context. Only recipients for failing builds will be
         returned.
         """
         recipient_lists = map(
             lambda build_context: self.get_build_recipients(build_context['build']),
-            collection_context['builds'])
+            collection_context.builds)
         return list(set([r for rs in recipient_lists for r in rs]))
 
     def get_build_recipients(self, build):
+        # type: (Build) -> List[unicode]
         """
         Returns a list of recipients for a build.
 

changes/listeners/phabricator_listener.py

@@ -193,7 +193,7 @@ def build_finished_handler(build_id, **kwargs):
 
     context = build_context_lib.get_collection_context(builds)
 
-    message = '\n\n'.join([_get_message_for_build_context(x) for x in context['builds']])
+    message = '\n\n'.join([_get_message_for_build_context(x) for x in context.builds])
 
     post_comment(target, message, phab)
 

changes/queue/task.py

@@ -430,6 +430,7 @@ def _report_created(self):
         statsreporter.stats().incr('new_task_created_' + self.task_name)
 
     def _report_lag(self, first_run_time):
+        # type: (datetime) -> None
         """
         Reports the time it took from creation to just before the first run of the Task; on subsequent
         runs no reporting will occur.
@@ -438,7 +439,6 @@ def _report_lag(self, first_run_time):
         Args:
             first_run_time (datetime): When the task started running.
         """
-        # type: (datetime) -> None
         t = Task.query.filter(
             Task.task_name == self.task_name,
             Task.task_id == self.task_id,

changes/utils/http.py

@@ -14,6 +14,7 @@ def build_internal_uri(path, app=current_app):
 
 
 def build_patch_uri(patch_id, app=current_app):
+    # type: (uuid.UUID, Any) -> str
     """
     Generate the URI to be used by slaves for fetching a patch.
 
@@ -25,7 +26,6 @@ def build_patch_uri(patch_id, app=current_app):
         str: URI to be used when fetching the patch on a build slave.
 
     """
-    # type: (uuid.UUID, Any) -> str
     base = app.config.get('PATCH_BASE_URI') or app.config['INTERNAL_BASE_URI']
     return _concat_uri_path(base,
                             '/api/0/patches/{0}/?raw=1'.format(patch_id.hex))

changes/vcs/base.py

@@ -7,7 +7,7 @@
 import tempfile
 
 from subprocess import Popen, PIPE, check_call, CalledProcessError
-from typing import Any, List, Set, Union  # NOQA
+from typing import Any, List, Optional, Set, Union  # NOQA
 
 from changes.constants import PROJECT_ROOT
 from changes.db.utils import create_or_update, get_or_create, try_create
@@ -155,6 +155,11 @@ def _execute_subproccess(self, proc, *args, **kwargs):
             raise CommandError(args[0], proc.returncode, stdout, stderr)
         return stdout
 
+    @staticmethod
+    def get_clone_command(remote_url, path, revision, clean=True, cache_dir=None):
+        # type: (str, str, str, bool, Optional[str]) -> str
+        raise NotImplementedError
+
     def exists(self):
         return os.path.exists(self.path)
 

tests/changes/lib/test_build_context_lib.py

@@ -60,17 +60,17 @@ def test_with_result(result):
             if result is not Result.passed and result is not Result.failed:
                 result = Result.unknown
 
-            assert context['title'] == '%s %s - %s' % (
+            assert context.title == '%s %s - %s' % (
                 'D{}'.format(revision_id), str(result).lower(), job.build.label)
-            assert len(context['builds']) == 1
-            assert context['result'] == result
-            assert context['target_uri'] == revision_url
-            assert context['target'] == 'D{}'.format(revision_id)
-            assert context['label'] == build.label
-            assert context['date_created'] == build.date_created
-            assert context['author'] == build.author
-            assert context['commit_message'] == ''
-            assert context['failing_tests_count'] == 0
+            assert len(context.builds) == 1
+            assert context.result == result
+            assert context.target_uri == revision_url
+            assert context.target == 'D{}'.format(revision_id)
+            assert context.label == build.label
+            assert context.date_created == build.date_created
+            assert context.author == build.author
+            assert context.commit_message == ''
+            assert context.failing_tests_count == 0
 
         test_with_result(Result.passed)
         test_with_result(Result.skipped)
@@ -121,18 +121,18 @@ def create_build(result):
 
         first_build = builds_correct_order[0]
 
-        assert context['title'] == '%s failed - %s' % (
+        assert context.title == '%s failed - %s' % (
             'D{}'.format(revision_id), first_build.label)
-        for i in range(len(context['builds'])):
-            assert context['builds'][i]['build'] == builds_correct_order[i]
-        assert context['result'] == Result.failed
-        assert context['target_uri'] == revision_url
-        assert context['target'] == 'D{}'.format(revision_id)
-        assert context['label'] == first_build.label
-        assert context['date_created'] == build1.date_created
-        assert context['author'] == first_build.author
-        assert context['commit_message'] == ''
-        assert context['failing_tests_count'] == 0
+        for i in range(len(context.builds)):
+            assert context.builds[i]['build'] == builds_correct_order[i]
+        assert context.result == Result.failed
+        assert context.target_uri == revision_url
+        assert context.target == 'D{}'.format(revision_id)
+        assert context.label == first_build.label
+        assert context.date_created == build1.date_created
+        assert context.author == first_build.author
+        assert context.commit_message == ''
+        assert context.failing_tests_count == 0
 
 
 class GetLogClippingTestCase(TestCase):

tests/changes/listeners/test_mail.py

@@ -177,7 +177,8 @@ def test_diff_passed_and_failed(self):
         )
         db.session.commit()
 
-        recipients = MailNotificationHandler().get_collection_recipients({"builds": [{'build': patch_build}, {'build': patch_build2}]})
+        mock_context = mock.Mock(builds=[{'build': patch_build}, {'build': patch_build2}])
+        recipients = MailNotificationHandler().get_collection_recipients(mock_context)
         assert recipients == []
 
     def test_diff_all_failed(self):
@@ -216,7 +217,8 @@ def test_diff_all_failed(self):
         )
         db.session.commit()
 
-        recipients = MailNotificationHandler().get_collection_recipients({"builds": [{'build': patch_build}, {'build': patch_build2}]})
+        mock_context = mock.Mock(builds=[{'build': patch_build}, {'build': patch_build2}])
+        recipients = MailNotificationHandler().get_collection_recipients(mock_context)
         assert recipients == [author_recipient]