Merge lp:~ursinha/bzr-pqm/add-noqa-incr-lpland into lp:bzr-pqm

[Ursula Junque]

= 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.

[John A Meinel (jameinel)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Ursula Junque wrote:
> 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.

As far as reviewing goes, I'm going to trust that you know the Launchpad
requirements more than I do. As such, I'm not really auditing this part.
I'll just look at the code itself.

Personally, I really don't like this notation:
+__metaclass__ = type

Certainly in bzrlib itself we always do:

class Foo(object):

instead.

I'm certainly inclined to be less strict in bzr-pqm, esp. as I think the
Launchpad standard way of doing it is with __metaclass__. (Also, I see
that test_lpland was using __metaclass__ as well.)

I'm going to just land this, but mostly I'm trusting that you've thought
the logic through. Certainly everything I've seen seems well thought out.

John
=:->
 merge: approve

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAkxcPVQACgkQJdeBCYSNAAMDNQCcC6688GqhnisqHAjqVti2q3T/
L6kAnivByx3OiMqv8oDA4sV4ZTgosN0Z
=zgkD
-----END PGP SIGNATURE-----

review: Approve

=== 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'