Code review comment for lp:~lifeless/launchpad/milestones

Hi Rob,

Couple of nitpicks, but otherwise okay. Marking it needs fixing because
of the over-long test (see below) but if we can't split that up I'm
happy for it to land as-is.

> === modified file 'lib/lp/bugs/tests/test_bugtask.py'
> --- lib/lp/bugs/tests/test_bugtask.py 2010-07-14 14:11:15 +0000
> +++ lib/lp/bugs/tests/test_bugtask.py 2010-08-17 09:41:19 +0000
> @@ -792,6 +796,28 @@
> result = target.searchTasks(None, modified_since=date)
> self.assertEqual([task2], list(result))
>
> + def test_private_bug_view_permissions_cached(self):
> + """private bugs from a search know the user can see the bugs."""

Captialise at the start of the sentence ;).

> + target = self.makeBugTarget()
> + person = self.login()
> + self.factory.makeBug(product=target, private=True, owner=person)
> + self.factory.makeBug(product=target, private=True, owner=person)
> + # Search style and parameters taken from the milestone index view where
> + # the issue was discovered.
> + login_person(person)
> + tasks =target.searchTasks(BugTaskSearchParams(person, omit_dupes=True,

Needs a space after the =.

> + orderby=['status', '-importance', 'id']))

> === modified file 'lib/lp/registry/browser/tests/test_milestone.py'
> --- lib/lp/registry/browser/tests/test_milestone.py 2010-08-16 18:39:11 +0000
> +++ lib/lp/registry/browser/tests/test_milestone.py 2010-08-17 09:41:19 +0000
> @@ -105,5 +116,61 @@
> self.assertEqual(0, product.development_focus.all_bugtasks.count())
>
>
> -def test_suite():
> - return unittest.TestLoader().loadTestsFromName(__name__)
> +class TestMilestoneIndex(TestCaseWithFactory):
> +
> + layer = DatabaseFunctionalLayer
> +
> + def test_more_private_bugs_query_count_is_constant(self):

The test should have a comment at the start of it to save me having to
parse the method name.

> + product = self.factory.makeProduct()
> + login_person(product.owner)
> + milestone = self.factory.makeMilestone(
> + productseries=product.development_focus)
> + bug1 = self.factory.makeBug(product=product, private=True,
> + owner=product.owner)
> + bug1.bugtasks[0].transitionToMilestone(milestone, product.owner)
> + # We look at the page as someone who is a member of a team and the team
> + # is subscribed to the bugs, so that we don't get trivial shortcuts
> + # avoiding queries : test the worst case.
> + subscribed_team = self.factory.makeTeam()
> + viewer = self.factory.makePerson(password="test")
> + with person_logged_in(subscribed_team.teamowner):
> + subscribed_team.addMember(viewer, subscribed_team.teamowner)
> + bug1.subscribe(subscribed_team, product.owner)
> + bug1_url = canonical_url(bug1)
> + milestone_url = canonical_url(milestone)
> + browser = self.getUserBrowser(user=viewer)
> + # Seed the cookie cache and any other cross-request state we may gain
> + # in future. See canonical.launchpad.webapp.serssion: _get_secret.
> + browser.open(milestone_url)
> + collector = QueryCollector()
> + collector.register()
> + self.addCleanup(collector.unregister)
> + # This page is rather high in queries : 46 as a baseline. 20100817
> + page_query_limit = 46
> + browser.open(milestone_url)
> + # Check that the test found the bug
> + self.assertTrue(bug1_url in browser.contents)
> + self.assertThat(collector, HasQueryCount(LessThan(page_query_limit)))
> + with_1_private_bug = collector.count
> + with_1_queries = ["%s: %s" % (pos, stmt[3]) for (pos, stmt) in
> + enumerate(collector.queries)]
> + login_person(product.owner)
> + bug2 = self.factory.makeBug(product=product, private=True,
> + owner=product.owner)
> + bug2.bugtasks[0].transitionToMilestone(milestone, product.owner)
> + bug2.subscribe(subscribed_team, product.owner)
> + bug2_url = canonical_url(bug2)
> + bug3 = self.factory.makeBug(product=product, private=True,
> + owner=product.owner)
> + bug3.bugtasks[0].transitionToMilestone(milestone, product.owner)
> + bug3.subscribe(subscribed_team, product.owner)
> + logout()
> + browser.open(milestone_url)
> + self.assertTrue(bug2_url in browser.contents)
> + self.assertThat(collector, HasQueryCount(LessThan(page_query_limit)))
> + with_3_private_bugs = collector.count
> + with_3_queries = ["%s: %s" % (pos, stmt[3]) for (pos, stmt) in
> + enumerate(collector.queries)]
> + self.assertEqual(with_1_private_bug, with_3_private_bugs,
> + "different query count: \n%s\n******************\n%s\n" % (
> + '\n'.join(with_1_queries), '\n'.join(with_3_queries)))

At 54 lines this test's a bit on the long side, but I'm struggling to
come up with a way of making it shorter that isn't just dividing it into
two around the "# Check that the test found the bug" line. Any ideas?
I'm not sure it's worth worrying about in this case.