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 project.all_milestones: You can optimize by fetching only active_milestones. Difference is pretty big, at least for me (3 runs for "landscape" project with 4 active and 138 total milestones): Active: 0:00:00.448329 All: 0:00:03.164043 Active: 0:00:00.397278 All: 0:00:03.176703 Active: 0:00:00.308273 All: 0:00:03.583689 > + if default is not None: > + if milestone.name == default: > + milestones.append(milestone) > + return milestones > + else: > + continue This logic is a bit weird (got me confused for a bit): it would be clearer if you separated the "if default" branch from the other one since you are re-evaluating "default" in each iteration. Then you can also get the best of both worlds: if default is not None: for milestone in project.ALL_milestones: if milestone.name == default: return [milestone] self.logger.warning(...) return list(project.ACTIVE_milestones) (uppercased for emphasis, both ALL and ACTIVE should be lowercase). > + if not milestone.is_active: > + continue > + milestones.append(milestone) > + if default is not None and not len(milestones): > + self.logger.warning("Default Milestone not found: %s" % default) Are we sure we want to allow inactive milestones as well? Does Launchpad allow setting a bugtask target to one? If not, we should switch the above to never using ALL_milestones. One can also optimize fetching the default milestone by hand-crafting the URL and using Launchpad.load() directly. > + def _find_target_milestone(self, project, now): ... > + milestones = self._find_milestones(project) > + if len(milestones) == 0: > + return None > + > + # Purpose here is to Move all milestones with no date to the end of the I guess you originally started the sentence with "Move": it shouldn't be capitalized anymore :) > + # list in lexical order. > + date_milestones = [] > + name_milestones = [] > + for milestone in milestones: > + if milestone.date_targeted is None: > + name_milestones.append(milestone) > + else: > + date_milestones.append(milestone) > + milestones = sorted(date_milestones, key=lambda x: x.date_targeted) > + milestones.extend(sorted(name_milestones, key=lambda x: x.name)) > + > + previous_milestone = milestones[0] > + if previous_milestone.date_targeted is None or \ > + now < previous_milestone.date_targeted: > + return previous_milestone > + for milestone in milestones: > + if now > previous_milestone.date_targeted: > + if milestone.date_targeted is None or \ > + now < milestone.date_targeted: > + return milestone > + previous_milestone = milestone > + return milestones[-1] This can be optimized to be done with a single pass through the list of milestones (instead of 2 passes plus 1 sorting pass). Probably not relevant for the sizes we'd be dealing with here, so you may not want to bother with this: def _find_target_milestone(self, project, now): milestones = self._find_milestones(project) earliest_after = latest_before = untargeted = None for milestone in milestones: if milestone.date_targeted is None: if untargeted is not None: if milestone.name < untargeted.name: untargeted = milestone else: untargeted = milestone elif milestone.date_targeted > now: if earliest_after is not None: if earliest_after.date_targeted > milestone.date_targeted: earliest_after = milestone else: earliest_after = milestone elif milestone.date_targeted < now: if latest_before is not None: if latest_before.date_targeted < milestone.date_targeted: latest_before = milestone else: latest_before = milestone if earliest_after is not None: return earliest_after elif untargeted is not None: return untargeted else: return latest_before This avoids any special casing for len()==0 or 1 as well, and I had it passing all the tests as well. It can surely be made shorter, but I think this strikes a good balance between readability (maintainability), terseness and performance. > === modified file 'tarmac/plugins/tests/test_bugresolver.py' > @@ -28,6 +30,25 @@ > super(BugResolverTests, self).setUp() > self.proposal = Thing() > self.plugin = BugResolver() > + self.plugin.config = { > + "set_milestone": "False", > + "default_milestone": None} > + self.milestone0 = Thing(name="0", is_active=True, date_targeted=None) > + self.milestone1 = Thing( > + name="1", is_active=True, > + date_targeted=datetime.utcnow() - timedelta(weeks=2)) > + self.milestone2 = Thing( > + name="2", is_active=True, > + date_targeted=datetime.utcnow() + timedelta(weeks=2)) > + self.milestone3 = Thing( > + name="3", is_active=True, > + date_targeted=datetime.utcnow() + timedelta(weeks=6)) > + self.milestone4 = Thing(name="4", is_active=True, date_targeted=None) > + self.milestone5 = Thing(name="5", is_active=True, date_targeted=None) > + self.milestone6 = Thing( > + name="6", is_active=False, > + date_targeted=datetime.utcnow() + timedelta(weeks=2), > + bug=Thing(id=12345), bug_target_name="foo_project") Naming milestones like this makes it much harder to understand the tests later on. If milestones were named something like milestone_untargeted_a, milestone_2_weeks_ago, milestone_2_weeks_ahead, milestone_6_weeks_ahead, milestone_untargeted_b, milestone_untargeted_c, milestone_2_weeks_ahead_inactive the tests would read much better. You could go for a bit more semantic like "near_future" and "far_future", or "latest" (6 weeks ahead) and "earliest" (2 weeks ago) to make sure that whoever modifies them understands what tests expect of them. > @@ -40,12 +61,28 @@ ... > + self.projects[0].all_milestones = self.milestones > + self.projects[1].all_milestones = [] Similar holds here: project_with_milestones and project_without_milestones are clearer, imho at least. If you go for active_milestones in the implementation instead (which I believe you should), you'll have to change this as well. After looking at the tests though, I believe you should define milestones and projects in tests themselves: you only ever use one or two of the milestones, and it's always best to keep the relevant data close at hand. People do have differing opinions on these things, so I emphasise that this is nothing more than mine. :) > @@ -71,6 +108,21 @@ ... > + def test_run_with_set_milestone(self): > + """Test plug-in with the set_milestone setting runs correctly.""" > + target = Thing(fixed_bugs=self.bugs.keys(), > + lp_branch=Thing(project=self.projects[0], > + bzr_identity='lp:target'), > + config=Thing(set_milestone="true")) > + all_bugs = self.bugs > + launchpad = Thing(bugs=all_bugs) I guess you should directly reference self.bugs in this call and get rid of the all_bugs temp variable (since you are not using it after). > @@ -109,3 +161,89 @@ ... > + def test__find_target_milestone_older(self): Any reason you are using double underscore after "test"? Rest of the code seems to be using "test_...". > + """Test that dates before milestones return the oldest.""" I'd add "oldest in the future" or "first in the future" since "oldest" had me stumped for a moment. > + milestone = self.plugin._find_target_milestone( > + self.projects[0], > + self.milestone1.date_targeted - timedelta(weeks=1)) > + self.assertEqual(milestone, self.milestone1) As I said, it would read much better if it was now = self.milestone_2_weeks_ago.date_targeted - timedelta(weeks=1) and then assertEqual(milestone, self.milestone_2_weeks_ago) If you go for semantic naming, it'd make sense to use small timedeltas as well (1 second is enough). > + def test__find_target_milestone_newer_no_expected_date(self): > + """Test that dates after milestones return the least sorted no-expected-date.""" > + self.projects[0].all_milestones = self.milestones_extended This is more of a clue that you should be setting up milestones and projects in tests themselves. And you don't need all you've got in the setUp, you just need the minimum to test the behaviour you want. For instance, here I can see three different tests for untargeted milestones: 1. Test lexical sorting milestone1 = Thing(name='b', date_targeted = None) milestone2 = Thing(name='a', date_targeted = None) assert that milestone2 is selected 2. Test that date_targeted is considered the future compared to past milestones milestone_past.date_targeted = -1 week milestone_untargeted.date_targeted = None assert that milestone_untargeted is selected 3. Test that date_targeted is ignored if there are milestones for the future milestone_future.date_targeted = +1 week milestone_untargeted.date_targeted = None assert that milestone_future is selected > + def test__find_target_milestone_with_default(self): > + """Test that specifying a default gets a specific milestone.""" > + self.projects[0].all_milestones = self.milestones_extended > + self.plugin.config["default_milestone"] = "6" This is interesting: you actually want to set inactive milestones as well? Are you sure Launchpad will let you do that (I know you can't do it from the UI, but maybe API is more relaxed)? If so, ignore my comment from earlier. > + milestone = self.plugin._find_target_milestone( > + self.projects[0], self.milestone3.date_targeted + timedelta(weeks=1)) > + self.assertEqual(milestone, self.milestone6) > + self.plugin.config["default_milestone"] = "5" > + milestone = self.plugin._find_target_milestone( > + self.projects[0], self.milestone3.date_targeted + timedelta(weeks=1)) > + self.assertEqual(milestone, self.milestone5) Any reason you are doing the same check twice? I.e. what would make the first test with default_milestone set to "6" pass and not with "5"? If there are actual reasons, it'd be good to split them into separate tests and explain a bit better what corner cases are we testing. > + def test__find_milestone(self): > + """Test that given a project, the list of milestones is returned.""" > + self.plugin.logger.warning = MagicMock() > + milestones = self.plugin._find_milestones(self.projects[0]) > + self.assertEqual(len(milestones), 3) > + milestones = self.plugin._find_milestones(self.projects[1]) > + self.assertEqual(len(milestones), 0) This should be two tests: one for defined milestones, and one for no-milestones. > + # Search for a specific milestone that isn't there (this triggers the > + # warning log) > + self.plugin.config["default_milestone"] = "FOO" > + milestones = self.plugin._find_milestones(self.projects[0]) > + self.assertEqual(len(milestones), 0) This should be a second test eg. "find_milestone_default_not_found". > + # Add in the milestones with no dates, and make sure I can search > + # for something specific. > + self.projects[0].all_milestones = self.milestones_extended > + self.plugin.config["default_milestone"] = "5" > + milestones = self.plugin._find_milestones(self.projects[0]) > + self.assertEqual(len(milestones), 1) > + self.assertEqual(milestones[0], self.milestone5) > + > + # Make sure warning log was only called once. > + self.assertEqual(self.plugin.logger.warning.call_count, 1) This should be a third test. It'd also be good to switch to milestone "4" (iow, not the first in the list just to make sure that it's not broken in such a way to return the first in the list). > + def test__get_and_parse_config(self): > + """Test config parsing.""" > + config = self.plugin._get_and_parse_config( > + Thing(config=Thing(set_milestone="True"))) > + self.assertEqual(config["set_milestone"], True) > + > + config = self.plugin._get_and_parse_config( > + Thing(config=Thing(set_milestone="1"))) > + self.assertEqual(config["set_milestone"], True) > + > + config = self.plugin._get_and_parse_config( > + Thing(config=Thing(set_milestone="true"))) > + self.assertEqual(config["set_milestone"], True) > + > + config = self.plugin._get_and_parse_config( > + Thing(config=Thing(default_milestone="A"))) > + self.assertEqual(config["default_milestone"], "A") > + > + config = self.plugin._get_and_parse_config( > + Thing(config=Thing(default_milestone=""))) > + self.assertEqual(config["default_milestone"], None) It'd be good to split these up into separate tests too. The trouble with tests testing multiple things is that when something breaks in an environment you don't control, you'll only get a report of the first test failure. You fix that one, and the next one pops up only when whoever reported it tries it out again. Also, you are not testing the defaults at all. It'd be good to have a test for that as well (i.e. pass a config object with none of them defined and make sure that both set_milestone and default_milestone are set to False and None respectively).