Merge lp:~cjwatson/launchpad/refactor-bugtask-tests into lp:launchpad

[Colin Watson]

Refactor several bugtask browser tests to use record_two_runs, improving test isolation.

Refactor several bugtask browser tests to use record_two_runs rather than similar inline code that didn't do as good a job of isolating tests.

I've improved record_two_runs in a couple of ways in the process to make it even more accurate and easy to use. It now clears celebrity descriptors as well, and it gains an optional login_method argument which is useful in browser tests (since setupBrowserForUser calls logout).

[William Grant (wgrant)]

review: Approve (code)

=== modified file 'lib/lp/app/interfaces/launchpad.py'
--- lib/lp/app/interfaces/launchpad.py	2014-11-28 22:28:40 +0000
+++ lib/lp/app/interfaces/launchpad.py	2015-06-30 08:37:26 +0000
@@ -73,6 +73,9 @@
         """Return true if there is an IPerson celebrity with the given name.
         """
 
+    def clearCache():
+        """Clear any cached celebrities."""
+
 
 class IServiceUsage(Interface):
     """Pillar service usages."""

=== modified file 'lib/lp/app/utilities/celebrities.py'
--- lib/lp/app/utilities/celebrities.py	2013-05-10 06:28:17 +0000
+++ lib/lp/app/utilities/celebrities.py	2015-06-30 08:37:26 +0000
@@ -176,4 +176,13 @@
         return mirror
 
     def isCelebrityPerson(self, name):
+        """See `ILaunchpadCelebrities`."""
         return str(name) in PersonCelebrityDescriptor.names
+
+    @classmethod
+    def clearCache(cls):
+        """See `ILaunchpadCelebrities`."""
+        for name in cls.__dict__:
+            desc = getattr(cls, name)
+            if isinstance(desc, CelebrityDescriptor):
+                desc.id = None

=== modified file 'lib/lp/bugs/browser/tests/test_bugtask.py'
--- lib/lp/bugs/browser/tests/test_bugtask.py	2015-06-25 07:39:40 +0000
+++ lib/lp/bugs/browser/tests/test_bugtask.py	2015-06-30 08:37:26 +0000
@@ -17,8 +17,8 @@
 from pytz import UTC
 import simplejson
 import soupmatchers
-from storm.store import Store
 from testtools.matchers import (
+    Equals,
     LessThan,
     Not,
     )
@@ -80,9 +80,9 @@
     login,
     login_person,
     person_logged_in,
+    record_two_runs,
     TestCaseWithFactory,
     )
