Am 13.04.2010 13:57, schrieb Jelmer Vernooij:
> This branch should make process-accepted a bit more robust. It changes it to file an OOPS when it encounters an exception while processing an item in the queue, rather than re-raising that exception, skipping the processing of the rest of the queue and rolling back the transactions.
>
> This should make it impossible for a single problematic package to block process-accepted, as happened about a week ago.
Very good, thank you for fixing this!
> === modified file 'lib/lp/soyuz/scripts/processaccepted.py'
> --- lib/lp/soyuz/scripts/processaccepted.py 2010-02-23 10:30:56 +0000
> +++ lib/lp/soyuz/scripts/processaccepted.py 2010-04-13 11:56:59 +0000
> @@ -11,12 +11,16 @@
> 'ProcessAccepted',
> ]
>
> +import sys
> +
> from zope.component import getUtility
>
> from lp.archiveuploader.tagfiles import parse_tagfile_lines
> from lp.bugs.interfaces.bug import IBugSet
> from lp.bugs.interfaces.bugtask import BugTaskStatus
> from canonical.launchpad.interfaces.launchpad import ILaunchpadCelebrities
> +from canonical.launchpad.webapp.errorlog import (
> + ErrorReportingUtility, ScriptRequest)
> from canonical.launchpad.webapp.interfaces import NotFoundError
> from lp.soyuz.interfaces.archive import ArchivePurpose, IArchiveSet
> from lp.registry.interfaces.distribution import IDistributionSet
> @@ -244,11 +248,15 @@
> for queue_item in queue_items:
> try:
> queue_item.realiseUpload(self.logger)
Um, I thought we use en_US as the standard language ... ;-)
Not for you to fix here, though. (realizeUpload)
OK, we talked about this on IRC. I suggest that you put the "getUtility().getByName" call into the setUp method and use something like "self.distribution" here. If possible, use a factory-made distribution, if not, add a comment in setUp as to why you are using "ubuntutest".
Actually, TestsStyleGuide proposes to use underscores, like in function names. Also, I'd be more specific about what is going on. How about "test_oops_reporting"?
> + """Test that a broken package doesn't block the publication of other
> + packages."""
> + # Attempt to upload one source to a frozen series
> + distroseries = self.factory.makeDistroSeries(
> + distribution=getUtility(IDistributionSet).getByName('ubuntutest'))
Oh, it was in a different state before (by default)? I am not very familiar with SeriesStatus, I admit ... May you could make this clearer with a comment or two?
> + # Also upload some other things
> + other_source = self.createWaitingAcceptancePackage()
> + script = self.getScript([])
> + script.main()
> +
> + # The other source should be published now
> + published_main = self.stp.ubuntutest.main_archive.getPublishedSources(
> + name=self.test_package_name)
> + self.assertEqual(published_main.count(), 1)
> +
> + # And an oops should be filed for the first
> + error_utility = ErrorReportingUtility()
> + error_report = error_utility.getLastOopsReport()
> + fp = StringIO()
> + error_report.write(fp)
> + error_text = fp.getvalue()
> + self.failUnless("error-explanation=Failure processing queue_item"
> + in error_text)
I started out using "failUnless", too, but was pointed by a reviewer to the fact that "assert" is used far more in our code. You even see it in this file.
Also, it would be nice to be able to use "assertEqual" here to compare the error text completely. Can you not extract the id of the queue item to put into the string? Also, the line wrapping needs to be fixed.
Am 13.04.2010 13:57, schrieb Jelmer Vernooij:
> This branch should make process-accepted a bit more robust. It changes it to file an OOPS when it encounters an exception while processing an item in the queue, rather than re-raising that exception, skipping the processing of the rest of the queue and rolling back the transactions.
>
> This should make it impossible for a single problematic package to block process-accepted, as happened about a week ago.
Very good, thank you for fixing this!
> === modified file 'lib/lp/ soyuz/scripts/ processaccepted .py' soyuz/scripts/ processaccepted .py 2010-02-23 10:30:56 +0000 soyuz/scripts/ processaccepted .py 2010-04-13 11:56:59 +0000 der.tagfiles import parse_tagfile_lines interfaces. bug import IBugSet interfaces. bugtask import BugTaskStatus launchpad. interfaces. launchpad import ILaunchpadCeleb rities launchpad. webapp. errorlog import ( tility, ScriptRequest) launchpad. webapp. interfaces import NotFoundError interfaces. archive import ArchivePurpose, IArchiveSet interfaces. distribution import IDistributionSet realiseUpload( self.logger)
> --- lib/lp/
> +++ lib/lp/
> @@ -11,12 +11,16 @@
> 'ProcessAccepted',
> ]
>
> +import sys
> +
> from zope.component import getUtility
>
> from lp.archiveuploa
> from lp.bugs.
> from lp.bugs.
> from canonical.
> +from canonical.
> + ErrorReportingU
> from canonical.
> from lp.soyuz.
> from lp.registry.
> @@ -244,11 +248,15 @@
> for queue_item in queue_items:
> try:
> queue_item.
Um, I thought we use en_US as the standard language ... ;-)
Not for you to fix here, though. (realizeUpload)
> - except: explanation' , message)] properties) tility( ) raising( sys.exc_ info(), request) error(' %s (%s)' % (message,
> - self.logger.error(
> - "Failure processing queue_item %d" % (
> - queue_item.id), exc_info=True)
> - raise
> + except Exception:
> + message = "Failure processing queue_item %d" % (
> + queue_item.id)
> + properties = [('error-
> + request = ScriptRequest(
> + error_utility = ErrorReportingU
> + error_utility.
> + self.logger.
> + request.oopsid))
Please put the newline right after the opening "(", as I think that conforms more to our coding style (similar to a function call in looks).
I have never had to file an OOPS before so I trust that this is the right way to do it.
> else: queue_ids. append( queue_item. id) soyuz/tests/ test_processacc epted.py' soyuz/tests/ test_processacc epted.py 2010-02-23 10:30:56 +0000 soyuz/tests/ test_processacc epted.py 2010-04-13 11:56:59 +0000 accepted. py""" launchpad. scripts import QuietFakeLogger launchpad. webapp. errorlog import ErrorReportingU tility ssLayer interfaces. distribution import IDistributionSet interfaces. series import SeriesStatus interfaces. archive import ArchivePurpose interfaces. publishing import PackagePublishi ngStatus scripts. processaccepted import ProcessAccepted ceptancePackage (self, archive=None): ceptancePackage (self, archive=None, distroseries=None, t).getByName( 'ubuntu' ).main_ archive t).getByName( 'ubuntutest' ).main_ archive
> processed_
>
>
> === modified file 'lib/lp/
> --- lib/lp/
> +++ lib/lp/
> @@ -3,13 +3,17 @@
>
> """Test process-
>
> +from cStringIO import StringIO
> +
> from zope.component import getUtility
>
> from canonical.config import config
> from canonical.
> +from canonical.
> from canonical.testing import LaunchpadZopele
>
> from lp.registry.
> +from lp.registry.
> from lp.soyuz.
> from lp.soyuz.
> from lp.soyuz.
> @@ -39,13 +43,45 @@
> script.txn = self.layer.txn
> return script
>
> - def createWaitingAc
> + def createWaitingAc
> + sourcename=None):
> """Create some pending publications."""
> if archive is None:
> archive = getUtility(
> - IDistributionSe
> + IDistributionSe
OK, we talked about this on IRC. I suggest that you put the "getUtility( ).getByName" call into the setUp method and use something like "self.distribution" here. If possible, use a factory-made distribution, if not, add a comment in setUp as to why you are using "ubuntutest".
> + if sourcename is None: package_ name getPubSource( self.test_ package_ name, spr_only=True) sourcename, distroseries= distroseries, self):
> + sourcename = self.test_
> return self.stp.
> - archive=archive, sourcename=
> + archive=archive, sourcename=
> + spr_only=True)
> +
> + def testRobustness(
Actually, TestsStyleGuide proposes to use underscores, like in function names. Also, I'd be more specific about what is going on. How about "test_oops_ reporting" ?
> + """Test that a broken package doesn't block the publication of other makeDistroSerie s( getUtility( IDistributionSe t).getByName( 'ubuntutest' ))
> + packages."""
> + # Attempt to upload one source to a frozen series
> + distroseries = self.factory.
> + distribution=
What makes the series frozen, I wonder?
> + broken_source = self.createWait ingAcceptancePa ckage( distroseries, sourcename= "notaccepted" ) SUPPORTED
> + distroseries=
> + distroseries.status = SeriesStatus.
Oh, it was in a different state before (by default)? I am not very familiar with SeriesStatus, I admit ... May you could make this clearer with a comment or two?
> + # Also upload some other things ingAcceptancePa ckage() ubuntutest. main_archive. getPublishedSou rces( test_package_ name) l(published_ main.count( ), 1) tility( ) getLastOopsRepo rt() write(fp) ("error- explanation= Failure processing queue_item"
> + other_source = self.createWait
> + script = self.getScript([])
> + script.main()
> +
> + # The other source should be published now
> + published_main = self.stp.
> + name=self.
> + self.assertEqua
> +
> + # And an oops should be filed for the first
> + error_utility = ErrorReportingU
> + error_report = error_utility.
> + fp = StringIO()
> + error_report.
> + error_text = fp.getvalue()
> + self.failUnless
> + in error_text)
I started out using "failUnless", too, but was pointed by a reviewer to the fact that "assert" is used far more in our code. You even see it in this file.
Also, it would be nice to be able to use "assertEqual" here to compare the error text completely. Can you not extract the id of the queue item to put into the string? Also, the line wrapping needs to be fixed.
> rchives( self):
> def testAcceptCopyA
Ah, I see you have had bad examples ... ;-) I think this is really "test_accept_ copy_archives" ?
> """Test that publications in a copy archive are accepted properly."""
Nothing really bad that needs serious fixing but please do consider applying my suggestions before landing this.
Cheers,
Henning