registry team mailing list archive
-
registry team
-
Mailing list archive
-
Message #10162
[Merge] lp:~ursinha/bzr-pqm/add-noqa-incr-lpland into lp:bzr-pqm
Ursula Junque has proposed merging lp:~ursinha/bzr-pqm/add-noqa-incr-lpland into lp:bzr-pqm.
Requested reviews:
Bzr-pqm-devel (bzr-pqm-devel)
Related bugs:
#602137 add bzr lp-land support to no-qa and incr QA tags
https://bugs.launchpad.net/bugs/602137
= Summary =
This branch is a fix for bug 602137, adding to bzr lp-land three options: --testfix, --no-qa and --incr. --no-qa option should add the [no-qa] tag to the commit message, --incremental option should add the [incr] tag, and --testfix, [testfix]. This enforces the QA policy of now allowing fixes to be committed without bugs unless explicitely pointed.
== Proposed fix ==
The implementation is pretty simple. A method checks if the --no-qa, --incremental or --testfix options are given to bzr lp-land, and combined with the bugs found in the mp decides if the merge can continue based on the QA policy:
* If the mp has no linked bugs and no no-qa tag, ec2 land should fail because without bugs and no-qa tag that commit would be an orphaned one.
* If the mp has bugs and the no-qa commit tag, that's ok. The bug #s and the no-qa tag will be added to the commit message. This will be properly parsed and handled by qa-tagger script after the branch landed.
* If the mp has no linked bugs but has the no-qa commit tag, that's ok. The commit will be qa-untestable, therefore not orphaned.
* If the mp has no linked bugs but has the incr commit tag, ec2 land should fail, because bugs are needed when the fix is incremental.
* A commit cannot be no-qa and incr at the same time.
== Tests ==
I've tested the newly created methods by adding more tests to test_lpland.py. I'm also testing get_commit_message using a very basic implementation of a fakemethod, that mocks launchpadlib's responses.
bzr selftest pqm
== Demo and Q/A ==
I've tested by running the command in a real merge proposal in Launchpad, using the --dry-run option, to check if the commit message is correctly parsed and tagged by bzr lp-land. I landed my branch that does similar changes to ec2 land (https://code.edge.launchpad.net/~ursinha/launchpad/add-ec2land-rules-orphaned-branches-no-conflicts/+merge/31386) using it.
devel/add-ec2land-rules-orphaned-branches-no-conflicts $ bzr lp-land
Results should be as following:
* Both --incremental and --no-qa at the same time: should error.
* without --no-qa and nor linked bugs: should error.
* with --incremental and no linked bugs: should error.
* with --incremental and linked bugs: should work.
* with --no-qa and no linked bugs: should work.
* with --no-qa and linked bugs: should work.
--
https://code.launchpad.net/~ursinha/bzr-pqm/add-noqa-incr-lpland/+merge/31703
Your team Bzr-pqm-devel is requested to review the proposed merge of lp:~ursinha/bzr-pqm/add-noqa-incr-lpland into lp:bzr-pqm.
=== modified file '__init__.py'
--- __init__.py 2010-04-14 10:48:03 +0000
+++ __init__.py 2010-08-03 22:01:20 +0000
@@ -18,6 +18,7 @@
from bzrlib.commands import Command, register_command
from bzrlib.option import Option
+from bzrlib.errors import BzrCommandError
version_info = (1, 4, 0, 'dev', 0)
@@ -83,7 +84,7 @@
def run(self, location=None, message=None, public_location=None,
dry_run=False, submit_branch=None, ignore_local=False):
- from bzrlib import errors, trace, bzrdir
+ from bzrlib import trace, bzrdir
if __name__ != 'bzrlib.plugins.pqm':
trace.warning('The bzr-pqm plugin needs to be called'
' "bzrlib.plugins.pqm" not "%s"\n'
@@ -103,12 +104,12 @@
b.lock_read()
self.add_cleanup(b.unlock)
if relpath and not tree and location != '.':
- raise errors.BzrCommandError(
+ raise BzrCommandError(
'No working tree was found, but we were not given the '
'exact path to the branch.\n'
'We found a branch at: %s' % (b.base,))
if message is None:
- raise errors.BzrCommandError(
+ raise BzrCommandError(
'You must supply a commit message for the pqm to use.')
submit(b, message=message, dry_run=dry_run,
public_location=public_location,
@@ -125,18 +126,53 @@
takes_args = ['location?']
- takes_options = [Option(
- 'dry-run', help='Display the PQM message instead of sending.')]
+ takes_options = [
+ Option('dry-run', help='Display the PQM message instead of sending.'),
+ Option(
+ 'testfix',
+ help="This is a testfix (tags commit with [testfix])."),
+ Option(
+ 'no-qa',
+ help="Does not require QA (tags commit with [no-qa])."),
+ Option(
+ 'incremental',
+ help="Incremental to other bug fix (tags commit with [incr])."),
+ ]
- def run(self, location=None, dry_run=False):
+ def run(self, location=None, dry_run=False, testfix=False,
+ no_qa=False, incremental=False):
from bzrlib.plugins.pqm.lpland import Submitter
from bzrlib import branch as _mod_branch
+ from bzrlib.plugins.pqm.lpland import (
+ MissingReviewError, MissingBugsError, MissingBugsIncrementalError)
+
+
+ if no_qa and incremental:
+ raise BzrCommandError(
+ "--no-qa and --incremental cannot be given at the same time.")
+
branch = _mod_branch.Branch.open_containing('.')[0]
if dry_run:
outf = self.outf
else:
outf = None
- submitter = Submitter(branch, location).run(outf)
+ try:
+ submitter = Submitter(branch, location, testfix, no_qa, incremental
+ ).run(outf)
+ except MissingReviewError:
+ raise BzrCommandError(
+ "Cannot land branches that haven't got approved code "
+ "reviews. Get an 'Approved' vote so we can fill in the "
+ "[r=REVIEWER] section.")
+ except MissingBugsError:
+ raise BzrCommandError(
+ "Branch doesn't have linked bugs and doesn't have no-qa "
+ "option set. Use --no-qa, or link the related bugs to the "
+ "branch.")
+ except MissingBugsIncrementalError:
+ raise BzrCommandError(
+ "--incremental option requires bugs linked to the branch. "
+ "Link the bugs or remove the --incremental option.")
register_command(cmd_pqm_submit)
@@ -147,8 +183,7 @@
from bzrlib.tests import TestLoader
from unittest import TestSuite
- import test_lpland
- import test_pqm_submit
+ from tests import test_lpland, test_pqm_submit
loader = TestLoader()
return TestSuite([
=== modified file 'lpland.py'
--- lpland.py 2010-04-07 09:47:52 +0000
+++ lpland.py 2010-08-03 22:01:20 +0000
@@ -33,6 +33,14 @@
"""Raised when we try to get a review message without enough reviewers."""
+class MissingBugsError(Exception):
+ """Merge proposal has no linked bugs and no [no-qa] tag."""
+
+
+class MissingBugsIncrementalError(Exception):
+ """Merge proposal has the [incr] tag but no linked bugs."""
+
+
class LaunchpadBranchLander:
name = 'launchpad-branch-lander'
@@ -171,25 +179,31 @@
# XXX: JonathanLange 2009-09-24: No unit tests.
return branch.composePublicURL(scheme="bzr+ssh")
- def get_commit_message(self, commit_text, testfix=False):
+ def get_commit_message(self, commit_text, testfix=False, no_qa=False,
+ incremental=False):
"""Get the Launchpad-style commit message for a merge proposal."""
reviews = self.get_reviews()
bugs = self.get_bugs()
- if testfix:
- testfix = '[testfix]'
- else:
- testfix = ''
- return '%s%s%s %s' % (
- testfix,
+
+ tags = ''.join([
+ get_testfix_clause(testfix),
get_reviewer_clause(reviews),
get_bugs_clause(bugs),
- commit_text)
+ get_qa_clause(bugs, no_qa,
+ incremental),
+ ])
+
+ return '%s %s' % (tags, commit_text)
class Submitter(object):
- def __init__(self, branch, location):
+ def __init__(self, branch, location, testfix=False, no_qa=False,
+ incremental=False):
self.branch = branch
+ self.testfix = testfix
+ self.no_qa = no_qa
+ self.incremental = incremental
self.config = self.branch.get_config()
self.mail_to = self.config.get_user_option('pqm_email')
if not self.mail_to:
@@ -211,10 +225,11 @@
submission.check_public_branch()
@staticmethod
- def set_message(submission, mp):
+ def set_message(submission, mp, testfix, no_qa, incremental):
pqm_command = ''.join(submission.to_lines())
commit_message = mp.commit_message or ''
- start_message = mp.get_commit_message(commit_message)
+ start_message = mp.get_commit_message(commit_message, testfix, no_qa,
+ incremental)
message = msgeditor.edit_commit_message(
'pqm command:\n%s' % pqm_command,
start_message=start_message).rstrip('\n')
@@ -227,7 +242,8 @@
mp = self.lander.load_merge_proposal(self.location)
submission = self.submission(mp)
self.check_submission(submission)
- self.set_message(submission, mp)
+ self.set_message(submission, mp, self.testfix, self.no_qa,
+ self.incremental)
email = submission.to_email(self.mail_from, self.mail_to)
if outf is not None:
outf.write(email.as_string())
@@ -256,6 +272,39 @@
return '[bug=%s]' % ','.join(str(bug.id) for bug in bugs)
+def get_testfix_clause(testfix=False):
+ """Get the testfix clause."""
+ if testfix:
+ testfix_clause = '[testfix]'
+ else:
+ testfix_clause = ''
+ return testfix_clause
+
+
+def get_qa_clause(bugs, no_qa=False, incremental=False):
+ """Check the no-qa and incremental options, getting the qa clause.
+
+ The qa clause will always be or no-qa, or incremental or no tags, never
+ both at the same time.
+ """
+ qa_clause = ""
+
+ if not bugs and not no_qa and not incremental:
+ raise MissingBugsError
+
+ if incremental and not bugs:
+ raise MissingBugsIncrementalError
+
+ if incremental:
+ qa_clause = '[incr]'
+ elif no_qa:
+ qa_clause = '[no-qa]'
+ else:
+ qa_clause = ''
+
+ return qa_clause
+
+
def get_reviewer_handle(reviewer):
"""Get the handle for 'reviewer'.
=== added directory 'tests'
=== added file 'tests/__init__.py'
=== added file 'tests/fakemethod.py'
--- tests/fakemethod.py 1970-01-01 00:00:00 +0000
+++ tests/fakemethod.py 2010-08-03 22:01:20 +0000
@@ -0,0 +1,21 @@
+__metaclass__ = type
+
+class FakeMethod:
+ """Catch any function or method call, and record the fact."""
+
+ def __init__(self, result=None, failure=None):
+ """Set up a fake function or method.
+
+ :param result: Value to return.
+ :param failure: Exception to raise.
+ """
+ self.result = result
+ self.failure = failure
+
+ def __call__(self, *args, **kwargs):
+ """Catch an invocation to the method."""
+
+ if self.failure is None:
+ return self.result
+ else:
+ raise self.failure
=== renamed file 'test_lpland.py' => 'tests/test_lpland.py'
--- test_lpland.py 2010-03-31 14:41:33 +0000
+++ tests/test_lpland.py 2010-08-03 22:01:20 +0000
@@ -13,7 +13,11 @@
from bzrlib.plugins.pqm.lpland import (
get_bazaar_host, get_bugs_clause, get_reviewer_clause,
- get_reviewer_handle, MissingReviewError)
+ get_reviewer_handle, get_testfix_clause, get_qa_clause,
+ MissingReviewError, MissingBugsError, MissingBugsIncrementalError,
+ MergeProposal)
+
+from fakemethod import FakeMethod
class FakeBug:
@@ -48,6 +52,16 @@
self.network = network
+class FakeLPMergeProposal:
+ """Fake launchpadlib MergeProposal object.
+
+ Only used for the purposes of testing.
+ """
+
+ def __init__(self, root=None):
+ self._root = root
+
+
class TestBugsClaused(unittest.TestCase):
"""Tests for `get_bugs_clause`."""
@@ -70,6 +84,64 @@
self.assertEqual('[bug=20,45]', bugs_clause)
+class TestGetTestfixClause(unittest.TestCase):
+ """Tests for `get_testfix_clause`"""
+
+ def test_no_testfix(self):
+ testfix = False
+ self.assertEqual('', get_testfix_clause(testfix))
+
+ def test_is_testfix(self):
+ testfix = True
+ self.assertEqual('[testfix]', get_testfix_clause(testfix))
+
+
+class TestGetQaClause(unittest.TestCase):
+ """Tests for `get_qa_clause`"""
+
+ def test_no_bugs_no_option_given(self):
+ bugs = None
+ no_qa = False
+ incr = False
+ self.assertRaises(MissingBugsError, get_qa_clause, bugs, no_qa,
+ incr)
+
+ def test_bugs_noqa_option_given(self):
+ bug1 = FakeBug(20)
+ no_qa = True
+ incr = False
+ self.assertEqual('[no-qa]',
+ get_qa_clause([bug1], no_qa, incr))
+
+ def test_no_bugs_noqa_option_given(self):
+ bugs = None
+ no_qa = True
+ incr = False
+ self.assertEqual('[no-qa]',
+ get_qa_clause(bugs, no_qa, incr))
+
+ def test_bugs_no_option_given(self):
+ bug1 = FakeBug(20)
+ no_qa = False
+ incr = False
+ self.assertEqual('',
+ get_qa_clause([bug1], no_qa, incr))
+
+ def test_bugs_incr_option_given(self):
+ bug1 = FakeBug(20)
+ no_qa = False
+ incr = True
+ self.assertEqual('[incr]',
+ get_qa_clause([bug1], no_qa, incr))
+
+ def test_no_bugs_incr_option_given(self):
+ bugs = None
+ no_qa = False
+ incr = True
+ self.assertRaises(MissingBugsIncrementalError,
+ get_qa_clause, bugs, no_qa, incr)
+
+
class TestGetReviewerHandle(unittest.TestCase):
"""Tests for `get_reviewer_handle`."""
@@ -96,6 +168,91 @@
self.assertEqual('foo', get_reviewer_handle(person))
+class TestGetCommitMessage(unittest.TestCase):
+
+ def setUp(self):
+ self.mp = MergeProposal(FakeLPMergeProposal())
+ self.fake_bug = FakeBug(20)
+ self.fake_person = self.makePerson('foo')
+
+ def makePerson(self, name):
+ return FakePerson(name, [])
+
+ def test_commit_with_bugs(self):
+ incr = False
+ no_qa = False
+ testfix = False
+
+ self.mp.get_bugs = FakeMethod([self.fake_bug])
+ self.mp.get_reviews = FakeMethod({None : [self.fake_person]})
+
+ self.assertEqual("[r=foo][ui=none][bug=20] Foobaring the sbrubble.",
+ self.mp.get_commit_message("Foobaring the sbrubble.",
+ testfix, no_qa, incr))
+
+ def test_commit_no_bugs_no_noqa(self):
+ incr = False
+ no_qa = False
+ testfix = False
+
+ self.mp.get_bugs = FakeMethod([])
+ self.mp.get_reviews = FakeMethod({None : [self.fake_person]})
+
+ self.assertRaises(MissingBugsError, self.mp.get_commit_message,
+ testfix, no_qa, incr)
+
+ def test_commit_no_bugs_with_noqa(self):
+ incr = False
+ no_qa = True
+ testfix = False
+
+ self.mp.get_bugs = FakeMethod([])
+ self.mp.get_reviews = FakeMethod({None : [self.fake_person]})
+
+ self.assertEqual("[r=foo][ui=none][no-qa] Foobaring the sbrubble.",
+ self.mp.get_commit_message("Foobaring the sbrubble.",
+ testfix, no_qa, incr))
+
+ def test_commit_bugs_with_noqa(self):
+ incr = False
+ no_qa = True
+ testfix = False
+
+ self.mp.get_bugs = FakeMethod([self.fake_bug])
+ self.mp.get_reviews = FakeMethod({None : [self.fake_person]})
+
+ self.assertEqual(
+ "[r=foo][ui=none][bug=20][no-qa] Foobaring the sbrubble.",
+ self.mp.get_commit_message("Foobaring the sbrubble.",
+ testfix, no_qa, incr))
+
+ def test_commit_bugs_with_incr(self):
+ incr = True
+ no_qa = False
+ testfix = False
+
+ self.mp.get_bugs = FakeMethod([self.fake_bug])
+ self.mp.get_reviews = FakeMethod({None : [self.fake_person]})
+
+ self.assertEqual(
+ "[r=foo][ui=none][bug=20][incr] Foobaring the sbrubble.",
+ self.mp.get_commit_message("Foobaring the sbrubble.",
+ testfix, no_qa, incr))
+
+ def test_commit_no_bugs_with_incr(self):
+ incr = True
+ no_qa = False
+ testfix = False
+
+ self.mp.get_bugs = FakeMethod([self.fake_bug])
+ self.mp.get_reviews = FakeMethod({None : [self.fake_person]})
+
+ self.assertEqual(
+ "[r=foo][ui=none][bug=20][incr] Foobaring the sbrubble.",
+ self.mp.get_commit_message("Foobaring the sbrubble.",
+ testfix, no_qa, incr))
+
+
class TestGetReviewerClause(unittest.TestCase):
"""Tests for `get_reviewer_clause`."""
=== renamed file 'test_pqm_submit.py' => 'tests/test_pqm_submit.py'
Follow ups
