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

Proposed by Māris Fogels on 2010-12-08
Status: Merged
Approved by: Māris Fogels on 2010-12-15
Approved revision: 394
Merged at revision: 383
Proposed branch: lp:~mars/tarmac/fix-merge-error-updates-proposals
Merge into: lp:~launchpad/tarmac/lp-tarmac
Diff against target: 285 lines (+166/-19)
4 files modified
tarmac/bin/commands.py (+18/-5)
tarmac/branch.py (+14/-12)
tarmac/tests/test_branch.py (+35/-0)
tarmac/tests/test_commands.py (+99/-2)
To merge this branch: bzr merge lp:~mars/tarmac/fix-merge-error-updates-proposals
Reviewer Review Type Date Requested Status
Diogo Matsubara (community) 2010-12-08 Approve on 2010-12-10
Review via email: mp+43135@code.launchpad.net

Commit message

Fix the handling of TarmacMergeErrors raised by plugins.

Description of the change

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

To post a comment you must log in.
Diogo Matsubara (matsubara) wrote :

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!

review: Approve
Māris Fogels (mars) wrote :

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) wrote :

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

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'tarmac/bin/commands.py'
2--- tarmac/bin/commands.py 2010-11-29 13:51:42 +0000
3+++ tarmac/bin/commands.py 2010-12-15 16:38:15 +0000
4@@ -160,8 +160,9 @@
5 while True:
6 try:
7 (hook, source, proposal) = results.next()
8- self.logger.debug('Firing %s hook' % hook)
9- tarmac_hooks[hook].fire(self, target, source, proposal)
10+
11+ self._fire_merge_hook(
12+ results, hook, target, source, proposal)
13 except StopIteration:
14 break
15 # This except is here because we need the else and can't have it
16@@ -174,6 +175,16 @@
17 finally:
18 target.cleanup()
19
20+ def _fire_merge_hook(self, callback_gen, hook, target, source, proposal):
21+ '''Fire a tarmac hook and correctly handle any errors raised.'''
22+ self.logger.debug('Firing %s hook' % hook)
23+ try:
24+ tarmac_hooks[hook].fire(self, target, source, proposal)
25+ except TarmacMergeError, e:
26+ # Pass merge errors from plugins back to the target
27+ # branch for handling and cleanup.
28+ callback_gen.throw(e)
29+
30 def run(self, branch_url=None, debug=False, http_debug=False,
31 launchpad=None, imply_commit_message=False):
32 if debug:
33@@ -243,14 +254,16 @@
34 try:
35 (hook, source, proposal) = results.next()
36 collected_proposals.append((source, proposal))
37- self.logger.debug('Firing %s hook' % hook)
38- tarmac_hooks[hook].fire(self, target, source, proposal)
39+ self._fire_merge_hook(
40+ results, hook, target, source, proposal)
41 except StopIteration:
42 break
43 try:
44 tarmac_hooks['tarmac_bundle_merge'].fire(
45 self, target, collected_proposals)
46- except TarmacMergeError:
47+ except TarmacMergeError, failure:
48+ for source, proposal in collected_proposals:
49+ target.update_proposal_with_error(proposal, failure)
50 # Uncommit and revert changes up to the point where target
51 # was before the merge.
52 target.uncommit_and_revert(restore_revno)
53
54=== modified file 'tarmac/branch.py'
55--- tarmac/branch.py 2010-11-29 13:51:42 +0000
56+++ tarmac/branch.py 2010-12-15 16:38:15 +0000
57@@ -198,16 +198,7 @@
58 yield ("tarmac_pre_commit", source, proposal)
59
60 except TarmacMergeError, failure:
61- self.logger.warn(
62- u'Merging %(source)s into %(target)s failed: %(msg)s' %
63- {'source': proposal.source_branch.display_name,
64- 'target': proposal.target_branch.display_name,
65- 'msg': str(failure)})
66- if failure.comment:
67- comment = failure.comment
68- else:
69- comment = str(failure)
70- self.update_proposal(proposal, comment)
71+ self.update_proposal_with_error(proposal, failure)
72 self.cleanup()
73 continue
74 except PointlessMerge:
75@@ -246,6 +237,19 @@
76 proposal.setStatus(status=u'Needs review')
77 proposal.lp_save()
78
79+ def update_proposal_with_error(self, proposal, error_exception):
80+ """Update a merge proposal with a TarmacMergeError exception."""
81+ self.logger.warn(
82+ u'Merging %(source)s into %(target)s failed: %(msg)s' %
83+ {'source': proposal.source_branch.display_name,
84+ 'target': proposal.target_branch.display_name,
85+ 'msg': str(error_exception)})
86+ if error_exception.comment:
87+ comment = error_exception.comment
88+ else:
89+ comment = str(error_exception)
90+ self.update_proposal(proposal, comment)
91+
92 def _get_reviews(self, proposal):
93 """Get the set of reviews from the proposal."""
94 reviews = []
95@@ -396,5 +400,3 @@
96 self._proposals.append(entry)
97
98 return self._proposals
99-
100-
101
102=== modified file 'tarmac/tests/test_branch.py'
103--- tarmac/tests/test_branch.py 2010-11-11 19:36:16 +0000
104+++ tarmac/tests/test_branch.py 2010-12-15 16:38:15 +0000
105@@ -23,6 +23,7 @@
106 from bzrlib.errors import PointlessMerge
107
108 from tarmac import branch
109+from tarmac.exceptions import TarmacMergeError
110 from tarmac.tests import BranchTestCase
111 from tarmac.tests.mock import MockLPBranch, Thing
112
113@@ -209,3 +210,37 @@
114 self.assertEqual(
115 rev.properties.get('merge_url'),
116 'http://code.launchpad.net/proposal1')
117+
118+ def test_merge_error_updates_proposal(self):
119+ # Setup
120+ branch = self.branch1
121+ proposal = self.proposals[0]
122+
123+ # We need to have valid proposals for the to-be merged branch.
124+ proposal.reviewed_revid=self.branch2.bzr_branch.last_revision()
125+ branch._proposals = [proposal]
126+
127+ # Ensure the merge fails.
128+ short_message = 'short error message'
129+ long_message = 'long error message'
130+ def injects_merge_error(*args):
131+ raise TarmacMergeError(short_message, long_message)
132+ branch.merge = injects_merge_error
133+
134+ # Override the default setStatus behaviour, which is a NOP.
135+ def trace_set_status(status=None):
136+ proposal.queue_status = status
137+ proposal.setStatus = trace_set_status
138+ proposal.setStatus(u'Approved')
139+
140+ # Act
141+ try:
142+ for callback in branch.merge_branches():
143+ pass
144+ except TarmacMergeError:
145+ pass
146+
147+ # Verify
148+ self.assertTrue(hasattr(self, 'error'))
149+ self.assertEqual(self.error.comment, long_message)
150+ self.assertEqual(proposal.queue_status, u'Needs review')
151
152=== modified file 'tarmac/tests/test_commands.py'
153--- tarmac/tests/test_commands.py 2010-11-25 14:36:37 +0000
154+++ tarmac/tests/test_commands.py 2010-12-15 16:38:15 +0000
155@@ -8,7 +8,9 @@
156 from tarmac.bin.registry import CommandRegistry
157 from tarmac.branch import Branch
158 from tarmac.config import TarmacConfig
159-from tarmac.exceptions import UnapprovedChanges
160+from tarmac.exceptions import TarmacMergeError, UnapprovedChanges
161+from tarmac.hooks import TarmacHookPoint, tarmac_hooks
162+from tarmac.plugins import TarmacPlugin
163 from tarmac.tests import TarmacTestCase, BranchTestCase
164 from tarmac.tests.mock import MockLPBranch, Thing
165
166@@ -286,6 +288,39 @@
167 u'%s into lp:branch1, which is not '
168 u'Superseded.' % branch3.lp_branch.bzr_identity)
169
170+ def test_plugin_merge_errors_are_handled(self):
171+ # Setup
172+ self.proposals[1].reviewed_revid = \
173+ self.branch2.bzr_branch.last_revision()
174+
175+ # Create a plugin that throws an error
176+ class MergeErrorInjector(TarmacPlugin):
177+ has_run = False
178+ long_message = 'a long error message'
179+
180+ def run(self, *args):
181+ self.__class__.has_run = True
182+ raise TarmacMergeError('message', self.long_message)
183+
184+ plugin = MergeErrorInjector()
185+
186+ # Replace the original hook point with our custom hook and plugin
187+ original_hookpoint = tarmac_hooks.pop('tarmac_pre_commit')
188+ self.addCleanup(tarmac_hooks.__setitem__, 'tarmac_pre_commit',
189+ original_hookpoint)
190+
191+ new_hookpoint = TarmacHookPoint('tarmac_pre_commit', '', (0), False)
192+ new_hookpoint.hook(plugin, 'Throws a merge error')
193+ tarmac_hooks.create_hook(new_hookpoint)
194+
195+ # Act
196+ self.command.run(launchpad=self.launchpad)
197+
198+ # Verify
199+ self.assertTrue(MergeErrorInjector.has_run)
200+ self.assertIsInstance(self.error, TarmacMergeError)
201+ self.assertEqual(self.error.comment, MergeErrorInjector.long_message)
202+
203
204 class TestBundleMergeCommmand(BranchTestCase):
205
206@@ -324,12 +359,48 @@
207 self.branch3.lp_branch.revision_count += 1
208 self.branches[1].landing_candidates = self.proposals
209
210- def test_run(self):
211+ def approve_all_proposals(self):
212+ '''Approve all the merge proposals to be used in testing.'''
213 self.proposals[1].reviewed_revid = \
214 self.branch2.bzr_branch.last_revision()
215 self.proposals[2].reviewed_revid = \
216 self.branch3.bzr_branch.last_revision()
217+
218+ def install_merge_error_injecting_plugin(self, hook_name):
219+ '''Define and install a plugin that throws a TarmacMergeError.'''
220+ class MergeErrorInjector(TarmacPlugin):
221+ has_run = False
222+ long_message = 'a long error message'
223+
224+ def run(self, *args):
225+ self.__class__.has_run = True
226+ raise TarmacMergeError('message', self.long_message)
227+
228+ plugin = MergeErrorInjector()
229+ self.install_plugin(plugin, hook_name)
230+ return plugin
231+
232+ def install_plugin(self, plugin, hook_name):
233+ '''Install a plugin at the given hookpoint name.
234+
235+ Replaces all existing hooks at that point.
236+ '''
237+ original_hookpoint = tarmac_hooks.pop(hook_name)
238+ self.addCleanup(tarmac_hooks.__setitem__, hook_name,
239+ original_hookpoint)
240+
241+ new_hookpoint = TarmacHookPoint(hook_name, '', (0), False)
242+ new_hookpoint.hook(plugin, 'Custom hook function: {0}'.format(plugin))
243+ tarmac_hooks.create_hook(new_hookpoint)
244+
245+ def test_run(self):
246+ # Setup
247+ self.approve_all_proposals()
248+
249+ # Act
250 self.command.run(launchpad=self.launchpad)
251+
252+ # Verify
253 # Check the 2 approved proposals were merged into target.
254 target = self.branches[1]._internal_bzr_branch
255 last_rev = target.last_revision()
256@@ -343,3 +414,29 @@
257 u'http://code.launchpad.net/proposal2')
258 #XXX need to grab the previuous revision as well and do the
259 # assertions.
260+
261+ def test_plugin_merge_errors_are_handled(self):
262+ # Setup
263+ self.approve_all_proposals()
264+ plugin = self.install_merge_error_injecting_plugin('tarmac_pre_commit')
265+
266+ # Act
267+ self.command.run(launchpad=self.launchpad)
268+
269+ # Verify
270+ self.assertTrue(plugin.has_run)
271+ self.assertIsInstance(self.error, TarmacMergeError)
272+ self.assertEqual(self.error.comment, plugin.long_message)
273+
274+ def test_tarmac_bundle_merge_hookpoint_errors_are_handled(self):
275+ # Setup
276+ self.approve_all_proposals()
277+ plugin = self.install_merge_error_injecting_plugin('tarmac_bundle_merge')
278+
279+ # Act
280+ self.command.run(launchpad=self.launchpad)
281+
282+ # Verify
283+ self.assertTrue(plugin.has_run)
284+ self.assertIsInstance(self.error, TarmacMergeError)
285+ self.assertEqual(self.error.comment, plugin.long_message)

Subscribers

People subscribed via source and target branches

to all changes: