launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

William Grant has proposed merging lp:~wgrant/launchpad/suppress-pushbugcomments-errors into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #735270 in Launchpad itself: "Need to catch BugTrackerAuthenticationErrors in pushBugComments"
  https://bugs.launchpad.net/launchpad/+bug/735270

For more details, see:
https://code.launchpad.net/~wgrant/launchpad/suppress-pushbugcomments-errors/+merge/53381

BugWatchUpdater.updateBugWatch logs as OOPSes exceptions from importBugComments, pushBugComments and linkLaunchpadBug. This branch changes it to log known errors (BugWatchUpdateErrors) at INFO, leaving only unknown exceptions to OOPS.
-- 
https://code.launchpad.net/~wgrant/launchpad/suppress-pushbugcomments-errors/+merge/53381
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wgrant/launchpad/suppress-pushbugcomments-errors into lp:launchpad.

=== modified file 'lib/lp/bugs/scripts/checkwatches/bugwatchupdater.py'
--- lib/lp/bugs/scripts/checkwatches/bugwatchupdater.py	2011-01-23 04:54:38 +0000
+++ lib/lp/bugs/scripts/checkwatches/bugwatchupdater.py	2011-03-15 08:21:42 +0000
@@ -19,6 +19,7 @@
 from canonical.launchpad.interfaces.message import IMessageSet
 from canonical.launchpad.webapp.publisher import canonical_url
 from lp.app.errors import NotFoundError
+from lp.bugs.externalbugtracker.base import BugWatchUpdateError
 from lp.bugs.interfaces.bug import IBugSet
 from lp.bugs.interfaces.bugwatch import BugWatchActivityStatus
 from lp.bugs.scripts.checkwatches.base import (
@@ -90,11 +91,14 @@
                     self.linkLaunchpadBug()
             except Exception, ex:
                 error_message = str(ex)
-                oops_id = self.error(
-                    "Failure updating bug %r on %s (local bug: %s)." %
-                        (self.remote_bug, self.external_bugtracker.baseurl,
-                        self.local_bug),
-                    self.oops_properties)
+                log_message = (
+                    "Failure updating bug %r on %s (local bug: %s)" %
+                    (self.remote_bug, self.external_bugtracker.baseurl,
+                    self.local_bug))
+                if isinstance(ex, BugWatchUpdateError):
+                    self.logger.info('%s: %s' % (log_message, ex))
+                else:
+                    oops_id = self.error(log_message, self.oops_properties)
             else:
                 error_status = None
 

=== modified file 'lib/lp/bugs/scripts/checkwatches/tests/test_bugwatchupdater.py'
--- lib/lp/bugs/scripts/checkwatches/tests/test_bugwatchupdater.py	2010-12-14 13:50:30 +0000
+++ lib/lp/bugs/scripts/checkwatches/tests/test_bugwatchupdater.py	2011-03-15 08:21:42 +0000
@@ -11,6 +11,7 @@
 import transaction
 
 from canonical.testing.layers import LaunchpadZopelessLayer
+from lp.bugs.externalbugtracker.base import BugWatchUpdateError
 from lp.bugs.externalbugtracker.bugzilla import BugzillaAPI
 from lp.bugs.interfaces.bugtask import (
     BugTaskImportance,
@@ -48,7 +49,10 @@
 
 
 class BrokenCommentSyncingExternalBugTracker(TestExternalBugTracker):
-    """An ExternalBugTracker that can't sync comments."""
+    """An ExternalBugTracker that can't sync comments.
+
+    Its exceptions are not known, so it should generate OOPSes.
+    """
 
     import_comments_error_message = "Can't import comments, sorry."
     push_comments_error_message = "Can't push comments, sorry."
@@ -64,6 +68,18 @@
         raise Exception(self.back_link_error_message)
 
 
+class KnownBrokenCommentSyncingExternalBugTracker(TestExternalBugTracker):
+    """An ExternalBugTracker that fails in a known manner.
+
+    It should not generate OOPSes.
+    """
+
+    import_comments_error_message = "Can't import comments, sorry."
+
+    def getCommentIds(self, remote_bug_id):
+        raise BugWatchUpdateError(self.import_comments_error_message)
+
+
 class LoggingBugWatchUpdater(BugWatchUpdater):
     """A BugWatchUpdater that logs what's going on."""
 
@@ -93,12 +109,16 @@
         self.bug_watch = self.factory.makeBugWatch(bug_task=self.bug_task)
 
     def _checkLastErrorAndMessage(self, expected_last_error,
-                                  expected_message):
+                                  expected_message, want_oops=True):
         """Check the latest activity and last_error_type for a BugWatch."""
         latest_activity = self.bug_watch.activity[0]
         self.assertEqual(expected_last_error, self.bug_watch.last_error_type)
         self.assertEqual(expected_last_error, latest_activity.result)
         self.assertEqual(expected_message, latest_activity.message)
+        if want_oops:
+            self.assertIsNot(None, latest_activity.oops_id)
+        else:
+            self.assertIs(None, latest_activity.oops_id)
 
     def test_updateBugWatch(self):
         # Calling BugWatchUpdater.updateBugWatch() will update the
@@ -175,6 +195,25 @@
             BugWatchActivityStatus.BACKLINK_FAILED,
             external_bugtracker.back_link_error_message)
 
+    def test_known_error_handling(self):
+        # Some errors are known to be the remote end's fault, and should
+        # not generate OOPSes. These are still logged in
+        # BugWatchActivity.
+        external_bugtracker = KnownBrokenCommentSyncingExternalBugTracker(
+            'http://example.com')
+        bug_watch_updater = make_bug_watch_updater(
+            self.checkwatches_master, self.bug_watch, external_bugtracker,
+            can_import_comments=True)
+
+        bug_watch_updater.updateBugWatch(
+            'FIXED', BugTaskStatus.FIXRELEASED, 'LOW',
+            BugTaskImportance.LOW)
+
+        self._checkLastErrorAndMessage(
+            BugWatchActivityStatus.COMMENT_IMPORT_FAILED,
+            external_bugtracker.import_comments_error_message,
+            want_oops=False)
+
     def test_comment_bools_inherited(self):
         # BugWatchUpdater.updateBugWatches() doesn't have to be passed
         # values for can_import_comments, can_push_comments and