Merge ~cjwatson/launchpad:test-notification-unittest into launchpad:master

[Colin Watson]

Rewrite notification doctests as unit tests

The `zope.container` removal in c4be9ccd819340ce13c04b84889ba7e536da1975 meant that the notifications doctests didn't clean up after themselves properly any more due to using `provideAdapter` directly rather than something like `ZopeAdapterFixture`, causing test isolation errors. Rewrite these doctests as unit tests, making it easier to avoid this problem using fixtures.

[Andrey Fedoseev (andrey-fedoseev)]

review: Approve

diff --git a/lib/lp/services/webapp/doc/notification-text-escape.rst b/lib/lp/services/webapp/doc/notification-text-escape.rst
deleted file mode 100644
index 4c13fb6..0000000
--- a/lib/lp/services/webapp/doc/notification-text-escape.rst
+++ /dev/null
@@ -1,88 +0,0 @@
-Notification Text Escaping
-==========================
-
-There are a number of user actions that may generate on-screen
-notifications, such as moving a bug or deleting a branch.  Some of
-these notifications display potentially unsafe text that is obtained
-from the user.  In order to prevent a cross-site-scripting attack,
-HTML characters in notifications must be escaped.  However, there are
-special cases where notifications from known safe sources must be
-allowed to pass HTML through.  This document exercises these
-mechanisms.
-
-    >>> from lp.services.webapp.notifications import (
-    ...	     NotificationResponse, NotificationRequest)
-    >>> def new_response():
-    ...     response = NotificationResponse()
-    ...     request  = NotificationRequest()
-    ...     request.response = response
-    ...     response._request = request
-    ...	    return response
-    >>>
-
-Plain text passed into the object's addNotification() method is
-unchanged:
-
-    >>> response = new_response()
-    >>> response.addNotification('clean')
-    >>> for notification in response.notifications:
-    ...   print(notification.message)
-    clean
-
-But text containing markup is CGI-escaped:
-
-    >>> response = new_response()
-    >>> response.addNotification(u'<br/>dirty')
-    >>> for notification in response.notifications:
-    ...     print(notification.message)
-    &lt;br/&gt;dirty
-
-
-If the object passed to addNotification() publishes the
-IStructuredString interface, then a string will be returned with the
-appropriate sections escaped and unescaped.
-
-    >>> from lp.services.webapp.interfaces import IStructuredString
-    >>> from lp.services.webapp.escaping import structured
-    >>> msg = u'<b>%(escaped)s</b>'
-    >>> structured_text = structured(msg, escaped=u'<br/>foo')
-    >>> IStructuredString.providedBy(structured_text)
-    True
-    >>> print(structured_text.escapedtext)
-    <b>&lt;br/&gt;foo</b>
-
-    >>> response = new_response()
-    >>> response.addNotification(structured_text)
-    >>> for notification in response.notifications:
-    ...     print(notification.message)
-    <b>&lt;br/&gt;foo</b>
-
-Passing an object to addNotification() that is an instance of
-zope.i18n.Message will be escaped in the same
-manner as raw text.
-
-    >>> import zope.i18n
-    >>> msgtxt   = zope.i18n.Message(u'<br/>foo')
-    >>> response = new_response()
-    >>> response.addNotification(msgtxt)
-    >>> for notification in response.notifications:
-    ...     print(notification.message)
-    &lt;br/&gt;foo
-
-To pass internationalized text that contains markup, one may call
-structured() directly with an internationalized object.  structured()
-performs the translation and substitution, and the resulting object
-may then be passed to addNotification().
-
-    >>> from lp import _
-    >>> msgid   = _(u'<good/>%(evil)s')
-    >>> escapee = '<evil/>'
-    >>> text    = structured(msgid, evil=escapee)
-    >>> print(text.escapedtext)
-    <good/>&lt;evil/&gt;
-
-    >>> response = new_response()
-    >>> response.addNotification(text)
-    >>> for notification in response.notifications:
-    ...     print(notification.message)
-    <good/>&lt;evil/&gt;
diff --git a/lib/lp/services/webapp/notifications.py b/lib/lp/services/webapp/notifications.py
index 9e76431..12a4516 100644
--- a/lib/lp/services/webapp/notifications.py
+++ b/lib/lp/services/webapp/notifications.py
@@ -41,35 +41,7 @@ class NotificationRequest:
     """NotificationRequest extracts notifications to display to the user
     from the request and session
 
-    It is designed to be mixed in with an IBrowserRequest
-
-    By default, there are no notifications
-
-    >>> request = NotificationRequest()
-    >>> len(request.notifications)
-    0
-    >>> INotificationRequest.providedBy(request)
-    True
-
-    >>> request = NotificationRequest()
-    >>> session = ISession(request)[SESSION_KEY]
-    >>> notifications = NotificationList()
-    >>> session['notifications'] = notifications
-    >>> notifications.append(Notification(0, 'Fnord'))
-    >>> for notification in request.notifications:
-    ...     print(notification.message)
-    Fnord
-
-    Note that NotificationRequest.notifications also returns any notifications
-    that have been added so far in this request, making it the single source
-    you need to interogate to display notifications to the user.
-
-    >>> response = INotificationResponse(request)
-    >>> response.addNotification('Aargh')
-    >>> for notification in request.notifications:
-    ...     print(notification.message)
-    Fnord
-    Aargh
+    It is designed to be mixed in with an IBrowserRequest.
     """
 
     @property
@@ -86,84 +58,6 @@ class NotificationResponse:
 
     It needs to be mixed in with an IHTTPApplicationResponse so its redirect
     method intercepts the default behaviour.
-
-    >>> class MyNotificationResponse(NotificationResponse, MockResponse):
-    ...     pass
-    >>> response = MyNotificationResponse()
-    >>> INotificationResponse.providedBy(response)
-    True
-    >>> request = NotificationRequest()
-    >>> request.response = response
-    >>> response._request = request
-    >>> request.principal = None # full IRequests are zope.security
-    ... # participations, and NotificationResponse.redirect expects a
-    ... # principal, as in the full IRequest interface.
-
-    >>> len(response.notifications)
-    0
-
-    >>> response.addNotification("something")
-    >>> len(response.notifications)
-    1
-
-    >>> response.removeAllNotifications()
-    >>> len(response.notifications)
-    0
-
-    >>> msg = structured("<b>%(escaped)s</b>", escaped="<Fnord>")
-    >>> response.addNotification(msg)
-
-    >>> response.addNotification("Whatever", BrowserNotificationLevel.DEBUG)
-    >>> response.addDebugNotification('Debug')
-    >>> response.addInfoNotification('Info')
-    >>> response.addWarningNotification('Warning')
-
-    And an odd one to test Bug #54987
-
-    >>> from lp import _
-    >>> response.addErrorNotification(_('Error${value}', mapping={'value':''}))
-
-    >>> INotificationList.providedBy(response.notifications)
-    True
-
-    >>> for notification in response.notifications:
-    ...     print("%d -- %s" % (notification.level, notification.message))
-    20 -- <b>&lt;Fnord&gt;</b>
-    10 -- Whatever
-    10 -- Debug
-    20 -- Info
-    30 -- Warning
-    40 -- Error
-
-    >>> response.redirect("http://example.com?foo=bar")
-    302: http://example.com?foo=bar
-
-    Once redirect has been called, any notifications that have been set
-    are stored in the session
-
-    >>> for notification in ISession(request)[SESSION_KEY]['notifications']:
-    ...     print("%d -- %s" % (notification.level, notification.message))
-    ...     break
-    20 -- <b>&lt;Fnord&gt;</b>
-
-    If there are no notifications, the session is not touched. This ensures
-    that we don't needlessly burden the session storage.
-
-    >>> response = MyNotificationResponse()
-    >>> request = NotificationRequest()
-    >>> request.response = response
-    >>> response._request = request
-
-    >>> session = ISession(request)[SESSION_KEY]
-    >>> del ISession(request)[SESSION_KEY]['notifications']
-    >>> 'notifications' in session
-    False
-    >>> len(response.notifications)
-    0
-    >>> response.redirect("http://example.com")
-    302: http://example.com
-    >>> 'notifications' in session
-    False
     """
 
     # We stuff our Notifications here until we are sure we should persist it
@@ -245,34 +139,7 @@ class NotificationResponse:
 
 @implementer(INotificationList)
 class NotificationList(list):
-    """
-    Collection of INotification instances with a creation date
-
-    >>> notifications = NotificationList()
-    >>> notifications.created <= datetime.utcnow()
-    True
-    >>> notifications[0]
-    Traceback (most recent call last):
-    ...
-    IndexError: list index out of range
-
-    >>> debug = BrowserNotificationLevel.DEBUG
-    >>> error = BrowserNotificationLevel.ERROR
-    >>> notifications.append(Notification(error, u'An error'))
-    >>> notifications.append(Notification(debug, u'A debug message'))
-    >>> for notification in notifications:
-    ...     print(notification.message)
-    An error
-    A debug message
-
-    The __getitem__ method is also overloaded to allow TALES expressions
-    to easily retrieve lists of notifications that match a particular
-    notification level.
-
-    >>> for notification in notifications['debug']:
-    ...     print(notification.message)
-    A debug message
-    """
+    """Collection of INotification instances with a creation date."""
 
     created = None
 
diff --git a/lib/lp/services/webapp/tests/test_doc.py b/lib/lp/services/webapp/tests/test_doc.py
index fa82b88..b8a2e9c 100644
--- a/lib/lp/services/webapp/tests/test_doc.py
+++ b/lib/lp/services/webapp/tests/test_doc.py
@@ -8,7 +8,6 @@ Run the doctests and pagetests.
 import os
 
 from lp.services.testing import build_test_suite
-from lp.services.webapp.tests import test_notifications
 from lp.testing.layers import (
     FunctionalLayer,
     LaunchpadFunctionalLayer,
@@ -29,10 +28,6 @@ special = {
         '../doc/canonical_url.rst',
         setUp=setUp, tearDown=tearDown,
         layer=FunctionalLayer,),
-    'notification-text-escape.rst': LayeredDocFileSuite(
-        '../doc/notification-text-escape.rst',
-        setUp=test_notifications.setUp,
-        stdout_logging=False, layer=FunctionalLayer),
     'test_adapter.rst': LayeredDocFileSuite(
         '../doc/test_adapter.rst',
         setUp=setGlobs,
diff --git a/lib/lp/services/webapp/tests/test_notifications.py b/lib/lp/services/webapp/tests/test_notifications.py
index fda9346..434dac1 100644
--- a/lib/lp/services/webapp/tests/test_notifications.py
+++ b/lib/lp/services/webapp/tests/test_notifications.py
@@ -3,10 +3,13 @@
 
 """Module docstring goes here."""
 
-from doctest import DocTestSuite
-import unittest
+from datetime import datetime
 
-from zope.component import provideAdapter
+from testtools.matchers import (
+    MatchesListwise,
+    MatchesStructure,
+    )
+from zope.i18n import Message
 from zope.interface import implementer
 from zope.publisher.browser import TestRequest
 from zope.publisher.interfaces.browser import IBrowserRequest
@@ -16,12 +19,24 @@ from zope.session.interfaces import (
     ISessionData,
     )
 
+from lp import _
 from lp.services.webapp.escaping import structured
 from lp.services.webapp.interfaces import (
+    BrowserNotificationLevel,
+    INotificationList,
     INotificationRequest,
     INotificationResponse,
+    IStructuredString,
+    )
+from lp.services.webapp.notifications import (
+    Notification,
+    NotificationList,
+    NotificationRequest,
+    NotificationResponse,
+    SESSION_KEY,
     )
-from lp.services.webapp.notifications import NotificationResponse
+from lp.testing import TestCase
+from lp.testing.fixture import ZopeAdapterFixture
 from lp.testing.layers import FunctionalLayer
 
 
@@ -48,11 +63,20 @@ class MockSessionData(dict):
 @implementer(IHTTPApplicationResponse)
 class MockHTTPApplicationResponse:
 
+    def __init__(self, *args, **kwargs):
+        super().__init__(*args, **kwargs)
+        self.redirect_log = []
+
     def redirect(self, location, status=None, trusted=False):
-        """Just report the redirection to the doctest"""
+        """Just log the redirection."""
         if status is None:
             status = 302
-        print('%d: %s' % (status, location))
+        self.redirect_log.append((status, location))
+
+
+class MyNotificationResponse(
+        NotificationResponse, MockHTTPApplicationResponse):
+    pass
 
 
 def adaptNotificationRequestToResponse(request):
@@ -65,33 +89,251 @@ def adaptNotificationRequestToResponse(request):
         return response
 
 
-def setUp(test):
-    mock_session = MockSession()
-    provideAdapter(lambda x: mock_session, (INotificationRequest,), ISession)
-    provideAdapter(lambda x: mock_session, (INotificationResponse,), ISession)
-    provideAdapter(
-        adaptNotificationRequestToResponse,
-        (INotificationRequest,), INotificationResponse)
+class TestNotificationsBase:
+
+    def setUp(self):
+        super().setUp()
+
+        mock_session = MockSession()
+        self.useFixture(ZopeAdapterFixture(
+            lambda x: mock_session, (INotificationRequest,), ISession))
+        self.useFixture(ZopeAdapterFixture(
+            lambda x: mock_session, (INotificationResponse,), ISession))
+        self.useFixture(ZopeAdapterFixture(
+            adaptNotificationRequestToResponse,
+            (INotificationRequest,), INotificationResponse))
+
+        mock_browser_request = TestRequest()
+        self.useFixture(ZopeAdapterFixture(
+            lambda x: mock_browser_request, (INotificationRequest,),
+            IBrowserRequest))
+
+
+class TestNotificationRequest(TestNotificationsBase, TestCase):
+
+    layer = FunctionalLayer
+
+    def test_provides_interface(self):
+        request = NotificationRequest()
+        self.assertTrue(INotificationRequest.providedBy(request))
+
+    def test_no_notifications_by_default(self):
+        # By default, there are no notifications.
+        request = NotificationRequest()
+        self.assertEqual(0, len(request.notifications))
+
+    def test_single_notification(self):
+        request = NotificationRequest()
+        session = ISession(request)[SESSION_KEY]
+        notifications = NotificationList()
+        session["notifications"] = notifications
+        notifications.append(Notification(0, "Fnord"))
+        self.assertThat(
+            request.notifications,
+            MatchesListwise([MatchesStructure.byEquality(message="Fnord")]))
+
+    def test_multiple_notifications(self):
+        # NotificationRequest.notifications also returns any notifications
+        # that have been added so far in this request, making it the single
+        # source you need to interrogate to display notifications to the
+        # user.
+        request = NotificationRequest()
+        session = ISession(request)[SESSION_KEY]
+        notifications = NotificationList()
+        session["notifications"] = notifications
+        notifications.append(Notification(0, "Fnord"))
+        response = INotificationResponse(request)
+        response.addNotification("Aargh")
+        self.assertThat(
+            request.notifications,
+            MatchesListwise([
+                MatchesStructure.byEquality(message="Fnord"),
+                MatchesStructure.byEquality(message="Aargh"),
+                ]))
+
+
+class TestNotificationResponse(TestNotificationsBase, TestCase):
+
+    layer = FunctionalLayer
+
+    def test_provides_interface(self):
+        response = NotificationResponse()
+        self.assertTrue(INotificationResponse.providedBy(response))
+
+    def makeNotificationResponse(self):
+        # Return a `NotificationResponse` linked to a `NotificationRequest`.
+        response = MyNotificationResponse()
+        request = NotificationRequest()
+        request.response = response
+        response._request = request
+        # Full IRequests are zope.security participations, and
+        # NotificationResponse.redirect expects a principal, as in the full
+        # IRequest interface.
+        request.principal = None
+        return response
+
+    def test_no_notifications(self):
+        response = self.makeNotificationResponse()
+        self.assertEqual(0, len(response.notifications))
+
+    def test_addNotification(self):
+        response = self.makeNotificationResponse()
+        response.addNotification("something")
+        self.assertEqual(1, len(response.notifications))
+
+    def test_removeAllNotifications(self):
+        response = self.makeNotificationResponse()
+        response.addNotification("something")
+        response.removeAllNotifications()
+        self.assertEqual(0, len(response.notifications))
+
+    def test_many_notifications(self):
+        response = self.makeNotificationResponse()
+        msg = structured("<b>%(escaped)s</b>", escaped="<Fnord>")
+        response.addNotification(msg)
+        response.addNotification("Whatever", BrowserNotificationLevel.DEBUG)
+        response.addDebugNotification("Debug")
+        response.addInfoNotification("Info")
+        response.addWarningNotification("Warning")
+        # And an odd one to test
+        # https://bugs.launchpad.net/launchpad/+bug/54987
+        response.addErrorNotification(
+            _("Error${value}", mapping={"value": ""}))
+
+        self.assertTrue(INotificationList.providedBy(response.notifications))
+        self.assertThat(response.notifications, MatchesListwise([
+            MatchesStructure.byEquality(
+                level=20, message="<b>&lt;Fnord&gt;</b>"),
+            MatchesStructure.byEquality(level=10, message="Whatever"),
+            MatchesStructure.byEquality(level=10, message="Debug"),
+            MatchesStructure.byEquality(level=20, message="Info"),
+            MatchesStructure.byEquality(level=30, message="Warning"),
+            MatchesStructure.byEquality(level=40, message="Error"),
+            ]))
+
+    def test_no_notifications_session_untouched(self):
+        # If there are no notifications, the session is not touched.  This
+        # ensures that we don't needlessly burden the session storage.
+        response = self.makeNotificationResponse()
+        session = ISession(response._request)[SESSION_KEY]
+        self.assertNotIn("notifications", session)
+        self.assertEqual(0, len(response.notifications))
+        response.redirect("http://example.com")
+        self.assertEqual([(302, "http://example.com")], response.redirect_log)
+        self.assertNotIn("notifications", session)
+
+
+class TestNotificationResponseTextEscaping(TestNotificationsBase, TestCase):
+    """Test notification text escaping.
+
+    There are a number of user actions that may generate on-screen
+    notifications, such as moving a bug or deleting a branch.  Some of these
+    notifications display potentially unsafe text that is obtained from the
+    user.  In order to prevent a cross-site-scripting attack, HTML
+    characters in notifications must be escaped.  However, there are special
+    cases where notifications from known safe sources must be allowed to
+    pass HTML through.
+    """
+
+    layer = FunctionalLayer
+
+    def makeNotificationResponse(self):
+        # Return a `NotificationResponse` linked to a `NotificationRequest`.
+        response = MyNotificationResponse()
+        request = NotificationRequest()
+        request.response = response
+        response._request = request
+        return response
+
+    def test_addNotification_plain_text(self):
+        # Plain text is left unchanged.
+        response = self.makeNotificationResponse()
+        response.addNotification("clean")
+        self.assertThat(
+            response.notifications,
+            MatchesListwise([MatchesStructure.byEquality(message="clean")]))
+
+    def test_addNotification_html(self):
+        # HTML is escaped.
+        response = self.makeNotificationResponse()
+        response.addNotification("<br/>dirty")
+        self.assertThat(
+            response.notifications,
+            MatchesListwise([
+                MatchesStructure.byEquality(message="&lt;br/&gt;dirty"),
+                ]))
+
+    def test_addNotification_structured_string(self):
+        # If the object passed to `addNotification` publishes the
+        # `IStructuredString` interface, then a string is returned with the
+        # appropriate sections escaped and unescaped.
+        structured_text = structured("<b>%(escaped)s</b>", escaped="<br/>foo")
+        self.assertTrue(IStructuredString.providedBy(structured_text))
+        self.assertEqual("<b>&lt;br/&gt;foo</b>", structured_text.escapedtext)
+        response = self.makeNotificationResponse()
+        response.addNotification(structured_text)
+        self.assertThat(
+            response.notifications,
+            MatchesListwise([
+                MatchesStructure.byEquality(message="<b>&lt;br/&gt;foo</b>"),
+                ]))
+
+    def test_addNotification_i18n_message(self):
+        # An instance of `zope.i18n.Message` is escaped in the same manner
+        # as raw text.
+        response = self.makeNotificationResponse()
+        response.addNotification(Message("<br/>foo"))
+        self.assertThat(
+            response.notifications,
+            MatchesListwise([
+                MatchesStructure.byEquality(message="&lt;br/&gt;foo"),
+                ]))
+
+    def test_addNotification_structured_internationalized(self):
+        # To pass internationalized text that contains markup, one may call
+        # `structured` directly with an internationalized object.
+        # `structured` performs the translation and substitution, and the
+        # resulting object may then be passed to `addNotification`.
+        structured_text = structured(_("<good/>%(evil)s"), evil="<evil/>")
+        self.assertEqual("<good/>&lt;evil/&gt;", structured_text.escapedtext)
+        response = self.makeNotificationResponse()
+        response.addNotification(structured_text)
+        self.assertThat(
+            response.notifications,
+            MatchesListwise([
+                MatchesStructure.byEquality(message="<good/>&lt;evil/&gt;"),
+                ]))
 
-    mock_browser_request = TestRequest()
-    provideAdapter(
-        lambda x: mock_browser_request, (INotificationRequest,),
-        IBrowserRequest)
 
-    test.globs['MockResponse'] = MockHTTPApplicationResponse
-    test.globs['structured'] = structured
+class TestNotificationList(TestNotificationsBase, TestCase):
 
+    layer = FunctionalLayer
 
-def test_suite():
-    suite = unittest.TestSuite()
-    doctest_suite = DocTestSuite(
-        'lp.services.webapp.notifications',
-        setUp=setUp,
-        )
-    doctest_suite.layer = FunctionalLayer
-    suite.addTest(doctest_suite)
-    return suite
+    def test_empty(self):
+        notifications = NotificationList()
+        self.assertLessEqual(notifications.created, datetime.utcnow())
+        self.assertRaises(IndexError, notifications.__getitem__, 0)
 
+    def test_iterate(self):
+        notifications = NotificationList()
+        notifications.append(
+            Notification(BrowserNotificationLevel.ERROR, "An error"))
+        notifications.append(
+            Notification(BrowserNotificationLevel.DEBUG, "A debug message"))
+        self.assertThat(notifications, MatchesListwise([
+            MatchesStructure.byEquality(message="An error"),
+            MatchesStructure.byEquality(message="A debug message"),
+            ]))
 
-if __name__ == '__main__':
-    unittest.main(defaultTest='test_suite')
+    def test___getitem__(self):
+        # The __getitem__ method is also overloaded to allow TALES
+        # expressions to easily retrieve lists of notifications that match a
+        # particular notification level.
+        notifications = NotificationList()
+        notifications.append(
+            Notification(BrowserNotificationLevel.ERROR, "An error"))
+        notifications.append(
+            Notification(BrowserNotificationLevel.DEBUG, "A debug message"))
+        self.assertThat(notifications["debug"], MatchesListwise([
+            MatchesStructure.byEquality(message="A debug message"),
+            ]))