launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #01056
[Merge] lp:~wallyworld/launchpad/consolidate-factory-makeSeries into lp:launchpad/devel
Ian Booth has proposed merging lp:~wallyworld/launchpad/consolidate-factory-makeSeries into lp:launchpad/devel with lp:~wallyworld/launchpad/tales-linkify-broken-links as a prerequisite.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers): code
When coding the changes for branch "tales-linkify-broken-links", it was noticed that there were 2 similar factory methods to create a test product series: lp.testing.factory.makeSeries and lp.testing.factory.makeProductSeries
This branch consolidates the two methods into one: makeProductSeries. There were over 200 uses of makeProductSeries so it was easier to keep it and remove the makeSeries method.
Implementation
Only unit test and doc test changes were required. The makeSeries() method was slightly different to the
makeProductSeries() method in that it returned a naked series and did a syncUpdate(). However,
removeSecurityProxy() was used if required in the calling code and removing syncUpdate() seemed to make
no difference to the tests passing. In fact, the (obsolete) makeSeries() factory method was the only one which
used syncUpdate(). Perhaps this was why a separate method was created? But the doc string make no mention of
it and the tests pass so it looks ok to remove it.
Tests
bin/test -vvt productrelease-views.txt
bin/test -vvt browser-views.txt
bin/test -vvt test_autoapproval
bin/test -vvt test_branch
bin/test -vvt test_branchlookup
bin/test -vvt test_milestone
Lint
Lint is clean. It initially wasn't but the issues were there already so I made some drive-by fixes.
--
https://code.launchpad.net/~wallyworld/launchpad/consolidate-factory-makeSeries/+merge/35632
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wallyworld/launchpad/consolidate-factory-makeSeries into lp:launchpad/devel.
=== modified file 'lib/lp/code/model/tests/test_branch.py'
--- lib/lp/code/model/tests/test_branch.py 2010-09-09 02:49:47 +0000
+++ lib/lp/code/model/tests/test_branch.py 2010-09-16 05:08:47 +0000
@@ -1385,7 +1385,7 @@
def test_branchWithSeriesRequirements(self):
"""Deletion requirements for a series' branch are right."""
- series = self.factory.makeSeries(branch=self.branch)
+ series = self.factory.makeProductSeries(branch=self.branch)
self.assertEqual(
{series: ('alter',
_('This series is linked to this branch.'))},
@@ -1393,8 +1393,8 @@
def test_branchWithSeriesDeletion(self):
"""break_links allows deleting a series' branch."""
- series1 = self.factory.makeSeries(branch=self.branch)
- series2 = self.factory.makeSeries(branch=self.branch)
+ series1 = self.factory.makeProductSeries(branch=self.branch)
+ series2 = self.factory.makeProductSeries(branch=self.branch)
self.branch.destroySelf(break_references=True)
self.assertEqual(None, series1.branch)
self.assertEqual(None, series2.branch)
@@ -1489,7 +1489,7 @@
def test_ClearSeriesBranch(self):
"""ClearSeriesBranch.__call__ must clear the user branch."""
- series = removeSecurityProxy(self.factory.makeSeries(
+ series = removeSecurityProxy(self.factory.makeProductSeries(
branch=self.branch))
ClearSeriesBranch(series, self.branch)()
self.assertEqual(None, series.branch)
=== modified file 'lib/lp/code/model/tests/test_branchlookup.py'
--- lib/lp/code/model/tests/test_branchlookup.py 2010-08-20 20:31:18 +0000
+++ lib/lp/code/model/tests/test_branchlookup.py 2010-09-16 05:08:47 +0000
@@ -87,17 +87,20 @@
def test_junk(self):
branch = self.factory.makePersonalBranch()
- result = self.branch_set.getIdAndTrailingPath('/' + branch.unique_name)
+ result = self.branch_set.getIdAndTrailingPath(
+ '/' + branch.unique_name)
self.assertEqual((branch.id, ''), result)
def test_product(self):
branch = self.factory.makeProductBranch()
- result = self.branch_set.getIdAndTrailingPath('/' + branch.unique_name)
+ result = self.branch_set.getIdAndTrailingPath(
+ '/' + branch.unique_name)
self.assertEqual((branch.id, ''), result)
def test_source_package(self):
branch = self.factory.makePackageBranch()
- result = self.branch_set.getIdAndTrailingPath('/' + branch.unique_name)
+ result = self.branch_set.getIdAndTrailingPath(
+ '/' + branch.unique_name)
self.assertEqual((branch.id, ''), result)
def test_trailing_slash(self):
@@ -383,7 +386,7 @@
def test_product_series(self):
# `traverse` resolves the path to a product series to the product
# series itself.
- series = self.factory.makeSeries()
+ series = self.factory.makeProductSeries()
short_name = '%s/%s' % (series.product.name, series.name)
self.assertTraverses(short_name, series)
@@ -504,7 +507,7 @@
# getByLPPath returns the branch and the trailing path (with no
# series) if the given path is inside an existing branch.
branch = self.factory.makeProductBranch()
- path = '%s/foo/bar/baz' % (branch.unique_name,)
+ path = '%s/foo/bar/baz' % (branch.unique_name)
self.assertEqual(
(branch, 'foo/bar/baz'), self.branch_lookup.getByLPPath(path))
@@ -530,7 +533,7 @@
# getByLPPath returns the branch and the trailing path (with no
# series) if the given path is inside an existing branch.
branch = self.factory.makePersonalBranch()
- path = '%s/foo/bar/baz' % (branch.unique_name,)
+ path = '%s/foo/bar/baz' % (branch.unique_name)
self.assertEqual(
(branch, 'foo/bar/baz'),
self.branch_lookup.getByLPPath(path))
@@ -556,7 +559,7 @@
def test_no_product_series_branch(self):
# getByLPPath raises `NoLinkedBranch` if there's no branch registered
# linked to the requested series.
- series = self.factory.makeSeries()
+ series = self.factory.makeProductSeries()
short_name = '%s/%s' % (series.product.name, series.name)
exception = self.assertRaises(
NoLinkedBranch, self.branch_lookup.getByLPPath, short_name)
@@ -640,9 +643,8 @@
# linked branch but is followed by extra path segments, then we return
# the linked branch but chop off the extra segments. We might want to
# change this behaviour in future.
- series = self.factory.makeSeries()
- branch = self.factory.makeProductBranch(series.product)
- series.branch = branch
+ branch= self.factory.makeBranch()
+ series = self.factory.makeProductSeries(branch=branch)
result = self.branch_lookup.getByLPPath(
'%s/%s/other/bits' % (series.product.name, series.name))
self.assertEqual((branch, u'other/bits'), result)
=== modified file 'lib/lp/code/xmlrpc/tests/test_branch.py'
--- lib/lp/code/xmlrpc/tests/test_branch.py 2010-09-07 05:39:39 +0000
+++ lib/lp/code/xmlrpc/tests/test_branch.py 2010-09-16 05:08:47 +0000
@@ -139,8 +139,9 @@
# and to the branch associated with the product series 'series' on
# 'product'.
product = self.factory.makeProduct()
- branch = branch=self.factory.makeProductBranch(product=product)
- series = self.factory.makeSeries(product=product, branch=branch)
+ branch = self.factory.makeProductBranch(product=product)
+ series = self.factory.makeProductSeries(
+ product=product, branch=branch)
lp_path = '%s/%s' % (product.name, series.name)
self.assertResolves(lp_path, branch.unique_name, lp_path)
@@ -151,7 +152,7 @@
def test_series_has_no_branch(self):
# A series with no branch resolves to the writable alias.
- series = self.factory.makeSeries(branch=None)
+ series = self.factory.makeProductSeries(branch=None)
self.assertOnlyWritableResolves(
'%s/%s' % (series.product.name, series.name))
@@ -317,7 +318,7 @@
# attributes of a private branch and these tests are running as an
# anonymous user.
branch = self.factory.makeAnyBranch(private=True)
- series = self.factory.makeSeries(branch=branch)
+ series = self.factory.makeProductSeries(branch=branch)
lp_path='%s/%s' % (series.product.name, series.name)
self.assertOnlyWritableResolves(lp_path)
=== modified file 'lib/lp/registry/browser/tests/browser-views.txt'
--- lib/lp/registry/browser/tests/browser-views.txt 2009-12-13 11:55:40 +0000
+++ lib/lp/registry/browser/tests/browser-views.txt 2010-09-16 05:08:47 +0000
@@ -18,7 +18,7 @@
>>> class MilestoneCreatorView(LaunchpadView, MilestoneOverlayMixin):
... """A stub view to verify a mixin."""
>>> product = factory.makeProduct(name='app')
- >>> series = factory.makeSeries(name='stable', product=product)
+ >>> series = factory.makeProductSeries(name='stable', product=product)
>>> view = MilestoneCreatorView(series, LaunchpadTestRequest())
>>> print view.milestone_form_uri
http://launchpad.dev/app/stable/+addmilestone/++form++
=== modified file 'lib/lp/registry/browser/tests/productrelease-views.txt'
--- lib/lp/registry/browser/tests/productrelease-views.txt 2010-04-29 15:21:05 +0000
+++ lib/lp/registry/browser/tests/productrelease-views.txt 2010-09-16 05:08:47 +0000
@@ -9,10 +9,11 @@
A new ProductRelease can be created using ProductReleaseAddView.
+ >>> from zope.security.proxy import removeSecurityProxy
>>> owner = factory.makePerson(name='app-owner')
>>> product = factory.makeProduct(name='app', owner=owner)
- >>> series = factory.makeSeries(name='simple', product=product)
- >>> milestone = series.newMilestone('0.1')
+ >>> series = factory.makeProductSeries(name='simple', product=product)
+ >>> milestone = removeSecurityProxy(series).newMilestone('0.1')
>>> print milestone.active
True
=== modified file 'lib/lp/registry/browser/tests/test_milestone.py'
--- lib/lp/registry/browser/tests/test_milestone.py 2010-08-25 04:25:02 +0000
+++ lib/lp/registry/browser/tests/test_milestone.py 2010-09-16 05:08:47 +0000
@@ -18,7 +18,7 @@
login,
login_person,
logout,
- person_logged_in,
+ person_logged_in,
TestCaseWithFactory,
)
from lp.testing.matchers import HasQueryCount
@@ -34,7 +34,8 @@
def setUp(self):
TestCaseWithFactory.setUp(self)
self.product = self.factory.makeProduct()
- self.series = self.factory.makeSeries(product=self.product)
+ self.series = (
+ self.factory.makeProductSeries(product=self.product))
owner = self.product.owner
login_person(owner)
@@ -169,12 +170,13 @@
layer = DatabaseFunctionalLayer
def test_more_private_bugs_query_count_is_constant(self):
- # This test tests that as we add more private bugs to a milestone index
- # page, the number of queries issued by the page does not change.
- # It also sets a cap on the queries for this page: if the baseline
- # were to increase, the test would fail. As the baseline is very large
- # already, if the test fails due to such a change, please cut some more
- # of the existing fat out of it rather than increasing the cap.
+ # This test tests that as we add more private bugs to a milestone
+ # index page, the number of queries issued by the page does not
+ # change. It also sets a cap on the queries for this page: if the
+ # baseline were to increase, the test would fail. As the baseline
+ # is very large already, if the test fails due to such a change,
+ # please cut some more of the existing fat out of it rather than
+ # increasing the cap.
product = self.factory.makeProduct()
login_person(product.owner)
milestone = self.factory.makeMilestone(
@@ -182,9 +184,9 @@
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.
+ # 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):
@@ -206,7 +208,7 @@
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
+ 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,
@@ -223,7 +225,7 @@
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
+ 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" % (
=== modified file 'lib/lp/testing/factory.py'
--- lib/lp/testing/factory.py 2010-09-16 05:08:45 +0000
+++ lib/lp/testing/factory.py 2010-09-16 05:08:47 +0000
@@ -66,7 +66,6 @@
Message,
MessageChunk,
)
-from canonical.launchpad.ftests._sqlobject import syncUpdate
from canonical.launchpad.interfaces import (
IMasterStore,
IStore,
@@ -882,11 +881,21 @@
def makeProductSeries(self, product=None, name=None, owner=None,
summary=None, date_created=None, branch=None):
- """Create and return a new ProductSeries."""
+ """Create a new, arbitrary ProductSeries.
+
+ :param branch: If supplied, the branch to set as
+ ProductSeries.branch.
+ :param date_created: If supplied, the date the series is created.
+ :param name: If supplied, the name of the series.
+ :param owner: If supplied, the owner of the series.
+ :param product: If supplied, the series is created for this product.
+ Otherwise, a new product is created.
+ :param summary: If supplied, the product series summary.
+ """
if product is None:
product = self.makeProduct()
if owner is None:
- owner = self.makePerson()
+ owner = product.owner
if name is None:
name = self.getUniqueString()
if summary is None:
@@ -1777,29 +1786,6 @@
MessageChunk(message=message, sequence=1, content=content)
return message
- def makeSeries(self, branch=None, name=None, product=None):
- """Create a new, arbitrary ProductSeries.
-
- :param branch: If supplied, the branch to set as
- ProductSeries.branch.
- :param name: If supplied, the name of the series.
- :param product: If supplied, the series is created for this product.
- Otherwise, a new product is created.
- """
- if product is None:
- product = self.makeProduct()
- if name is None:
- name = self.getUniqueString()
- # We don't want to login() as the person used to create the product,
- # so we remove the security proxy before creating the series.
- naked_product = removeSecurityProxy(product)
- series = naked_product.newSeries(
- product.owner, name, self.getUniqueString(), branch)
- if branch is not None:
- series.branch = branch
- syncUpdate(series)
- return series
-
def makeLanguage(self, language_code=None, name=None):
"""Makes a language given the language_code and name."""
if language_code is None:
=== modified file 'lib/lp/translations/tests/test_autoapproval.py'
--- lib/lp/translations/tests/test_autoapproval.py 2010-08-30 20:06:26 +0000
+++ lib/lp/translations/tests/test_autoapproval.py 2010-09-16 05:08:47 +0000
@@ -172,7 +172,7 @@
def setUp(self):
super(TestGuessPOFileCustomLanguageCode, self).setUp()
self.product = self.factory.makeProduct()
- self.series = self.factory.makeSeries(product=self.product)
+ self.series = self.factory.makeProductSeries(product=self.product)
self.queue = TranslationImportQueue()
self.template = POTemplateSubset(productseries=self.series).new(
'test', 'test', 'test.pot', self.product.owner)
@@ -311,7 +311,8 @@
def _setUpProduct(self):
"""Set up a `Product` with release series and two templates."""
self.product = self.factory.makeProduct()
- self.productseries = self.factory.makeSeries(product=self.product)
+ self.productseries = self.factory.makeProductSeries(
+ product=self.product)
product_subset = POTemplateSubset(productseries=self.productseries)
self.producttemplate1 = product_subset.new(
'test1', 'test1', 'test.pot', self.product.owner)
