launchpad-reviewers team mailing list archive

[Jonathan Lange <jml@xxxxxxxxxxxxx>]

Jonathan Lange has proposed merging lp:~jml/launchpad/subject-and-attachment into lp:launchpad/devel.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)


This branch changes the subject and attachment name of the EC2 email.

For most email, the subject will become:

  Test Results: branch-name -> devel: SUCCESS
  Test Results: branch-name -> db-devel: FAILURE

For test runs against a trunk:

  Test Results: devel r12345: SUCCESS

The reasoning is that it's nice to be able to quickly distinguish between newly incoming mail. I thought about dropping the "SUCCESS" bit in order to preserve threading, but not sure it's worth it.

The attachment then becomes:

  branch-name.subunit.gz

In anticipation of better support for just clicking on the damn attachment.

-- 
https://code.launchpad.net/~jml/launchpad/subject-and-attachment/+merge/33544
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jml/launchpad/subject-and-attachment into lp:launchpad/devel.

=== modified file 'lib/devscripts/ec2test/remote.py'
--- lib/devscripts/ec2test/remote.py	2010-08-18 18:04:38 +0000
+++ lib/devscripts/ec2test/remote.py	2010-08-24 16:03:45 +0000
@@ -314,12 +314,12 @@
         conn = bzrlib.smtp_connection.SMTPConnection(self._bzr_config)
         conn.send_email(message)
 
-    def get_trunk_details(self):
+    def get_target_details(self):
         """Return (branch_url, revno) for trunk."""
         branch = bzrlib.branch.Branch.open(self._local_branch_path)
         return branch.get_parent().encode('utf-8'), branch.revno()
 
-    def get_branch_details(self):
+    def get_source_details(self):
         """Return (branch_url, revno) for the branch we're merging in.
 
         If we're not merging in a branch, but instead just testing a trunk,
@@ -337,12 +337,18 @@
 
     def get_nick(self):
         """Get the nick of the branch we are testing."""
-        details = self.get_branch_details()
+        details = self.get_source_details()
         if not details:
-            details = self.get_trunk_details()
+            details = self.get_target_details()
         url, revno = details
         return self._last_segment(url)
 
+    def get_revno(self):
+        """Get the revno of the branch we are testing."""
+        if self._revno is not None:
+            return self._revno
+        return bzrlib.branch.Branch.open(self._local_branch_path).revno()
+
     def get_merge_description(self):
         """Get a description of the merge request.
 
@@ -353,10 +359,10 @@
         # XXX: JonathanLange 2010-08-17: Not actually used yet. I think it
         # would be a great thing to have in the subject of the emails we
         # receive.
-        source = self.get_branch_details()
+        source = self.get_source_details()
         if not source:
-            return self.get_nick()
-        target = self.get_trunk_details()
+            return '%s r%s' % (self.get_nick(), self.get_revno())
+        target = self.get_target_details()
         return '%s => %s' % (
             self._last_segment(source[0]), self._last_segment(target[0]))
 
@@ -391,7 +397,8 @@
             status = 'SUCCESS'
         else:
             status = 'FAILURE'
-        subject = 'Test results: %s' % (status,)
+        subject = 'Test results: %s: %s' % (
+            self.get_merge_description(), status)
         message['Subject'] = subject
 
         # Make the body.
@@ -403,7 +410,8 @@
         zipped_log = MIMEApplication(full_log_gz, 'x-gzip')
         zipped_log.add_header(
             'Content-Disposition', 'attachment',
-            filename='%s.log.gz' % self.get_nick())
+            filename='%s-r%s.subunit.gz' % (
+                self.get_nick(), self.get_revno()))
         message.attach(zipped_log)
         return message
 
@@ -609,14 +617,14 @@
             ''')
 
         # Describe the trunk branch.
-        trunk, trunk_revno = self._request.get_trunk_details()
+        trunk, trunk_revno = self._request.get_target_details()
         msg = '%s, revision %d\n' % (trunk, trunk_revno)
         add_to_html('''\
             <p><strong>%s</strong></p>
             ''' % (escape(msg),))
         log(msg)
 
-        branch_details = self._request.get_branch_details()
+        branch_details = self._request.get_source_details()
         if not branch_details:
             add_to_html('<p>(no merged branch)</p>\n')
             log('(no merged branch)')

=== modified file 'lib/devscripts/ec2test/tests/test_remote.py'
--- lib/devscripts/ec2test/tests/test_remote.py	2010-08-18 17:36:12 +0000
+++ lib/devscripts/ec2test/tests/test_remote.py	2010-08-24 16:03:45 +0000
@@ -235,31 +235,49 @@
         req = self.make_request(emails=['foo@xxxxxxxxxxx'])
         self.assertEqual(True, req.wants_email)
 
-    def test_get_trunk_details(self):
+    def test_get_target_details(self):
         parent = 'http://example.com/bzr/branch'
         tree = self.make_trunk(parent)
         req = self.make_request(trunk=tree)
         self.assertEqual(
-            (parent, tree.branch.revno()), req.get_trunk_details())
-
-    def test_get_branch_details_no_commits(self):
+            (parent, tree.branch.revno()), req.get_target_details())
+
+    def test_get_revno_target_only(self):
+        # If there's only a target branch, then the revno is the revno of that
+        # branch.
+        parent = 'http://example.com/bzr/branch'
+        tree = self.make_trunk(parent)
+        req = self.make_request(trunk=tree)
+        self.assertEqual(tree.branch.revno(), req.get_revno())
+
+    def test_get_revno_source_and_target(self):
+        # If we're merging in a branch, then the revno is the revno of the
+        # branch we're merging in.
+        tree = self.make_trunk()
+        # Fake a merge, giving silly revision ids.
+        tree.add_pending_merge('foo', 'bar')
+        req = self.make_request(
+            branch_url='https://example.com/bzr/thing', revno=42, trunk=tree)
+        self.assertEqual(42, req.get_revno())
+
+    def test_get_source_details_no_commits(self):
         req = self.make_request(trunk=self.make_trunk())
-        self.assertEqual(None, req.get_branch_details())
+        self.assertEqual(None, req.get_source_details())
 
-    def test_get_branch_details_no_merge(self):
+    def test_get_source_details_no_merge(self):
         tree = self.make_trunk()
         tree.commit(message='foo')
         req = self.make_request(trunk=tree)
-        self.assertEqual(None, req.get_branch_details())
+        self.assertEqual(None, req.get_source_details())
 
-    def test_get_branch_details_merge(self):
+    def test_get_source_details_merge(self):
         tree = self.make_trunk()
         # Fake a merge, giving silly revision ids.
         tree.add_pending_merge('foo', 'bar')
         req = self.make_request(
             branch_url='https://example.com/bzr/thing', revno=42, trunk=tree)
         self.assertEqual(
-            ('https://example.com/bzr/thing', 42), req.get_branch_details())
+            ('https://example.com/bzr/thing', 42), req.get_source_details())
 
     def test_get_nick_trunk_only(self):
         tree = self.make_trunk(parent_url='http://example.com/bzr/db-devel')
@@ -277,7 +295,8 @@
     def test_get_merge_description_trunk_only(self):
         tree = self.make_trunk(parent_url='http://example.com/bzr/db-devel')
         req = self.make_request(trunk=tree)
-        self.assertEqual('db-devel', req.get_merge_description())
+        self.assertEqual(
+            'db-devel r%s' % req.get_revno(), req.get_merge_description())
 
     def test_get_merge_description_merge(self):
         tree = self.make_trunk(parent_url='http://example.com/bzr/db-devel/')
@@ -355,12 +374,16 @@
     def test_report_email_subject_success(self):
         req = self.make_request(emails=['foo@xxxxxxxxxxx'])
         email = req._build_report_email(True, 'foo', 'gobbledygook')
-        self.assertEqual('Test results: SUCCESS', email['Subject'])
+        self.assertEqual(
+            'Test results: %s: SUCCESS' % req.get_merge_description(),
+            email['Subject'])
 
     def test_report_email_subject_failure(self):
         req = self.make_request(emails=['foo@xxxxxxxxxxx'])
         email = req._build_report_email(False, 'foo', 'gobbledygook')
-        self.assertEqual('Test results: FAILURE', email['Subject'])
+        self.assertEqual(
+            'Test results: %s: FAILURE' % req.get_merge_description(),
+            email['Subject'])
 
     def test_report_email_recipients(self):
         req = self.make_request(emails=['foo@xxxxxxxxxxx', 'bar@xxxxxxxxxxx'])
@@ -388,7 +411,8 @@
         self.assertIsInstance(attachment, MIMEApplication)
         self.assertEqual('application/x-gzip', attachment['Content-Type'])
         self.assertEqual(
-            'attachment; filename="%s.log.gz"' % req.get_nick(),
+            'attachment; filename="%s-r%s.subunit.gz"' % (
+                req.get_nick(), req.get_revno()),
             attachment['Content-Disposition'])
         self.assertEqual(
             "gobbledygook", attachment.get_payload().decode('base64'))
@@ -623,8 +647,8 @@
         [body, attachment] = user_message.get_payload()
         self.assertEqual('application/x-gzip', attachment['Content-Type'])
         self.assertEqual(
-            'attachment; filename="%s.log.gz"' % request.get_nick(),
-            attachment['Content-Disposition'])
+            'attachment;',
+            attachment['Content-Disposition'][:len('attachment;')])
         attachment_contents = attachment.get_payload().decode('base64')
         uncompressed = gunzip_data(attachment_contents)
         self.assertEqual(