launchpad-reviewers team mailing list archive

[Brad Crittenden <bac@xxxxxxxxxxxxx>]

Brad Crittenden has proposed merging lp:~bac/launchpad/bug-885192 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #885192 in Launchpad itself: "Code import OOPS: TypeError: log_oops_if_published() takes exactly 1 argument (2 given)"
  https://bugs.launchpad.net/launchpad/+bug/885192

For more details, see:
https://code.launchpad.net/~bac/launchpad/bug-885192/+merge/81500

= Summary =

The signature for a simple callback was incorrect, causing an OOPS.

== Proposed fix ==

Fixed the signature, added a test.  Also changed errorlog.py to not
assume 'type' was available .

== Pre-implementation notes ==

Brief IRC chat with lifeless

== Implementation details ==

As above.

== Tests ==

bin/test -vvt test_workermonitor

== Demo and Q/A ==

None

= Launchpad lint =

Will fix lint.

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/codehosting/codeimport/tests/test_workermonitor.py
  lib/canonical/launchpad/webapp/errorlog.py
  lib/lp/codehosting/codeimport/workermonitor.py

./lib/lp/codehosting/codeimport/tests/test_workermonitor.py
     636: E301 expected 1 blank line, found 0
./lib/canonical/launchpad/webapp/errorlog.py
     315: E301 expected 1 blank line, found 0
-- 
https://code.launchpad.net/~bac/launchpad/bug-885192/+merge/81500
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~bac/launchpad/bug-885192 into lp:launchpad.

=== modified file 'lib/canonical/launchpad/webapp/errorlog.py'
--- lib/canonical/launchpad/webapp/errorlog.py	2011-10-28 06:43:50 +0000
+++ lib/canonical/launchpad/webapp/errorlog.py	2011-11-07 22:15:31 +0000
@@ -92,11 +92,11 @@
 class ErrorReport:
     implements(IErrorReport)
 
-    def __init__(self, id, type, value, time, tb_text, username,
+    def __init__(self, _id, _type, value, time, tb_text, username,
                  url, duration, req_vars, timeline, informational=None,
                  branch_nick=None, revno=None, topic=None, reporter=None):
-        self.id = id
-        self.type = type
+        self.id = _id
+        self.type = _type
         self.value = value
         self.time = time
         self.topic = topic
@@ -199,7 +199,7 @@
     else:
         # Request has an UnauthenticatedPrincipal.
         login = 'unauthenticated'
-        if report['type'] in (
+        if _get_type(report) in (
             _ignored_exceptions_for_unauthenticated_users):
             report['ignore'] = True
 
@@ -256,6 +256,10 @@
     report['timeline'] = statements
 
 
+def _get_type(report):
+    return report.get('type', 'No exception type')
+
+
 class ErrorReportingUtility:
     implements(IErrorReportingUtility)
 
@@ -334,7 +338,7 @@
                 operator.methodcaller('get', 'ignore'))
         #  - have a type listed in self._ignored_exceptions.
         self._oops_config.filters.append(
-                lambda report: report['type'] in self._ignored_exceptions)
+                lambda report: _get_type(report) in self._ignored_exceptions)
         #  - have a missing or offset REFERER header with a type listed in
         #    self._ignored_exceptions_for_offsite_referer
         self._oops_config.filters.append(self._filter_bad_urls_by_referer)
@@ -369,7 +373,7 @@
 
     def _filter_bad_urls_by_referer(self, report):
         """Filter if the report was generated because of a bad offsite url."""
-        if report['type'] in self._ignored_exceptions_for_offsite_referer:
+        if _get_type(report) in self._ignored_exceptions_for_offsite_referer:
             was_http = report.get('url', '').lower().startswith('http')
             if was_http:
                 req_vars = dict(report.get('req_vars', ()))

=== modified file 'lib/lp/codehosting/codeimport/tests/test_workermonitor.py'
--- lib/lp/codehosting/codeimport/tests/test_workermonitor.py	2011-10-25 03:24:52 +0000
+++ lib/lp/codehosting/codeimport/tests/test_workermonitor.py	2011-11-07 22:15:31 +0000
@@ -536,7 +536,7 @@
     """Tests for `CodeImportWorkerMonitor.run` that don't launch a subprocess.
     """
 
-    run_tests_with = AsynchronousDeferredRunTest
+    run_tests_with = AsynchronousDeferredRunTest.make_factory(timeout=20)
 
     class WorkerMonitor(CodeImportWorkerMonitor):
         """See `CodeImportWorkerMonitor`.
@@ -617,6 +617,31 @@
         worker_monitor.finishJob = finishJob
         return worker_monitor.run()
 
+    def test_log_oops(self):
+        # Ensure an OOPS is logged if published.
+        errorlog.globalErrorUtility.configure(
+            config_factory=oops_twisted.Config,
+            publisher_adapter=oops_twisted.defer_publisher)
+        failure_msg = "test_log_oops expected failure"
+        worker_monitor = self.WorkerMonitor(
+            defer.fail(RuntimeError(failure_msg)))
+
+        def finishJob(reason):
+            from twisted.python import failure
+            return worker_monitor._logOopsFromFailure(
+                failure.Failure())
+
+        worker_monitor.finishJob = finishJob
+        d = worker_monitor.run()
+        def check_log_file(ignored):
+            worker_monitor._log_file.seek(0)
+            log_text = worker_monitor._log_file.read()
+            self.assertIn(
+                "Failure: exceptions.RuntimeError: " + failure_msg,
+                log_text)
+        d.addCallback(check_log_file)
+        return d
+
 
 def nuke_codeimport_sample_data():
     """Delete all the sample data that might interfere with tests."""

=== modified file 'lib/lp/codehosting/codeimport/workermonitor.py'
--- lib/lp/codehosting/codeimport/workermonitor.py	2011-10-28 06:43:50 +0000
+++ lib/lp/codehosting/codeimport/workermonitor.py	2011-11-07 22:15:31 +0000
@@ -142,9 +142,10 @@
 
     def _logOopsFromFailure(self, failure):
         config = errorlog.globalErrorUtility._oops_config
-        context = {'twisted_failure': failure,
+        context = {
+            'twisted_failure': failure,
             'request': errorlog.ScriptRequest(
-                [('code_import_job_id', self._job_id)], self._branch_url)
+                [('code_import_job_id', self._job_id)], self._branch_url),
             }
         report = config.create(context)
 
@@ -152,10 +153,11 @@
             if ids:
                 self._logger.info(
                     "Logged OOPS id %s: %s: %s",
-                    report['id'], report['type'], report['value'])
+                    report['id'], report.get('type', 'No exception type'),
+                    report.get('value', 'No execption value'))
 
         d = config.publish(report)
-        d.addCallback(log_oops_if_published, report)
+        d.addCallback(log_oops_if_published)
         return d
 
     def _trap_nosuchcodeimportjob(self, failure):