On Thu, Aug 5, 2010 at 10:00 PM, James Westby <email address hidden> wrote:
...
> Hi,
>
> Apologies for the bumper branch. Very small changes snowballed
> very quickly.

It happens.

> This branch moves SoyuzTestPublisher to back on to the
> factory, as a precursor to moving it away from sampledata.
>
> It now runs pretty much the same code inside the factory as it
> was running before, and now SoyuzTestPublisher is just a layer
> on top of the factory with some conveniences for complex
> tests.
>

This seems like a sensible approach.

> The major change is that there are now lots more proxied objects
> in soyuz tests, so most of the changes are fallout from that.
> There's a lot of removeSecurityProxy involved there, as most of
> the problems we in test setup code that is reaching around the
> interface to do test set up. I'm not entirely happy that we have
> to do that, they should be able to come in by the front door, but
> fixing all of that would be too much for one branch.
>

Agreed.

...
> Breakdown of the changes, as not all are immediately obvious. It's perhaps
> best to review in reverse order of the changes listed here (odd that bzr
> diff gave me the files in roughly reverse order).
>
> lib/lp/archivepublisher/tests/test_dominator.py - just making the values explicit.
>
> lib/lp/archivepublisher/tests/test_publisher.py, lib/lp/soyuz/scripts/tests/test_publishdistro.py, lib/lp/soyuz/tests/test_publishing_top_level_api.py - using sync() from SQLObject, which isn't
> on the interface.
>
> lib/lp/archiveuploader/tests/nascentupload-ddebs.txt - getPubSource now creates a user, which
> requires the extra setup that only used to be done later.
>
> lib/lp/soyuz/doc/archive-files.txt - setting a value that could be set at creation time
> if there were an appropriate way to do that. It's just set up though, so this has the
> same effect.
>
> lib/lp/soyuz/doc/packageupload-lookups.txt - Using rSP to ensure that the repr is
> what is expected.
>
> lib/lp/soyuz/doc/publishing.txt - changing purpose of an archive or parent series aren't normally allowed. New archives/series could be created instead. Changing the published
> component isn't possible, but you could republish in the new component.
>
> lib/lp/soyuz/doc/sourcepackagerelease.txt - deletion is protected, but this is just test
> cleanup, so we don't care.
>
> lib/lp/soyuz/scripts/tests/test_changeoverride.py - specifying values and changing the
> version of a publication, which isn't normally allowed.
>
> lib/lp/soyuz/scripts/tests/test_copypackage.py - various instances of bypassing the
> interface for test setup.
>
> lib/lp/soyuz/tests/test_binarypackagebuild.py - the proxy stops you concatenating lists, so
> listify each of them.
>
> lib/lp/soyuz/tests/test_publish_archive_indexes.py - specify all the tested values. Also
> a case of forcing invalid data in to a field. I also deleted some copy paste comments
> that made no sense in the context.
>
> lib/lp/soyuz/tests/test_publishing.py - the big one, using the factory and lp.testing.sampledata rather than creating objects itself, then a whole bunch of fixes
> from the fallout of this due to the proxy.
>
> Thanks,
>

My pleasure.

Most of the review comments are about dropping calls to sync()
entirely and adding comments to certain calls to removeSecurityProxy –
it's good to know why a test feels the need to fiddle with internals.

Also, I feel obliged to include this snippet of a conversation I had
with Julian, in case they are relevant to this patch or trigger new
thinking about the no-more-sampledata work you've been doing.

#launchpad-code, 2010-08-03, 16:13-16:19:
<bigjools> jml: I'm a bit uncomfortable with the factory changes to skip STP
<jml> bigjools, why so?
<bigjools> jml: a few reasons.  One, because it's not differentiating
between binary and source publications. Two, someone will want to make
it do binaries at some point and there will be pain because they're an
order of magnitiude more complicated to set up, and three, the STP
relies on sample data because there's a load of publisher config in it
which is not being set up in the factory methods so you can end up
with data
<bigjools>  that can't exist in the real world.
<bigjools> and probably more problems if I look harder
<bigjools> not insurmountable of course, but nonetheless the problems exist

Thanks so much for doing this work. I really appreciate it.

jml

> === modified file 'lib/lp/archivepublisher/tests/test_publisher.py'
> --- lib/lp/archivepublisher/tests/test_publisher.py     2010-07-22 08:19:27 +0000
> +++ lib/lp/archivepublisher/tests/test_publisher.py     2010-08-05 20:59:48 +0000
> @@ -83,7 +83,7 @@
>         publisher.A_publish(False)
>         self.layer.txn.commit()
>
> -        pub_source.sync()
> +        removeSecurityProxy(pub_source).sync()

I don't think you even need to call sync. Do the tests pass without the call?

>         self.assertDirtyPocketsContents(
>             [('breezy-autotest', 'RELEASE')], publisher.dirty_pockets)
>         self.assertEqual(pub_source.status, PackagePublishingStatus.PUBLISHED)
> @@ -226,8 +226,8 @@
>         publisher.A_publish(force_publishing=False)
>         self.layer.txn.commit()
>
> -        pub_source.sync()
> -        pub_source2.sync()
> +        removeSecurityProxy(pub_source).sync()
> +        removeSecurityProxy(pub_source2).sync()

Ditto.

>         self.assertDirtyPocketsContents(
>             [('hoary-test', 'RELEASE')], publisher.dirty_pockets)
>         self.assertEqual(pub_source2.status,
> @@ -259,8 +259,8 @@
>         publisher.A_publish(force_publishing=False)
>         self.layer.txn.commit()
>
> -        pub_source.sync()
> -        pub_source2.sync()
> +        removeSecurityProxy(pub_source).sync()
> +        removeSecurityProxy(pub_source2).sync()

Ditto.

>         self.assertDirtyPocketsContents(
>             [('breezy-autotest', 'UPDATES')], publisher.dirty_pockets)
>         self.assertEqual(pub_source.status, PackagePublishingStatus.PUBLISHED)
> @@ -360,7 +360,7 @@
>         publisher.A_publish(False)
>         self.layer.txn.commit()
>
> -        pub_source.sync()
> +        removeSecurityProxy(pub_source).sync()

Ditto.

> === modified file 'lib/lp/soyuz/doc/archive-files.txt'
> --- lib/lp/soyuz/doc/archive-files.txt  2010-05-11 07:09:18 +0000
> +++ lib/lp/soyuz/doc/archive-files.txt  2010-08-05 20:59:48 +0000
> @@ -145,7 +145,7 @@
>
>     >>> diff_name = 'test-pkg_1.0_1.1.diff.gz'
>     >>> diff = test_publisher.addMockFile(diff_name)
> -    >>> package_diff.diff_content = diff
> +    >>> removeSecurityProxy(package_diff).diff_content = diff
>

I think a comment here would be appropriate, explaining why we're
going via the back door.

> === modified file 'lib/lp/soyuz/doc/packageupload-lookups.txt'
> --- lib/lp/soyuz/doc/packageupload-lookups.txt  2010-03-06 04:57:40 +0000
> +++ lib/lp/soyuz/doc/packageupload-lookups.txt  2010-08-05 20:59:48 +0000
> @@ -98,6 +98,7 @@
>     # Create a testing source and its binaries in
>     # ubuntutest/breezy-autotest/i386.
>     >>> from lp.soyuz.tests.test_publishing import SoyuzTestPublisher
> +    >>> from zope.security.proxy import removeSecurityProxy
>     >>> login('<email address hidden>')
>     >>> test_publisher = SoyuzTestPublisher()
>     >>> test_publisher.prepareBreezyAutotest()
> @@ -112,7 +113,7 @@
>  The `SourcePackageRelease` 'package_upload' and 'upload_changesfile'
>
>     >>> original_source_upload = source.sourcepackagerelease.package_upload
> -    >>> print original_source_upload
> +    >>> print removeSecurityProxy(original_source_upload)

Really? I'm surprised this change makes any difference at all. What do
you get without it?

> === modified file 'lib/lp/soyuz/doc/publishing.txt'
> --- lib/lp/soyuz/doc/publishing.txt     2010-07-12 03:14:42 +0000
> +++ lib/lp/soyuz/doc/publishing.txt     2010-08-05 20:59:48 +0000
> @@ -2,10 +2,11 @@
>  SourcePackagePublishingHistory
>  ==============================
>
> -This class provides public access to publishing records via a SQL view.
> +This class provides public access to publishing records.
>
> -    >>> from canonical.launchpad.database import SourcePackagePublishingHistory
> -    >>> from canonical.launchpad.database import BinaryPackagePublishingHistory
> +    >>> from lp.soyuz.model.publishing import (
> +    ...     BinaryPackagePublishingHistory, SourcePackagePublishingHistory)
> +    >>> from zope.security.proxy import removeSecurityProxy
>
>  Select a publishing record from the sampledata (pmount is a
>  interesting one):
> @@ -173,7 +174,7 @@
>  FULLY_BUILT.
>
>     >>> from lp.soyuz.interfaces.archive import ArchivePurpose
> -    >>> spph.archive.purpose = ArchivePurpose.COPY
> +    >>> removeSecurityProxy(spph.archive).purpose = ArchivePurpose.COPY

A short comment here along the lines of your review introduction would
help readability.

>     >>> build_status_summary = spph.getStatusSummaryForBuilds()
>     >>> print_build_status_summary(build_status_summary)
>     FULLYBUILT
> @@ -181,7 +182,7 @@
>     i386 build of abc 666 in ubuntutest breezy-autotest RELEASE
>
>     # Just set the purpose back before continuing on.
> -    >>> spph.archive.purpose = ArchivePurpose.PRIMARY
> +    >>> removeSecurityProxy(spph.archive).purpose = ArchivePurpose.PRIMARY
>

Likewise.

> @@ -779,8 +780,8 @@
>  This simulates a rebuild in of the same source in a more recent
>  distroseries, like rebuilding SRUs for constant sources.
>
> -    >>> breezy_autotest.parent_series = None
> -    >>> hoary_test.parent_series = breezy_autotest
> +    >>> removeSecurityProxy(breezy_autotest).parent_series = None
> +    >>> removeSecurityProxy(hoary_test).parent_series = breezy_autotest
>

Again, could you add something explaining why we have to use rSP?

>     >>> ppa_source.createMissingBuilds()
>     []
> @@ -791,8 +792,8 @@
>  Now, let's check the opposite, as if the copy was from a more recent
>  distroseries to a older one, like a backport rebuild.
>
> -    >>> breezy_autotest.parent_series = hoary_test
> -    >>> hoary_test.parent_series = None
> +    >>> removeSecurityProxy(breezy_autotest).parent_series = hoary_test
> +    >>> removeSecurityProxy(hoary_test).parent_series = None
>

Ditto.

> @@ -1877,7 +1877,8 @@
>  It works in the same way for binaries.
>
>     >>> multiverse = getUtility(IComponentSet)['multiverse']
> -    >>> test_binary.binarypackagerelease.component = multiverse
> +    >>> naked_bpr = removeSecurityProxy(test_binary.binarypackagerelease)
> +    >>> naked_bpr.component = multiverse

Same.

> === modified file 'lib/lp/soyuz/doc/sourcepackagerelease.txt'
> --- lib/lp/soyuz/doc/sourcepackagerelease.txt   2010-05-19 15:39:52 +0000
> +++ lib/lp/soyuz/doc/sourcepackagerelease.txt   2010-08-05 20:59:48 +0000
...
> @@ -349,7 +349,8 @@
>  private PPA, it gets deleted from that private PPA.  At this point the
>  package is still public:
>
> -    >>> private_publication.requestDeletion(cprov)
> +    >>> from zope.security.proxy import removeSecurityProxy
> +    >>> removeSecurityProxy(private_publication).requestDeletion(cprov)

Why rSP? Perhaps this should instead be written as:

    >>> with celebrity_logged_in('admin'):
    ...     private_publication.requestDeletion(cprov)

If not, please add a comment.

> === modified file 'lib/lp/soyuz/scripts/tests/test_changeoverride.py'
> --- lib/lp/soyuz/scripts/tests/test_changeoverride.py   2010-07-20 12:06:36 +0000
> +++ lib/lp/soyuz/scripts/tests/test_changeoverride.py   2010-08-05 20:59:48 +0000
...
>         build = binaries[0].binarypackagerelease.build
>         other_binary = self.test_publisher.uploadBinaryForBuild(
>             build, 'boingo-data')
> -        other_binary.version = '0.9'
> +        removeSecurityProxy(other_binary).version = '0.9'

Comment please.

> === modified file 'lib/lp/soyuz/scripts/tests/test_copypackage.py'
> --- lib/lp/soyuz/scripts/tests/test_copypackage.py      2010-07-20 12:06:36 +0000
> +++ lib/lp/soyuz/scripts/tests/test_copypackage.py      2010-08-05 20:59:48 +0000
> @@ -683,7 +683,7 @@
>         utc = pytz.timezone('UTC')
>         old_date = datetime.datetime(1970, 1, 1, tzinfo=utc)
>         a_binary_file = binaries[0].binarypackagerelease.files[0]
> -        a_binary_file.libraryfile.expires = old_date
> +        removeSecurityProxy(a_binary_file.libraryfile).expires = old_date
>

Comment please.

>         # Now source-only copies are allowed.
>         copy_checker = CopyChecker(archive, include_binaries=False)
> @@ -714,7 +714,7 @@
>         expire = datetime.datetime.now(utc) + datetime.timedelta(days=365)
>
>         a_source_file = source.sourcepackagerelease.files[0]
> -        a_source_file.libraryfile.expires = expire
> +        removeSecurityProxy(a_source_file.libraryfile).expires = expire
>

And here too :)

>         copy_checker = CopyChecker(archive, include_binaries=False)
>         self.assertRaisesWithContent(
> @@ -1065,7 +1065,7 @@
>         # something else.
>         source = self.createDelayedCopyContext()
>         contrib = getUtility(IComponentSet).new('contrib')
> -        source.sourcepackagerelease.component = contrib
> +        removeSecurityProxy(source.sourcepackagerelease).component = contrib

Likewise.

>         [build] = source.getBuilds()
>         [binary] = build.binarypackages
>         binary.override(component=contrib)
> @@ -1089,7 +1089,7 @@
>         # something else.
>         source = self.createDelayedCopyContext()
>         contrib = getUtility(IComponentSet).new('contrib')
> -        source.sourcepackagerelease.component = contrib
> +        removeSecurityProxy(source.sourcepackagerelease).component = contrib

Ditto.

>         [build] = source.getBuilds()
>         [binary] = build.binarypackages
>         binary.override(component=contrib)
> @@ -2265,7 +2265,8 @@
>
>         # Override the copied binarypackagerelease to 'universe'.
>         for binary in ppa_binaries:
> -            binary.binarypackagerelease.component = universe
> +            removeSecurityProxy(binary.binarypackagerelease).component = (
> +                universe)
>

Comment please.

>         self.layer.txn.commit()
>
> @@ -2365,7 +2366,8 @@
>                 distroseries=warty, archive=archive, pocket=pocket,
>                 changes_file_content=changes_file_content,
>                 status=PackagePublishingStatus.PUBLISHED)
> -            source.sourcepackagerelease.changelog_entry = (
> +            naked_spr = removeSecurityProxy(source.sourcepackagerelease)
> +            naked_spr.changelog_entry = (
>                 "Required for close_bugs_for_sourcepublication")

Comment please.

>             binaries = test_publisher.getPubBinaries(
>                 pub_source=source, distroseries=warty, archive=archive,
>
> === modified file 'lib/lp/soyuz/scripts/tests/test_publishdistro.py'
> --- lib/lp/soyuz/scripts/tests/test_publishdistro.py    2010-03-09 07:29:18 +0000
> +++ lib/lp/soyuz/scripts/tests/test_publishdistro.py    2010-08-05 20:59:48 +0000
> @@ -72,7 +72,7 @@
>
>         rc, out, err = self.runPublishDistroScript()
>
> -        pub_source.sync()
> +        removeSecurityProxy(pub_source).sync()

Pretty sure you don't need sync() at all.

>         self.assertEqual(0, rc, "Publisher failed with:\n%s\n%s" % (out, err))
>         self.assertEqual(pub_source.status, PackagePublishingStatus.PUBLISHED)
>
> @@ -88,7 +88,7 @@
>         pub_source = self.getPubSource(filecontent='foo')
>         self.layer.txn.commit()
>         self.runPublishDistro()
> -        pub_source.sync()
> +        removeSecurityProxy(pub_source).sync()
>

Do you need sync? I don't think so.

>         random_person = getUtility(IPersonSet).getByName('name16')
>         pub_source.requestDeletion(random_person)
> @@ -96,7 +96,7 @@
>         self.assertTrue(pub_source.scheduleddeletiondate is None,
>             "pub_source.scheduleddeletiondate should not be set, and it is.")
>         self.runPublishDistro()
> -        pub_source.sync()
> +        removeSecurityProxy(pub_source).sync()

Perhaps sync() isn't necessary any more. A lot of these tests are
quite old. Give it a try without it.

>         self.assertTrue(pub_source.scheduleddeletiondate is not None,
>             "pub_source.scheduleddeletiondate should be set, and it's not.")
>
> @@ -123,8 +123,8 @@
>
>         self.runPublishDistro(['-s', 'hoary-test'])
>
> -        pub_source.sync()
> -        pub_source2.sync()
> +        removeSecurityProxy(pub_source).sync()
> +        removeSecurityProxy(pub_source2).sync()

Storm invalidates the use of sync(), IIRC. Don't think you need to do
this any more.

>         self.assertEqual(pub_source.status, PackagePublishingStatus.PENDING)
>         self.assertEqual(
>             pub_source2.status, PackagePublishingStatus.PUBLISHED)
> @@ -212,9 +212,9 @@
>
>         self.runPublishDistro(['--ppa'])
>
> -        pub_source.sync()
> -        pub_source2.sync()
> -        pub_source3.sync()
> +        removeSecurityProxy(pub_source).sync()
> +        removeSecurityProxy(pub_source2).sync()
> +        removeSecurityProxy(pub_source3).sync()

sync() isn't needed in the new world.

>         self.assertEqual(pub_source.status, PackagePublishingStatus.PENDING)
>         self.assertEqual(
>             pub_source2.status, PackagePublishingStatus.PUBLISHED)
> @@ -246,21 +246,21 @@
...
> -        pub_source.sync()
> +        removeSecurityProxy(pub_source).sync()

I question the necessity of this call to sync()

>         self.assertEqual(pub_source.status, PackagePublishingStatus.PENDING)
>
>         # Now publish the private PPAs and make sure they are really
>         # published.
>         self.runPublishDistro(['--private-ppa'])
>
> -        pub_source.sync()
> +        removeSecurityProxy(pub_source).sync()

Likewise, this call to sync seems to pass beyond the walls of
necessity into mere frippery.

> === modified file 'lib/lp/soyuz/tests/test_publish_archive_indexes.py'
> --- lib/lp/soyuz/tests/test_publish_archive_indexes.py  2010-07-20 12:06:36 +0000
> +++ lib/lp/soyuz/tests/test_publish_archive_indexes.py  2010-08-05 20:59:48 +0000
> @@ -8,6 +8,10 @@
>  import tempfile
>  import unittest
>
> +from zope.security.proxy import removeSecurityProxy
> +
> +from lp.soyuz.interfaces.binarypackagerelease import BinaryPackageFormat
> +from lp.soyuz.interfaces.publishing import PackagePublishingPriority
>  from lp.soyuz.tests.test_publishing import TestNativePublishingBase
>
>
> @@ -31,8 +35,14 @@
>         the package in question.
>         """
>         pub_source = self.getPubSource(
> +            sourcename='foo', dsc_binaries='foo-bin',
> +            version='666', section='base',
>             builddepends='fooish', builddependsindep='pyfoo',
> -            build_conflicts='bar', build_conflicts_indep='pybar')
> +            build_conflicts='bar', build_conflicts_indep='pybar',
> +            dsc_maintainer_rfc822='Foo Bar <email address hidden>',
> +            architecturehintlist='all', dsc_standards_version='3.6.2',
> +            dsc_format='1.0', filename='foo_666.dsc',
> +            filecontent='I do not care about sources.')
>
>         self.assertEqual(
>             [u'Package: foo',
> @@ -52,16 +62,29 @@
>              u' 5913c3ad52c14a62e6ae7eef51f9ef42 28 foo_666.dsc'],
>             pub_source.getIndexStanza().splitlines())
>
> +    def getPubSourceForBinary(self):
> +        return self.getPubSource(
> +            sourcename='foo', dsc_maintainer_rfc822='Foo Bar <email address hidden>')
> +
>     def testBinaryStanza(self):
>         """Check just-created binary publication Index stanza.
>
>         See also testSourceStanza, it must present something similar for
>         binary packages.
>         """
> +        pub_source = self.getPubSourceForBinary()
>         pub_binaries = self.getPubBinaries(
> +            binaryname='foo-bin',
> +            priority=PackagePublishingPriority.STANDARD, section="base",
> +            installed_size=100, architecturespecific=True,
> +            version='666',
>             depends='biscuit', recommends='foo-dev', suggests='pyfoo',
>             conflicts='old-foo', replaces='old-foo', provides='foo-master',
> -            pre_depends='master-foo', enhances='foo-super', breaks='old-foo')
> +            pre_depends='master-foo', enhances='foo-super', breaks='old-foo',
> +            filecontent='bbbiiinnnaaarrryyy', summary='Foo app is great',
> +            format=BinaryPackageFormat.DEB,
> +            description='Well ...\nit does nothing, though',
> +            pub_source=pub_source)
>         pub_binary = pub_binaries[0]
>         self.assertEqual(
>             [u'Package: foo-bin',
> @@ -114,8 +137,15 @@
>             "Normal\nNormal"
>             "\n.\n.\n."
>             "\n %s" % ('x' * 100))
> +        pub_source = self.getPubSourceForBinary()
>         pub_binary = self.getPubBinaries(
> -            description=description)[0]
> +            description=description, pub_source=pub_source,
> +            binaryname='foo-bin',
> +            priority=PackagePublishingPriority.STANDARD,
> +            section="base", installed_size=100, architecturespecific=False,
> +            format=BinaryPackageFormat.DEB,
> +            version="666", filecontent="bbbiiinnnaaarrryyy",
> +            summary='Foo app is great')[0]
>
>         self.assertEqual(
>             [u'Package: foo-bin',
> @@ -150,8 +180,14 @@
>         'utf-8' for disk writing.
>         """
>         description = u'Using non-ascii as: \xe7\xe3\xe9\xf3'
> +        pub_source = self.getPubSourceForBinary()
>         pub_binary = self.getPubBinaries(
> -            description=description)[0]
> +            description=description, pub_source=pub_source,
> +            binaryname='foo-bin',
> +            priority=PackagePublishingPriority.STANDARD, section="base",
> +            installed_size=100, architecturespecific=False,
> +            version="666", format=BinaryPackageFormat.DEB,
> +            filecontent='bbbiiinnnaaarrryyy', summary='Foo app is great')[0]
>
>         self.assertEqual(
>             [u'Package: foo-bin',
> @@ -200,7 +236,9 @@
>
>     def test_getIndexStanza_binary_stanza(self):
>         """Check a binary stanza with APT parser."""
> -        pub_binary = self.getPubBinaries()[0]
> +        pub_binary = self.getPubBinaries(
> +            binaryname='foo-bin', summary='Foo app is great',
> +            description='Well ...\n it does nothing, though')[0]
>
>         parser = self.write_stanza_and_reparse(pub_binary.getIndexStanza())
>
> @@ -211,7 +249,8 @@
>
>     def test_getIndexStanza_source_stanza(self):
>         """Check a source stanza with APT parser."""
> -        pub_source = self.getPubSource()
> +        pub_source = self.getPubSource(
> +            sourcename="foo", dsc_maintainer_rfc822='Foo Bar <email address hidden>')
>
>         parser = self.write_stanza_and_reparse(pub_source.getIndexStanza())
>
> @@ -243,12 +282,12 @@
>         This test can be removed if the parser is fixed and the corrupt
>         data has been cleaned.
>         """
> -        pub_source = self.getPubSource()
> -
>         # An example of a corrupt dsc_binaries field. We need to ensure
>         # that the corruption is not carried over into the index stanza.
> -        pub_source.sourcepackagerelease.dsc_binaries = (
> -            'foo_bin,\nbar_bin,\nzed_bin')
> +        pub_source = self.getPubSource(
> +            sourcename="foo", version="666")
> +        naked_spr = removeSecurityProxy(pub_source.sourcepackagerelease)
> +        naked_spr.dsc_binaries = "foo_bin,\nbar_bin,\nzed_bin"
>

Couldn't you pass dsc_binaries as an keyword argument to getPubSource()?

> === modified file 'lib/lp/soyuz/tests/test_publishing.py'
> --- lib/lp/soyuz/tests/test_publishing.py       2010-08-02 02:13:52 +0000
> +++ lib/lp/soyuz/tests/test_publishing.py       2010-08-05 20:59:48 +0000
...
> @@ -540,7 +518,8 @@
>         for pub in pubs:
>             self.checkPastDate(pub.datesuperseded)
>             if supersededby is not None:
> -                if isinstance(pub, BinaryPackagePublishingHistory):
> +                if isinstance(
> +                    removeSecurityProxy(pub), BinaryPackagePublishingHistory):

There's a zope helper for this...
    from zope.security.proxy import isinstance as zope_isinstance

Makes the intent slightly clearer.

(Although, what the hell, almost every use of isinstance is a sign
that more polymorphism is needed).

>                     dominant = supersededby.binarypackagerelease.build
>                 else:
>                     dominant = supersededby.sourcepackagerelease
> @@ -611,7 +590,7 @@
>         self.layer.commit()
>
>         foo_name = "%s/main/f/foo/foo_666.dsc" % self.pool_dir
> -        pub_source.sync()
> +        removeSecurityProxy(pub_source).sync()

sync() isn't necessary.

>         self.assertEqual(
>             pub_source.status, PackagePublishingStatus.PUBLISHED)
>         self.assertEqual(open(foo_name).read().strip(), 'foo is happy')
> @@ -623,7 +602,7 @@
>         pub_source2.publish(self.disk_pool, self.logger)
>         self.layer.commit()
>
> -        pub_source2.sync()
> +        removeSecurityProxy(pub_source2).sync()

sync() isn't necessary.

>         self.assertEqual(
>             pub_source2.status, PackagePublishingStatus.PENDING)
>         self.assertEqual(open(foo_name).read().strip(), 'foo is happy')
> @@ -640,7 +619,7 @@
>         self.layer.commit()
>         bar_name = "%s/main/b/bar/bar_666.dsc" % self.pool_dir
>         self.assertEqual(open(bar_name).read().strip(), 'bar is good')
> -        pub_source.sync()
> +        removeSecurityProxy(pub_source).sync()

sync() is obsolete, I think.

>         self.assertEqual(
>             pub_source.status, PackagePublishingStatus.PUBLISHED)
>
> @@ -648,7 +627,7 @@
>             sourcename='bar', filecontent='bar is good')
>         pub_source2.publish(self.disk_pool, self.logger)
>         self.layer.commit()
> -        pub_source2.sync()
> +        removeSecurityProxy(pub_source2).sync()

I don't think you need to sync().

>         self.assertEqual(
>             pub_source2.status, PackagePublishingStatus.PUBLISHED)
>
> @@ -668,8 +647,8 @@
>         pub_source2.publish(self.disk_pool, self.logger)
>         self.layer.commit()
>
> -        pub_source.sync()
> -        pub_source2.sync()
> +        removeSecurityProxy(pub_source).sync()
> +        removeSecurityProxy(pub_source2).sync()

sync() isn't necessary.

>         self.assertEqual(
>             pub_source.status, PackagePublishingStatus.PUBLISHED)
>         self.assertEqual(
> @@ -688,7 +667,7 @@
>         pub_source3.publish(self.disk_pool, self.logger)
>         self.layer.commit()
>
> -        pub_source3.sync()
> +        removeSecurityProxy(pub_source3).sync()

sync() probably isn't necessary.

>         self.assertEqual(
>             pub_source3.status, PackagePublishingStatus.PENDING)
>
> @@ -709,7 +688,7 @@
>         pub_source.publish(test_disk_pool, self.logger)
>         self.layer.commit()
>
> -        pub_source.sync()
> +        removeSecurityProxy(pub_source).sync()

Probably don't need sync()

> @@ -795,7 +774,7 @@
>
>         # Adjust the source package release original component.
>         universe = getUtility(IComponentSet)['universe']
> -        source.sourcepackagerelease.component = universe
> +        removeSecurityProxy(source.sourcepackagerelease).component = universe
>

Comment here would be useful, I think. Maybe more about why we're
changing the component than why you can't do it without rSP.

>         self.copyAndCheck(source, source.distroseries, 'universe')
>
...
> === modified file 'lib/lp/soyuz/tests/test_publishing_top_level_api.py'
> --- lib/lp/soyuz/tests/test_publishing_top_level_api.py 2010-07-20 12:06:36 +0000
> +++ lib/lp/soyuz/tests/test_publishing_top_level_api.py 2010-08-05 20:59:48 +0000
> @@ -3,6 +3,8 @@
>
>  """Test top-level publication API in Soyuz."""
>
> +from zope.security.proxy import removeSecurityProxy
> +
>  from lp.soyuz.tests.test_publishing import TestNativePublishingBase
>
>  from lp.registry.interfaces.series import SeriesStatus
> @@ -109,8 +111,8 @@
>         self._publish(pocket=pocket)
>
>         # source and binary PUBLISHED in database.
> -        pub_source.sync()
> -        pub_bin.sync()
> +        removeSecurityProxy(pub_source).sync()
> +        removeSecurityProxy(pub_bin).sync()

Don't need sync(), I think.

>         self.assertEqual(pub_source.status, PackagePublishingStatus.PUBLISHED)
>         self.assertEqual(pub_bin.status, PackagePublishingStatus.PUBLISHED)
>

Woot. Thanks.
jml