Merge lp:~sinzui/launchpad/stop-shouting-0 into lp:launchpad

[Curtis Hovey]

This is my branch to stop the shouting about naked objects in tests.

    lp:~sinzui/launchpad/stop-shouting-0
    Diff size: 290
    Test command: ./bin/test -vv \
          -t registry.*test_(distroseries|sourcepackage
          -t milestone-views -t productseries-views
    Pre-implementation: jml, bigjools
    Target release: 10.08

Stop the shouting about naked objects in tests
----------------------------------------------

I cannot work because I keep seeing annoying messages. This branch address
the ones that affect my feature branch.

ADDENDUM: There was a real error in a test that would have been caught if
the test has not removed the proxy. I will explain below.

Rules
-----

    * Return a proxied object from the factory.
    * Do not unwrap objects, log in to make the factory hack the attribute
      for the test.

QA
--

None, this is a test runner.

Lint
----

Linting changed files:
  lib/lp/registry/browser/tests/milestone-views.txt
  lib/lp/registry/browser/tests/productseries-views.txt
  lib/lp/registry/tests/test_distroseries.py
  lib/lp/registry/tests/test_sourcepackage.py
  lib/lp/testing/factory.py

Test
----

    * lib/lp/registry/browser/tests/milestone-views.txt
      * Login as the project owner to set the driver.
    * lib/lp/registry/browser/tests/productseries-views.txt
      * Use a better factory method to get a product series
      * Wrap the long line.
      * Print the title of the enum ()
      * Change the factory to hack the non-writable ProductSeries.datecreated
    * lib/lp/registry/tests/test_distroseries.py
      * Login as the distro or project owner to set the object field.
      * NOTE: product.bugtracker was misspelled in the first test!
        The prioritised listing is incrementing the score for *missing*
        information. Thus by adding a bugtracker, to cold, we expect that
        package to have a lower score...it is missing less information.
        This test was always wrong.
    * lib/lp/registry/tests/test_sourcepackage.py
      * Login as project owner to set the status.

Implementation
--------------

    * lib/lp/testing/factory.py
      * Hush lint: remove unused or redundant imports, wrap long lines.
      * Return proxied milestone, productseries,
        SourcePackagePublishingHistory (note that it was created, then
        looked up again??).
      * DistributionSourcePackage is implicitly wrapped when called via
        distribution.getSourcePackage()

[Brad Crittenden (bac)]

Thanks for doing this work Curtis. Hopefully the other areas of LP will do the same soon.

review: Approve (code)

=== modified file 'lib/canonical/launchpad/doc/tales.txt'
--- lib/canonical/launchpad/doc/tales.txt	2010-07-15 08:38:19 +0000
+++ lib/canonical/launchpad/doc/tales.txt	2010-08-03 22:52:48 +0000
@@ -635,7 +635,8 @@
 When the ProductReleaseFile is not signed, the link for the signature is
 not included.
 
-    >>> release_file = factory.makeProductReleaseFile(signed=False)
+    >>> release_file = factory.makeProductReleaseFile(
+    ...     signed=False)
     >>> html = test_tales("release_file/fmt:link", release_file=release_file)
     >>> soup = BeautifulSoup(html)
     >>> print_hrefs_with_titles(html)
@@ -649,8 +650,8 @@
 
 HTML in the file description is escaped in the fmt:link.
 
-    >>> release_file.description = (
-    ...     '><script>XSS failed</script>')
+    >>> release_file = factory.makeProductReleaseFile(
+    ...     signed=False, description='><script>XSS failed</script>')
     >>> print test_tales("release_file/fmt:link", release_file=release_file)
     <img ...
     <a title="&gt;&lt;script&gt;XSS failed&lt;/script&gt; (4 bytes)"
@@ -661,7 +662,7 @@
 Product series
 ..............
 
-    >>> product_series = factory.makeSeries()
+    >>> product_series = factory.makeProductSeries()
     >>> test_tales("product_series/fmt:link", product_series=product_series)
     u'... series...'
 

=== modified file 'lib/lp/registry/browser/tests/milestone-views.txt'
--- lib/lp/registry/browser/tests/milestone-views.txt	2010-07-20 17:50:45 +0000
+++ lib/lp/registry/browser/tests/milestone-views.txt	2010-08-03 22:52:48 +0000
@@ -579,13 +579,10 @@
 The driver of a series that belongs to an `IDerivativeDistribution` is a
 release manager and can create milestones.
 
-    >>> from lp.testing.factory import (
-    ...     remove_security_proxy_and_shout_at_engineer)
     >>> distroseries = factory.makeDistroRelease(name='pumpkin')
     >>> driver = factory.makePerson(name='a-driver')
-    >>> naked_distroseries = remove_security_proxy_and_shout_at_engineer(
-    ...     distroseries)
-    >>> naked_distroseries.driver = driver
+    >>> login_person(distroseries.distribution.owner)
+    >>> distroseries.driver = driver
     >>> login_person(driver)
 
     >>> form = {

=== modified file 'lib/lp/registry/browser/tests/product-files-views.txt'
--- lib/lp/registry/browser/tests/product-files-views.txt	2010-06-11 14:25:40 +0000
+++ lib/lp/registry/browser/tests/product-files-views.txt	2010-08-03 22:52:48 +0000
@@ -4,7 +4,8 @@
 Test for the product/+download page.
 
     >>> product = factory.makeProduct(name='alfajore')
-    >>> productseries = factory.makeSeries(product=product, name="sammy")
+    >>> productseries = factory.makeProductSeries(
+    ...     product=product, name="sammy")
     >>> milestone = factory.makeMilestone(productseries=productseries,
     ...                                   name="apple")
     >>> release_file = factory.makeProductReleaseFile(
@@ -28,7 +29,8 @@
 
     >>> product = factory.makeProduct(name='bombilla')
     >>> for i in range(1,5):
-    ...     productseries = factory.makeSeries(product=product, name="s%d"%i)
+    ...     productseries = factory.makeProductSeries(
+    ...         product=product, name="s%d"%i)
     ...     for j in range(1,4):
     ...         milestone = factory.makeMilestone(productseries=productseries,
     ...                                           name="%d.%d"%(i,j))

=== modified file 'lib/lp/registry/browser/tests/productseries-views.txt'
--- lib/lp/registry/browser/tests/productseries-views.txt	2010-07-20 17:50:45 +0000
+++ lib/lp/registry/browser/tests/productseries-views.txt	2010-08-03 22:52:48 +0000
@@ -12,7 +12,7 @@
     >>> from lp.testing.menu import check_menu_links
 
     >>> product = factory.makeProduct(name='app')
-    >>> series = factory.makeSeries(name='simple', product=product)
+    >>> series = factory.makeProductSeries(name='simple', product=product)
     >>> check_menu_links(ProductSeriesOverviewMenu(series))
     True
 
@@ -80,7 +80,8 @@
     >>> script = find_tag_by_id(view.render(), 'milestone-script')
     >>> print script
     <script id="milestone-script" type="text/javascript">
-        YUI().use(... 'lp.registry.milestoneoverlay', 'lp.registry.milestonetable'...
+        YUI().use(... 'lp.registry.milestoneoverlay',
+                      'lp.registry.milestonetable'...
             var series_uri = '/app/simple';
             var milestone_form_uri = '.../app/simple/+addmilestone/++form++';
             var milestone_row_uri =
@@ -130,7 +131,7 @@
 
     >>> from lp.registry.interfaces.series import SeriesStatus
 
-    >>> print series.status
+    >>> print series.status.title
     Active Development
     >>> view.is_obsolete
     False
@@ -255,20 +256,13 @@
 
     >>> from datetime import datetime
     >>> from pytz import UTC
-    >>> from lp.testing.factory import (
-    ...     remove_security_proxy_and_shout_at_engineer)
 
+    >>> test_date = datetime(2009, 05, 01, 19, 34, 24, tzinfo=UTC)
     >>> product = factory.makeProduct(name="field", displayname='Field')
     >>> productseries = factory.makeProductSeries(
-    ...     product=product, name='rabbit')
+    ...     product=product, name='rabbit', date_created=test_date)
     >>> productseries.releasefileglob = 'http://eg.dom/rabbit/*'
 
-    # Hack the creation date for testing purposes.
-    >>> test_date = datetime(2009, 05, 01, 19, 34, 24, tzinfo=UTC)
-    >>> naked_productseries = remove_security_proxy_and_shout_at_engineer(
-    ...     productseries)
-    >>> naked_productseries.datecreated = test_date
-
 Users without edit permission cannot access the view.
 
     >>> from canonical.launchpad.webapp.authorization import check_permission

=== modified file 'lib/lp/registry/browser/tests/test_breadcrumbs.py'
--- lib/lp/registry/browser/tests/test_breadcrumbs.py	2010-04-28 10:13:00 +0000
+++ lib/lp/registry/browser/tests/test_breadcrumbs.py	2010-08-03 22:52:48 +0000
@@ -10,6 +10,7 @@
 from canonical.launchpad.interfaces.launchpad import ILaunchpadCelebrities
 from canonical.launchpad.webapp.publisher import canonical_url
 
+from lp.testing import login_person
 from lp.testing.breadcrumbs import BaseBreadcrumbTestCase
 
 
@@ -90,6 +91,7 @@
         self.assertEqual(self.milestone.name, last_crumb.text)
 
     def test_milestone_with_code_name(self):
+        login_person(self.milestone.productseries.product.owner)
         self.milestone.code_name = "duck"
         crumbs = self.getBreadcrumbsForObject(self.milestone)
         last_crumb = crumbs[-1]

=== modified file 'lib/lp/registry/configure.zcml'
--- lib/lp/registry/configure.zcml	2010-07-28 19:18:14 +0000
+++ lib/lp/registry/configure.zcml	2010-08-03 22:52:48 +0000
@@ -403,6 +403,8 @@
                 bugtargetdisplayname
                 bugtargetname
                 getBugCounts
+                bug_count
+                total_bug_heat
                 bug_subscriptions
                 getSubscriptions
                 getSubscription

=== modified file 'lib/lp/registry/tests/test_distribution.py'
--- lib/lp/registry/tests/test_distribution.py	2010-06-21 13:33:36 +0000
+++ lib/lp/registry/tests/test_distribution.py	2010-08-03 22:52:48 +0000
@@ -5,8 +5,6 @@
 
 __metaclass__ = type
 
-import unittest
-
 from zope.security.proxy import removeSecurityProxy
 
 from lp.registry.tests.test_distroseries import (
@@ -115,7 +113,7 @@
 
     def test_get_current(self):
         distro = self.factory.makeDistribution()
-        series = self.factory.makeDistroSeries(distribution=distro, 
+        series = self.factory.makeDistroSeries(distribution=distro,
             status=SeriesStatus.CURRENT)
         self.assertEquals([series],
             list(distro.getSeriesByStatus(SeriesStatus.CURRENT)))
@@ -133,16 +131,12 @@
 
     def test_get_by_name(self):
         distro = self.factory.makeDistribution()
-        series = self.factory.makeDistroSeries(distribution=distro, 
+        series = self.factory.makeDistroSeries(distribution=distro,
             name="dappere")
         self.assertEquals(series, distro.getSeries("dappere"))
 
     def test_get_by_version(self):
         distro = self.factory.makeDistribution()
-        series = self.factory.makeDistroSeries(distribution=distro, 
+        series = self.factory.makeDistroSeries(distribution=distro,
             name="dappere", version="42.6")
         self.assertEquals(series, distro.getSeries("42.6"))
-
-
-def test_suite():
-    return unittest.TestLoader().loadTestsFromName(__name__)

=== modified file 'lib/lp/registry/tests/test_distroseries.py'
--- lib/lp/registry/tests/test_distroseries.py	2010-07-29 15:16:43 +0000
+++ lib/lp/registry/tests/test_distroseries.py	2010-08-03 22:52:48 +0000
@@ -3,10 +3,10 @@
 
 """Tests for distroseries."""
 
+from __future__ import with_statement
+
 __metaclass__ = type
 
-import unittest
-
 import transaction
 
 from zope.component import getUtility
@@ -23,8 +23,7 @@
 from lp.soyuz.interfaces.publishing import (
     active_publishing_status, PackagePublishingStatus)
 from lp.soyuz.model.processor import ProcessorFamilySet
-from lp.testing import TestCase, TestCaseWithFactory
-from lp.testing.factory import remove_security_proxy_and_shout_at_engineer
+from lp.testing import person_logged_in, TestCase, TestCaseWithFactory
 from lp.soyuz.tests.test_publishing import SoyuzTestPublisher
 from lp.translations.interfaces.translations import (
     TranslationsBranchImportMode)
@@ -293,12 +292,11 @@
         self.linkPackage('hot')
         self.makeSeriesPackage('cold')
         product_series = self.linkPackage('cold')
-        naked_product_series = remove_security_proxy_and_shout_at_engineer(
-            product_series)
-        naked_product_series.product.bugtraker = self.factory.makeBugTracker()
+        with person_logged_in(product_series.product.owner):
+            product_series.product.bugtracker = self.factory.makeBugTracker()
         packagings = self.series.getPrioritizedPackagings()
         names = [packaging.sourcepackagename.name for packaging in packagings]
-        expected = [u'hot', u'cold', u'linked']
+        expected = [u'hot', u'linked', u'cold']
         self.assertEqual(expected, names)
 
     def test_getPrioritizedPackagings_branch(self):
@@ -306,9 +304,8 @@
         self.linkPackage('translatable')
         self.makeSeriesPackage('withbranch')
         product_series = self.linkPackage('withbranch')
-        naked_product_series = remove_security_proxy_and_shout_at_engineer(
-            product_series)
-        naked_product_series.branch = self.factory.makeBranch()
+        with person_logged_in(product_series.product.owner):
+            product_series.branch = self.factory.makeBranch()
         packagings = self.series.getPrioritizedPackagings()
         names = [packaging.sourcepackagename.name for packaging in packagings]
         expected = [u'translatable', u'linked', u'withbranch']
@@ -320,11 +317,10 @@
         self.linkPackage('translatable')
         self.makeSeriesPackage('importabletranslatable')
         product_series = self.linkPackage('importabletranslatable')
-        naked_product_series = remove_security_proxy_and_shout_at_engineer(
-            product_series)
-        naked_product_series.branch = self.factory.makeBranch()
-        naked_product_series.translations_autoimport_mode = (
-            TranslationsBranchImportMode.IMPORT_TEMPLATES)
+        with person_logged_in(product_series.product.owner):
+            product_series.branch = self.factory.makeBranch()
+            product_series.translations_autoimport_mode = (
+                TranslationsBranchImportMode.IMPORT_TEMPLATES)
         packagings = self.series.getPrioritizedPackagings()
         names = [packaging.sourcepackagename.name for packaging in packagings]
         expected = [u'translatable', u'linked', u'importabletranslatable']
@@ -357,9 +353,8 @@
 
         new_distroseries = (
             self.factory.makeDistroRelease(name=u"sampleseries"))
-        naked_new_distroseries = remove_security_proxy_and_shout_at_engineer(
-            new_distroseries)
-        naked_new_distroseries.hide_all_translations = False
+        with person_logged_in(new_distroseries.distribution.owner):
+            new_distroseries.hide_all_translations = False
         transaction.commit()
         translatables = self._get_translatables()
         self.failUnlessEqual(
@@ -381,7 +376,8 @@
                 translatables,
                 self._ref_translatables(u"sampleseries")))
 
-        naked_new_distroseries.hide_all_translations = True
+        with person_logged_in(new_distroseries.distribution.owner):
+            new_distroseries.hide_all_translations = True
         transaction.commit()
         translatables = self._get_translatables()
         self.failUnlessEqual(
@@ -409,7 +405,3 @@
             NoSuchDistroSeries,
             getUtility(IDistroSeriesSet).fromSuite,
             distribution, 'doesntexist')
-
-
-def test_suite():
-    return unittest.TestLoader().loadTestsFromName(__name__)

=== modified file 'lib/lp/registry/tests/test_sourcepackage.py'
--- lib/lp/registry/tests/test_sourcepackage.py	2010-07-20 17:50:45 +0000
+++ lib/lp/registry/tests/test_sourcepackage.py	2010-08-03 22:52:48 +0000
@@ -3,6 +3,8 @@
 
 """Unit tests for ISourcePackage implementations."""
 
+from __future__ import with_statement
+
 __metaclass__ = type
 
 import unittest
@@ -21,8 +23,7 @@
 from lp.soyuz.interfaces.publishing import PackagePublishingStatus
 from lp.code.interfaces.seriessourcepackagebranch import (
     IMakeOfficialBranchLinks)
-from lp.testing import TestCaseWithFactory
-from lp.testing.factory import remove_security_proxy_and_shout_at_engineer
+from lp.testing import person_logged_in, TestCaseWithFactory
 from lp.testing.views import create_initialized_view
 from canonical.testing.layers import DatabaseFunctionalLayer
 
@@ -253,17 +254,13 @@
 
         self.obsolete_productseries = self.factory.makeProductSeries(
             name='obsolete', product=self.product)
-        naked_obsolete_productseries = (
-            remove_security_proxy_and_shout_at_engineer(
-                self.obsolete_productseries))
-        naked_obsolete_productseries.status = SeriesStatus.OBSOLETE
+        with person_logged_in(self.product.owner):
+            self.obsolete_productseries.status = SeriesStatus.OBSOLETE
 
         self.dev_productseries = self.factory.makeProductSeries(
             name='current', product=self.product)
-        naked_dev_productseries = (
-            remove_security_proxy_and_shout_at_engineer(
-                self.dev_productseries))
-        naked_dev_productseries.status = SeriesStatus.DEVELOPMENT
+        with person_logged_in(self.product.owner):
+            self.dev_productseries.status = SeriesStatus.DEVELOPMENT
 
         self.distribution = self.factory.makeDistribution(
             name='youbuntu', displayname='Youbuntu', owner=self.owner)

=== modified file 'lib/lp/testing/factory.py'
--- lib/lp/testing/factory.py	2010-08-03 17:27:52 +0000
+++ lib/lp/testing/factory.py	2010-08-03 22:52:48 +0000
@@ -111,8 +111,6 @@
 from lp.codehosting.codeimport.worker import CodeImportSourceDetails
 
 from lp.registry.interfaces.distribution import IDistributionSet
-from lp.registry.model.distributionsourcepackage import (
-    DistributionSourcePackage)
 from lp.registry.interfaces.distroseries import IDistroSeries
 from lp.registry.interfaces.gpg import GPGKeyAlgorithm, IGPGKeySet
 from lp.registry.interfaces.mailinglist import (
@@ -161,17 +159,17 @@
     ANONYMOUS,
     login,
     login_as,
+    person_logged_in,
     run_with_login,
     temp_dir,
     time_counter,
     )
 from lp.translations.interfaces.potemplate import IPOTemplateSet
-from lp.translations.interfaces.translationgroup import (
-    ITranslationGroupSet)
 from lp.translations.interfaces.translationimportqueue import (
     RosettaImportStatus)
 from lp.translations.interfaces.translationfileformat import (
     TranslationFileFormat)
+from lp.translations.interfaces.translationgroup import ITranslationGroupSet
 from lp.translations.interfaces.translationsperson import ITranslationsPerson
 from lp.translations.interfaces.translationtemplatesbuildjob import (
     ITranslationTemplatesBuildJobSource)
@@ -630,13 +628,20 @@
         self, product=None, distribution=None, productseries=None, name=None):
         if product is None and distribution is None and productseries is None:
             product = self.makeProduct()
-        if productseries is not None:
-            product = productseries.product
+        if distribution is None:
+            if productseries is not None:
+                product = productseries.product
+            else:
+                productseries = self.makeProductSeries(product=product)
+            distroseries = None
+        else:
+            distroseries = self.makeDistroRelease(distribution=distribution)
         if name is None:
             name = self.getUniqueString()
-        return Milestone(product=product, distribution=distribution,
-                         productseries=productseries,
-                         name=name)
+        return ProxyFactory(
+            Milestone(product=product, distribution=distribution,
+                      productseries=productseries, distroseries=distroseries,
+                      name=name))
 
     def makeProcessor(self, family=None, name=None, title=None,
                       description=None):
@@ -682,8 +687,10 @@
         if milestone is None:
             milestone = self.makeMilestone(product=product,
                                            productseries=productseries)
-        return milestone.createProductRelease(
-            milestone.product.owner, datetime.now(pytz.UTC))
+        with person_logged_in(milestone.productseries.product.owner):
+            release = milestone.createProductRelease(
+                milestone.product.owner, datetime.now(pytz.UTC))
+        return release
 
     def makeProductReleaseFile(self, signed=True,
                                product=None, productseries=None,
@@ -699,12 +706,14 @@
             release = self.makeProductRelease(product=product,
                                               productseries=productseries,
                                               milestone=milestone)
-        return release.addReleaseFile(
-            'test.txt', 'test', 'text/plain',
-            uploader=release.milestone.product.owner,
-            signature_filename=signature_filename,
-            signature_content=signature_content,
-            description=description)
+        with person_logged_in(release.milestone.product.owner):
+            release_file = release.addReleaseFile(
+                'test.txt', 'test', 'text/plain',
+                uploader=release.milestone.product.owner,
+                signature_filename=signature_filename,
+                signature_content=signature_content,
+                description=description)
+        return release_file
 
     def makeProduct(
         self, name=None, project=None, displayname=None,
@@ -747,7 +756,7 @@
         return product
 
     def makeProductSeries(self, product=None, name=None, owner=None,
-                          summary=None):
+                          summary=None, date_created=None):
         """Create and return a new ProductSeries."""
         if product is None:
             product = self.makeProduct()
@@ -760,8 +769,11 @@
         # 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)
-        return ProxyFactory(
-            naked_product.newSeries(owner=owner, name=name, summary=summary))
+        series = naked_product.newSeries(
+            owner=owner, name=name, summary=summary)
+        if date_created is not None:
+            series.datecreated = date_created
+        return ProxyFactory(series)
 
     def makeProject(self, name=None, displayname=None, title=None,
                     homepageurl=None, summary=None, owner=None,
@@ -2437,7 +2449,7 @@
             dsc_binaries=dsc_binaries,
             date_uploaded=date_uploaded)
 
-        sspph = SourcePackagePublishingHistory(
+        return ProxyFactory(SourcePackagePublishingHistory(
             distroseries=distroseries,
             sourcepackagerelease=spr,
             component=spr.component,
@@ -2447,11 +2459,7 @@
             dateremoved=dateremoved,
             scheduleddeletiondate=scheduleddeletiondate,
             pocket=pocket,
-            archive=archive)
-
-        # SPPH and SSPPH IDs are the same, since they are SPPH is a SQLVIEW
-        # of SSPPH and other useful attributes.
-        return SourcePackagePublishingHistory.get(sspph.id)
+            archive=archive))
 
     def makeBinaryPackagePublishingHistory(self, binarypackagerelease=None,
                                            distroarchseries=None,
@@ -2588,7 +2596,7 @@
         if distribution is None:
             distribution = self.makeDistribution()
 
-        return DistributionSourcePackage(distribution, sourcepackagename)
+        return distribution.getSourcePackage(sourcepackagename)
 
     def makeEmailMessage(self, body=None, sender=None, to=None,
                          attachments=None, encode_attachments=False):
@@ -2768,7 +2776,7 @@
 # other Python libraries.
 unwrapped_types = (
     BaseRecipeBranch, DSCFile, InstanceType, MergeDirective2, Message,
-    datetime, int, str, unicode,)
+    datetime, int, str, unicode)
 
 
 def is_security_proxied_or_harmless(obj):
@@ -2793,7 +2801,7 @@
     def __init__(self, method_name):
         super(UnproxiedFactoryMethodWarning, self).__init__(
             "PLEASE FIX: LaunchpadObjectFactory.%s returns an "
-            "unproxied object." % (method_name,))
+            "unproxied object." % (method_name, ))
 
 
 class ShouldThisBeUsingRemoveSecurityProxy(UserWarning):