Merge lp:~dpb/tarmac/set-milestone-2 into lp:tarmac

Proposed by David Britton
Status: Merged
Approved by: dobey
Approved revision: 441
Merged at revision: 432
Proposed branch: lp:~dpb/tarmac/set-milestone-2
Merge into: lp:tarmac
Diff against target: 477 lines (+381/-3)
5 files modified
docs/introduction.txt (+16/-0)
tarmac/plugins/__init__.py (+18/-0)
tarmac/plugins/bugresolver.py (+106/-0)
tarmac/plugins/tests/test_bugresolver.py (+196/-3)
tarmac/plugins/tests/test_tarmac_plugin.py (+45/-0)
To merge this branch: bzr merge lp:~dpb/tarmac/set-milestone-2
Reviewer Review Type Date Requested Status
dobey Approve
Chad Smith Approve
Данило Шеган (community) Approve
Review via email: mp+210888@code.launchpad.net

Commit message

Add an option to bugresolver to set the milestone if not already set.

Description of the change

When resolving a bug, set the milestone if not already set.

Milestone is selected using the following algorithm:

1) nearest milestone with target date in the future
2) if no milestones are in the future, sort remaining open milestones lexically by name and pick the first
3) If all milestones have dates and we are past all dates, pick the newest of those.
4) override all that logic with a config setting of "default_milestone" which is the specific milestone name to use (which will even select a non-active milestone).

This behavior only engages if the config setting 'set_milestone = True" is present in your config file.

You can test with: trial -j4 tarmac (or ./run-tests)

Read "Hacking" for a small list of deps needed for testing.

To post a comment you must log in.
Revision history for this message
dobey (dobey) wrote :

Can you please file bugs for new features, and link them? You'll also need to set the commit message.

review: Needs Fixing
Revision history for this message
David Britton (dpb) wrote :

On Thu, Mar 13, 2014 at 07:42:35PM -0000, Rodney Dawes wrote:
> Review: Needs Fixing
>
> Can you please file bugs for new features, and link them? You'll also need to set the commit message.

Yes, but we want to do an internal review first, so I hadn't done
everything yet. Please feel free to ignore until we get there.

--
David Britton <email address hidden>

Revision history for this message
Данило Шеган (danilo) wrote :
Download full text (17.0 KiB)

Hi David,

Thanks for taking on this. The branch looks pretty good, though, with
tarmac not being code-base I am familiar with, I'll try to infer some of
the policies from the surrounding code and make a few suggestions along
the way (mostly for code structure and tests).

The review is going to be long, but mostly mechanical/cosmetical stuff.

I simply can't restrain myself suggesting a more optimal approach
sometimes, even though it probably makes no difference for small numbers
of milestones most everybody is going to have. That means that you need
not follow up on any of them :)

  review needs-fixing

Cheers,
Danilo

> === modified file 'docs/introduction.txt'
> @@ -263,6 +263,22 @@
...
> + # the one that sorts least amoung them the target. If this
algorithm

s/amoung/among/

> === modified file 'tarmac/plugins/__init__.py'
> + def get_config(self, attribute, default, *args):
...
> + if hasattr(obj, "config"):
> + if hasattr(obj.config, attribute):

You can simplify this as

            if hasattr(obj, "config") and hasattr(obj.config,
attribute):

to save one indentation level (Python will not evaluate right side of
"and" when the left one evaluates to False). Not a big deal here, but
can be handy when your ifs start getting deeply nested.

I am not sure this method really belongs as-is in the TarmacPlugin
either: it seems to be very generic (eg. no mention of "self" in the
body, and it's not used anywhere inside TarmacPlugin methods). At the
very least, it should be a function.

I don't see unit tests for this method either (and they should be pretty
simple to write).

> === modified file 'tarmac/plugins/bugresolver.py'
> + def _get_and_parse_config(self, *args):
> + """
> + Retrieve and parse configuration settings for this plugin.
> + Return as a dict structure.
> + """
> + set_milestone = self.get_config("set_milestone", "False",
*args)
> + if set_milestone.lower() == "true" or set_milestone == "1":
> + set_milestone = True
> + else:
> + set_milestone = False
> +
> + default = self.get_config("default_milestone", None, *args)
> + if default is not None and not len(default):
> + default = None

