Bazaar PQM Plugin

Merge lp:~abentley/bzr-pqm/autoland-improvements into lp:bzr-pqm

  1. autoland-improvements
  2. Merge into devel
Proposed by Aaron Bentley
Status: Merged
Approved by: Jelmer Vernooij
Approved revision: 91
Merged at revision: 88
Proposed branch: lp:~abentley/bzr-pqm/autoland-improvements
Merge into: lp:bzr-pqm
Diff against target: 462 lines (+297/-19)
2 files modified
lpland.py (+24/-7)
tests/test_lpland.py (+273/-12)
To merge this branch: bzr merge lp:~abentley/bzr-pqm/autoland-improvements
Reviewer Review Type Date Requested Status
Jelmer Vernooij (community) Approve
Review via email: mp+95001@code.launchpad.net

Commit message

lp-land improvements from Launchpad's autoland module.

Description of the change

Port enhancements from Launchpad's autoland module to lp-land.

When using a branch to determine a merge proposal, only needs-review and approved proposals are considered.

If a proposal is approved, but has no approve votes, the person who marked it approved is considered to have approved it.

When determining bugs associated with the branch, bugs that are 'Fix committed' or 'Fix released' are ignored.

When determining stakeholders to mail about landings, stakeholders with no address (i.e. teams) are handled safely.

