launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #28785
[Merge] ~cjwatson/launchpad:test-notification-unittest into launchpad:master
Colin Watson has proposed merging ~cjwatson/launchpad:test-notification-unittest into launchpad:master.
Commit message:
Rewrite notification doctests as unit tests
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/426667
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.
--
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:test-notification-unittest into launchpad:master.
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)
- <br/>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><br/>foo</b>
-
- >>> response = new_response()
- >>> response.addNotification(structured_text)
- >>> for notification in response.notifications:
- ... print(notification.message)
- <b><br/>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)
- <br/>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/><evil/>
-
- >>> response = new_response()
- >>> response.addNotification(text)
- >>> for notification in response.notifications:
- ... print(notification.message)
- <good/><evil/>
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><Fnord></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><Fnord></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><Fnord></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="<br/>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><br/>foo</b>", structured_text.escapedtext)
+ response = self.makeNotificationResponse()
+ response.addNotification(structured_text)
+ self.assertThat(
+ response.notifications,
+ MatchesListwise([
+ MatchesStructure.byEquality(message="<b><br/>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="<br/>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/><evil/>", structured_text.escapedtext)
+ response = self.makeNotificationResponse()
+ response.addNotification(structured_text)
+ self.assertThat(
+ response.notifications,
+ MatchesListwise([
+ MatchesStructure.byEquality(message="<good/><evil/>"),
+ ]))
- 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"),
+ ]))
