Launchpad itself

Merge lp:~thumper/launchpad/log-diffstat-error-and-continue into lp:launchpad/db-devel

  1. log-diffstat-error-and-continue
  2. Merge into db-devel
Proposed by Tim Penhey
Status: Merged
Merged at revision: not available
Proposed branch: lp:~thumper/launchpad/log-diffstat-error-and-continue
Merge into: lp:launchpad/db-devel
Diff against target: 68 lines
2 files modified
lib/lp/code/model/diff.py (+14/-1)
lib/lp/code/model/tests/test_diff.py (+10/-1)
To merge this branch: bzr merge lp:~thumper/launchpad/log-diffstat-error-and-continue
Reviewer Review Type Date Requested Status
Michael Hudson-Doyle Approve
Brad Crittenden release-critical Pending
Review via email: mp+12400@code.launchpad.net
To post a comment you must log in.
Revision history for this message
Tim Penhey (thumper) wrote :

This branch doesn't fix the oopses in bug 436325, but it does log them earlier and stops it failing to continue.

This is a bit of a band-aid because we don't want to stop generating the diffstats, nor do we want the diff jobs to fail if there is an error generating the diff.

Revision history for this message
Tim Penhey (thumper) wrote :

Oh yeah, here is the test:

test_fromFile_withError

I'll run it through ec2 to check there are no other failures.

Revision history for this message
Michael Hudson-Doyle (mwhudson) wrote :

Two minor points:

+ info = sys.exc_info()
+ getUtility(IErrorReportingUtility).raising(info)

Why not just say getUtility(IErrorReportingUtility).raising(sys.exc_info())?

+ # If the diffstat isn't real, we want to record an oops but continue.

This comment isn't very clear. Maybe:

# If the diff is formatted such that generating the diffstat fails, we want to record
# an oops but continue.

?

review: Approve
Revision history for this message
Tim Penhey (thumper) wrote :

Sounds good. Thanks.

=== modified file 'lib/lp/code/model/diff.py'
--- lib/lp/code/model/diff.py 2009-09-25 02:54:56 +0000
+++ lib/lp/code/model/diff.py 2009-09-25 03:40:24 +0000
@@ -161,8 +161,7 @@
         try:
             diffstat = cls.generateDiffstat(diff_content_bytes)
         except Exception:
- info = sys.exc_info()
- getUtility(IErrorReportingUtility).raising(info)
+ getUtility(IErrorReportingUtility).raising(sys.exc_info())
             # Set the diffstat to be empty.
             diffstat = {}
         return cls(diff_text=diff_text, diff_lines_count=diff_lines_count,

=== modified file 'lib/lp/code/model/tests/test_diff.py'
--- lib/lp/code/model/tests/test_diff.py 2009-09-25 02:54:56 +0000
+++ lib/lp/code/model/tests/test_diff.py 2009-09-25 03:41:21 +0000
@@ -151,7 +151,8 @@
                          diff.diffstat)

     def test_fromFile_withError(self):