To post a comment you must log in.
Revision history for this message
Jelmer Vernooij (jelmer) :
review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lpland.py'
2--- lpland.py 2012-02-16 17:07:07 +0000
3+++ lpland.py 2012-02-28 17:10:39 +0000
4@@ -91,16 +91,19 @@
5 """Get the merge proposal from the branch."""
6
7 lp_branch = self.get_lp_branch(branch)
8- proposals = lp_branch.landing_targets
9+ proposals = [
10+ mp for mp in lp_branch.landing_targets
11+ if mp.queue_status in ('Needs review', 'Approved')]
12 if len(proposals) == 0:
13 raise BzrCommandError(
14- "The public branch has no source merge proposals. "
15+ "The public branch has no active source merge proposals. "
16 "You must have a merge proposal before attempting to "
17 "land the branch.")
18 elif len(proposals) > 1:
19 raise BzrCommandError(
20- "The public branch has multiple source merge proposals. "
21- "You must provide the URL to the one you wish to use.")
22+ "The public branch has multiple active source merge "
23+ "proposals. You must provide the URL to the one you wish to"
24+ " use.")
25 return MergeProposal(proposals[0])
26
27
28@@ -143,9 +146,11 @@
29 :return: A set of `IPerson`s.
30 """
31 # XXX: JonathanLange 2009-09-24: No unit tests.
32- return set(
33+ emails = set(
34 map(get_email,
35 [self._mp.source_branch.owner, self._launchpad.me]))
36+ emails.discard(None)
37+ return emails
38
39 def get_reviews(self):
40 """Return a dictionary of all Approved reviews.
41@@ -163,6 +168,8 @@
42 continue
43 reviewers = reviews.setdefault(vote.review_type, [])
44 reviewers.append(vote.reviewer)
45+ if self.is_approved and not reviews:
46+ reviews[None] = [self._mp.reviewer]
47 return reviews
48
49 def get_bugs(self):
50@@ -196,7 +203,6 @@
51
52 return '%s %s' % (tags, commit_text)
53
54-
55 class Submitter(object):
56 """Class that submits a to PQM from a merge proposal."""
57
58@@ -281,6 +287,8 @@
59 # error when the email address isn't set. e.g. with name12 in the sample
60 # data. e.g. "httplib2.RelativeURIError: Only absolute URIs are allowed.
61 # uri = tag:launchpad.net:2008:redacted".
62+ if email_object is None:
63+ return None # A team most likely.
64 return email_object.email
65
66
67@@ -292,7 +300,16 @@
68 """
69 if not bugs:
70 return ''
71- return '[bug=%s]' % ','.join(str(bug.id) for bug in bugs)
72+ bug_ids = []
73+ for bug in bugs:
74+ for task in bug.bug_tasks:
75+ if (task.bug_target_name == 'launchpad'
76+ and task.status not in ['Fix Committed', 'Fix Released']):
77+ bug_ids.append(str(bug.id))
78+ break
79+ if not bug_ids:
80+ return ''
81+ return '[bug=%s]' % ','.join(bug_ids)
82
83
84 def get_testfix_clause(testfix=False):
85
86=== modified file 'tests/test_lpland.py'
87--- tests/test_lpland.py 2012-02-02 17:19:07 +0000
88+++ tests/test_lpland.py 2012-02-28 17:10:39 +0000
89@@ -1,4 +1,4 @@
90-# Copyright 2010 Canonical Ltd. This software is licensed under the
91+# Copyright 2010-2012 Canonical Ltd. This software is licensed under the
92 # GNU Affero General Public License version 3 (see the file LICENSE).
93
94 """Tests for automatic landing thing."""
95@@ -6,6 +6,7 @@
96 __metaclass__ = type
97
98 from cStringIO import StringIO
99+import re
100 from textwrap import dedent
101 import unittest
102
103@@ -15,19 +16,39 @@
104 )
105 from bzrlib.plugins.pqm.lpland import (
106 get_bugs_clause,
107+ get_email,
108 get_reviewer_clause,
109 get_reviewer_handle,
110 get_testfix_clause,
111 get_qa_clause,
112+ LaunchpadBranchLander,
113 MissingReviewError,
114 MissingBugsError,
115 MissingBugsIncrementalError,
116 MergeProposal,
117 Submitter,
118 )
119-from bzrlib.tests import TestCaseWithTransport
120-
121-from fakemethod import FakeMethod
122+from bzrlib.tests import (
123+ TestCase,
124+ TestCaseWithTransport,
125+ )
126+
127+from bzrlib.plugins.pqm.tests.fakemethod import FakeMethod
128+
129+DEFAULT=object()
130+
131+class FakeLaunchpad:
132+
133+ def __init__(self, branches=None):
134+ self.branches = FakeLaunchpadBranches(branches)
135+ self.me = FakePerson()
136+
137+
138+class FakeBugTask:
139+
140+ def __init__(self, target_name, status):
141+ self.bug_target_name = target_name
142+ self.status = status
143
144
145 class FakeBug:
146@@ -36,8 +57,17 @@
147 Only used for the purposes of testing.
148 """
149
150- def __init__(self, id):
151+ def __init__(self, id, bug_tasks=None):
152 self.id = id
153+ if bug_tasks is None:
154+ bug_tasks = [FakeBugTask('launchpad', 'Triaged')]
155+ self.bug_tasks = bug_tasks
156+
157+
158+class FakeEmailAddress:
159+
160+ def __init__(self, email):
161+ self.email = email
162
163
164 class FakePerson:
165@@ -46,9 +76,14 @@
166 Only used for the purposes of testing.
167 """
168
169- def __init__(self, name='jrandom', irc_handles=()):
170+ def __init__(self, name='jrandom', irc_handles=(), email=DEFAULT):
171 self.name = name
172 self.irc_nicknames = list(irc_handles)
173+ if email is not DEFAULT:
174+ self.preferred_email_address = email
175+ else:
176+ self.preferred_email_address = FakeEmailAddress(
177+ 'jrandom@example.org')
178
179
180 class FakeIRC:
181@@ -62,11 +97,33 @@
182 self.network = network
183
184
185+class FakeBzrBranch:
186+
187+ def __init__(self):
188+ pass
189+
190+ def get_public_branch(self):
191+ return 'public'
192+
193+
194+class FakeLaunchpadBranches:
195+
196+ def __init__(self, branches):
197+ self.branches = branches
198+
199+ def getByUrl(self, url):
200+ for branch in self.branches:
201+ if branch.location == url:
202+ return branch
203+
204+
205 class FakeBranch:
206
207 def __init__(self, location):
208 self.location = location
209 self.linked_bugs = [FakeBug(5)]
210+ self.landing_targets = []
211+ self.owner = FakePerson()
212
213 def composePublicURL(self, scheme):
214 return self.location
215@@ -92,12 +149,80 @@
216 Only used for the purposes of testing.
217 """
218
219- def __init__(self, root=None):
220+ def __init__(self, root=None, queue_status='Approved', votes=None,
221+ reviewer=None):
222+ if root is None:
223+ root = FakeLaunchpad()
224 self._root = root
225 self.source_branch = FakeBranch('lp_source')
226 self.target_branch = FakeBranch('lp_target')
227 self.commit_message = 'Message1'
228- self.votes = [FakeVote()]
229+ if votes is not None:
230+ self.votes = votes
231+ else:
232+ self.votes = [FakeVote()]
233+ self.queue_status = queue_status
234+ self.reviewer = reviewer
235+
236+ def lp_save(self):
237+ pass
238+
239+
240+class TestGetEmail(TestCase):
241+
242+ def test_get_email(self):
243+ self.assertEqual('jrandom@example.org', get_email(FakePerson()))
244+
245+ def test_get_email_none(self):
246+ self.assertIs(None, get_email(FakePerson(email=None)))
247+
248+
249+class TestPQMRegexAcceptance(unittest.TestCase):
250+ """Tests if the generated commit message is accepted by PQM regexes."""
251+
252+ def setUp(self):
253+ # PQM regexes; might need update once in a while
254+ self.devel_open_re = ("(?is)^\s*(:?\[testfix\])?\[(?:"
255+ "release-critical=[^\]]+|rs?=[^\]]+)\]")
256+ self.dbdevel_normal_re = ("(?is)^\s*(:?\[testfix\])?\[(?:"
257+ "release-critical|rs?=[^\]]+)\]")
258+
259+ self.mp = MergeProposal(FakeLPMergeProposal())
260+ self.fake_bug = FakeBug(20)
261+ self.fake_person = FakePerson('foo', [])
262+ self.mp.get_bugs = FakeMethod([self.fake_bug])
263+ self.mp.get_reviews = FakeMethod({None : [self.fake_person]})
264+
265+ def assertRegexpMatches(self, text, expected_regexp, msg=None):
266+ """Fail the test unless the text matches the regular expression.
267+
268+ Method default in Python 2.7. Can be removed as soon as LP goes 2.7.
269+ """
270+ if isinstance(expected_regexp, basestring):
271+ expected_regexp = re.compile(expected_regexp)
272+ if not expected_regexp.search(text):
273+ msg = msg or "Regexp didn't match"
274+ msg = '%s: %r not found in %r' % (msg, expected_regexp.pattern,
275+ text)
276+ raise self.failureException(msg)
277+
278+ def _test_commit_message_match(self, incr, no_qa, testfix):
279+ commit_message = self.mp.get_commit_message("Foobaring the sbrubble.",
280+ testfix, no_qa, incr)
281+ self.assertRegexpMatches(commit_message, self.devel_open_re)
282+ self.assertRegexpMatches(commit_message, self.dbdevel_normal_re)
283+
284+ def test_testfix_match(self):
285+ self._test_commit_message_match(incr=False, no_qa=False, testfix=True)
286+
287+ def test_regular_match(self):
288+ self._test_commit_message_match(incr=False, no_qa=False, testfix=False)
289+
290+ def test_noqa_match(self):
291+ self._test_commit_message_match(incr=False, no_qa=True, testfix=False)
292+
293+ def test_incr_match(self):
294+ self._test_commit_message_match(incr=True, no_qa=False, testfix=False)
295
296
297 class TestBugsClaused(unittest.TestCase):
298@@ -121,6 +246,36 @@
299 bugs_clause = get_bugs_clause([bug1, bug2])
300 self.assertEqual('[bug=20,45]', bugs_clause)
301
302+ def test_fixed_bugs_are_excluded(self):
303+ # If a bug is fixed then it is excluded from the bugs clause.
304+ bug1 = FakeBug(20)
305+ bug2 = FakeBug(45, bug_tasks=[
306+ FakeBugTask('fake-project', 'Fix Released')])
307+ bug3 = FakeBug(67, bug_tasks=[
308+ FakeBugTask('fake-project', 'Fix Committed')])
309+ bugs_clause = get_bugs_clause([bug1, bug2, bug3])
310+ self.assertEqual('[bug=20]', bugs_clause)
311+
312+ def test_bugs_open_on_launchpad_are_included(self):
313+ # If a bug has been fixed on one target but not in launchpad, then it
314+ # is included in the bugs clause, because it's relevant to launchpad
315+ # QA.
316+ bug = FakeBug(20, bug_tasks=[
317+ FakeBugTask('fake-project', 'Fix Released'),
318+ FakeBugTask('launchpad', 'Triaged')])
319+ bugs_clause = get_bugs_clause([bug])
320+ self.assertEqual('[bug=20]', bugs_clause)
321+
322+ def test_bugs_fixed_on_launchpad_but_open_in_others_are_excluded(self):
323+ # If a bug has been fixed in Launchpad but not fixed on a different
324+ # target, then it is excluded from the bugs clause, since we don't
325+ # want to QA it.
326+ bug = FakeBug(20, bug_tasks=[
327+ FakeBugTask('fake-project', 'Triaged'),
328+ FakeBugTask('launchpad', 'Fix Released')])
329+ bugs_clause = get_bugs_clause([bug])
330+ self.assertEqual('', bugs_clause)
331+
332
333 class TestGetTestfixClause(unittest.TestCase):
334 """Tests for `get_testfix_clause`"""
335@@ -193,10 +348,7 @@
336
337 def test_rollback_and_noqa_and_incr_given(self):
338 bugs = None
339- no_qa = True
340- incr = True
341- self.assertEqual('[rollback=123]',
342- get_qa_clause(bugs, rollback=123))
343+ self.assertEqual('[rollback=123]', get_qa_clause(bugs, rollback=123))
344
345
346 class TestGetReviewerHandle(unittest.TestCase):
347@@ -401,6 +553,115 @@
348 self.assertRaises(MissingReviewError, self.get_reviewer_clause, {})
349
350
351+class TestLaunchpadBranchLander(TestCase):
352+
353+ def get_lander(self, landing_targets=None):
354+ branch = FakeBranch('public')
355+ if landing_targets is not None:
356+ branch.landing_targets = landing_targets
357+ launchpad = FakeLaunchpad([branch])
358+ return LaunchpadBranchLander(launchpad)
359+
360+ def test_get_merge_proposal_from_branch_no_proposals(self):
361+ lander = self.get_lander()
362+ branch = FakeBzrBranch()
363+ e = self.assertRaises(errors.BzrCommandError,
364+ lander.get_merge_proposal_from_branch, branch)
365+ self.assertEqual('The public branch has no active source merge'
366+ ' proposals. You must have a merge proposal before'
367+ ' attempting to land the branch.', str(e))
368+
369+ def test_get_merge_proposal_one_proposal(self):
370+ proposal = FakeLPMergeProposal()
371+ lander = self.get_lander([proposal])
372+ branch = FakeBzrBranch()
373+ lander_proposal = lander.get_merge_proposal_from_branch(branch)
374+ self.assertIs(proposal, lander_proposal._mp)
375+
376+ def test_get_merge_proposal_two_proposal(self):
377+ lander = self.get_lander([FakeLPMergeProposal(),
378+ FakeLPMergeProposal()])
379+ branch = FakeBzrBranch()
380+ e = self.assertRaises(errors.BzrCommandError,
381+ lander.get_merge_proposal_from_branch, branch)
382+ self.assertEqual('The public branch has multiple active source merge'
383+ ' proposals. You must provide the URL to the one'
384+ ' you wish to use.', str(e))
385+
386+ def test_get_merge_proposal_inactive(self):
387+ for status in ['Rejected', 'Work in progress', 'Merged', 'Queued',
388+ 'Code failed to merge', 'Superseded']:
389+ proposal = FakeLPMergeProposal(queue_status=status)
390+ lander = self.get_lander([proposal])
391+ branch = FakeBzrBranch()
392+ e = self.assertRaises(errors.BzrCommandError,
393+ lander.get_merge_proposal_from_branch,
394+ branch)
395+ self.assertEqual('The public branch has no active source merge'
396+ ' proposals. You must have a merge proposal'
397+ ' before attempting to land the branch.',
398+ str(e))
399+
400+ def test_get_merge_proposal_active(self):
401+ branch = FakeBzrBranch()
402+ for status in ['Approved', 'Needs review']:
403+ proposal = FakeLPMergeProposal(queue_status=status)
404+ lander = self.get_lander([proposal])
405+ lander_proposal = lander.get_merge_proposal_from_branch(branch)
406+ self.assertIs(proposal, lander_proposal._mp)
407+
408+
409+class TestMergeProposal(TestCase):
410+
411+ def test_get_reviews_none_unapproved(self):
412+ """Reviewer is not considered for un-approved propoals."""
413+ reviewer = FakePerson()
414+ proposal = FakeLPMergeProposal(queue_status='Needs review', votes=[],
415+ reviewer=reviewer)
416+ mp = MergeProposal(proposal)
417+ approvals = mp.get_reviews()
418+ self.assertEqual({}, approvals)
419+
420+ def test_get_reviews_none_approved(self):
421+ """Approving a proposal counts as a vote if other approvals."""
422+ reviewer = FakePerson()
423+ proposal = FakeLPMergeProposal(queue_status='Approved', votes=[],
424+ reviewer=reviewer)
425+ mp = MergeProposal(proposal)
426+ approvals = mp.get_reviews()
427+ self.assertEqual({None: [reviewer]}, approvals)
428+
429+ def test_get_reviews_one_approved(self):
430+ """As long as there is one approve vote, don't use reviewer."""
431+ reviewer = FakePerson()
432+ proposal = FakeLPMergeProposal(queue_status='Approved',
433+ votes=[FakeVote()],
434+ reviewer=reviewer)
435+ mp = MergeProposal(proposal)
436+ approvals = mp.get_reviews()
437+ self.assertEqual({None: [proposal.votes[0].reviewer]}, approvals)
438+
439+ def test_get_stakeholder_emails(self):
440+ lp = FakeLaunchpad()
441+ mp = MergeProposal(FakeLPMergeProposal(root=lp))
442+ lp.me.preferred_email_address = FakeEmailAddress('lander@example.org')
443+ owner = mp._mp.source_branch.owner
444+ owner.preferred_email_address = FakeEmailAddress('owner@example.org')
445+ expected = set(['owner@example.org', 'lander@example.org'])
446+ emails = mp.get_stakeholder_emails()
447+ self.assertEqual(expected, emails)
448+
449+ def test_get_stakeholder_emails_none(self):
450+ lp = FakeLaunchpad()
451+ mp = MergeProposal(FakeLPMergeProposal(root=lp))
452+ lp.me.preferred_email_address = FakeEmailAddress('lander@example.org')
453+ owner = mp._mp.source_branch.owner
454+ owner.preferred_email_address = None
455+ expected = set(['lander@example.org'])
456+ emails = mp.get_stakeholder_emails()
457+ self.assertEqual(expected, emails)
458+
459+
460 class TestSubmitter(TestCaseWithTransport):
461
462 def make_submitter(self):

Subscribers

People subscribed via source and target branches

to all changes:
to status/vote changes: