Hi Graham, This mad refactorage is looking good :) I have a few comments but nothing major, so r=me. Gavin. > === modified file 'lib/lp/bugs/doc/externalbugtracker.txt' > --- lib/lp/bugs/doc/externalbugtracker.txt 2010-04-21 10:33:55 +0000 > +++ lib/lp/bugs/doc/externalbugtracker.txt 2010-04-29 10:42:49 +0000 > @@ -641,7 +641,7 @@ > > === Converting statuses === > > -Once it has retrieved the bugs from the remote server, CheckwatchesMaster > +Once it has retrieved the bugs from the remote server, RemoteBugUpdater > attempts to convert their statuses into Launchpad BugTaskStatuses by > calling the convertRemoteStatus() method on the ExternalBugTracker via > its own _convertRemoteStatus() method. > @@ -660,17 +660,17 @@ > ... else: > ... raise UnknownRemoteStatusError(remote_status) > > -CheckwatchesMaster._convertRemoteStatus() will handle these errors and will > +RemoteBugUpdater._convertRemoteStatus() will handle these errors and will > return BugTaskStatus.UNKNOWN when they occur. It will also log a > warning. > > - >>> status = bug_watch_updater._convertRemoteStatus( > - ... StatusConvertingExternalBugTracker(), 'new') > + >>> remote_bug_updater = bug_watch_updater._makeRemoteBugUpdater( > + ... StatusConvertingExternalBugTracker(), '1', [1], []) > + >>> status = remote_bug_updater._convertRemoteStatus('new') > >>> print status.title > New > > - >>> status = bug_watch_updater._convertRemoteStatus( > - ... StatusConvertingExternalBugTracker(), 'spam') > + >>> status = remote_bug_updater._convertRemoteStatus('spam') > WARNING...Unknown remote status 'spam'. (OOPS-...) > >>> print status.title > Unknown > > === modified file 'lib/lp/bugs/scripts/checkwatches/bugwatchupdater.py' > --- lib/lp/bugs/scripts/checkwatches/bugwatchupdater.py 2010-04-26 12:34:47 +0000 > +++ lib/lp/bugs/scripts/checkwatches/bugwatchupdater.py 2010-04-29 10:42:49 +0000 > @@ -26,9 +26,12 @@ > from lp.bugs.interfaces.bug import IBugSet > from lp.bugs.scripts.checkwatches.base import ( > WorkingBase, commit_before) > +from lp.bugs.scripts.checkwatches.utilities import ( > + get_remote_system_oops_properties) > from lp.registry.interfaces.person import PersonCreationRationale > > > + > class BugWatchUpdater(WorkingBase): > """Handles the updating of a single BugWatch for checkwatches.""" > > @@ -81,10 +84,6 @@ > @commit_before > def importBugComments(self): > """Import all the comments from the remote bug.""" > - # Avoid circularity. > - from lp.bugs.scripts.checkwatches.core import ( > - get_remote_system_oops_properties) > - > with self.transaction: > local_bug_id = self.bug_watch.bug.id > remote_bug_id = self.bug_watch.remotebug > > === modified file 'lib/lp/bugs/scripts/checkwatches/core.py' > --- lib/lp/bugs/scripts/checkwatches/core.py 2010-04-26 12:34:47 +0000 > +++ lib/lp/bugs/scripts/checkwatches/core.py 2010-04-29 10:42:49 +0000 > @@ -37,22 +37,21 @@ > from canonical.database.constants import UTC_NOW > from canonical.database.sqlbase import flush_database_updates > from canonical.launchpad.interfaces import ( > - BugTaskStatus, BugWatchActivityStatus, CreateBugParams, > - IBugTrackerSet, IBugWatchSet, IDistribution, ILaunchpadCelebrities, > - IPersonSet, ISupportsCommentImport, ISupportsCommentPushing, > - PersonCreationRationale, UNKNOWN_REMOTE_STATUS) > + CreateBugParams, IBugTrackerSet, IBugWatchSet, IDistribution, > + ILaunchpadCelebrities, IPersonSet, ISupportsCommentImport, > + ISupportsCommentPushing, PersonCreationRationale) > from canonical.launchpad.scripts.logger import log as default_log > > from lp.bugs import externalbugtracker > from lp.bugs.externalbugtracker import ( > - BATCH_SIZE_UNLIMITED, BugNotFound, BugTrackerConnectError, > - BugWatchUpdateError, InvalidBugId, PrivateRemoteBug, > - UnknownBugTrackerTypeError, UnknownRemoteStatusError, UnparseableBugData, > - UnparseableBugTrackerVersion, UnsupportedBugTrackerVersion) > + BATCH_SIZE_UNLIMITED, BugWatchUpdateError, > + UnknownBugTrackerTypeError) > from lp.bugs.interfaces.externalbugtracker import ISupportsBackLinking > from lp.bugs.scripts.checkwatches.base import ( > WorkingBase, commit_before, with_interaction) > -from lp.bugs.scripts.checkwatches.bugwatchupdater import BugWatchUpdater > +from lp.bugs.scripts.checkwatches.remotebugupdater import RemoteBugUpdater > +from lp.bugs.scripts.checkwatches.utilities import ( > + get_bugwatcherrortype_for_error) > from lp.services.scripts.base import LaunchpadCronScript > > > @@ -77,30 +76,6 @@ > """Time difference between ourselves and the remote server is too much.""" > > > -_exception_to_bugwatcherrortype = [ > - (BugTrackerConnectError, BugWatchActivityStatus.CONNECTION_ERROR), > - (PrivateRemoteBug, BugWatchActivityStatus.PRIVATE_REMOTE_BUG), > - (UnparseableBugData, BugWatchActivityStatus.UNPARSABLE_BUG), > - (UnparseableBugTrackerVersion, > - BugWatchActivityStatus.UNPARSABLE_BUG_TRACKER), > - (UnsupportedBugTrackerVersion, > - BugWatchActivityStatus.UNSUPPORTED_BUG_TRACKER), > - (UnknownBugTrackerTypeError, > - BugWatchActivityStatus.UNSUPPORTED_BUG_TRACKER), > - (InvalidBugId, BugWatchActivityStatus.INVALID_BUG_ID), > - (BugNotFound, BugWatchActivityStatus.BUG_NOT_FOUND), > - (PrivateRemoteBug, BugWatchActivityStatus.PRIVATE_REMOTE_BUG), > - (socket.timeout, BugWatchActivityStatus.TIMEOUT)] > - > -def get_bugwatcherrortype_for_error(error): > - """Return the correct `BugWatchActivityStatus` for a given error.""" > - for exc_type, bugwatcherrortype in _exception_to_bugwatcherrortype: > - if isinstance(error, exc_type): > - return bugwatcherrortype > - else: > - return BugWatchActivityStatus.UNKNOWN > - > - > def unique(iterator): > """Generate only unique items from an iterator.""" > seen = set() > @@ -126,23 +101,6 @@ > int(SUGGESTED_BATCH_SIZE_PROPORTION * num_watches)) > > > -def get_remote_system_oops_properties(remote_system): > - """Return (name, value) tuples describing a remote system. > - > - Each item in the list is intended for use as an OOPS property. > - > - :remote_system: The `ExternalBugTracker` instance from which the > - OOPS properties should be extracted. > - """ > - return [ > - ('batch_size', remote_system.batch_size), > - ('batch_query_threshold', remote_system.batch_query_threshold), > - ('sync_comments', remote_system.sync_comments), > - ('externalbugtracker', remote_system.__class__.__name__), > - ('baseurl', remote_system.baseurl) > - ] > - > - > class CheckwatchesMaster(WorkingBase): > """Takes responsibility for updating remote bug watches.""" > > @@ -168,6 +126,17 @@ > else: > self._syncable_gnome_products = list(SYNCABLE_GNOME_PRODUCTS) > > + def _makeRemoteBugUpdater(self, external_bugtracker, remote_bug, > + bug_watch_ids, unmodified_remote_ids): > + """Create and return a `RemoteBugUpdater` instance. > + > + This method exists purely for the sake of being able to > + overrride it in tests. Have you been listening to Christina Aguilera recently? s/rrr/rr/ Another way to do this with less boiler-plate is to have a remote_bug_updater_factory class attribute. The signature of _makeRemoteBugUpdater() is identical to RemoteBugUpdater.__init__(). > + """ > + return RemoteBugUpdater( > + self, external_bugtracker, remote_bug, bug_watch_ids, > + unmodified_remote_ids) > + > @with_interaction > def _bugTrackerUpdaters(self, bug_tracker_names=None): > """Yields functions that can be used to update each bug tracker.""" > @@ -457,37 +426,6 @@ > self.logger.debug( > "No watches to update on %s" % bug_tracker.baseurl) > > - def _convertRemoteStatus(self, remotesystem, remote_status): > - """Convert a remote bug status to a Launchpad status and return it. > - > - :param remotesystem: The `IExternalBugTracker` instance > - representing the remote system. > - :param remote_status: The remote status to be converted into a > - Launchpad status. > - > - If the remote status cannot be mapped to a Launchpad status, > - BugTaskStatus.UNKNOWN will be returned and a warning will be > - logged. > - """ > - # We don't bother trying to convert UNKNOWN_REMOTE_STATUS. > - if remote_status == UNKNOWN_REMOTE_STATUS: > - return BugTaskStatus.UNKNOWN > - > - try: > - launchpad_status = remotesystem.convertRemoteStatus( > - remote_status) > - except UnknownRemoteStatusError: > - # We log the warning, since we need to know about statuses > - # that we don't handle correctly. > - self.warning( > - "Unknown remote status '%s'." % remote_status, > - get_remote_system_oops_properties(remotesystem), > - sys.exc_info()) > - > - launchpad_status = BugTaskStatus.UNKNOWN > - > - return launchpad_status > - > def _getRemoteIdsToCheck(self, remotesystem, bug_watches, > server_time=None, now=None, batch_size=None): > """Return the remote bug IDs to check for a set of bug watches. > @@ -609,17 +547,6 @@ > 'unmodified_remote_ids': sorted(unmodified_remote_ids), > } > > - def _getBugWatchesForRemoteBug(self, remote_bug_id, bug_watch_ids): > - """Return a list of bug watches for the given remote bug. > - > - The returned watches will all be members of `bug_watch_ids`. > - > - This method exists primarily to be overridden during testing. > - """ > - return list( > - getUtility(IBugWatchSet).getBugWatchesForRemoteBug( > - remote_bug_id, bug_watch_ids)) > - > # XXX gmb 2008-11-07 [bug=295319] > # This method is 186 lines long. It needs to be shorter. > @commit_before > @@ -706,124 +633,12 @@ > "Comment importing supported, but server time can't be" > " trusted. No comments will be imported.") > > - error_type_messages = { > - BugWatchActivityStatus.INVALID_BUG_ID: > - ("Invalid bug %(bug_id)r on %(base_url)s " > - "(local bugs: %(local_ids)s)."), > - BugWatchActivityStatus.BUG_NOT_FOUND: > - ("Didn't find bug %(bug_id)r on %(base_url)s " > - "(local bugs: %(local_ids)s)."), > - BugWatchActivityStatus.PRIVATE_REMOTE_BUG: > - ("Remote bug %(bug_id)r on %(base_url)s is private " > - "(local bugs: %(local_ids)s)."), > - } > - error_type_message_default = ( > - "remote bug: %(bug_id)r; " > - "base url: %(base_url)s; " > - "local bugs: %(local_ids)s" > - ) > - > for remote_bug_id in all_remote_ids: > - with self.transaction: > - bug_watches = self._getBugWatchesForRemoteBug( > - remote_bug_id, bug_watch_ids) > - # If there aren't any bug watches for this remote bug, > - # just log a warning and carry on. > - if len(bug_watches) == 0: > - self.warning( > - "Spurious remote bug ID: No watches found for " > - "remote bug %s on %s" % ( > - remote_bug_id, remotesystem.baseurl)) > - continue > - # Mark them all as checked. > - for bug_watch in bug_watches: > - bug_watch.lastchecked = UTC_NOW > - bug_watch.next_check = None > - # Next if this one is definitely unmodified. > - if remote_bug_id in unmodified_remote_ids: > - continue > - # Save the remote bug URL for error reporting. > - remote_bug_url = bug_watches[0].url > - # Save the list of local bug IDs for error reporting. > - local_ids = ", ".join( > - str(bug_id) for bug_id in sorted( > - watch.bug.id for watch in bug_watches)) > - > - try: > - new_remote_status = None > - new_malone_status = None > - new_remote_importance = None > - new_malone_importance = None > - error = None > - oops_id = None > - > - # XXX: 2007-10-17 Graham Binns > - # This nested set of try:excepts isn't really > - # necessary and can be refactored out when bug > - # 136391 is dealt with. > - try: > - new_remote_status = ( > - remotesystem.getRemoteStatus(remote_bug_id)) > - new_malone_status = self._convertRemoteStatus( > - remotesystem, new_remote_status) > - new_remote_importance = ( > - remotesystem.getRemoteImportance(remote_bug_id)) > - new_malone_importance = ( > - remotesystem.convertRemoteImportance( > - new_remote_importance)) > - except (InvalidBugId, BugNotFound, PrivateRemoteBug), ex: > - error = get_bugwatcherrortype_for_error(ex) > - message = error_type_messages.get( > - error, error_type_message_default) > - oops_id = self.warning( > - message % { > - 'bug_id': remote_bug_id, > - 'base_url': remotesystem.baseurl, > - 'local_ids': local_ids, > - }, > - properties=[ > - ('URL', remote_bug_url), > - ('bug_id', remote_bug_id), > - ('local_ids', local_ids), > - ] + get_remote_system_oops_properties( > - remotesystem), > - info=sys.exc_info()) > - > - for bug_watch in bug_watches: > - bug_watch_updater = BugWatchUpdater( > - self, bug_watch, remotesystem) > - > - bug_watch_updater.updateBugWatch( > - new_remote_status, new_malone_status, > - new_remote_importance, new_malone_importance, > - can_import_comments, can_push_comments, > - can_back_link, error, oops_id) > - > - except (KeyboardInterrupt, SystemExit): > - # We should never catch KeyboardInterrupt or SystemExit. > - raise > - > - except Exception, error: > - # Send the error to the log. > - oops_id = self.error( > - "Failure updating bug %r on %s (local bugs: %s)." % > - (remote_bug_id, bug_tracker_url, local_ids), > - properties=[ > - ('URL', remote_bug_url), > - ('bug_id', remote_bug_id), > - ('local_ids', local_ids)] + > - get_remote_system_oops_properties(remotesystem)) > - # We record errors against the bug watches and update > - # their lastchecked dates so that we don't try to > - # re-check them every time checkwatches runs. > - error_type = get_bugwatcherrortype_for_error(error) > - with self.transaction: > - for bug_watch in bug_watches: > - bug_watch.lastchecked = UTC_NOW > - bug_watch.next_check = None > - bug_watch.last_error_type = error_type > - bug_watch.addActivity( > - result=error_type, oops_id=oops_id) > + remote_bug_updater = self._makeRemoteBugUpdater( > + remotesystem, remote_bug_id, bug_watch_ids, > + unmodified_remote_ids) > + remote_bug_updater.updateRemoteBug( > + can_import_comments, can_push_comments, can_back_link) > > def importBug(self, external_bugtracker, bugtracker, bug_target, > remote_bug): > > === added file 'lib/lp/bugs/scripts/checkwatches/remotebugupdater.py' > --- lib/lp/bugs/scripts/checkwatches/remotebugupdater.py 1970-01-01 00:00:00 +0000 > +++ lib/lp/bugs/scripts/checkwatches/remotebugupdater.py 2010-04-29 10:42:49 +0000 > @@ -0,0 +1,206 @@ > +# Copyright 2009 Canonical Ltd. This software is licensed under the > +# GNU Affero General Public License version 3 (see the file LICENSE). > + > +"""Classes and logic for the remote bug updater.""" > + > +from __future__ import with_statement > + > +__metaclass__ = type > +__all__ = [ > + 'RemoteBugUpdater', > + ] > + > +import sys > + > +from zope.component import getUtility > + > +from canonical.database.constants import UTC_NOW > + > +from lp.bugs.externalbugtracker import ( > + BugNotFound, InvalidBugId, PrivateRemoteBug, UnknownRemoteStatusError) > + > +from lp.bugs.interfaces.bugtask import BugTaskStatus > +from lp.bugs.interfaces.bugwatch import BugWatchActivityStatus, IBugWatchSet > +from lp.bugs.interfaces.externalbugtracker import UNKNOWN_REMOTE_STATUS > +from lp.bugs.scripts.checkwatches.base import WorkingBase, commit_before > +from lp.bugs.scripts.checkwatches.bugwatchupdater import BugWatchUpdater > +from lp.bugs.scripts.checkwatches.utilities import ( > + get_bugwatcherrortype_for_error, get_remote_system_oops_properties) > + > + > +class RemoteBugUpdater(WorkingBase): > + > + def __init__(self, parent, external_bugtracker, remote_bug, > + bug_watch_ids, unmodified_remote_ids): > + self.initFromParent(parent) > + self.external_bugtracker = external_bugtracker > + self.bug_tracker_url = external_bugtracker.baseurl > + self.remote_bug = remote_bug > + self.bug_watch_ids = bug_watch_ids > + self.unmodified_remote_ids = unmodified_remote_ids > + > + self.error_type_messages = { > + BugWatchActivityStatus.INVALID_BUG_ID: > + ("Invalid bug %(bug_id)r on %(base_url)s " > + "(local bugs: %(local_ids)s)."), > + BugWatchActivityStatus.BUG_NOT_FOUND: > + ("Didn't find bug %(bug_id)r on %(base_url)s " > + "(local bugs: %(local_ids)s)."), > + BugWatchActivityStatus.PRIVATE_REMOTE_BUG: > + ("Remote bug %(bug_id)r on %(base_url)s is private " > + "(local bugs: %(local_ids)s)."), > + } > + self.error_type_message_default = ( > + "remote bug: %(bug_id)r; " > + "base url: %(base_url)s; " > + "local bugs: %(local_ids)s" > + ) > + > + def _getBugWatchesForRemoteBug(self): > + """Return a list of bug watches for the current remote bug. > + > + The returned watches will all be members of `self.bug_watch_ids`. > + > + This method exists primarily to be overridden during testing. > + """ > + return list( > + getUtility(IBugWatchSet).getBugWatchesForRemoteBug( > + self.remote_bug, self.bug_watch_ids)) > + > + @commit_before > + def updateRemoteBug(self, can_import_comments=False, > + can_push_comments=False, can_back_link=False): The can_* arguments don't change for the life of the RemoteBugUpdater, so I think it makes sense to set them in __init__(). This could be done by just passing server_time in. > + # Avoid circular imports Already avoided. > + with self.transaction: > + bug_watches = self._getBugWatchesForRemoteBug() > + # If there aren't any bug watches for this remote bug, > + # just log a warning and carry on. > + if len(bug_watches) == 0: > + self.warning( > + "Spurious remote bug ID: No watches found for " > + "remote bug %s on %s" % ( > + self.remote_bug, self.external_bugtracker.baseurl)) > + return > + # Mark them all as checked. > + for bug_watch in bug_watches: > + bug_watch.lastchecked = UTC_NOW > + bug_watch.next_check = None > + # Next if this one is definitely unmodified. s/Next/Return/ > + if self.remote_bug in self.unmodified_remote_ids: > + return > + # Save the remote bug URL for error reporting. > + remote_bug_url = bug_watches[0].url > + # Save the list of local bug IDs for error reporting. > + local_ids = ", ".join( > + str(bug_id) for bug_id in sorted( > + watch.bug.id for watch in bug_watches)) > + > + try: > + new_remote_status = None > + new_malone_status = None > + new_remote_importance = None > + new_malone_importance = None > + error = None > + oops_id = None > + > + # XXX: 2007-10-17 Graham Binns > + # This nested set of try:excepts isn't really > + # necessary and can be refactored out when bug > + # 136391 is dealt with. > + try: > + new_remote_status = ( > + self.external_bugtracker.getRemoteStatus( > + self.remote_bug)) > + new_malone_status = self._convertRemoteStatus( > + new_remote_status) > + new_remote_importance = ( > + self.external_bugtracker.getRemoteImportance( > + self.remote_bug)) > + new_malone_importance = ( > + self.external_bugtracker.convertRemoteImportance( > + new_remote_importance)) > + except (InvalidBugId, BugNotFound, PrivateRemoteBug), ex: > + error = get_bugwatcherrortype_for_error(ex) > + message = self.error_type_messages.get( > + error, self.error_type_message_default) > + oops_id = self.warning( > + message % { > + 'bug_id': self.remote_bug, > + 'base_url': self.external_bugtracker.baseurl, > + 'local_ids': local_ids, > + }, > + properties=[ > + ('URL', remote_bug_url), > + ('bug_id', self.remote_bug), > + ('local_ids', local_ids), > + ] + get_remote_system_oops_properties( > + self.external_bugtracker), > + info=sys.exc_info()) > + > + for bug_watch in bug_watches: > + bug_watch_updater = BugWatchUpdater( > + self, bug_watch, self.external_bugtracker) > + > + bug_watch_updater.updateBugWatch( > + new_remote_status, new_malone_status, > + new_remote_importance, new_malone_importance, > + can_import_comments, can_push_comments, > + can_back_link, error, oops_id) I didn't notice this before; the error and oops from syncing the status are passed into BugWatchUpdater to do things with, yet we also update bug watches in this function. Not something to change in this branch, it's just something else to consider when fixing up the exception handling. > + > + except (KeyboardInterrupt, SystemExit): > + # We should never catch KeyboardInterrupt or SystemExit. > + raise > + Now that KeyboardInterrupt and SystemExit are not subclasses of Exception this handler (in this context) is basically a no-op, so consider removing it. > + except Exception, error: > + # Send the error to the log. > + oops_id = self.error( > + "Failure updating bug %r on %s (local bugs: %s)." % > + (self.remote_bug, self.bug_tracker_url, local_ids), > + properties=[ > + ('URL', remote_bug_url), > + ('bug_id', self.remote_bug), > + ('local_ids', local_ids)] + > + get_remote_system_oops_properties( > + self.external_bugtracker)) > + # We record errors against the bug watches and update > + # their lastchecked dates so that we don't try to > + # re-check them every time checkwatches runs. > + error_type = get_bugwatcherrortype_for_error(error) > + with self.transaction: > + for bug_watch in bug_watches: > + bug_watch.lastchecked = UTC_NOW > + bug_watch.next_check = None > + bug_watch.last_error_type = error_type > + bug_watch.addActivity( > + result=error_type, oops_id=oops_id) > + > + def _convertRemoteStatus(self, remote_status): > + """Convert a remote bug status to a Launchpad status and return it. > + > + :param remote_status: The remote status to be converted into a > + Launchpad status. > + > + If the remote status cannot be mapped to a Launchpad status, > + BugTaskStatus.UNKNOWN will be returned and a warning will be > + logged. > + """ > + # We don't bother trying to convert UNKNOWN_REMOTE_STATUS. > + if remote_status == UNKNOWN_REMOTE_STATUS: > + return BugTaskStatus.UNKNOWN > + > + try: > + launchpad_status = self.external_bugtracker.convertRemoteStatus( > + remote_status) > + except UnknownRemoteStatusError: > + # We log the warning, since we need to know about statuses > + # that we don't handle correctly. > + self.warning( > + "Unknown remote status '%s'." % remote_status, > + get_remote_system_oops_properties( > + self.external_bugtracker), > + sys.exc_info()) > + > + launchpad_status = BugTaskStatus.UNKNOWN > + > + return launchpad_status > + > > === modified file 'lib/lp/bugs/scripts/checkwatches/tests/test_core.py' > --- lib/lp/bugs/scripts/checkwatches/tests/test_core.py 2010-04-21 21:06:56 +0000 > +++ lib/lp/bugs/scripts/checkwatches/tests/test_core.py 2010-04-29 10:42:49 +0000 > @@ -24,9 +24,11 @@ > from lp.bugs.externalbugtracker.bugzilla import BugzillaAPI > from lp.bugs.interfaces.bugtracker import IBugTrackerSet > from lp.bugs.scripts import checkwatches > +from lp.bugs.scripts.checkwatches.base import ( > + CheckWatchesErrorUtility, WorkingBase) > from lp.bugs.scripts.checkwatches.core import ( > - CheckwatchesMaster, TwistedThreadScheduler) > -from lp.bugs.scripts.checkwatches.base import CheckWatchesErrorUtility > + CheckwatchesMaster, LOGIN, TwistedThreadScheduler) > +from lp.bugs.scripts.checkwatches.remotebugupdater import RemoteBugUpdater > from lp.bugs.tests.externalbugtracker import ( > TestBugzillaAPIXMLRPCTransport, TestExternalBugTracker, new_bugtracker) > from lp.testing import TestCaseWithFactory, ZopeTestInSubProcess > @@ -57,10 +59,10 @@ > return self > > > -class NoBugWatchesByRemoteBugUpdater(checkwatches.CheckwatchesMaster): > - """A subclass of CheckwatchesMaster with methods overridden for testing.""" > +class NoBugWatchesByRemoteBugUpdater(RemoteBugUpdater): > + """A subclass of RemoteBugUpdater with methods overridden for testing.""" > > - def _getBugWatchesForRemoteBug(self, remote_bug_id, bug_watch_ids): > + def _getBugWatchesForRemoteBug(self): > """Return an empty list. > > This method overrides _getBugWatchesForRemoteBug() so that bug > @@ -154,10 +156,8 @@ > > def test_bug_497141(self): > # Regression test for bug 497141. KeyErrors raised in > - # CheckwatchesMaster.updateBugWatches() shouldn't cause > + # RemoteBugUpdater.updateRemoteBug() shouldn't cause > # checkwatches to abort. > - updater = NoBugWatchesByRemoteBugUpdater( > - transaction.manager, QuietFakeLogger()) > > # Create a couple of bug watches for testing purposes. > bug_tracker = self.factory.makeBugTracker() > @@ -170,17 +170,27 @@ > remote_system = NonConnectingBugzillaAPI( > bug_tracker.baseurl, xmlrpc_transport=test_transport) > > - # Calling updateBugWatches() shouldn't raise a KeyError, even > - # though with our broken updater _getExternalBugTrackersAndWatches() > - # will return an empty dict. > - updater.updateBugWatches(remote_system, bug_watches) > - > - # An error will have been logged instead of the KeyError being > - # raised. > - error_utility = CheckWatchesErrorUtility() > - last_oops = error_utility.getLastOopsReport() > - self.assertTrue( > - last_oops.value.startswith('Spurious remote bug ID')) > + working_base = WorkingBase() > + working_base.init(LOGIN, transaction.manager, QuietFakeLogger()) > + > + for bug_watch in bug_watches: > + updater = NoBugWatchesByRemoteBugUpdater( > + working_base, remote_system, bug_watch.remotebug, > + [bug_watch.id], []) > + > + # Calling updateRemoteBug() shouldn't raise a KeyError, > + # even though with our broken updater > + # _getExternalBugTrackersAndWatches() will return an empty > + # dict. > + updater.updateRemoteBug(remote_system, bug_watches) > + > + # An error will have been logged instead of the KeyError being > + # raised. > + error_utility = CheckWatchesErrorUtility() > + last_oops = error_utility.getLastOopsReport() > + self.assertTrue( > + last_oops.value.startswith('Spurious remote bug ID'), > + "Unexpected last OOPS value: %s" % last_oops.value) > > def test_suggest_batch_size(self): > class RemoteSystem: pass > @@ -197,6 +207,37 @@ > checkwatches.core.suggest_batch_size(remote_system, 99999) > self.failUnlessEqual(247, remote_system.batch_size) > > + def test__makeRemoteBugUpdater(self): > + # CheckwatchesMaster._makeRemoteBugUpdater() will return a > + # RemoteBugUpdater instance for a given remote bug, external bug > + # tracker and set of bug watches. > + remote_system = BugzillaAPI('http://example.com') > + remote_bug_id = '42' > + bug_watch_ids = [1, 2] > + unmodified_remote_ids = ['76'] > + > + checkwatches_master = CheckwatchesMaster(transaction) > + updater = checkwatches_master._makeRemoteBugUpdater( > + remote_system, remote_bug_id, bug_watch_ids, > + unmodified_remote_ids) > + > + self.assertEqual( > + remote_system, updater.external_bugtracker, > + "Unexpected external_bugtracker for RemoteBugUpdater.") > + self.assertEqual( > + remote_bug_id, updater.remote_bug, > + "RemoteBugUpdater's remote_bug should be '%s', was '%s'" % > + (remote_bug_id, updater.remote_bug)) > + self.assertEqual( > + bug_watch_ids, updater.bug_watch_ids, > + "RemoteBugUpdater's bug_watch_ids should be '%s', were '%s'" % > + (bug_watch_ids, updater.bug_watch_ids)) > + self.assertEqual( > + unmodified_remote_ids, updater.unmodified_remote_ids, > + "RemoteBugUpdater's unmodified_remote_ids should be '%s', " > + "were '%s'" % > + (unmodified_remote_ids, updater.unmodified_remote_ids)) If you go for the remote_bug_updater_factory idea from earlier, then this test could be moved into a new test_remotebugupdater module, and be called test_create(). > + > > class TestUpdateBugsWithLinkedQuestions(unittest.TestCase): > """Tests for updating bugs with linked questions.""" > > === added file 'lib/lp/bugs/scripts/checkwatches/utilities.py' > --- lib/lp/bugs/scripts/checkwatches/utilities.py 1970-01-01 00:00:00 +0000 > +++ lib/lp/bugs/scripts/checkwatches/utilities.py 2010-04-29 10:42:49 +0000 > @@ -0,0 +1,61 @@ > +# Copyright 2010 Canonical Ltd. This software is licensed under the > +# GNU Affero General Public License version 3 (see the file LICENSE). > + > +"""Utility functions for checkwatches.""" > + > +__metaclass__ = type > +__all__ = [ > + 'get_bugwatcherrortype_for_error', > + 'get_remote_system_oops_properties', > + ] > + > +import socket > + > +from lp.bugs.externalbugtracker import ( > + BugNotFound, BugTrackerConnectError, InvalidBugId, PrivateRemoteBug, > + UnknownBugTrackerTypeError, UnparseableBugData, > + UnparseableBugTrackerVersion, UnsupportedBugTrackerVersion) > + > +from lp.bugs.interfaces.bugwatch import BugWatchActivityStatus > + > + > +_exception_to_bugwatcherrortype = [ > + (BugTrackerConnectError, BugWatchActivityStatus.CONNECTION_ERROR), > + (PrivateRemoteBug, BugWatchActivityStatus.PRIVATE_REMOTE_BUG), > + (UnparseableBugData, BugWatchActivityStatus.UNPARSABLE_BUG), > + (UnparseableBugTrackerVersion, > + BugWatchActivityStatus.UNPARSABLE_BUG_TRACKER), > + (UnsupportedBugTrackerVersion, > + BugWatchActivityStatus.UNSUPPORTED_BUG_TRACKER), > + (UnknownBugTrackerTypeError, > + BugWatchActivityStatus.UNSUPPORTED_BUG_TRACKER), > + (InvalidBugId, BugWatchActivityStatus.INVALID_BUG_ID), > + (BugNotFound, BugWatchActivityStatus.BUG_NOT_FOUND), > + (PrivateRemoteBug, BugWatchActivityStatus.PRIVATE_REMOTE_BUG), > + (socket.timeout, BugWatchActivityStatus.TIMEOUT)] > + > + > +def get_bugwatcherrortype_for_error(error): > + """Return the correct `BugWatchActivityStatus` for a given error.""" > + for exc_type, bugwatcherrortype in _exception_to_bugwatcherrortype: > + if isinstance(error, exc_type): > + return bugwatcherrortype > + else: > + return BugWatchActivityStatus.UNKNOWN > + > + > +def get_remote_system_oops_properties(remote_system): > + """Return (name, value) tuples describing a remote system. > + > + Each item in the list is intended for use as an OOPS property. > + > + :remote_system: The `ExternalBugTracker` instance from which the > + OOPS properties should be extracted. > + """ > + return [ > + ('batch_size', remote_system.batch_size), > + ('batch_query_threshold', remote_system.batch_query_threshold), > + ('sync_comments', remote_system.sync_comments), > + ('externalbugtracker', remote_system.__class__.__name__), > + ('baseurl', remote_system.baseurl) > + ] > > === modified file 'lib/lp/bugs/scripts/tests/test_bugimport.py' > --- lib/lp/bugs/scripts/tests/test_bugimport.py 2010-04-21 10:30:24 +0000 > +++ lib/lp/bugs/scripts/tests/test_bugimport.py 2010-04-29 10:42:49 +0000 Consider moving this into checkwatches/tests. > @@ -28,6 +28,7 @@ > from lp.bugs.scripts import bugimport > from lp.bugs.scripts.bugimport import ET > from lp.bugs.scripts.checkwatches import CheckwatchesMaster > +from lp.bugs.scripts.checkwatches.remotebugupdater import RemoteBugUpdater > from lp.registry.interfaces.person import IPersonSet, PersonCreationRationale > from lp.registry.interfaces.product import IProductSet > from lp.registry.model.person import generate_nick > @@ -887,6 +888,27 @@ > """ > return BugTaskImportance.UNKNOWN > 2 blank lines. > +class TestRemoteBugUpdater(RemoteBugUpdater): We need to use a new prefix for test doubles, because "Test" is also used for test cases. It's confusing! Unfortunately, it's also prevalent. Ordinarily I would think about bringing this up at a reviewer meeting, but I'm on Landscape next week. If I add an agenda item, can I put your name by it? > + > + def __init__(self, parent, external_bugtracker, remote_bug, > + bug_watch_ids, unmodified_remote_ids, bugtracker): > + super(TestRemoteBugUpdater, self). __init__( > + parent, external_bugtracker, remote_bug, bug_watch_ids, > + unmodified_remote_ids) > + self.bugtracker = bugtracker > + > + def _getBugWatchesForRemoteBug(self): > + """Returns a list of fake bug watch objects. > + > + We override this method so that we always return bug watches > + from our list of fake bug watches. > + """ > + return [ > + bug_watch for bug_watch in ( > + self.bugtracker.watches_needing_update) > + if (bug_watch.remotebug == self.remote_bug and > + bug_watch.id in self.bug_watch_ids) > + ] > > > class TestCheckwatchesMaster(CheckwatchesMaster): > @@ -901,27 +923,20 @@ > """See `CheckwatchesMaster`.""" > return [(TestExternalBugTracker(bug_tracker.baseurl), bug_watches)] > > - def _getBugWatchesForRemoteBug(self, remote_bug_id, bug_watch_ids): > - """Returns a list of fake bug watch objects. > - > - We override this method so that we always return bug watches > - from our list of fake bug watches. > - """ > - return [ > - bug_watch for bug_watch in ( > - self.bugtracker.watches_needing_update) > - if (bug_watch.remotebug == remote_bug_id and > - bug_watch.id in bug_watch_ids) > - ] > - > - > -class CheckBugWatchesErrorRecoveryTestCase(unittest.TestCase): > + def _makeRemoteBugUpdater(self, external_bugtracker, remote_bug, > + bug_watch_ids, unmodified_remote_ids): > + return TestRemoteBugUpdater( > + self, external_bugtracker, remote_bug, bug_watch_ids, > + unmodified_remote_ids, self.bugtracker) > + > + > +class CheckwatchesErrorRecoveryTestCase(unittest.TestCase): > """Test that errors in the bugwatch import process don't > invalidate the entire run. > """ > layer = LaunchpadZopelessLayer > > - def test_checkbugwatches_error_recovery(self): > + def test_checkwatches_error_recovery(self): > > firefox = getUtility(IProductSet).get(4) > foobar = getUtility(IPersonSet).get(16) > If possible, can you move any relevant unit tests for remotebugupdater.py and utilities.py into new test modules named for them?