Why check for len() here? I assume you wanted default == "", so I
suggest you be explicit. It'd also make the check simpler since it'd be
just

  if default == "":
      default = None

> + def _set_milestone_on_task(self, project, task):
...

I know you are testing this with test_run_with_set_milestone() but it'd
be good to have a unit test or two for this as well. I can see a few
cases here:

 - config option not set, no-op on task milestone
 - milestone already set, no-op on task milestone, warning logged
 - config option set, milestone is being set, action logged

> + def _find_milestones(self, project):
> + """
> + Return list of milestones in a project. Filter list by
active status.
> + If the config `default_milestone` is set filter by that
instead.
> + """
> + default = self.config["default_milestone"]
> + milestones = []
> + for milestone in pro...

review: Needs Fixing
Revision history for this message
David Britton (dpb) wrote :
Download full text (15.1 KiB)

On Wed, Mar 19, 2014 at 05:09:17PM -0000, Данило Шеган wrote:
> Review: Needs Fixing

Wow, thanks for the Review Danilo -- very helpful. I implemented most
of your suggestions, but left a couple on the table. I'll note below...

Please re-review when you get a chance.

> s/amoung/among/

Fixed.

>
> > === modified file 'tarmac/plugins/__init__.py'
> > + def get_config(self, attribute, default, *args):
> ...
> > + if hasattr(obj, "config"):
> > + if hasattr(obj.config, attribute):
>
> You can simplify this as
>
> if hasattr(obj, "config") and hasattr(obj.config, attribute):

Fixed.

>
> I am not sure this method really belongs as-is in the TarmacPlugin
> either: it seems to be very generic (eg. no mention of "self" in the
> body, and it's not used anywhere inside TarmacPlugin methods). At the
> very least, it should be a function.

Thanks -- I changed it to a @staticmethod. I'm willing to move it, but
I can't really find a good place. I saw this type of logic repeated in
many plugins, so I figured the plugins base class would be a good place
to start. I guess it can always be moved in the future. Let me know...

>
> I don't see unit tests for this method either (and they should be pretty
> simple to write).

Fixed.

>
> > === modified file 'tarmac/plugins/bugresolver.py'
> > + def _get_and_parse_config(self, *args):
> > + """
> > + Retrieve and parse configuration settings for this plugin.
> > + Return as a dict structure.
> > + """
> > + set_milestone = self.get_config("set_milestone", "False", *args)
> > + if set_milestone.lower() == "true" or set_milestone == "1":
> > + set_milestone = True
> > + else:
> > + set_milestone = False
> > +
> > + default = self.get_config("default_milestone", None, *args)
> > + if default is not None and not len(default):
> > + default = None
>
> Why check for len() here? I assume you wanted default == "", so I
> suggest you be explicit. It'd also make the check simpler since it'd be
> just
>
> if default == "":
> default = None

Fixed.

>
> > + def _set_milestone_on_task(self, project, task):
> ...
>
> I know you are testing this with test_run_with_set_milestone() but it'd
> be good to have a unit test or two for this as well. I can see a few
> cases here:
>
> - config option not set, no-op on task milestone
> - milestone already set, no-op on task milestone, warning logged
> - config option set, milestone is being set, action logged

Good catch. Fixed.

>
> > + def _find_milestones(self, project):
> > + """
> > + Return list of milestones in a project. Filter list by
> active status.
> > + If the config `default_milestone` is set filter by that
> instead.
> > + """
> > + default = self.config["default_milestone"]
> > + milestones = []
> > + for milestone in project.all_milestones:
>
> You can optimize by fetching only active_milestones.

Fixed. I didn't know about active_milestones. Indeed, that simplified
much of the code.

>
> > + if default is not None:
> > + ...

Revision history for this message
Данило Шеган (danilo) wrote :

Hi David,

Thanks for bearing with me. Most everything looks good, but you named your new test file

  test_tarmac_pluign.py

The rest is really good now, thanks.

  review approve

> OK, I took your suggestion. Though I still like the previous one as I
> found it easier to follow. I was curious, so I timed some stuff:
>
> 1000 milestones, 10,000 iterstions:
>
> old: 46.1424999237
> new: 44.1123301983
>
> 1,000,000 milestones, 10 iterations:
>
> old: 57.2593920231
> new: 49.6416790485
>
> As expected, n log n nature of the sort is slowing things down when large
> amounts of records come into play. And... I think this is quickly
> entering a theoretical discussion. :)

Heh, indeed. Thanks for testing it out: the difference might rather amount to
Python memory management (you are still dealing with duplicated structures in
the original variant) since the loop is much more complicated (and thus
slower) in "my" proposal (and sorted() is heavily optimized in Python).

So, in the end, it was probably not worth it even for large sets of
milestones, but it was fun nevertheless :)

review: Approve
Revision history for this message
Chad Smith (chad.smith) wrote :

[1] in test_bugresolver setUp:
  Not that it'll take a long time to run setUp, but we should move self.now to the top and use self.now when creating all the self.milestone_* attributes so they are all based on the same now.
 247 + self.now = datetime.utcnow()

[2]test_run_with_set_milestone docstr is unhelpful with the expectations of what "correctly" is. Maybe a bit more to explain why 1 milestone is left None, one added and the 3rd left unchanged.
 + """Test plug-in with the set_milestone setting runs correctly."""

[3] test_run_with_set_milestone can use self.assertIsNone()
277 + self.assertEqual(self.bugs['0'].bug_tasks[1].milestone, None)

[4] In test__find_target_milestone_between can we have an assert to prove that the date is between milestone_past and milestone_future? Something like:

now = self.milestone_past.date_targeted + timedelta(weeks=1)
self.assertTrue(now , self.milestone_future)
milestone = self.plugin._find_target_milestone(
     self.projects[1], now)

[5] variable naming for in _get_and_parse_config() clarity default -> default_milestone
96 + default = self.get_config("default_milestone", None, *args)
97 + if default == "":
98 + default = None

[6] docstring fix in _find_target_milestone
            3) the last milestone in the list (covers len()==1 case).
            "the last" -> "the most recent"

Revision history for this message
Chad Smith (chad.smith) wrote :

Ran through a few live trials of this with a development project and was able to validate lexical milestone setting, leaving existing milestones unchanged and assigning active milestones at the closest specific dates. Nice job +1!

review: Approve
Revision history for this message
David Britton (dpb) wrote :

OK, I've deployed to our jenkins environment. We will get some use of it then submit upstream for inclusion.

Revision history for this message
David Britton (dpb) wrote :

We've been running this now for 2 months, it's working well and is an optional feature. Asking for inclusion in upstream.

Revision history for this message
dobey (dobey) wrote :

> We've been running this now for 2 months, it's working well and is an optional
> feature. Asking for inclusion in upstream.

OK. It'll probably be a couple weeks more before I can get to reviewing it, due to sprint and vacation. I'll try to review it as soon as I can though.

Revision history for this message
David Britton (dpb) wrote :

Thanks!

On Fri, May 23, 2014 at 2:26 PM, Rodney Dawes <email address hidden>wrote:

> > We've been running this now for 2 months, it's working well and is an
> optional
> > feature. Asking for inclusion in upstream.
>
> OK. It'll probably be a couple weeks more before I can get to reviewing
> it, due to sprint and vacation. I'll try to review it as soon as I can
> though.
> --
>
> https://code.launchpad.net/~davidpbritton/tarmac/set-milestone-2/+merge/210888
> You are the owner of lp:~davidpbritton/tarmac/set-milestone-2.
>

--
David Britton <email address hidden>

