launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/soyuz-test-mail-ordering into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #904220 in Launchpad itself: "Soyuz tests with lists of e-mail addresses are non-deterministic"
  https://bugs.launchpad.net/launchpad/+bug/904220

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/soyuz-test-mail-ordering/+merge/85979

= Summary =

Fix a couple of test failures on i386 due to relying on the iteration order of sets.

== Proposed fix ==

Introduce an lp.testing.mail_helpers.sort_addresses helper and use that in the doctests.

print_emails does some similar sorting already, but I wanted something to just sort the addresses and nothing else, and it couldn't be easily extracted.  At three lines, it wasn't worth agonising over.  I couldn't find any existing helper that did quite the right thing; feel free to correct me if I missed something.

== Pre-implementation notes =

Curtis pointed out in the bug that it was more usual to do the sorting in the tests rather than in the tested code.  I'm more comfortable with that anyway.

== Implementation details ==

None.

== Tests ==

bin/test -vvct 'soyuz-set-of-uploads|xx-queue-pages'

== Demo and Q/A ==

None.

== lint ==

Loads of lint in lib/lp/soyuz/stories/soyuz/xx-queue-pages.txt that looks like lint not really understanding doctests.  I fixed a bit of pre-existing lint in lib/lp/testing/mail_helpers.py.
-- 
https://code.launchpad.net/~cjwatson/launchpad/soyuz-test-mail-ordering/+merge/85979
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/soyuz-test-mail-ordering into lp:launchpad.

=== modified file 'lib/lp/soyuz/doc/soyuz-set-of-uploads.txt'
--- lib/lp/soyuz/doc/soyuz-set-of-uploads.txt	2011-12-09 01:21:32 +0000
+++ lib/lp/soyuz/doc/soyuz-set-of-uploads.txt	2011-12-16 01:46:35 +0000
@@ -265,7 +265,10 @@
     ...     if loglevel is None or loglevel <= logging.INFO:
     ...         print "Upload complete."
 
-    >>> from lp.testing.mail_helpers import pop_notifications
+    >>> from lp.testing.mail_helpers import (
+    ...     pop_notifications,
+    ...     sort_addresses,
+    ...     )
     >>> def read_email():
     ...     """Pop all emails from the test mailbox, and summarize them.
     ...
@@ -274,7 +277,7 @@
     ...     line.
     ...     """
     ...     for message in pop_notifications(commit=False):
-    ...         print "To:", message['to']
+    ...         print "To:", sort_addresses(message['to'])
     ...         print "Subject:", message['subject']
     ...         print message.get_payload()[0].as_string()
     ...         print ''
@@ -305,8 +308,8 @@
 
     >>> read_email()
     To:
-	Foo Bar <foo.bar@xxxxxxxxxxxxx>,
-	Daniel Silverstone <daniel.silverstone@xxxxxxxxxxxxx>
+	Daniel Silverstone <daniel.silverstone@xxxxxxxxxxxxx>,
+	Foo Bar <foo.bar@xxxxxxxxxxxxx>
     Subject: bar_1.0-3_source.changes rejected
     ...
 

=== modified file 'lib/lp/soyuz/stories/soyuz/xx-queue-pages.txt'
--- lib/lp/soyuz/stories/soyuz/xx-queue-pages.txt	2011-12-08 19:37:55 +0000
+++ lib/lp/soyuz/stories/soyuz/xx-queue-pages.txt	2011-12-16 01:46:35 +0000
@@ -302,7 +302,10 @@
 Swallow any email generated at the upload:
 
   >>> from lp.services.mail import stub
-  >>> from lp.testing.mail_helpers import pop_notifications
+  >>> from lp.testing.mail_helpers import (
+  ...     pop_notifications,
+  ...     sort_addresses,
+  ...     )
   >>> swallow = pop_notifications()
 
 Set up a second browser on the same page to simulate accidentally posting to the
@@ -345,9 +348,9 @@
 distroseries' announcement list (see nascentupload-announcements.txt).
 
   >>> [notification, announcement] = pop_notifications()
-  >>> print notification['To']
-  Foo Bar <foo.bar@xxxxxxxxxxxxx>,
-  Daniel Silverstone <daniel.silverstone@xxxxxxxxxxxxx>
+  >>> print sort_addresses(notification['To'])
+  Daniel Silverstone <daniel.silverstone@xxxxxxxxxxxxx>,
+  Foo Bar <foo.bar@xxxxxxxxxxxxx>
   >>> print announcement['To']
   autotest_changes@xxxxxxxxxx
 

=== modified file 'lib/lp/testing/mail_helpers.py'
--- lib/lp/testing/mail_helpers.py	2010-12-20 22:16:13 +0000
+++ lib/lp/testing/mail_helpers.py	2011-12-16 01:46:35 +0000
@@ -48,6 +48,12 @@
     return sorted(notifications, key=sort_key)
 
 
+def sort_addresses(header):
+    """Sort an address-list in an e-mail header field body."""
+    addresses = set(address.strip() for address in header.split(','))
+    return ", ".join(sorted(addresses))
+
+
 def print_emails(include_reply_to=False, group_similar=False,
                  include_rationale=False, notifications=None):
     """Pop all messages from stub.test_emails and print them with
@@ -77,7 +83,7 @@
         body = message.get_payload()
         if group_similar:
             # Strip the first line as it's different for each recipient.
-            body = body[body.find('\n')+1:]
+            body = body[body.find('\n') + 1:]
         if body in distinct_bodies and group_similar:
             message, existing_recipients = distinct_bodies[body]
             distinct_bodies[body] = (
@@ -96,7 +102,7 @@
                 '%s: %s' % (rationale_header, message[rationale_header]))
         print 'Subject:', message['Subject']
         print body
-        print "-"*40
+        print "-" * 40
 
 
 def print_distinct_emails(include_reply_to=False, include_rationale=True):