Merge lp:~mars/tarmac/fix-merge-error-updates-proposals into lp:~launchpad/tarmac/lp-tarmac

[Māris Fogels]

Fix the handling of TarmacMergeErrors raised by plugins.

Hi,

This branch fixes the handling of TarmacMergeErrors raised by plugins. I fixed this by either updating all affected merge proposals manually, or by passing the proposal update responsibilites back to the Branch object. Tests are included for the hooks that should handle TarmacMergeErrors. I also did a bit of refactoring to support cleaner interaction between the Command and Branch objects.

All tests pass.

Maris

[Diogo Matsubara (matsubara)]

Hi Mars,

I have a small comment about some commented out code on lines 39 and 40. It looks like you commented out the code but the intention was to remove it.

Another thing is that you define the MergeErrorInjector twice in test code. Is there a reason for that?

Other than that, it looks good. Thanks!

[Diogo Matsubara (matsubara)]

review: Approve

[Māris Fogels (mars)]

Hi Diogo,

Thanks, you are right, 39 and 40 are stale code. Also, I defined the MergeInjector twice because it uses class-level variables (yuck) to track if the method was called. Not the cleanest, but it works for now.

Thank you for the review!

Maris

[Māris Fogels (mars)]

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-11-29 13:51:42 +0000
+++ tarmac/bin/commands.py	2010-12-15 16:38:15 +0000
@@ -160,8 +160,9 @@
             while True:
                 try:
                     (hook, source, proposal) = results.next()
-                    self.logger.debug('Firing %s hook' % hook)
-                    tarmac_hooks[hook].fire(self, target, source, proposal)
+
+                    self._fire_merge_hook(
+                        results, hook, target, source, proposal)
                 except StopIteration:
                     break
         # This except is here because we need the else and can't have it
@@ -174,6 +175,16 @@
         finally:
             target.cleanup()
 
+    def _fire_merge_hook(self, callback_gen, hook, target, source, proposal):
+        '''Fire a tarmac hook and correctly handle any errors raised.'''
+        self.logger.debug('Firing %s hook' % hook)
+        try:
+            tarmac_hooks[hook].fire(self, target, source, proposal)
+        except TarmacMergeError, e:
+            # Pass merge errors from plugins back to the target
+            # branch for handling and cleanup.
+            callback_gen.throw(e)
+
     def run(self, branch_url=None, debug=False, http_debug=False,
             launchpad=None, imply_commit_message=False):
         if debug:
@@ -243,14 +254,16 @@
                 try:
                     (hook, source, proposal) = results.next()
                     collected_proposals.append((source, proposal))
-                    self.logger.debug('Firing %s hook' % hook)
-                    tarmac_hooks[hook].fire(self, target, 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:
+            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)

=== modified file 'tarmac/branch.py'
--- tarmac/branch.py	2010-11-29 13:51:42 +0000
+++ tarmac/branch.py	2010-12-15 16:38:15 +0000
@@ -198,16 +198,7 @@
                 yield ("tarmac_pre_commit", source, proposal)
 
             except TarmacMergeError, failure:
-                self.logger.warn(
-                    u'Merging %(source)s into %(target)s failed: %(msg)s' %
-                    {'source': proposal.source_branch.display_name,
-                     'target': proposal.target_branch.display_name,
-                     'msg': str(failure)})
-                if failure.comment:
-                    comment = failure.comment
-                else:
-                    comment = str(failure)
-                self.update_proposal(proposal, comment)
+                self.update_proposal_with_error(proposal, failure)
                 self.cleanup()
                 continue
             except PointlessMerge:
@@ -246,6 +237,19 @@
         proposal.setStatus(status=u'Needs review')
         proposal.lp_save()
 
+    def update_proposal_with_error(self, proposal, error_exception):
+        """Update a merge proposal with a TarmacMergeError exception."""
+        self.logger.warn(
+            u'Merging %(source)s into %(target)s failed: %(msg)s' %
+            {'source': proposal.source_branch.display_name,
+             'target': proposal.target_branch.display_name,
+             'msg': str(error_exception)})
+        if error_exception.comment:
+            comment = error_exception.comment
+        else:
+            comment = str(error_exception)
+        self.update_proposal(proposal, comment)
+
     def _get_reviews(self, proposal):
         """Get the set of reviews from the proposal."""
         reviews = []
@@ -396,5 +400,3 @@
             self._proposals.append(entry)
 
         return self._proposals
-
-

=== modified file 'tarmac/tests/test_branch.py'
--- tarmac/tests/test_branch.py	2010-11-11 19:36:16 +0000
+++ tarmac/tests/test_branch.py	2010-12-15 16:38:15 +0000
@@ -23,6 +23,7 @@
 from bzrlib.errors import PointlessMerge
 
 from tarmac import branch
+from tarmac.exceptions import TarmacMergeError
 from tarmac.tests import BranchTestCase
 from tarmac.tests.mock import MockLPBranch, Thing
 
@@ -209,3 +210,37 @@
         self.assertEqual(
             rev.properties.get('merge_url'),
             'http://code.launchpad.net/proposal1')
+
+    def test_merge_error_updates_proposal(self):
+        # Setup
+        branch = self.branch1
+        proposal = self.proposals[0]
+
+        # We need to have valid proposals for the to-be merged branch.
+        proposal.reviewed_revid=self.branch2.bzr_branch.last_revision()
+        branch._proposals = [proposal]
+
+        # Ensure the merge fails.
+        short_message = 'short error message'
+        long_message = 'long error message'
+        def injects_merge_error(*args):
+            raise TarmacMergeError(short_message, long_message)
+        branch.merge = injects_merge_error
+
+        # Override the default setStatus behaviour, which is a NOP.
+        def trace_set_status(status=None):
+            proposal.queue_status = status
+        proposal.setStatus = trace_set_status
+        proposal.setStatus(u'Approved')
+
+        # Act
+        try:
+            for callback in branch.merge_branches():
+                pass
+        except TarmacMergeError:
+            pass
+
+        # Verify
+        self.assertTrue(hasattr(self, 'error'))
+        self.assertEqual(self.error.comment, long_message)
+        self.assertEqual(proposal.queue_status, u'Needs review')

=== modified file 'tarmac/tests/test_commands.py'
--- tarmac/tests/test_commands.py	2010-11-25 14:36:37 +0000
+++ tarmac/tests/test_commands.py	2010-12-15 16:38:15 +0000
@@ -8,7 +8,9 @@
 from tarmac.bin.registry import CommandRegistry
 from tarmac.branch import Branch
 from tarmac.config import TarmacConfig
-from tarmac.exceptions import UnapprovedChanges
+from tarmac.exceptions import TarmacMergeError, UnapprovedChanges
+from tarmac.hooks import TarmacHookPoint, tarmac_hooks
+from tarmac.plugins import TarmacPlugin
 from tarmac.tests import TarmacTestCase, BranchTestCase
 from tarmac.tests.mock import MockLPBranch, Thing
 
@@ -286,6 +288,39 @@
                          u'%s into lp:branch1, which is not '
                          u'Superseded.' % branch3.lp_branch.bzr_identity)
 
+    def test_plugin_merge_errors_are_handled(self):
+        # Setup
+        self.proposals[1].reviewed_revid = \
+            self.branch2.bzr_branch.last_revision()
+
+        # Create a plugin that throws an error
+        class MergeErrorInjector(TarmacPlugin):
+            has_run = False
+            long_message = 'a long error message'
+
+            def run(self, *args):
+                self.__class__.has_run = True
+                raise TarmacMergeError('message', self.long_message)
+
+        plugin = MergeErrorInjector()
+
+        # Replace the original hook point with our custom hook and plugin
+        original_hookpoint = tarmac_hooks.pop('tarmac_pre_commit')
+        self.addCleanup(tarmac_hooks.__setitem__, 'tarmac_pre_commit',
+                        original_hookpoint)
+
+        new_hookpoint = TarmacHookPoint('tarmac_pre_commit', '', (0), False)
+        new_hookpoint.hook(plugin, 'Throws a merge error')
+        tarmac_hooks.create_hook(new_hookpoint)
+
+        # Act
+        self.command.run(launchpad=self.launchpad)
+
+        # Verify
+        self.assertTrue(MergeErrorInjector.has_run)
+        self.assertIsInstance(self.error, TarmacMergeError)
+        self.assertEqual(self.error.comment, MergeErrorInjector.long_message)
+
 
 class TestBundleMergeCommmand(BranchTestCase):
 
@@ -324,12 +359,48 @@
         self.branch3.lp_branch.revision_count += 1
         self.branches[1].landing_candidates = self.proposals
 
-    def test_run(self):
+    def approve_all_proposals(self):
+        '''Approve all the merge proposals to be used in testing.'''
         self.proposals[1].reviewed_revid = \
             self.branch2.bzr_branch.last_revision()
         self.proposals[2].reviewed_revid = \
             self.branch3.bzr_branch.last_revision()
+
+    def install_merge_error_injecting_plugin(self, hook_name):
+        '''Define and install a plugin that throws a TarmacMergeError.'''
+        class MergeErrorInjector(TarmacPlugin):
+            has_run = False
+            long_message = 'a long error message'
+
+            def run(self, *args):
+                self.__class__.has_run = True
+                raise TarmacMergeError('message', self.long_message)
+
+        plugin = MergeErrorInjector()
+        self.install_plugin(plugin, hook_name)
+        return plugin
+
+    def install_plugin(self, plugin, hook_name):
+        '''Install a plugin at the given hookpoint name.
+
+        Replaces all existing hooks at that point.
+        '''
+        original_hookpoint = tarmac_hooks.pop(hook_name)
+        self.addCleanup(tarmac_hooks.__setitem__, hook_name,
+                        original_hookpoint)
+
+        new_hookpoint = TarmacHookPoint(hook_name, '', (0), False)
+        new_hookpoint.hook(plugin, 'Custom hook function: {0}'.format(plugin))
+        tarmac_hooks.create_hook(new_hookpoint)
+
+    def test_run(self):
+        # Setup
+        self.approve_all_proposals()
+
+        # Act
         self.command.run(launchpad=self.launchpad)
+
+        # Verify
         # Check the 2 approved proposals were merged into target.
         target = self.branches[1]._internal_bzr_branch
         last_rev = target.last_revision()
@@ -343,3 +414,29 @@
             u'http://code.launchpad.net/proposal2')
         #XXX need to grab the previuous revision as well and do the
         # assertions.
+
+    def test_plugin_merge_errors_are_handled(self):
+        # Setup
+        self.approve_all_proposals()
+        plugin = self.install_merge_error_injecting_plugin('tarmac_pre_commit')
+
+        # 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)
+
+    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)