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: > > + if milestone.name == default: > > + milestones.append(milestone) > > + return milestones > > + else: > > + continue > > This logic is a bit weird (got me confused for a bit): [...] Fixed as suggested (more or less). The weird logic was to find active_milestones and specific active or inactive ones in the same loop, which I'm no longer doing. yay! > > + 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. Fixed... Using active_milestones throughout now. > > One can also optimize fetching the default milestone by hand-crafting > the URL and using Launchpad.load() directly. I didn't take this one since the list of active milestones is so small. Even if it was 10,000, that takes just a fraction of a second. If you push, then I will change it of course. ;) > I guess you originally started the sentence with "Move": it shouldn't be > capitalized anymore :) FiXEd. > 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: 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. :) > > 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. ... Finally these points won me over (the special cases) and the fact you find it more readable. So, I'm fine with that. Whatever the case, I really appreciate your writing that all out. It helped your case that I could just cut-and-paste. LOL > > 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 Agreed. I followed the pattern in the file of using numbers and indexes, it was hard to follow. I fixed it with the more semantic approach, thanks for the suggestion. > > @@ -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. :) So, I didn't do this. I tend to agree with you to keep the data isolated with the test case, however the test suite was already written with a notion of a setUp that would define some datastructures in setUp that represent a mocked out launchpad dataset (bugs, projects, tasks, states). I tried to follow that by just adding milestones into that. Secondly, I think in this case, it helps to have some set datastructures so I don't have to define what a "project" is at the start of each test case. Since each test is interacting with a fake launchpad, I kind of like that it's all waiting there to test, pretty cheaply. But, I can see both sides. In the end, I didn't want to change all the other units tests that already make use of this testing model. > > + 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). Fixed. > > > @@ -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_...". Since I was testing the _find_target method (leading underscore). I don't see that exclusively used in tarmac, but it is in there. > > > + """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. I changed the statement. It was written from the absolute perspective, not the perspective of the date being tested. Hopefully it's a bit more clear now. I agree that date language gets to be very cumbersome. > > > + 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) Yup, fixed. > > + 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. Sure, I see the merit here, and I think there is perhaps an unbalance in the setUp, but I also like DRY in not setting up datastructures over and over. I've cleaned things up a bit, let me know what you think. > > 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 I belive these three cases are covered. Since I changed the code to use active_milestones, that helped straighted a couple test cases out. > > > + 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. I removed this use case (setting an inactive milestone), thanks for pointing out the futility. > > > + 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. One was for active, one inactive. Removed. > > > + 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. Fixed. > > > + # 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". Fixed. > > > + # 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). Fixed. > > > + 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. Fixed. > > 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). Added a test for defaults, Thanks. -- David Britton