Merge lp:~matsubara/tarmac/bundle-lander-fixes into lp:~launchpad/tarmac/lp-tarmac

[Diogo Matsubara]

Fixes issue with double notification in merge proposals and two notifications for each event. Add XXX pointing to bug 691563 as this is a temporary solution.

This branch fix the issue shown in https://code.launchpad.net/~matsubara/launchpad/test-ghost-update/+merge/43919 where there are duplicated comments and two notifications for the same event.

Due to the way the merge_branches() generator works, it yields twice, once for each pre and post commit hooks, so this branch fixes the duplicated comments by using a set() to collect the proposals the bundle lander will work on.

To fix the two notification for the same event problem, I moved an update_proposal_with_error() call in bin/commands.py to plugins/bundlecommand.py, so now it's the plugins responsibility t, in case o failure, update the merge proposal and uncommit and revert changes done. Since the plugin doesn't raise an error anymore in case of failure, I removed the test that was testing that and added more tests to test_bundlecommand.py to cover for its new responsibility.

[Gary Poster (gary)]

Cool, thank you.

In the abstract, I feel like we ought to keep the try: except handling in commands.py (cmd_bundle_merge). If a tarmac_bundle_merge hook raises an error, we should still inform the merge proposal about it. Our hook no longer raises the usual errors--but if it raises an unexpected error, isn't that even more important to report? And keeping the logic that reverts the branch in commands.py better matches the basic code structure. Moreover, I think we could change the exception we raise in the hook to have all the collected information that we want in the email, so that we could follow the usual tarmac pattern, letting cmd_bundle_merge handle the reversion, and still get the emails we want.

I'd like that change, but given your schedule and our schedule, that change requires time we don't have. We can put it in the category of "things to revisit when proposing a merge to trunk" or maybe "things for Gary or Benji or Ursula to look at next week." Maybe just an XXX with a quick explanation of the issue? A bug would be extra credit. :-)

Quick tweaks:

=== modified file 'tarmac/bin/commands.py'
    # proposal) tuple twice, once for each pre and pos commit hook.
pos -> post

=== modified file 'tarmac/plugins/bundlecommand.py'
    # Uncommit and revert changes up to the point where target
    # was before the merge.
Per my comments above, please add an additional comment explaining that we normally expect the tarmac command to do this, We are doing it here because of the emails, but we hope to change it in the future.

tarmac/tests/test_commands.py : maybe comment out the test, rather than removing it, per my comments above?

I'll approve this, so you can commit with just these small changes, but if you disagree with my comments about the larger future changes, please raise your concerns, here or on another channel (in which case you or I can summarize here afterwards).

Thank you

review: Approve

[Diogo Matsubara (matsubara)]

Hi Gary,

I filed bug 691563 for the XXX and added comments in the code. I fixed the typo as well and commented out the test that I had previously removed.

To answer your concerns and add some more of my own:

If tarmac_bundle_merge hook raises an unexpected error, the try/except block removed wouldn't be able do anything about it. It would be caught by the upper try/except and re-raised. Basically tarmac would crash and burn in that situation. I agree that keeping the logic to revert the branch in commands.py feels more correct than in the hook.

There's one thing not clear about your proposal though. If we change the exception raised in the hook to collect the information, put the try/except and revert code back in commands.py, who would be responsible for sending the email? Would that still be done in the hook code, but the proposal updates done outside of it? Is that the right understanding?

Thanks for the review.

[Diogo Matsubara (matsubara)]

There are additional revisions which have not been approved in review. Please seek review and approval of these new revisions.

=== modified file 'tarmac/bin/commands.py'
--- tarmac/bin/commands.py	2010-12-15 16:52:26 +0000
+++ tarmac/bin/commands.py	2010-12-17 14:22:15 +0000
@@ -269,24 +269,20 @@
             # revno to restore to if bundle merge fails.
             restore_revno = target.tree.branch.revno() + 1
             results = target.merge_branches()
-            collected_proposals = []
+            # Use a set to collect the proposals. We don't want
+            # duplicates since results generator will return the (source,
+            # proposal) tuple twice, once for each pre and post commit hook.
+            collected_proposals = set()
             while True:
                 try:
                     (hook, source, proposal) = results.next()
-                    collected_proposals.append((source, proposal))
+                    collected_proposals.add((source, proposal))
                     self._fire_merge_hook(
                         results, hook, target, source, proposal)
                 except StopIteration:
                     break
-            try:
-                tarmac_hooks['tarmac_bundle_merge'].fire(
-                    self, target, collected_proposals)
-            except TarmacMergeError, failure:
-                for source, proposal in collected_proposals:
-                    target.update_proposal_with_error(proposal, failure)
-                # Uncommit and revert changes up to the point where target
-                # was before the merge.
-                target.uncommit_and_revert(restore_revno)
+            tarmac_hooks['tarmac_bundle_merge'].fire(
+                self, target, restore_revno, collected_proposals)
 
         # This except is here because we need the else and can't have it
         # without an except as well.

=== modified file 'tarmac/plugins/bundlecommand.py'
--- tarmac/plugins/bundlecommand.py	2010-12-16 03:58:15 +0000
+++ tarmac/plugins/bundlecommand.py	2010-12-17 14:22:15 +0000
@@ -128,7 +128,7 @@
             smtp_connection = SMTPConnection(bzrlib.config.GlobalConfig())
         self._smtp_connection = smtp_connection
 
-    def run(self, command, target, collected_proposals):
+    def run(self, command, target, restore_revno, collected_proposals):
         """Plugin for running the registered bundle verify command on target.
 
         This is very similar to the Command plugin, but instead of running the
@@ -169,19 +169,21 @@
             if return_code != 0:
                 for source, proposal in collected_proposals:
                     self.do_failed(
-                        start_time, end_time, proposal, output)
-                message = u'Test command "%s" failed.' % self.verify_command
-                comment = (
-                    u'Bundle lander failed to land proposals: %s' %
-                    collected_proposals)
-                raise VerifyCommandFailed(message, comment)
+                        target, start_time, end_time, proposal, output)
+                # XXX matsubara 2010-12-17: The command code should be
+                # handling this, but due to the way we want emails formatted
+                # and sent, this is done here for now.
+                # See bug 691563 for more details.
+                # Uncommit and revert changes up to the point where target
+                # was before the merge.
+                target.uncommit_and_revert(restore_revno)
             else:
                 self.logger.info('==========')
                 self.logger.info(output.read())
         finally:
             output.close()
 
-    def do_failed(self, start_time, end_time, proposal, output):
+    def do_failed(self, target, start_time, end_time, proposal, output):
         '''Perform failure tests.
 
         In this case, the full failure is emailed to the proposal subscribers,
@@ -235,14 +237,7 @@
              'target': proposal.target_branch.display_name,
              'msg': str(message)})
 
-        subject = u'Re: [Merge] %(source)s into %(target)s' % {
-            "source": proposal.source_branch.display_name,
-            "target": proposal.target_branch.display_name}
-
-        proposal.createComment(subject=subject,
-                               content=comment)
-        proposal.setStatus(status=u'Needs review')
-        proposal.lp_save()
+        target.update_proposal(proposal, comment)
 
     def get_subscribers_emails(self, proposal):
         """Return a list of the proposal subscribers emails."""

=== modified file 'tarmac/plugins/tests/test_bundlecommand.py'
--- tarmac/plugins/tests/test_bundlecommand.py	2010-12-16 03:58:15 +0000
+++ tarmac/plugins/tests/test_bundlecommand.py	2010-12-17 14:22:15 +0000
@@ -11,7 +11,7 @@
 from tarmac.bin.registry import CommandRegistry
 from tarmac.plugins.bundlecommand import (
     BundleCommand, TestResultFormatter, VerifyCommandFailed)
-from tarmac.tests import TarmacTestCase
+from tarmac.tests import BranchTestCase
 from tarmac.tests.test_commands import FakeCommand
 from tarmac.tests.mock import Thing, MockMergeProposal
 
@@ -55,10 +55,6 @@
 
         return TestResultFormatter(start_time, end_time, result, proposal)
 
-    def lp_save(self, *args, **kwargs):
-        """Dummy lp_save method."""
-        pass
-
 
 class TestTestResultFormatter(TestCase, Helpers):
     """Tests for `TestResultFormatter`."""
@@ -176,14 +172,15 @@
             doctest.ELLIPSIS))
 
 
-class BundleCommandTests(TarmacTestCase, Helpers):
+class BundleCommandTests(BranchTestCase, Helpers):
     """Test the BundleCommand."""
 
     def setUp(self):
         """Set up additional data we need for tests."""
         super(BundleCommandTests, self).setUp()
-        self.collected_proposals = [
-            (Thing(),
+        # Use a set here because that's the object passed to BundleCommand.
+        self.collected_proposals = set(
+            [(Thing(),
             MockMergeProposal(
                 Thing(
                     display_name='lp:project/source',
@@ -214,8 +211,8 @@
                 votes=[
                     Thing(reviewer=Thing(
                         preferred_email_address=Thing(
-                        email='[e-mail]')))])),
-            ]
+                        email='[e-mail]')))]))]
+            )
         # Patch the plugin smtp_connection so emails are never actually sent.
         email_log = []
         smtp_connection = LoggingSMTPConnection(email_log)
@@ -229,7 +226,7 @@
             bundle_verify_command="/bin/true"),
             tree=Thing(abspath=os.path.abspath))
         self.plugin.run(
-            command=self.command, target=target,
+            command=self.command, target=target, restore_revno=123,
             collected_proposals=self.collected_proposals)
 
     def test_run_failure(self):
@@ -240,16 +237,28 @@
         proposal1, proposal2 = self.collected_proposals
         self.assertEqual(proposal1[1].queue_status, "Approved")
         self.assertEqual(proposal2[1].queue_status, "Approved")
-
-        target = Thing(config=Thing(
-            bundle_verify_command="/bin/false"),
-            tree=Thing(abspath=os.path.abspath))
-        self.assertRaises(VerifyCommandFailed,
-            self.plugin.run,
-            command=self.command, target=target,
+        # Proposals have no comments.
+        self.assertEqual(proposal1[1].all_comments, [])
+        self.assertEqual(proposal2[1].all_comments, [])
+
+        target = self.branch1
+        # Include the config to run the bundle verify command.
+        setattr(target.config, 'bundle_verify_command', '/bin/false')
+        # Set a fake method to target, so we can make sure it's called when
+        # the plugin fails to run.
+        def fake_uncommit_and_revert(restore_revno):
+            setattr(target, 'fake_revno', restore_revno)
+        target.uncommit_and_revert = fake_uncommit_and_revert
+
+        # Run the plugin.
+        self.plugin.run(
+            command=self.command, target=target, restore_revno=123,
             collected_proposals=self.collected_proposals)
 
         # There are two emails in the log. One for each proposal.
+        self.plugin._smtp_connection._log.sort(
+            key=lambda email: email.get('Subject'))
+        self.assertTrue(len(self.plugin._smtp_connection._log) == 2)
         email = self.plugin._smtp_connection._log.pop()
         self.assertEqual(
             email.get('Subject'),
@@ -259,19 +268,30 @@
             email.get('Subject'),
             'Test results: lp:project/source => lp:project: FAILURE')
 
-        # Proposals status is updated.
+        # Proposals statuses are updated.
         proposal1, proposal2 = self.collected_proposals
         self.assertEqual(proposal1[1].queue_status, "Needs review")
         self.assertEqual(proposal2[1].queue_status, "Needs review")
+        # Proposals have comments.
+        self.assertEqual(len(proposal1[1].all_comments), 1)
+        self.assertEqual(len(proposal2[1].all_comments), 1)
+
+        # Check the patched target.uncommit_and_revert() was run.
+        self.assertEqual(target.fake_revno, 123)
 
     def test_send_report_email(self):
         """Test send_report_email()."""
         formatter = self.make_formatter()
 
-        emails = self.plugin.get_subscribers_emails(
-            self.collected_proposals[0][1])
+        proposals = [
+            proposal for source, proposal in self.collected_proposals]
+        proposals.sort(
+            key=lambda proposal: proposal.source_branch.display_name)
+        emails = self.plugin.get_subscribers_emails(proposals[0])
+
         self.plugin.send_report_email(
             emails, formatter, 'some gzip content')
+        self.assertTrue(len(self.plugin._smtp_connection._log) == 1)
         email = self.plugin._smtp_connection._log.pop()
         self.assertEqual(
             email.get('To'), '[e-mail], [e-mail]')

=== modified file 'tarmac/tests/mock.py'
--- tarmac/tests/mock.py	2010-12-16 03:58:15 +0000
+++ tarmac/tests/mock.py	2010-12-17 14:22:15 +0000
@@ -62,7 +62,7 @@
     """A mock LP merge proposal."""
 
     def __init__(self, source_branch, target_branch, status, votes=()):
-        self.createComment = self.lp_save
+        self.all_comments = []
         self.queue_status = status
         self.source_branch = source_branch
         self.target_branch = target_branch
@@ -74,6 +74,9 @@
     def setStatus(self, status="Needs review"):
         self.queue_status = status
 
+    def createComment(self, subject=None, content=None):
+        self.all_comments.append((subject,content))
+
 
 class cmd_mock(TarmacCommand):
     '''A mock command.'''

=== modified file 'tarmac/tests/test_commands.py'
--- tarmac/tests/test_commands.py	2010-12-16 03:58:15 +0000
+++ tarmac/tests/test_commands.py	2010-12-17 14:22:15 +0000
@@ -444,15 +444,20 @@
         self.assertIsInstance(self.error, TarmacMergeError)
         self.assertEqual(self.error.comment, plugin.long_message)
 
-    def test_tarmac_bundle_merge_hookpoint_errors_are_handled(self):
-        # Setup
-        self.approve_all_proposals()
-        plugin = self.install_merge_error_injecting_plugin('tarmac_bundle_merge')
-
-        # Act
-        self.command.run(launchpad=self.launchpad)
-
-        # Verify
-        self.assertTrue(plugin.has_run)
-        self.assertIsInstance(self.error, TarmacMergeError)
-        self.assertEqual(self.error.comment, plugin.long_message)
+# XXX matsubara 2010-12-17: Test commented out because we're not handling
+# errors in the command itself but in the plugin. This is a temporary hack
+# and we want to change the way the bundle merge plugin and the command
+# interacts in the near future. See bug 691563 for more details.
+#    def test_tarmac_bundle_merge_hookpoint_errors_are_handled(self):
+#        # Setup
+#        self.approve_all_proposals()
+#        plugin = self.install_merge_error_injecting_plugin('tarmac_bundle_merge')
+#
+#        # Act
+#        self.command.run(launchpad=self.launchpad)
+#
+#        # Verify
+#        self.assertTrue(plugin.has_run)
+#        self.assertIsInstance(self.error, TarmacMergeError)
+#        self.assertEqual(self.error.comment, plugin.long_message)
+#