-from lp.testing._webservice import QueryCollector
 from lp.testing.layers import (
     DatabaseFunctionalLayer,
     LaunchpadFunctionalLayer,
@@ -115,65 +115,29 @@
 
     layer = LaunchpadFunctionalLayer
 
-    def invalidate_caches(self, obj):
-        store = Store.of(obj)
-        # Make sure everything is in the database.
-        store.flush()
-        # And invalidate the cache (not a reset, because that stops us using
-        # the domain objects)
-        store.invalidate()
-
     def test_rendered_query_counts_constant_with_team_memberships(self):
-        login(ADMIN_EMAIL)
         task = self.factory.makeBugTask()
-        person_no_teams = self.factory.makePerson()
-        person_with_teams = self.factory.makePerson()
-        for _ in range(10):
-            self.factory.makeTeam(members=[person_with_teams])
-        # count with no teams
+        person = self.factory.makePerson()
         url = canonical_url(task)
-        recorder = QueryCollector()
-        recorder.register()
-        self.addCleanup(recorder.unregister)
-        self.invalidate_caches(task)
-        self.getUserBrowser(url, person_no_teams)
+        recorder1, recorder2 = record_two_runs(
+            lambda: self.getUserBrowser(url, person),
+            lambda: self.factory.makeTeam(members=[person]),
+            0, 10, login_method=lambda: login(ADMIN_EMAIL))
         # This may seem large: it is; there is easily another 25% fat in
         # there.
-        # If this test is run in isolation, the query count is 80.
-        # Other tests in this TestCase could cache the
-        # "SELECT id, product, project, distribution FROM PillarName ..."
-        # query by previously browsing the task url, in which case the
-        # query count is decreased by one.
-        self.assertThat(recorder, HasQueryCount(LessThan(83)))
-        count_with_no_teams = recorder.count
-        # count with many teams
-        self.invalidate_caches(task)
-        self.getUserBrowser(url, person_with_teams)
-        # Allow an increase of one because storm bug 619017 causes additional
-        # queries, revalidating things unnecessarily. An increase which is
-        # less than the number of new teams shows it is definitely not
-        # growing per-team.
-        self.assertThat(recorder, HasQueryCount(
-            LessThan(count_with_no_teams + 3),
-            ))
+        self.assertThat(recorder1, HasQueryCount(LessThan(81)))
+        self.assertThat(recorder2, HasQueryCount(Equals(recorder1.count)))
 
     def test_rendered_query_counts_constant_with_attachments(self):
-        with celebrity_logged_in('admin'):
-            browses_under_limit = BrowsesWithQueryLimit(
-                85, self.factory.makePerson())
-
-            # First test with a single attachment.
-            task = self.factory.makeBugTask()
-            self.factory.makeBugAttachment(bug=task.bug)
-        self.assertThat(task, browses_under_limit)
-
-        with celebrity_logged_in('admin'):
-            # And now with 10.
-            task = self.factory.makeBugTask()
-            self.factory.makeBugTask(bug=task.bug)
-            for i in range(10):
-                self.factory.makeBugAttachment(bug=task.bug)
-        self.assertThat(task, browses_under_limit)
+        task = self.factory.makeBugTask()
+        person = self.factory.makePerson()
+        url = canonical_url(task)
+        recorder1, recorder2 = record_two_runs(
+            lambda: self.getUserBrowser(url, person),
+            lambda: self.factory.makeBugAttachment(bug=task.bug),
+            1, 9, login_method=lambda: login(ADMIN_EMAIL))
+        self.assertThat(recorder1, HasQueryCount(LessThan(82)))
+        self.assertThat(recorder2, HasQueryCount(Equals(recorder1.count)))
 
     def makeLinkedBranchMergeProposal(self, sourcepackage, bug, owner):
         with person_logged_in(owner):
@@ -199,22 +163,19 @@
             self.factory.makeBugTask(bug=bug, owner=owner, target=sp)
         task = bug.default_bugtask
         url = canonical_url(task)
-        recorder = QueryCollector()
-        recorder.register()
-        self.addCleanup(recorder.unregister)
-        self.invalidate_caches(task)
-        self.getUserBrowser(url, owner)
-        # At least 20 of these should be removed.
-        self.assertThat(recorder, HasQueryCount(LessThan(114)))
-        count_with_no_branches = recorder.count
-        for sp in sourcepackages:
-            self.makeLinkedBranchMergeProposal(sp, bug, owner)
-        self.invalidate_caches(task)
-        self.getUserBrowser(url, owner)  # This triggers the query recorder.
+
+        def make_merge_proposals():
+            for sp in sourcepackages:
+                self.makeLinkedBranchMergeProposal(sp, bug, owner)
+
+        recorder1, recorder2 = record_two_runs(
+            lambda: self.getUserBrowser(url, owner),
+            make_merge_proposals, 0, 1)
+        self.assertThat(recorder1, HasQueryCount(LessThan(87)))
         # Ideally this should be much fewer, but this tries to keep a win of
         # removing more than half of these.
         self.assertThat(
-            recorder, HasQueryCount(LessThan(count_with_no_branches + 46)))
+            recorder2, HasQueryCount(LessThan(recorder1.count + 41)))
 
     def test_interesting_activity(self):
         # The interesting_activity property returns a tuple of interesting
@@ -245,27 +206,19 @@
     def test_rendered_query_counts_constant_with_activities(self):
         # More queries are not used for extra bug activities.
         task = self.factory.makeBugTask()
+        person = self.factory.makePerson()
+        url = canonical_url(task)
 
         def add_activity(what, who):
             getUtility(IBugActivitySet).new(
                 task.bug, datetime.now(UTC), who, whatchanged=what)
 
-        # Render the view with one activity.
-        with celebrity_logged_in('admin'):
-            browses_under_limit = BrowsesWithQueryLimit(
-                83, self.factory.makePerson())
-            person = self.factory.makePerson()
-            add_activity("description", person)
-
-        self.assertThat(task, browses_under_limit)
-
-        # Render the view with many more activities by different people.
-        with celebrity_logged_in('admin'):
-            for _ in range(20):
-                person = self.factory.makePerson()
-                add_activity("description", person)
-
-        self.assertThat(task, browses_under_limit)
+        recorder1, recorder2 = record_two_runs(
+            lambda: self.getUserBrowser(url, person),
+            lambda: add_activity("description", self.factory.makePerson()),
+            1, 20, login_method=lambda: login(ADMIN_EMAIL))
+        self.assertThat(recorder1, HasQueryCount(LessThan(82)))
+        self.assertThat(recorder2, HasQueryCount(Equals(recorder1.count)))
 
     def test_rendered_query_counts_constant_with_milestones(self):
         # More queries are not used for extra milestones.
