Launchpad itself

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

  1. add-ec2land-rules-orphaned-branches
  2. Merge into devel
Proposed by Ursula Junque
Status: Rejected
Rejected by: Ursula Junque
Proposed branch: lp:~ursinha/launchpad/add-ec2land-rules-orphaned-branches
Merge into: lp:launchpad
Diff against target: 438 lines (+287/-22)
4 files modified
lib/devscripts/autoland.py (+51/-8)
lib/devscripts/ec2test/builtins.py (+26/-4)
lib/devscripts/tests/test_autoland.py (+207/-1)
lib/lp/registry/utilities/salesforce.py (+3/-9)
To merge this branch: bzr merge lp:~ursinha/launchpad/add-ec2land-rules-orphaned-branches
Reviewer Review Type Date Requested Status
Māris Fogels (community) Abstain
Jeroen T. Vermeulen (community) code Approve
Review via email: mp+29255@code.launchpad.net

Description of the change

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

= Launchpad lint =

Checking for conflicts. and issues in doctests and templates.
Running jslint, xmllint, pyflakes, and pylint.
Using normal rules.

Linting changed files:
  lib/devscripts/autoland.py
  lib/devscripts/tests/test_autoland.py
  lib/devscripts/ec2test/builtins.py

To post a comment you must log in.
Revision history for this message
Jeroen T. Vermeulen (jtv) wrote :
Download full text (7.8 KiB)

Nice MP, nice branch! Very thorough and clean. I've been seeing too many MPs without mention of design, caveats, QA, lint etc. lately and this one just has it all—plus a good description of what the branch is supposed to do.

Of course I have some notes:

> === modified file 'lib/devscripts/autoland.py'
> --- lib/devscripts/autoland.py 2010-03-05 21:47:29 +0000
> +++ lib/devscripts/autoland.py 2010-07-06 06:19:30 +0000
> @@ -12,6 +12,16 @@
> """Raised when we try to get a review message without enough reviewers."""
>
>
> +class MissingBugsError(Exception):
> + """Raised when we try to land a mp without 'no-qa' tag and no linked
> + bugs."""
> +
> +
> +class MissingBugsIncrError(Exception):
> + """Raised when we try to land a mp with the 'incr' tag and no linked
> + bugs."""

I don't think it's necessary to say "Raised when." The derivation from Exception and the name it clear already.

You could just say "Merge proposal has no linked bugs and no 'no-qa' tag," and "Merge proposal has the 'incr' tag but no linked bugs." That also gets around the ugly line breaks and the abbreviation "mp."

> class LaunchpadBranchLander:
>
> name = 'launchpad-branch-lander'
> @@ -152,21 +162,49 @@
> # 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,
> + incr=False):

That last parameter should be indented to align with the "self" parameter. We do this differently for method definitions than for method invocations.

> """Get the Launchpad-style commit message for a merge proposal."""
> reviews = self.get_reviews()
> bugs = self.get_bugs()
> + no_qa, incr = check_qa_clauses(bugs, no_qa, incr)
> +
> if testfix:
> testfix = '[testfix]'
> else:
> testfix = ''
> - return '%s%s%s %s' % (
> +
> + return '%s%s%s%s%s %s' % (
> testfix,
> get_reviewer_clause(reviews),
> get_bugs_clause(bugs),
> + no_qa,
> + incr,
> commit_text)
>
>
> +def check_qa_clauses(bugs, no_qa=False, incr=False):
> + """Check the no-qa and incr clauses."""

Good instinct to separate bits out like this so that they become unit-testable. This is what made me instantly like this branch. One bit that bothers me slightly is that it does two related but different things: check that the two options are appropriate, and compose a commit string fragment for them.

You also end up with get_commit_message doing different things: gather data, delegate the QA-related checking & composing to check_qa_clauses, and composing the rest of the commit string. AIUI it's the data gathering that makes unit-testing hard, and you did the right thing in not letting it sneak into your own function.

A rule of thumb I have for maintainable code is that a function should not mix abstractions. In get_commit_message we have both "fiddle with strings" and "delegate one area of the checking and string-fid...

Read more...

review: Needs Information (code)
Revision history for this message
Ursula Junque (ursinha) wrote :
Download full text (9.8 KiB)

Hi Jeroen!

Thanks for this review! Very useful comments. I reply inline.

* Jeroen T. Vermeulen <email address hidden> [2010-07-07 11:10:59 -0000]:

> Review: Needs Information code
> Nice MP, nice branch! Very thorough and clean. I've been seeing too many MPs without mention of design, caveats, QA, lint etc. lately and this one just has it all—plus a good description of what the branch is supposed to do.
>
> Of course I have some notes:
>
>
> > === modified file 'lib/devscripts/autoland.py'
> > --- lib/devscripts/autoland.py 2010-03-05 21:47:29 +0000
> > +++ lib/devscripts/autoland.py 2010-07-06 06:19:30 +0000
> > @@ -12,6 +12,16 @@
> > """Raised when we try to get a review message without enough reviewers."""
> >
> >
> > +class MissingBugsError(Exception):
> > + """Raised when we try to land a mp without 'no-qa' tag and no linked
> > + bugs."""
> > +
> > +
> > +class MissingBugsIncrError(Exception):
> > + """Raised when we try to land a mp with the 'incr' tag and no linked
> > + bugs."""
>
> I don't think it's necessary to say "Raised when." The derivation from Exception and the name it clear already.
>
> You could just say "Merge proposal has no linked bugs and no 'no-qa' tag," and "Merge proposal has the 'incr' tag but no linked bugs." That also gets around the ugly line breaks and the abbreviation "mp."

You're right, I've changed as suggested.

>
>
> > class LaunchpadBranchLander:
> >
> > name = 'launchpad-branch-lander'
> > @@ -152,21 +162,49 @@
> > # 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,
> > + incr=False):
>
> That last parameter should be indented to align with the "self" parameter. We do this differently for method definitions than for method invocations.

Done.

>
>
> > """Get the Launchpad-style commit message for a merge proposal."""
> > reviews = self.get_reviews()
> > bugs = self.get_bugs()
> > + no_qa, incr = check_qa_clauses(bugs, no_qa, incr)
> > +
> > if testfix:
> > testfix = '[testfix]'
> > else:
> > testfix = ''
> > - return '%s%s%s %s' % (
> > +
> > + return '%s%s%s%s%s %s' % (
> > testfix,
> > get_reviewer_clause(reviews),
> > get_bugs_clause(bugs),
> > + no_qa,
> > + incr,
> > commit_text)
> >
> >
> > +def check_qa_clauses(bugs, no_qa=False, incr=False):
> > + """Check the no-qa and incr clauses."""
>
> Good instinct to separate bits out like this so that they become unit-testable. This is what made me instantly like this branch. One bit that bothers me slightly is that it does two related but different things: check that the two options are appropriate, and compose a commit string fragment for them.
>
> You also end up with get_commit_message doing different things: gather data, delegate the QA-related checking & composing to check_qa_clauses, and composing the rest ...

Revision history for this message
Jeroen T. Vermeulen (jtv) wrote :

Hi Ursula,

My apologies for the slow response; plenty of professional distractions going on here! Great stuff you're doing here, with the fake objects especially. But of course still a few more things to look at:

--- lib/devscripts/autoland.py 2010-03-05 21:47:29 +0000
+++ lib/devscripts/autoland.py 2010-07-22 02:47:45 +0000
@@ -152,19 +160,47 @@
[...]
- if testfix:
- testfix = '[testfix]'
- else:
- testfix = ''
- return '%s%s%s %s' % (
- testfix,
- get_reviewer_clause(reviews),
- get_bugs_clause(bugs),
- commit_text)
+
+ clauses.append(get_reviewer_clause(reviews))
+ clauses.append(get_bugs_clause(bugs))
+ clauses.append(get_qa_clause(bugs, no_qa, incremental))
+ clauses.append(get_testfix_clause(testfix))

Two things here:

1. Are these really in the required order? PQM's regexes are rather strict. Do we test against the real, actual, current PQM regexes anywhere?

2. Why not make it easy and write:

        tags = ''.join([
            get_reviewer_clause(reviews),
            get_bugs_clause(bugs),
            get_qa_clause(bugs, no_qa, incremental),
            get_testfix_clause(testfix),
            ])

+def get_testfix_clause(testfix=False):
+ """Get the testfix clause."""
+ testfix_clause = ''
+ if testfix: testfix_clause = '[testfix]'
+ return testfix_clause

You may be taking my example a bit too literally. :-) The "if" should work, but I don't think our style guide allows it. Also, for this situation it's recommended to put the alternative in the "else" rather than unconditionally at the top:

    if testfix:
        testfix_clause = '[testfix']
    else:
        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]'
+ if no_qa: qa_clause = '[no-qa]'

Similar here, with the added note that our style guide requires and "else" for every "elif," so:

    if incremental:
        qa_clause = '[incr]'
    elif no_qa:
        qa_clause = '[no-qa]'
    else:
        qa_clause = ''

Apart from that, great work. I'd be really happy if you could ensure (reproducibly) that the PQM regexes will accept the output; apart from that and these tiny fixes, you're good to go.

review: Approve (code)
Revision history for this message
Māris Fogels (mars) wrote :

We discussed this on voice, and I made a few suggestions for the code. The next step is Ursula's choice.

review: Abstain
Revision history for this message
Ursula Junque (ursinha) wrote :

This branch has conflicts I couldn't solve without creating another branch and applying the same changes. The new mp for the fixed branch is in https://code.edge.launchpad.net/~ursinha/launchpad/add-ec2land-rules-orphaned-branches-no-conflicts/+merge/31386.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/devscripts/autoland.py'
2--- lib/devscripts/autoland.py 2010-03-05 21:47:29 +0000
3+++ lib/devscripts/autoland.py 2010-07-29 22:42:47 +0000
4@@ -12,6 +12,14 @@
5 """Raised when we try to get a review message without enough reviewers."""
6
7
8+class MissingBugsError(Exception):
9+ """Merge proposal has no linked bugs and no [no-qa] tag."""
10+
11+
12+class MissingBugsIncrementalError(Exception):
13+ """Merge proposal has the [incr] tag but no linked bugs."""
14+
15+
16 class LaunchpadBranchLander:
17
18 name = 'launchpad-branch-lander'
19@@ -152,19 +160,54 @@
20 # URL. Do it ourselves.
21 return URI(scheme='bzr+ssh', host=host, path='/' + branch.unique_name)
22
23- def get_commit_message(self, commit_text, testfix=False):
24+ def get_commit_message(self, commit_text, testfix=False, no_qa=False,
25+ incremental=False):
26 """Get the Launchpad-style commit message for a merge proposal."""
27 reviews = self.get_reviews()
28 bugs = self.get_bugs()
29- if testfix:
30- testfix = '[testfix]'
31- else:
32- testfix = ''
33- return '%s%s%s %s' % (
34- testfix,
35+
36+ tags = ''.join([
37+ get_testfix_clause(testfix),
38 get_reviewer_clause(reviews),
39 get_bugs_clause(bugs),
40- commit_text)
41+ get_qa_clause(bugs, no_qa,
42+ incremental),
43+ ])
44+
45+ return '%s %s' % (tags, commit_text)
46+
47+
48+def get_testfix_clause(testfix=False):
49+ """Get the testfix clause."""
50+ if testfix:
51+ testfix_clause = '[testfix]'
52+ else:
53+ testfix_clause = ''
54+ return testfix_clause
55+
56+
57+def get_qa_clause(bugs, no_qa=False, incremental=False):
58+ """Check the no-qa and incremental options, getting the qa clause.
59+
60+ The qa clause will always be or no-qa, or incremental or no tags, never both at
61+ the same time.
62+ """
63+ qa_clause = ""
64+
65+ if not bugs and not no_qa and not incremental:
66+ raise MissingBugsError
67+
68+ if incremental and not bugs:
69+ raise MissingBugsIncrementalError
70+
71+ if incremental:
72+ qa_clause = '[incr]'
73+ elif no_qa:
74+ qa_clause = '[no-qa]'
75+ else:
76+ qa_clause = ''
77+
78+ return qa_clause
79
80
81 def get_email(person):
82
83=== modified file 'lib/devscripts/ec2test/builtins.py'
84--- lib/devscripts/ec2test/builtins.py 2010-04-13 19:32:04 +0000
85+++ lib/devscripts/ec2test/builtins.py 2010-07-29 22:42:47 +0000
86@@ -321,7 +321,13 @@
87 Option('print-commit', help="Print the full commit message."),
88 Option(
89 'testfix',
90- help="Include the [testfix] prefix in the commit message."),
91+ help="This is a testfix (tags commit with [testfix])."),
92+ Option(
93+ 'no-qa',
94+ help="Does not require QA (tags commit with [no-qa])."),
95+ Option(
96+ 'incremental',
97+ help="Incremental to other bug fix (tags commit with [incr])."),
98 Option(
99 'commit-text', short_name='s', type=str,
100 help=(
101@@ -355,10 +361,12 @@
102 def run(self, merge_proposal=None, machine=None,
103 instance_type=DEFAULT_INSTANCE_TYPE, postmortem=False,
104 debug=False, commit_text=None, dry_run=False, testfix=False,
105- print_commit=False, force=False, attached=False):
106+ no_qa=False, incremental=False, print_commit=False, force=False,
107+ attached=False):
108 try:
109 from devscripts.autoland import (
110- LaunchpadBranchLander, MissingReviewError)
111+ LaunchpadBranchLander, MissingReviewError, MissingBugsError,
112+ MissingBugsIncrementalError)
113 except ImportError:
114 self.outf.write(
115 "***************************************************\n\n"
116@@ -374,6 +382,9 @@
117 if print_commit and dry_run:
118 raise BzrCommandError(
119 "Cannot specify --print-commit and --dry-run.")
120+ if no_qa and incremental:
121+ raise BzrCommandError(
122+ "--no-qa and --incremental cannot be given at the same time.")
123 lander = LaunchpadBranchLander.load()
124
125 if merge_proposal is None:
126@@ -396,12 +407,23 @@
127 "Commit text not specified. Use --commit-text, or specify a "
128 "message on the merge proposal.")
129 try:
130- commit_message = mp.get_commit_message(commit_text, testfix)
131+ commit_message = mp.get_commit_message(
132+ commit_text, testfix, no_qa, incremental)
133 except MissingReviewError:
134 raise BzrCommandError(
135 "Cannot land branches that haven't got approved code "
136 "reviews. Get an 'Approved' vote so we can fill in the "
137 "[r=REVIEWER] section.")
138+ except MissingBugsError:
139+ raise BzrCommandError(
140+ "Branch doesn't have linked bugs and doesn't have no-qa "
141+ "option set. Use --no-qa, or link the related bugs to the "
142+ "branch.")
143+ except MissingBugsIncrementalError:
144+ raise BzrCommandError(
145+ "--incremental option requires bugs linked to the branch. "
146+ "Link the bugs or remove the --incremental option.")
147+
148 if print_commit:
149 print commit_message
150 return
151
152=== modified file 'lib/devscripts/tests/test_autoland.py'
153--- lib/devscripts/tests/test_autoland.py 2009-10-08 23:22:16 +0000
154+++ lib/devscripts/tests/test_autoland.py 2010-07-29 22:42:47 +0000
155@@ -6,12 +6,17 @@
156 __metaclass__ = type
157
158 import unittest
159+import re
160
161 from launchpadlib.launchpad import EDGE_SERVICE_ROOT, STAGING_SERVICE_ROOT
162
163+from lp.testing.fakemethod import FakeMethod
164+
165 from devscripts.autoland import (
166 get_bazaar_host, get_bugs_clause, get_reviewer_clause,
167- get_reviewer_handle, MissingReviewError)
168+ get_reviewer_handle, get_qa_clause, get_testfix_clause,
169+ MissingReviewError, MissingBugsError, MissingBugsIncrementalError,
170+ MergeProposal)
171
172
173 class FakeBug:
174@@ -46,6 +51,64 @@
175 self.network = network
176
177
178+class FakeLPMergeProposal:
179+ """Fake launchpadlib MergeProposal object.
180+
181+ Only used for the purposes of testing.
182+ """
183+
184+ def __init__(self, root=None):
185+ self._root = root
186+
187+
188+class TestPQMRegexAcceptance(unittest.TestCase):
189+ """Tests if the generated commit message is accepted by PQM regexes."""
190+
191+ def setUp(self):
192+ # PQM regexes; might need update once in a while
193+ self.devel_open_re = ("(?is)^\s*(:?\[testfix\])?\[(?:"
194+ "release-critical=[^\]]+|rs?=[^\]]+)\]\[ui=(?:.+)\]")
195+ self.dbdevel_normal_re = ("(?is)^\s*(:?\[testfix\])?\[(?:"
196+ "release-critical|rs?=[^\]]+)\]")
197+
198+ self.mp = MergeProposal(FakeLPMergeProposal())
199+ self.fake_bug = FakeBug(20)
200+ self.fake_person = FakePerson('foo', [])
201+ self.mp.get_bugs = FakeMethod([self.fake_bug])
202+ self.mp.get_reviews = FakeMethod({None : [self.fake_person]})
203+
204+ def assertRegexpMatches(self, text, expected_regexp, msg=None):
205+ """Fail the test unless the text matches the regular expression.
206+
207+ Method default in Python 2.7. Can be removed as soon as LP goes 2.7.
208+ """
209+ if isinstance(expected_regexp, basestring):
210+ expected_regexp = re.compile(expected_regexp)
211+ if not expected_regexp.search(text):
212+ msg = msg or "Regexp didn't match"
213+ msg = '%s: %r not found in %r' % (msg, expected_regexp.pattern,
214+ text)
215+ raise self.failureException(msg)
216+
217+ def _test_commit_message_match(self, incr, no_qa, testfix):
218+ commit_message = self.mp.get_commit_message("Foobaring the sbrubble.",
219+ testfix, no_qa, incr)
220+ self.assertRegexpMatches(commit_message, self.devel_open_re)
221+ self.assertRegexpMatches(commit_message, self.dbdevel_normal_re)
222+
223+ def test_testfix_match(self):
224+ self._test_commit_message_match(incr=False, no_qa=False, testfix=True)
225+
226+ def test_regular_match(self):
227+ self._test_commit_message_match(incr=False, no_qa=False, testfix=False)
228+
229+ def test_noqa_match(self):
230+ self._test_commit_message_match(incr=False, no_qa=True, testfix=False)
231+
232+ def test_incr_match(self):
233+ self._test_commit_message_match(incr=True, no_qa=False, testfix=False)
234+
235+
236 class TestBugsClaused(unittest.TestCase):
237 """Tests for `get_bugs_clause`."""
238
239@@ -68,6 +131,149 @@
240 self.assertEqual('[bug=20,45]', bugs_clause)
241
242
243+class TestGetCommitMessage(unittest.TestCase):
244+
245+ def setUp(self):
246+ self.mp = MergeProposal(FakeLPMergeProposal())
247+ self.fake_bug = FakeBug(20)
248+ self.fake_person = self.makePerson('foo')
249+
250+ def makePerson(self, name):
251+ return FakePerson(name, [])
252+
253+ def test_commit_with_bugs(self):
254+ incr = False
255+ no_qa = False
256+ testfix = False
257+
258+ self.mp.get_bugs = FakeMethod([self.fake_bug])
259+ self.mp.get_reviews = FakeMethod({None : [self.fake_person]})
260+
261+ self.assertEqual("[r=foo][ui=none][bug=20] Foobaring the sbrubble.",
262+ self.mp.get_commit_message("Foobaring the sbrubble.",
263+ testfix, no_qa, incr))
264+
265+ def test_commit_no_bugs_no_noqa(self):
266+ incr = False
267+ no_qa = False
268+ testfix = False
269+
270+ self.mp.get_bugs = FakeMethod([])
271+ self.mp.get_reviews = FakeMethod({None : [self.fake_person]})
272+
273+ self.assertRaises(MissingBugsError, self.mp.get_commit_message,
274+ testfix, no_qa, incr)
275+
276+ def test_commit_no_bugs_with_noqa(self):
277+ incr = False
278+ no_qa = True
279+ testfix = False
280+
281+ self.mp.get_bugs = FakeMethod([])
282+ self.mp.get_reviews = FakeMethod({None : [self.fake_person]})
283+
284+ self.assertEqual("[r=foo][ui=none][no-qa] Foobaring the sbrubble.",
285+ self.mp.get_commit_message("Foobaring the sbrubble.",
286+ testfix, no_qa, incr))
287+
288+ def test_commit_bugs_with_noqa(self):
289+ incr = False
290+ no_qa = True
291+ testfix = False
292+
293+ self.mp.get_bugs = FakeMethod([self.fake_bug])
294+ self.mp.get_reviews = FakeMethod({None : [self.fake_person]})
295+
296+ self.assertEqual(
297+ "[r=foo][ui=none][bug=20][no-qa] Foobaring the sbrubble.",
298+ self.mp.get_commit_message("Foobaring the sbrubble.",
299+ testfix, no_qa, incr))
300+
301+ def test_commit_bugs_with_incr(self):
302+ incr = True
303+ no_qa = False
304+ testfix = False
305+
306+ self.mp.get_bugs = FakeMethod([self.fake_bug])
307+ self.mp.get_reviews = FakeMethod({None : [self.fake_person]})
308+
309+ self.assertEqual(
310+ "[r=foo][ui=none][bug=20][incr] Foobaring the sbrubble.",
311+ self.mp.get_commit_message("Foobaring the sbrubble.",
312+ testfix, no_qa, incr))
313+
314+ def test_commit_no_bugs_with_incr(self):
315+ incr = True
316+ no_qa = False
317+ testfix = False
318+
319+ self.mp.get_bugs = FakeMethod([self.fake_bug])
320+ self.mp.get_reviews = FakeMethod({None : [self.fake_person]})
321+
322+ self.assertEqual(
323+ "[r=foo][ui=none][bug=20][incr] Foobaring the sbrubble.",
324+ self.mp.get_commit_message("Foobaring the sbrubble.",
325+ testfix, no_qa, incr))
326+
327+
328+class TestGetTestfixClause(unittest.TestCase):
329+ """Tests for `get_testfix_clause`"""
330+
331+ def test_no_testfix(self):
332+ testfix = False
333+ self.assertEqual('', get_testfix_clause(testfix))
334+
335+ def test_is_testfix(self):
336+ testfix = True
337+ self.assertEqual('[testfix]', get_testfix_clause(testfix))
338+
339+
340+class TestGetQaClause(unittest.TestCase):
341+ """Tests for `get_qa_clause`"""
342+
343+ def test_no_bugs_no_option_given(self):
344+ bugs = None
345+ no_qa = False
346+ incr = False
347+ self.assertRaises(MissingBugsError, get_qa_clause, bugs, no_qa,
348+ incr)
349+
350+ def test_bugs_noqa_option_given(self):
351+ bug1 = FakeBug(20)
352+ no_qa = True
353+ incr = False
354+ self.assertEqual('[no-qa]',
355+ get_qa_clause([bug1], no_qa, incr))
356+
357+ def test_no_bugs_noqa_option_given(self):
358+ bugs = None
359+ no_qa = True
360+ incr = False
361+ self.assertEqual('[no-qa]',
362+ get_qa_clause(bugs, no_qa, incr))
363+
364+ def test_bugs_no_option_given(self):
365+ bug1 = FakeBug(20)
366+ no_qa = False
367+ incr = False
368+ self.assertEqual('',
369+ get_qa_clause([bug1], no_qa, incr))
370+
371+ def test_bugs_incr_option_given(self):
372+ bug1 = FakeBug(20)
373+ no_qa = False
374+ incr = True
375+ self.assertEqual('[incr]',
376+ get_qa_clause([bug1], no_qa, incr))
377+
378+ def test_no_bugs_incr_option_given(self):
379+ bugs = None
380+ no_qa = False
381+ incr = True
382+ self.assertRaises(MissingBugsIncrementalError,
383+ get_qa_clause, bugs, no_qa, incr)
384+
385+
386 class TestGetReviewerHandle(unittest.TestCase):
387 """Tests for `get_reviewer_handle`."""
388
389
390=== added directory 'lib/lp/registry/utilities'
391=== added file 'lib/lp/registry/utilities/__init__.py'
392=== renamed file 'lib/lp/services/salesforce/proxy.py' => 'lib/lp/registry/utilities/salesforce.py'
393--- lib/lp/services/salesforce/proxy.py 2010-07-07 19:41:07 +0000
394+++ lib/lp/registry/utilities/salesforce.py 2010-07-29 22:42:47 +0000
395@@ -1,4 +1,4 @@
396-# Copyright 2009-2010 Canonical Ltd. This software is licensed under the
397+# Copyright 2009 Canonical Ltd. This software is licensed under the
398 # GNU Affero General Public License version 3 (see the file LICENSE).
399
400 """Utilities for accessing the external Salesforce proxy."""
401@@ -20,7 +20,7 @@
402 from canonical.cachedproperty import cachedproperty
403 from canonical.config import config
404 from lp.registry.interfaces.product import IProductSet
405-from lp.services.salesforce.interfaces import (
406+from lp.registry.interfaces.salesforce import (
407 ISalesforceVoucher, ISalesforceVoucherProxy, SFDCError,
408 SVPAlreadyRedeemedException, SVPNotAllowedException, SVPNotFoundException,
409 SalesforceVoucherProxyException)
410@@ -110,9 +110,6 @@
411 """See `ISalesforceVoucherProxy`."""
412 identifier = self._getUserIdentitifier(user)
413 vouchers = self.server.getUnredeemedVouchers(identifier)
414- # Force the return value to be a list of dicts.
415- if isinstance(vouchers, dict):
416- vouchers = [vouchers]
417 return [Voucher(voucher) for voucher in vouchers]
418
419 @fault_mapper
420@@ -120,9 +117,6 @@
421 """See `ISalesforceVoucherProxy`."""
422 identifier = self._getUserIdentitifier(user)
423 vouchers = self.server.getAllVouchers(identifier)
424- # Force the return value to be a list of dicts.
425- if isinstance(vouchers, dict):
426- vouchers = [vouchers]
427 return [Voucher(voucher) for voucher in vouchers]
428
429 @fault_mapper
430@@ -146,7 +140,7 @@
431 status = self.server.redeemVoucher(voucher_id,
432 identifier,
433 project.id,
434- project.displayname)
435+ project.name)
436 return status
437
438 @fault_mapper