launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #18860
[Merge] lp:~cjwatson/launchpad/refactor-bugtask-tests into lp:launchpad
Colin Watson has proposed merging lp:~cjwatson/launchpad/refactor-bugtask-tests into lp:launchpad.
Commit message:
Refactor several bugtask browser tests to use record_two_runs, improving test isolation.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/refactor-bugtask-tests/+merge/263310
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).
The query count in test_rendered_query_counts_constant_with_many_bugtasks will be wrong until Kit lands his branch to stop exporting Product.inferred_vcs.
--
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/refactor-bugtask-tests into lp:launchpad.
=== 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 01:34:41 +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 01:34:41 +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 01:34:41 +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 01:34:41 +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 01:34:41 +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
Follow ups