@@ -274,7 +227,7 @@
 
         with celebrity_logged_in('admin'):
             browses_under_limit = BrowsesWithQueryLimit(
-                88, self.factory.makePerson())
+                82, self.factory.makePerson())
 
         self.assertThat(bug, browses_under_limit)
 
@@ -2046,29 +1999,15 @@
         view.initialize()
         return view
 
-    def invalidate_caches(self, obj):
-        store = Store.of(obj)
-        store.flush()
-        store.invalidate()
-
     def test_rendered_query_counts_constant_with_many_bugtasks(self):
         product = self.factory.makeProduct()
         url = canonical_url(product, view_name='+bugs')
-        bug = self.factory.makeBug(target=product)
-        buggy_product = self.factory.makeProduct()
-        buggy_url = canonical_url(buggy_product, view_name='+bugs')
-        for _ in range(10):
-            self.factory.makeBug(target=buggy_product)
-        recorder = QueryCollector()
-        recorder.register()
-        self.addCleanup(recorder.unregister)
-        self.invalidate_caches(bug)
-        # count with single task
-        self.getUserBrowser(url)
-        self.assertThat(recorder, HasQueryCount(LessThan(36)))
-        # count with many tasks
-        self.getUserBrowser(buggy_url)
-        self.assertThat(recorder, HasQueryCount(LessThan(36)))
+        recorder1, recorder2 = record_two_runs(
+            lambda: self.getUserBrowser(url),
+            lambda: self.factory.makeBug(target=product),
+            1, 10, login_method=lambda: login(ANONYMOUS))
+        self.assertThat(recorder1, HasQueryCount(LessThan(46)))
+        self.assertThat(recorder2, HasQueryCount(Equals(recorder1.count)))
 
     def test_mustache_model_in_json(self):
         """The IJSONRequestCache should contain mustache_model.

=== modified file 'lib/lp/soyuz/browser/tests/test_archive_packages.py'
--- lib/lp/soyuz/browser/tests/test_archive_packages.py	2013-05-10 06:44:11 +0000
+++ lib/lp/soyuz/browser/tests/test_archive_packages.py	2015-06-30 08:37:26 +0000
@@ -447,6 +447,4 @@
                 view.render()
         recorder1, recorder2 = record_two_runs(
             ppa_index_render, self.createPackage, 2, 3)
-
-        self.assertThat(
-            recorder2, HasQueryCount(LessThan(recorder1.count + 2)))
+        self.assertThat(recorder2, HasQueryCount(Equals(recorder1.count)))

=== modified file 'lib/lp/testing/__init__.py'
--- lib/lp/testing/__init__.py	2015-01-13 14:07:42 +0000
+++ lib/lp/testing/__init__.py	2015-06-30 08:37:26 +0000
@@ -125,6 +125,7 @@
     )
 from zope.testing.testrunner.runner import TestResult as ZopeTestResult
 
+from lp.app.interfaces.launchpad import ILaunchpadCelebrities
 from lp.app.interfaces.security import IAuthorization
 from lp.codehosting.vfs import (
     branch_id_to_path,
@@ -374,16 +375,21 @@
 
 
 def record_two_runs(tested_method, item_creator, first_round_number,
-                    second_round_number=None):
+                    second_round_number=None, login_method=None):
     """A helper that returns the two storm statement recorders
     obtained when running tested_method after having run the
     method {item_creator} {first_round_number} times and then
     again after having run the same method {second_round_number}
     times.
 
+    If {login_method} is not None, it is called before each batch of
+    {item_creator} calls.
+
     :return: a tuple containing the two recorders obtained by the successive
         runs.
     """
+    if login_method is not None:
+        login_method()
     for i in range(first_round_number):
         item_creator()
     # Record how many queries are issued when {tested_method} is
@@ -392,17 +398,21 @@
     flush_database_caches()
     if queryInteraction() is not None:
         clear_permission_cache()
+    getUtility(ILaunchpadCelebrities).clearCache()
     with StormStatementRecorder() as recorder1:
         tested_method()
     # Run {item_creator} {second_round_number} more times.
     if second_round_number is None:
         second_round_number = first_round_number
+    if login_method is not None:
+        login_method()
     for i in range(second_round_number):
         item_creator()
     # Record again the number of queries issued.
     flush_database_caches()
     if queryInteraction() is not None:
         clear_permission_cache()
+    getUtility(ILaunchpadCelebrities).clearCache()
     with StormStatementRecorder() as recorder2:
         tested_method()
     return recorder1, recorder2