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
1=== modified file 'lib/lp/code/model/diff.py'
2--- lib/lp/code/model/diff.py 2009-09-09 19:36:46 +0000
3+++ lib/lp/code/model/diff.py 2009-09-25 03:45:25 +0000
4@@ -6,6 +6,8 @@
5 __metaclass__ = type
6 __all__ = ['Diff', 'PreviewDiff', 'StaticDiff']
7
8+import sys
9+
10 from cStringIO import StringIO
11
12 from bzrlib.branch import Branch
13@@ -17,6 +19,7 @@
14 from sqlobject import ForeignKey, IntCol, StringCol
15 from storm.locals import Int, Reference, Storm, Unicode
16 from zope.component import getUtility
17+from zope.error.interfaces import IErrorReportingUtility
18 from zope.interface import classProvides, implements
19
20 from canonical.config import config
21@@ -150,7 +153,17 @@
22 diff_content.seek(0)
23 diff_content_bytes = diff_content.read(size)
24 diff_lines_count = len(diff_content_bytes.strip().split('\n'))
25- diffstat = cls.generateDiffstat(diff_content_bytes)
26+ # Generation of diffstat is currently failing in some circumstances.
27+ # See bug 436325. Since diffstats are incidental to the whole
28+ # process, we don't want failure here to kill the generation of the
29+ # diff itself, but we do want to hear about it. So log an error using
30+ # the error reporting utility.
31+ try:
32+ diffstat = cls.generateDiffstat(diff_content_bytes)
33+ except Exception:
34+ getUtility(IErrorReportingUtility).raising(sys.exc_info())
35+ # Set the diffstat to be empty.
36+ diffstat = {}
37 return cls(diff_text=diff_text, diff_lines_count=diff_lines_count,
38 diffstat=diffstat)
39
40
41=== modified file 'lib/lp/code/model/tests/test_diff.py'
42--- lib/lp/code/model/tests/test_diff.py 2009-09-01 17:38:57 +0000
43+++ lib/lp/code/model/tests/test_diff.py 2009-09-25 03:45:25 +0000
44@@ -13,7 +13,7 @@
45 from bzrlib.branch import Branch
46 import transaction
47
48-from canonical.launchpad.webapp import canonical_url
49+from canonical.launchpad.webapp import canonical_url, errorlog
50 from canonical.launchpad.webapp.testing import verifyObject
51 from canonical.testing import LaunchpadFunctionalLayer, LaunchpadZopelessLayer
52 from lp.code.model.diff import Diff, PreviewDiff, StaticDiff
53@@ -150,6 +150,15 @@
54 self.assertEqual({'bar': (0, 3), 'baz': (2, 0), 'foo': (2, 1)},
55 diff.diffstat)
56
57+ def test_fromFile_withError(self):
58+ # If the diff is formatted such that generating the diffstat fails, we
59+ # want to record an oops but continue.
60+ last_oops_id = errorlog.globalErrorUtility.lastid
61+ diff_bytes = "not a real diff"
62+ diff = Diff.fromFile(StringIO(diff_bytes), len(diff_bytes))
63+ self.assertNotEqual(last_oops_id, errorlog.globalErrorUtility.lastid)
64+ self.assertEqual({}, diff.diffstat)
65+
66
67 class TestStaticDiff(TestCaseWithFactory):
68 """Test that StaticDiff objects work."""

Subscribers

People subscribed via source and target branches

to status/vote changes: