launchpad-reviewers team mailing list archive

[Robert Collins <robertc@xxxxxxxxxxxxxxxxx>]

Robert Collins has proposed merging lp:~lifeless/launchpad/lessGetLastOops into lp:launchpad/devel.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)


Reduce some uses of getLastOopsReport which appears to be flaky in hard to reproduce situations, but nevertheless is doing to much work anyway.
-- 
https://code.launchpad.net/~lifeless/launchpad/lessGetLastOops/+merge/35605
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~lifeless/launchpad/lessGetLastOops into lp:launchpad/devel.

=== modified file 'lib/canonical/launchpad/webapp/errorlog.py'
--- lib/canonical/launchpad/webapp/errorlog.py	2010-09-12 11:43:36 +0000
+++ lib/canonical/launchpad/webapp/errorlog.py	2010-09-15 22:18:52 +0000
@@ -324,7 +324,7 @@
         # Check today
         oopsid, filename = self.log_namer._findHighestSerialFilename(time=now)
         if filename is None:
-            # Check yesterday
+            # Check yesterday, we may have just passed midnight.
             yesterday = now - datetime.timedelta(days=1)
             oopsid, filename = self.log_namer._findHighestSerialFilename(
                 time=yesterday)
@@ -484,6 +484,7 @@
             info.
         :param now: The datetime to use as the current time.  Will be
             determined if not supplied.  Useful for testing.
+        :return: The ErrorReport created.
         """
         return self._raising(
             info, request=request, now=now, informational=True)

=== modified file 'lib/lp/services/job/tests/test_runner.py'
--- lib/lp/services/job/tests/test_runner.py	2010-08-20 20:31:18 +0000
+++ lib/lp/services/job/tests/test_runner.py	2010-09-15 22:18:52 +0000
@@ -149,7 +149,6 @@
 
     def test_runAll_skips_lease_failures(self):
         """Ensure runAll skips jobs whose leases can't be acquired."""
-        last_oops = errorlog.globalErrorUtility.getLastOopsReport()
         job_1, job_2 = self.makeTwoJobs()
         job_2.job.acquireLease()
         runner = JobRunner([job_1, job_2])
@@ -158,8 +157,7 @@
         self.assertEqual(JobStatus.WAITING, job_2.job.status)
         self.assertEqual([job_1], runner.completed_jobs)
         self.assertEqual([job_2], runner.incomplete_jobs)
-        new_last_oops = errorlog.globalErrorUtility.getLastOopsReport()
-        self.assertEqual(last_oops.id, new_last_oops.id)
+        self.assertEqual([], self.oopses)
 
     def test_runAll_reports_oopses(self):
         """When an error is encountered, report an oops and continue."""
@@ -177,7 +175,7 @@
         self.assertEqual(JobStatus.FAILED, job_1.job.status)
         self.assertEqual(JobStatus.COMPLETED, job_2.job.status)
         reporter = errorlog.globalErrorUtility
-        oops = reporter.getLastOopsReport()
+        oops = self.oopses[-1]
         self.assertIn('Fake exception.  Foobar, I say!', oops.tb_text)
         self.assertEqual(1, len(oops.req_vars))
         self.assertEqual("{'foo': 'bar'}", oops.req_vars[0][1])
@@ -195,7 +193,7 @@
         runner = JobRunner([job_1, job_2])
         runner.runAll()
         reporter = getUtility(IErrorReportingUtility)
-        oops = reporter.getLastOopsReport()
+        oops = self.oopses[-1]
         self.assertEqual(1, len(oops.req_vars))
         self.assertEqual("{'foo': 'bar'}", oops.req_vars[0][1])
 
@@ -231,7 +229,7 @@
         runner.runAll()
         (notification,) = pop_notifications()
         reporter = errorlog.globalErrorUtility
-        oops = reporter.getLastOopsReport()
+        oops = self.oopses[-1]
         self.assertIn(
             'Launchpad encountered an internal error during the following'
             ' operation: appending a string to a list.  It was logged with id'
@@ -257,10 +255,8 @@
         job_1.user_error_types = (ExampleError,)
         job_1.error_recipients = ['jrandom@xxxxxxxxxxx']
         runner = JobRunner([job_1, job_2])
-        reporter = errorlog.globalErrorUtility
-        old_oops = reporter.getLastOopsReport()
         runner.runAll()
-        self.assertNoNewOops(old_oops)
+        self.assertEqual([], self.oopses)
         notifications = pop_notifications()
         self.assertEqual(1, len(notifications))
         body = notifications[0].get_payload(decode=True)