Merge lp:~ursinha/launchpad/add-ec2land-rules-orphaned-branches-no-conflicts into lp:launchpad

[Ursula Junque]

[r=henninge][ui=none][bug=601995] Adds the --incremental and --no-qa options to the ec2 land script, as part of the orphaned branches story. Also adds tests to the previous implemented methods.

This is the same set of changes as approved in https://code.edge.launchpad.net/~ursinha/launchpad/add-ec2land-rules-orphaned-branches/+merge/29255, with all suggestions applied. I had trouble solving a merge conflict in that one, so created this branch with all conflicts solved, tested and ready to land.

= Summary =

This branch is a fix for bug 601995, adding to ec2 land two options: --no-qa and --incr. --no-qa option should add the [no-qa] tag to the commit message, and --incr option should add the [incr] tag. 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 or the --incr options are passed to ec2 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 method by adding more tests to test_autoland.py.

./bin/test -vvt devscripts.*

It's not possible to test get_commit_message directly because it all depends on launchpadlib.

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

Considering staging is down for some time now, I've tested with this mp: https://code.edge.launchpad.net/~ursinha/qa-tagger/find_linked_branches-bug-591856/+merge/28025:

$ ec2 land https://code.edge.launchpad.net/~ursinha/qa-tagger/find_linked_branches-bug-591856/+merge/28025 --force --commit-text "Testing ec2 land" --dry-run [combinations below]

Results should be as following:
* Both --incr and --no-qa at the same time: should error.
* without --no-qa and nor linked bugs: should error.
* with --incr and no linked bugs: should error.
* with --incr and linked bugs: should work.
* with --no-qa and no linked bugs: should work.
* with --no-qa and linked bugs: should work.

[Henning Eggers (henninge)]

I compared the diffs and they are essentially identical. ;-)

review: Approve (code)

=== modified file 'lib/devscripts/autoland.py'
--- lib/devscripts/autoland.py	2010-03-05 21:47:29 +0000
+++ lib/devscripts/autoland.py	2010-07-30 15:13:50 +0000
@@ -12,6 +12,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'
@@ -152,19 +160,54 @@
         # URL. Do it ourselves.
         return URI(scheme='bzr+ssh', host=host, path='/' + branch.unique_name)
 
-    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)
+
+
+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_email(person):

=== modified file 'lib/devscripts/ec2test/builtins.py'
--- lib/devscripts/ec2test/builtins.py	2010-04-13 19:32:04 +0000
+++ lib/devscripts/ec2test/builtins.py	2010-07-30 15:13:50 +0000
@@ -321,7 +321,13 @@
         Option('print-commit', help="Print the full commit message."),
         Option(
             'testfix',
-            help="Include the [testfix] prefix in the commit message."),
+            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])."),
         Option(
             'commit-text', short_name='s', type=str,
             help=(
@@ -355,10 +361,12 @@
     def run(self, merge_proposal=None, machine=None,
             instance_type=DEFAULT_INSTANCE_TYPE, postmortem=False,
             debug=False, commit_text=None, dry_run=False, testfix=False,
-            print_commit=False, force=False, attached=False):
+            no_qa=False, incremental=False, print_commit=False, force=False,
+            attached=False):
         try:
             from devscripts.autoland import (
-                LaunchpadBranchLander, MissingReviewError)
+                LaunchpadBranchLander, MissingReviewError, MissingBugsError,
+                MissingBugsIncrementalError)
         except ImportError:
             self.outf.write(
                 "***************************************************\n\n"
@@ -374,6 +382,9 @@
         if print_commit and dry_run:
             raise BzrCommandError(
                 "Cannot specify --print-commit and --dry-run.")
+        if no_qa and incremental:
+            raise BzrCommandError(
+                "--no-qa and --incremental cannot be given at the same time.")
         lander = LaunchpadBranchLander.load()
 
         if merge_proposal is None:
@@ -396,12 +407,23 @@
                 "Commit text not specified. Use --commit-text, or specify a "
                 "message on the merge proposal.")
         try:
-            commit_message = mp.get_commit_message(commit_text, testfix)
+            commit_message = mp.get_commit_message(
+                commit_text, testfix, no_qa, incremental)
         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.")
+
         if print_commit:
             print commit_message
             return

=== modified file 'lib/devscripts/tests/test_autoland.py'
--- lib/devscripts/tests/test_autoland.py	2009-10-08 23:22:16 +0000
+++ lib/devscripts/tests/test_autoland.py	2010-07-30 15:13:50 +0000
@@ -6,12 +6,17 @@
 __metaclass__ = type
 
 import unittest
+import re
 
 from launchpadlib.launchpad import EDGE_SERVICE_ROOT, STAGING_SERVICE_ROOT
 
+from lp.testing.fakemethod import FakeMethod
+
 from devscripts.autoland import (
     get_bazaar_host, get_bugs_clause, get_reviewer_clause,
-    get_reviewer_handle, MissingReviewError)
+    get_reviewer_handle, get_qa_clause, get_testfix_clause,
+    MissingReviewError, MissingBugsError, MissingBugsIncrementalError,
+    MergeProposal)
 
 
 class FakeBug:
@@ -46,6 +51,64 @@
         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 TestPQMRegexAcceptance(unittest.TestCase):
+    """Tests if the generated commit message is accepted by PQM regexes."""
+
+    def setUp(self):
+        # PQM regexes; might need update once in a while
+        self.devel_open_re = ("(?is)^\s*(:?\[testfix\])?\[(?:"
+            "release-critical=[^\]]+|rs?=[^\]]+)\]\[ui=(?:.+)\]")
+        self.dbdevel_normal_re = ("(?is)^\s*(:?\[testfix\])?\[(?:"
+            "release-critical|rs?=[^\]]+)\]")
+
+        self.mp = MergeProposal(FakeLPMergeProposal())
+        self.fake_bug = FakeBug(20)
+        self.fake_person = FakePerson('foo', [])
+        self.mp.get_bugs = FakeMethod([self.fake_bug])
+        self.mp.get_reviews = FakeMethod({None : [self.fake_person]})
+
+    def assertRegexpMatches(self, text, expected_regexp, msg=None):
+        """Fail the test unless the text matches the regular expression.
+
+        Method default in Python 2.7. Can be removed as soon as LP goes 2.7.
+        """
+        if isinstance(expected_regexp, basestring):
+            expected_regexp = re.compile(expected_regexp)
+        if not expected_regexp.search(text):
+            msg = msg or "Regexp didn't match"
+            msg = '%s: %r not found in %r' % (msg, expected_regexp.pattern,
+                text)
+            raise self.failureException(msg)
+
+    def _test_commit_message_match(self, incr, no_qa, testfix):
+        commit_message = self.mp.get_commit_message("Foobaring the sbrubble.",
+            testfix, no_qa, incr)
+        self.assertRegexpMatches(commit_message, self.devel_open_re)
+        self.assertRegexpMatches(commit_message, self.dbdevel_normal_re)
+
+    def test_testfix_match(self):
+        self._test_commit_message_match(incr=False, no_qa=False, testfix=True)
+
+    def test_regular_match(self):
+        self._test_commit_message_match(incr=False, no_qa=False, testfix=False)
+
+    def test_noqa_match(self):
+        self._test_commit_message_match(incr=False, no_qa=True, testfix=False)
+
+    def test_incr_match(self):
+        self._test_commit_message_match(incr=True, no_qa=False, testfix=False)
+
+
 class TestBugsClaused(unittest.TestCase):
     """Tests for `get_bugs_clause`."""
 
@@ -68,6 +131,149 @@
         self.assertEqual('[bug=20,45]', bugs_clause)
 
 
+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 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`."""