Revision history for this message
dobey (dobey) :
review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'docs/introduction.txt'
2--- docs/introduction.txt 2014-02-09 23:06:33 +0000
3+++ docs/introduction.txt 2014-03-24 21:40:01 +0000
4@@ -263,6 +263,22 @@
5
6 bzr commit --fixes=lp:000000
7
8+This plugin supports options:
9+
10+ [lp:target_branch]
11+ # When setting `Fixed Committed`, set the milestone on the bug to the
12+ # closest upcoming milestone (based on the milestone "expected date")
13+ # If past all milestones, set to the newest. Milestones with no dates
14+ # set are assumed to be in the future and lexically sorted -- making
15+ # the one that sorts least among them the target. If this algorithm
16+ # doesn't work for you, see `default_milestone`.
17+ set_milestone = True
18+
19+ # When setting `Fixed Committed`, set the milestone on the bug to this.
20+ # Requires `set_milestone = True`. If this doesn't match any of your
21+ # active milestone names, a warning message will be printed.
22+ default_milestone = 12.10
23+
24
25 ========================
26 Installing other plugins
27
28=== modified file 'tarmac/plugins/__init__.py'
29--- tarmac/plugins/__init__.py 2010-07-18 16:01:17 +0000
30+++ tarmac/plugins/__init__.py 2014-03-24 21:40:01 +0000
31@@ -29,3 +29,21 @@
32
33 def run(self, *args, **kwargs):
34 '''Run the hook.'''
35+
36+ @staticmethod
37+ def get_config(attribute, default, *args):
38+ '''
39+ Lookup configuration and return to caller.
40+ Return value of `default` if not found. Typically you would call
41+ in the following manner:
42+
43+ value = get_config("my_config_setting", None, command, target)
44+
45+ @param attribute: The attribute to lookup.
46+ @param default: The default value to use if nothing found.
47+ @param *args: objects to probe looking for the requested attribute.
48+ '''
49+ for obj in args:
50+ if hasattr(obj, "config") and hasattr(obj.config, attribute):
51+ return getattr(obj.config, attribute)
52+ return default
53
54=== modified file 'tarmac/plugins/bugresolver.py'
55--- tarmac/plugins/bugresolver.py 2010-10-05 18:12:42 +0000
56+++ tarmac/plugins/bugresolver.py 2014-03-24 21:40:01 +0000
57@@ -16,6 +16,7 @@
58
59 from tarmac.hooks import tarmac_hooks
60 from tarmac.plugins import TarmacPlugin
61+from datetime import datetime
62
63
64 class BugResolver(TarmacPlugin):
65@@ -27,6 +28,9 @@
66 if not fixed_bugs:
67 return
68
69+ # Retrieve configuration
70+ self.config = self._get_and_parse_config(target, command)
71+
72 project = target.lp_branch.project
73 try:
74 series_name = target.lp_branch.bzr_identity.split('/')[1]
75@@ -53,10 +57,112 @@
76
77 if task:
78 task.status = u'Fix Committed'
79+ self._set_milestone_on_task(project, task)
80 task.lp_save()
81 else:
82 self.logger.info('Target %s/%s not found in bug #%s.',
83 project.name, series.name, bug_id)
84
85+ def _get_and_parse_config(self, *args):
86+ """
87+ Retrieve and parse configuration settings for this plugin.
88+ Return as a dict structure.
89+ """
90+ set_milestone = self.get_config("set_milestone", "False", *args)
91+ if set_milestone.lower() == "true" or set_milestone == "1":
92+ set_milestone = True
93+ else:
94+ set_milestone = False
95+
96+ default_milestone = self.get_config("default_milestone", None, *args)
97+ if default_milestone == "":
98+ default_milestone = None
99+
100+ return {
101+ "set_milestone": set_milestone,
102+ "default_milestone": default_milestone}
103+
104+ def _set_milestone_on_task(self, project, task):
105+ """
106+ Attempt to auto-determine the milestone to set, and set the milestone
107+ of the given task. If the task already has a milestone set => noop.
108+ Only processed if config setting `set_milestone` == True.
109+ """
110+ if not self.config["set_milestone"]:
111+ return
112+ task_milestone = task.milestone
113+ if task_milestone is not None:
114+ self.logger.info(
115+ "Task: %s/%s already has milestone: %s set, skipping" % (
116+ task.bug.id, task.bug_target_name, task_milestone.name))
117+ return
118+ now = datetime.utcnow()
119+ target_milestone = self._find_target_milestone(project, now)
120+ self.logger.info("%s/%s: Setting Milestone: %s",
121+ task.bug.id, task.bug_target_name, target_milestone)
122+ task.milestone = target_milestone
123+
124+ def _find_milestones(self, project):
125+ """
126+ Return list of active milestones in a project. If the config
127+ `default_milestone` is set filter by that. If not found, an
128+ empty list will be returned.
129+ """
130+ default = self.config["default_milestone"]
131+ if default is None:
132+ return list(project.active_milestones)
133+ for milestone in project.active_milestones:
134+ if milestone.name == default:
135+ return [milestone]
136+ self.logger.warning("Default Milestone not found: %s" % default)
137+ return []
138+
139+ def _find_target_milestone(self, project, now):
140+ """
141+ Find a target milestone when resolving a bug task.
142+
143+ Compare the selected datetime `now` to the list of milestones.
144+ Return the milestone where `targeted_date` is newer than the given
145+ datetime. If the given time is greater than all open milestones:
146+ target to the newest milestone in the list.
147+
148+ In this algorithm, milestones without targeted dates appear lexically
149+ sorted at the end of the list. So the lowest sorting one will get
150+ chosen if all milestones with dates attached are exhausted.
151+
152+ In other words, pick one of the milestones for the target. Preference:
153+ 1) closest milestone (by date) in the future
154+ 2) least lexically sorting milestone (by name)
155+ 3) the last milestone in the list (covers len()==1 case).
156+ """
157+ milestones = self._find_milestones(project)
158+ earliest_after = latest_before = untargeted = None
159+ for milestone in milestones:
160+ if milestone.date_targeted is None:
161+ if untargeted is not None:
162+ if milestone.name < untargeted.name:
163+ untargeted = milestone
164+ else:
165+ untargeted = milestone
166+ elif milestone.date_targeted > now:
167+ if earliest_after is not None:
168+ if earliest_after.date_targeted > milestone.date_targeted:
169+ earliest_after = milestone
170+ else:
171+ earliest_after = milestone
172+ elif milestone.date_targeted < now:
173+ if latest_before is not None:
174+ if latest_before.date_targeted < milestone.date_targeted:
175+ latest_before = milestone
176+ else:
177+ latest_before = milestone
178+
179+ if earliest_after is not None:
180+ return earliest_after
181+ elif untargeted is not None:
182+ return untargeted
183+ else:
184+ return latest_before
185+
186
187 tarmac_hooks['tarmac_post_commit'].hook(BugResolver(), 'Bug resolver')
188
189=== modified file 'tarmac/plugins/tests/test_bugresolver.py'
190--- tarmac/plugins/tests/test_bugresolver.py 2013-10-31 19:09:08 +0000
191+++ tarmac/plugins/tests/test_bugresolver.py 2014-03-24 21:40:01 +0000
192@@ -18,6 +18,8 @@
193 from tarmac.plugins.bugresolver import BugResolver
194 from tarmac.tests import TarmacTestCase
195 from tarmac.tests import Thing
196+from datetime import datetime, timedelta
197+from mock import MagicMock
198
199
200 class BugResolverTests(TarmacTestCase):
201@@ -26,8 +28,28 @@
202 def setUp(self):
203 """Set up data for the tests."""
204 super(BugResolverTests, self).setUp()
205+ self.now = datetime.utcnow()
206 self.proposal = Thing()
207 self.plugin = BugResolver()
208+ self.plugin.config = {
209+ "set_milestone": "False",
210+ "default_milestone": None}
211+ self.milestone_untargeted_a = Thing(name="a", date_targeted=None)
212+ self.milestone_past = Thing(
213+ name="past",
214+ date_targeted=self.now - timedelta(weeks=2))
215+ self.milestone_future = Thing(
216+ name="future",
217+ date_targeted=self.now + timedelta(weeks=2))
218+ self.milestone_far_future = Thing(
219+ name="far_future",
220+ date_targeted=self.now + timedelta(weeks=6))
221+ self.milestone_untargeted_b = Thing(name="b", date_targeted=None)
222+ self.milestone_untargeted_c = Thing(name="c", date_targeted=None)
223+ self.milestone_with_bug = Thing(
224+ name="with_bug",
225+ date_targeted=self.now - timedelta(weeks=3),
226+ bug=Thing(id=12345), bug_target_name="foo_project")
227 self.series = [Thing(name='trunk'),
228 Thing(name='stable')]
229 self.projects = [Thing(name='target',
230@@ -40,12 +62,29 @@
231 self.targets[0] = self.projects[0]
232 self.bugs = {'0': Thing(
233 bug_tasks=[Thing(target=self.targets[0], status=u'In Progress',
234- lp_save=self.lp_save),
235+ lp_save=self.lp_save, milestone=None,
236+ bug=Thing(id="0"),
237+ bug_target_name=self.targets[0].name),
238 Thing(target=self.targets[2], status=u'Incomplete',
239- lp_save=self.lp_save)]),
240+ lp_save=self.lp_save, milestone=None,
241+ bug=Thing(id="0"),
242+ bug_target_name=self.targets[2].name)]),
243 '1': Thing(
244 bug_tasks=[Thing(target=self.targets[1], status=u'Confirmed',
245- lp_save=self.lp_save)])}
246+ lp_save=self.lp_save,
247+ milestone=self.milestone_with_bug,
248+ bug=Thing(id="1"),
249+ bug_target_name=self.targets[2].name)])}
250+ # Insert out of order to make sure they sort correctly.
251+ self.milestones = [
252+ self.milestone_far_future, self.milestone_with_bug,
253+ self.milestone_past, self.milestone_future]
254+ self.milestones_extended = [
255+ self.milestone_untargeted_c, self.milestone_untargeted_b,
256+ self.milestone_untargeted_a]
257+ self.milestones_extended.extend(self.milestones)
258+ self.projects[0].active_milestones = self.milestones
259+ self.projects[1].active_milestones = self.milestones_extended
260
261 def getSeries(self, name=None):
262 """Faux getSeries for testing."""
263@@ -71,6 +110,27 @@
264 self.assertEqual(self.bugs['0'].bug_tasks[1].status, u'Incomplete')
265 self.assertEqual(self.bugs['1'].bug_tasks[0].status, u'Confirmed')
266
267+ def test_run_with_set_milestone(self):
268+ """
269+ Test plug-in with the set_milestone config setting = true. Will
270+ auto-resolve milestone using algorithm in find_target_milestone
271+ and set. Test that bug0/task0 and bug1/task0 get the correct
272+ milestone set. bug0/task1 is incomplete so should not be touched.
273+ """
274+ target = Thing(fixed_bugs=self.bugs.keys(),
275+ lp_branch=Thing(project=self.projects[0],
276+ bzr_identity='lp:target'),
277+ config=Thing(set_milestone="true"))
278+ launchpad = Thing(bugs=self.bugs)
279+ command = Thing(launchpad=launchpad)
280+ self.plugin.run(command=command, target=target, source=None,
281+ proposal=self.proposal)
282+ self.assertEqual(self.bugs['0'].bug_tasks[0].milestone,
283+ self.milestone_future)
284+ self.assertEqual(self.bugs['1'].bug_tasks[0].milestone,
285+ self.milestone_with_bug)
286+ self.assertIsNone(self.bugs['0'].bug_tasks[1].milestone)
287+
288 def test_run_with_no_bugs(self):
289 """Test that bug resolution for no bugs does nothing."""
290 target = Thing(fixed_bugs=None,
291@@ -109,3 +169,136 @@
292 self.assertEqual(self.bugs['0'].bug_tasks[0].status, u'In Progress')
293 self.assertEqual(self.bugs['0'].bug_tasks[1].status, u'Incomplete')
294 self.assertEqual(self.bugs['1'].bug_tasks[0].status, u'Confirmed')
295+
296+ def test__find_target_milestone_older(self):
297+ """Dates before all milestones return the oldest milestone."""
298+ milestone = self.plugin._find_target_milestone(
299+ self.projects[0],
300+ self.milestone_past.date_targeted - timedelta(weeks=1))
301+ self.assertEqual(milestone, self.milestone_past)
302+
303+ def test__find_target_milestone_between(self):
304+ """Test that dates between milestones return the closest newest."""
305+ milestone = self.plugin._find_target_milestone(
306+ self.projects[1],
307+ self.milestone_past.date_targeted + timedelta(weeks=1))
308+ self.assertTrue(self.now < self.milestone_future.date_targeted)
309+ self.assertTrue(self.now > self.milestone_past.date_targeted)
310+ self.assertEqual(milestone, self.milestone_future)
311+
312+ def test__find_target_milestone_newer(self):
313+ """Test that dates after all milestones return the newest."""
314+ milestone = self.plugin._find_target_milestone(
315+ self.projects[0],
316+ self.milestone_far_future.date_targeted + timedelta(weeks=1))
317+ self.assertEqual(milestone, self.milestone_far_future)
318+
319+ def test__find_target_milestone_lexical_sort_past_dates(self):
320+ """Dates after milestones return the least sorted no-expected-date."""
321+ milestone = self.plugin._find_target_milestone(
322+ self.projects[1],
323+ self.milestone_far_future.date_targeted + timedelta(weeks=1))
324+ self.assertEqual(milestone, self.milestone_untargeted_a)
325+
326+ def test__find_target_milestone_with_default(self):
327+ """Test that specifying a default gets a specific milestone."""
328+ self.plugin.config["default_milestone"] = "c"
329+ milestone = self.plugin._find_target_milestone(
330+ self.projects[1],
331+ self.milestone_far_future.date_targeted + timedelta(weeks=1))
332+ self.assertEqual(milestone, self.milestone_untargeted_c)
333+
334+ def test__find_milestone_positive(self):
335+ """Given a project, the list of milestones is returned."""
336+ milestones = self.plugin._find_milestones(self.projects[1])
337+ self.assertEqual(len(milestones), 7)
338+
339+ def test__find_milestone_negative(self):
340+ """Given a project with no milestones, _find_milestone handles it"""
341+ milestones = self.plugin._find_milestones(Thing(active_milestones=[]))
342+ self.assertEqual(len(milestones), 0)
343+
344+ def test__find_milestone_specific_negative(self):
345+ """Find a secific milestone that isn't there, check for log"""
346+ self.plugin.logger.warning = MagicMock()
347+ self.plugin.config["default_milestone"] = "FOO"
348+ milestones = self.plugin._find_milestones(self.projects[0])
349+ self.assertEqual(len(milestones), 0)
350+ self.assertEqual(self.plugin.logger.warning.call_count, 1)
351+
352+ def test__find_milestone_no_dates(self):
353+ """Find a specific milestones without a targeted date"""
354+ self.plugin.config["default_milestone"] = "b"
355+ milestones = self.plugin._find_milestones(self.projects[1])
356+ self.assertEqual(len(milestones), 1)
357+ self.assertEqual(milestones[0], self.milestone_untargeted_b)
358+
359+ def test__get_and_parse_config_set_milestone_true_upper(self):
360+ """Test config parsing - set_milestone: True."""
361+ config = self.plugin._get_and_parse_config(
362+ Thing(config=Thing(set_milestone="True")))
363+ self.assertEqual(config["set_milestone"], True)
364+
365+ def test__get_and_parse_config_set_milestone_one(self):
366+ """Test config parsing - set_milestone: 1."""
367+ config = self.plugin._get_and_parse_config(
368+ Thing(config=Thing(set_milestone="1")))
369+ self.assertEqual(config["set_milestone"], True)
370+
371+ def test__get_and_parse_config_set_milestone_true_lower(self):
372+ """Test config parsing - set_milestone: true."""
373+ config = self.plugin._get_and_parse_config(
374+ Thing(config=Thing(set_milestone="true")))
375+ self.assertEqual(config["set_milestone"], True)
376+
377+ def test__get_and_parse_config_default_milestone_A(self):
378+ """Test config parsing - default_milestone: A."""
379+ config = self.plugin._get_and_parse_config(
380+ Thing(config=Thing(default_milestone="A")))
381+ self.assertEqual(config["default_milestone"], "A")
382+
383+ def test__get_and_parse_config_default_milestone_none(self):
384+ """Test config parsing - default_milestone: ."""
385+ config = self.plugin._get_and_parse_config(
386+ Thing(config=Thing(default_milestone="")))
387+ self.assertEqual(config["default_milestone"], None)
388+
389+ def test__get_and_parse_config_default_milestone_default(self):
390+ """Test config parsing - defaults."""
391+ config = self.plugin._get_and_parse_config(
392+ Thing(config=Thing()))
393+ self.assertEqual(config["set_milestone"], False)
394+ self.assertEqual(config["default_milestone"], None)
395+
396+ def test__set_milestone_on_task_config_not_set(self):
397+ """config option not set, no-op"""
398+ self.plugin.logger.info = MagicMock()
399+ self.plugin.logger.warning = MagicMock()
400+ self.plugin.config = {
401+ "set_milestone": False, "default_milestone": None}
402+ self.plugin._set_milestone_on_task(
403+ self.projects[0], self.bugs['0'].bug_tasks[0])
404+ self.assertEqual(self.bugs['0'].bug_tasks[0].milestone, None)
405+ self.assertEqual(self.plugin.logger.info.call_count, 0)
406+ self.assertEqual(self.plugin.logger.warning.call_count, 0)
407+
408+ def test__set_milestone_on_task_milestone_already_set(self):
409+ """milestone is already set, should leave task untouched"""
410+ self.plugin.logger.info = MagicMock()
411+ self.plugin.config = {"set_milestone": True, "default_milestone": "past"}
412+ self.plugin._set_milestone_on_task(
413+ self.projects[0], self.bugs['1'].bug_tasks[0])
414+ self.assertEqual(
415+ self.bugs['1'].bug_tasks[0].milestone, self.milestone_with_bug)
416+ self.assertIn("already has milestone",
417+ self.plugin.logger.info.call_args[0][0])
418+
419+ def test__set_milestone_on_task_config_set(self):
420+ """config option set, milestone is being set, action logged"""
421+ self.plugin.logger.info = MagicMock()
422+ self.plugin.config = {"set_milestone": True, "default_milestone": None}
423+ self.plugin._set_milestone_on_task(
424+ self.projects[0], self.bugs['0'].bug_tasks[0])
425+ self.assertEqual(
426+ self.bugs['0'].bug_tasks[0].milestone, self.milestone_future)
427+ self.assertEqual(self.plugin.logger.info.call_count, 1)
428
429=== added file 'tarmac/plugins/tests/test_tarmac_plugin.py'
430--- tarmac/plugins/tests/test_tarmac_plugin.py 1970-01-01 00:00:00 +0000
431+++ tarmac/plugins/tests/test_tarmac_plugin.py 2014-03-24 21:40:01 +0000
432@@ -0,0 +1,45 @@
433+# Copyright 2014 Canonical, Ltd.
434+#
435+# This file is part of Tarmac.
436+#
437+# Tarmac is free software: you can redistribute it and/or modify
438+# it under the terms of the GNU General Public License version 3 as
439+# published by the Free Software Foundation.
440+#
441+# Tarmac is distributed in the hope that it will be useful,
442+# but WITHOUT ANY WARRANTY; without even the implied warranty of
443+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
444+# GNU General Public License for more details.
445+#
446+# You should have received a copy of the GNU General Public License
447+# along with Tarmac. If not, see <http://www.gnu.org/licenses/>.
448+"""Tests for base/abstract TarmacPlugin class."""
449+
450+from tarmac.plugins import TarmacPlugin
451+from tarmac.tests import Thing, TarmacTestCase
452+
453+class TestTarmacPlugin(TarmacTestCase):
454+ """Test the tarmacplugin abstract class."""
455+
456+ def test_get_config_missing(self):
457+ """get_config returns default values."""
458+ value = TarmacPlugin.get_config("foo", "default", Thing())
459+ self.assertEqual(value, "default")
460+ value = TarmacPlugin.get_config("foo", None, Thing(config=None))
461+ self.assertEqual(value, None)
462+
463+ def test_get_config_basic(self):
464+ """get_config returns values."""
465+ value = TarmacPlugin.get_config(
466+ "foo", "default", Thing(config=Thing(foo="bar")))
467+ self.assertEqual(value, "bar")
468+
469+ def test_get_config_multiple(self):
470+ """get_config returns values from multiple objects."""
471+ value = TarmacPlugin.get_config(
472+ "foo", "default",
473+ Thing(config=Thing(bar="bar")),
474+ Thing(config=Thing(foo="first")),
475+ Thing(config=Thing(foo="second")))
476+ self.assertEqual(value, "first")
477+

Subscribers

People subscribed via source and target branches