- # If the diffstat isn't real, we want to record an oops but continue.
+ # If the diff is formatted such that generating the diffstat fails, we
+ # want to record an oops but continue.
         last_oops_id = errorlog.globalErrorUtility.lastid
         diff_bytes = "not a real diff"
         diff = Diff.fromFile(StringIO(diff_bytes), len(diff_bytes))

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'lib/lp/code/model/diff.py'
--- lib/lp/code/model/diff.py 2009-09-09 19:36:46 +0000
+++ lib/lp/code/model/diff.py 2009-09-25 03:45:25 +0000
@@ -6,6 +6,8 @@
6__metaclass__ = type6__metaclass__ = type
7__all__ = ['Diff', 'PreviewDiff', 'StaticDiff']7__all__ = ['Diff', 'PreviewDiff', 'StaticDiff']
88
9import sys
10
9from cStringIO import StringIO11from cStringIO import StringIO
1012
11from bzrlib.branch import Branch13from bzrlib.branch import Branch
@@ -17,6 +19,7 @@
17from sqlobject import ForeignKey, IntCol, StringCol19from sqlobject import ForeignKey, IntCol, StringCol
18from storm.locals import Int, Reference, Storm, Unicode20from storm.locals import Int, Reference, Storm, Unicode
19from zope.component import getUtility21from zope.component import getUtility
22from zope.error.interfaces import IErrorReportingUtility
20from zope.interface import classProvides, implements23from zope.interface import classProvides, implements
2124
22from canonical.config import config25from canonical.config import config
@@ -150,7 +153,17 @@
150 diff_content.seek(0)153 diff_content.seek(0)
151 diff_content_bytes = diff_content.read(size)154 diff_content_bytes = diff_content.read(size)
152 diff_lines_count = len(diff_content_bytes.strip().split('\n'))155 diff_lines_count = len(diff_content_bytes.strip().split('\n'))
153 diffstat = cls.generateDiffstat(diff_content_bytes)156 # Generation of diffstat is currently failing in some circumstances.
157 # See bug 436325. Since diffstats are incidental to the whole
158 # process, we don't want failure here to kill the generation of the
159 # diff itself, but we do want to hear about it. So log an error using
160 # the error reporting utility.
161 try:
162 diffstat = cls.generateDiffstat(diff_content_bytes)
163 except Exception:
164 getUtility(IErrorReportingUtility).raising(sys.exc_info())
165 # Set the diffstat to be empty.
166 diffstat = {}
154 return cls(diff_text=diff_text, diff_lines_count=diff_lines_count,167 return cls(diff_text=diff_text, diff_lines_count=diff_lines_count,
155 diffstat=diffstat)168 diffstat=diffstat)
156169
157170
=== modified file 'lib/lp/code/model/tests/test_diff.py'
--- lib/lp/code/model/tests/test_diff.py 2009-09-01 17:38:57 +0000
+++ lib/lp/code/model/tests/test_diff.py 2009-09-25 03:45:25 +0000
@@ -13,7 +13,7 @@
13from bzrlib.branch import Branch13from bzrlib.branch import Branch
14import transaction14import transaction
1515
16from canonical.launchpad.webapp import canonical_url16from canonical.launchpad.webapp import canonical_url, errorlog
17from canonical.launchpad.webapp.testing import verifyObject17from canonical.launchpad.webapp.testing import verifyObject
18from canonical.testing import LaunchpadFunctionalLayer, LaunchpadZopelessLayer18from canonical.testing import LaunchpadFunctionalLayer, LaunchpadZopelessLayer
19from lp.code.model.diff import Diff, PreviewDiff, StaticDiff19from lp.code.model.diff import Diff, PreviewDiff, StaticDiff
@@ -150,6 +150,15 @@
150 self.assertEqual({'bar': (0, 3), 'baz': (2, 0), 'foo': (2, 1)},150 self.assertEqual({'bar': (0, 3), 'baz': (2, 0), 'foo': (2, 1)},
151 diff.diffstat)151 diff.diffstat)
152152
153 def test_fromFile_withError(self):
154 # If the diff is formatted such that generating the diffstat fails, we
155 # want to record an oops but continue.
156 last_oops_id = errorlog.globalErrorUtility.lastid
157 diff_bytes = "not a real diff"
158 diff = Diff.fromFile(StringIO(diff_bytes), len(diff_bytes))
159 self.assertNotEqual(last_oops_id, errorlog.globalErrorUtility.lastid)
160 self.assertEqual({}, diff.diffstat)
161
153162
154class TestStaticDiff(TestCaseWithFactory):163class TestStaticDiff(TestCaseWithFactory):
155 """Test that StaticDiff objects work."""164 """Test that StaticDiff objects work."""

Subscribers

People subscribed via source and target branches

to status/vote changes: