Code review comment for lp://qastaging/~dpb/tarmac/set-milestone-2

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"

« Back to merge proposal