Merge ~cjwatson/launchpad:doctest-capture-oopses into launchpad:master

Proposed by Colin Watson on 2019-11-20
Status: Merged
Approved by: Colin Watson on 2019-11-20
Approved revision: 59d9970306da5b92731b19e54d36cd69e9ac3e8c
Merge reported by: Otto Co-Pilot
Merged at revision: not available
Proposed branch: ~cjwatson/launchpad:doctest-capture-oopses
Merge into: launchpad:master
Diff against target: 242 lines (+52/-55)
6 files modified
lib/lp/app/stories/basics/xx-request-expired.txt (+1/-6)
lib/lp/app/stories/basics/xx-soft-timeout.txt (+1/-8)
lib/lp/bugs/doc/externalbugtracker.txt (+1/-7)
lib/lp/services/webapp/doc/test_adapter_timeout.txt.disabled (+1/-5)
lib/lp/soyuz/doc/gina.txt (+2/-5)
lib/lp/testing/systemdocs.py (+46/-24)
Reviewer Review Type Date Requested Status
Kristian Glass 2019-11-20 Approve on 2019-11-20
Review via email: mp+375762@code.launchpad.net

Commit message

Capture OOPSes in doctests

Description of the change

TestCase has captured OOPSes in Launchpad's unit tests since 2010, but we've never systematically done the same thing for doctests. This occasionally causes some problems: with amqp >= 2.4.0 it can mean that attempts to publish OOPSes to a nonexistent exchange leave unhandled errors lying around to trip up later tests (see https://code.launchpad.net/~cjwatson/python-oops-amqp/publisher-handle-channel-errors/+merge/367748), which is extremely confusing and difficult to debug, and I suspect has caused a number of transient failures on buildbot.

We now systematically capture OOPSes for doctests just as we do for unit tests, ensuring better test isolation. I cleaned up a few ad-hoc arrangements in individual doctests.

To post a comment you must log in.
review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/lib/lp/app/stories/basics/xx-request-expired.txt b/lib/lp/app/stories/basics/xx-request-expired.txt
2index 2e3306a..fe21067 100644
3--- a/lib/lp/app/stories/basics/xx-request-expired.txt
4+++ b/lib/lp/app/stories/basics/xx-request-expired.txt
5@@ -8,9 +8,6 @@ causing a soft timeout to be logged.
6
7 >>> from lp.services.config import config
8 >>> from textwrap import dedent
9- >>> from lp.testing.fixture import CaptureOops
10- >>> capture = CaptureOops()
11- >>> capture.setUp()
12 >>> test_data = dedent("""
13 ... [database]
14 ... db_statement_timeout: 1
15@@ -28,11 +25,9 @@ causing a soft timeout to be logged.
16 <title>Error: Timeout</title>
17 ...
18
19- >>> capture.oopses[0]['type']
20+ >>> oops_capture.oopses[-1]['type']
21 'RequestExpired'
22
23- >>> capture.cleanUp()
24-
25 Let's reset the config value we changed:
26
27 >>> base_test_data = config.pop('base_test_data')
28diff --git a/lib/lp/app/stories/basics/xx-soft-timeout.txt b/lib/lp/app/stories/basics/xx-soft-timeout.txt
29index 65fa191..9a2e175 100644
30--- a/lib/lp/app/stories/basics/xx-soft-timeout.txt
31+++ b/lib/lp/app/stories/basics/xx-soft-timeout.txt
32@@ -39,9 +39,6 @@ If we set soft_request_timeout to some value, the page will take
33 slightly longer then the soft_request_timeout value to generate, thus
34 causing a soft timeout to be logged.
35
36- >>> from lp.testing.fixture import CaptureOops
37- >>> capture = CaptureOops()
38- >>> capture.setUp()
39 >>> from textwrap import dedent
40 >>> test_data = (dedent("""
41 ... [database]
42@@ -57,13 +54,9 @@ causing a soft timeout to be logged.
43 ...
44 Soft timeout threshold is set to 1 ms. This page took ... ms to render.
45
46- >>> capture.oopses[0]['type']
47+ >>> oops_capture.oopses[-1]['type']
48 'SoftRequestTimeout'
49
50-Unregister from the oops collection:
51-
52- >>> capture.cleanUp()
53-
54 Since the page rendered correctly, we assume it's a soft timeout error,
55 since otherwise we would have gotten an OOPS page when we tried to
56 render the page.
57diff --git a/lib/lp/bugs/doc/externalbugtracker.txt b/lib/lp/bugs/doc/externalbugtracker.txt
58index 2a902f4..1e3a99a 100644
59--- a/lib/lp/bugs/doc/externalbugtracker.txt
60+++ b/lib/lp/bugs/doc/externalbugtracker.txt
61@@ -750,17 +750,13 @@ We temporarily silence the logging from this function because we're not
62 interested in it. Again, the watch's lastchecked field will also be
63 updated.
64
65- >>> from lp.testing.fixture import CaptureOops
66- >>> capture = CaptureOops()
67- >>> capture.setUp()
68-
69 >>> external_bugtracker.initialize_remote_bugdb_error = None
70 >>> for error in [UnparsableBugData, Exception]:
71 ... example_bugwatch.lastchecked = None
72 ... external_bugtracker.get_remote_status_error = error
73 ... bug_watch_updater.updateBugWatches(
74 ... external_bugtracker, [example_bugwatch])
75- ... oops = capture.oopses[-1]
76+ ... oops = oops_capture.oopses[-1]
77 ... print("%s: %s (%s; %s)" % (
78 ... example_bugwatch.last_error_type.title,
79 ... example_bugwatch.lastchecked is not None,
80@@ -768,8 +764,6 @@ updated.
81 Unparsable Bug: True (OOPS-...; http://bugs.example.com/show_bug.cgi?id=1)
82 Unknown: True (OOPS-...; http://bugs.example.com/show_bug.cgi?id=1)
83
84- >>> capture.cleanUp()
85-
86
87 Using `LookupTree` to map statuses
88 ----------------------------------
89diff --git a/lib/lp/services/webapp/doc/test_adapter_timeout.txt.disabled b/lib/lp/services/webapp/doc/test_adapter_timeout.txt.disabled
90index aca50bb..5940282 100644
91--- a/lib/lp/services/webapp/doc/test_adapter_timeout.txt.disabled
92+++ b/lib/lp/services/webapp/doc/test_adapter_timeout.txt.disabled
93@@ -82,9 +82,6 @@ exception, and a time machine.
94
95 Now we actually demonstrate the behaviour. The view did raise a TimeoutError.
96
97- >>> from lp.testing.fixture import CaptureOops
98- >>> capture = CaptureOops()
99- >>> capture.setUp()
100 >>> browser = setupBrowser()
101 >>> browser.open('http://launchpad.test/doom_test')
102 Traceback (most recent call last):
103@@ -98,12 +95,11 @@ The exception view did not.
104
105 The OOPS has the SQL from the main view.
106
107- >>> print capture.oopses[0]['timeline']
108+ >>> print oops_capture.oopses[-1]['timeline']
109 [(0, 0, 'SELECT ... FROM ... WHERE ...'), ...]
110
111 All that's left is to clean up after ourselves.
112
113- >>> capture.cleanUp()
114 >>> sm.unregisterAdapter(
115 ... DoomedView, required=(Interface, IBrowserRequest),
116 ... provided=IBrowserView, name='doom_test')
117diff --git a/lib/lp/soyuz/doc/gina.txt b/lib/lp/soyuz/doc/gina.txt
118index 76b7098..1f773df 100644
119--- a/lib/lp/soyuz/doc/gina.txt
120+++ b/lib/lp/soyuz/doc/gina.txt
121@@ -123,11 +123,9 @@ Let's set up the filesystem:
122
123 And give it a spin:
124
125- >>> from lp.testing.fixture import CaptureOops
126 >>> gina_proc = [sys.executable, 'scripts/gina.py', '-q',
127 ... 'hoary', 'breezy']
128- >>> with CaptureOops():
129- ... proc = subprocess.Popen(gina_proc, stderr=subprocess.PIPE)
130+ >>> proc = subprocess.Popen(gina_proc, stderr=subprocess.PIPE)
131
132 Check STDERR for the errors we expected:
133
134@@ -515,8 +513,7 @@ run.
135
136 >>> gina_proc = [sys.executable, 'scripts/gina.py', '-q',
137 ... 'hoary', 'breezy']
138- >>> with CaptureOops():
139- ... proc = subprocess.Popen(gina_proc, stderr=subprocess.PIPE)
140+ >>> proc = subprocess.Popen(gina_proc, stderr=subprocess.PIPE)
141 >>> print(proc.stderr.read())
142 ERROR Error processing package files for clearlooks
143 ...
144diff --git a/lib/lp/testing/systemdocs.py b/lib/lp/testing/systemdocs.py
145index 5a34004..d609bc8 100644
146--- a/lib/lp/testing/systemdocs.py
147+++ b/lib/lp/testing/systemdocs.py
148@@ -1,4 +1,4 @@
149-# Copyright 2009-2018 Canonical Ltd. This software is licensed under the
150+# Copyright 2009-2019 Canonical Ltd. This software is licensed under the
151 # GNU Affero General Public License version 3 (see the file LICENSE).
152
153 """Infrastructure for setting up doctests."""
154@@ -43,6 +43,7 @@ from lp.testing import (
155 verifyObject,
156 )
157 from lp.testing.factory import LaunchpadObjectFactory
158+from lp.testing.fixture import CaptureOops
159 from lp.testing.views import (
160 create_initialized_view,
161 create_view,
162@@ -91,6 +92,41 @@ class StdoutHandler(Handler):
163 record.levelname, record.name, self.format(record)))
164
165
166+def setUpStdoutLogging(test, prev_set_up=None,
167+ stdout_logging_level=logging.INFO):
168+ if prev_set_up is not None:
169+ prev_set_up(test)
170+ log = StdoutHandler('')
171+ log.setLoggerLevel(stdout_logging_level)
172+ log.install()
173+ test.globs['log'] = log
174+ # Store as instance attribute so we can uninstall it.
175+ test._stdout_logger = log
176+
177+
178+def tearDownStdoutLogging(test, prev_tear_down=None):
179+ if prev_tear_down is not None:
180+ prev_tear_down(test)
181+ reset_logging()
182+ test._stdout_logger.uninstall()
183+
184+
185+def setUpOopsCapturing(test, prev_set_up=None):
186+ oops_capture = CaptureOops()
187+ oops_capture.setUp()
188+ test.globs['oops_capture'] = oops_capture
189+ # Store as instance attribute so we can clean it up.
190+ test._oops_capture = oops_capture
191+ if prev_set_up is not None:
192+ prev_set_up(test)
193+
194+
195+def tearDownOopsCapturing(test, prev_tear_down=None):
196+ if prev_tear_down is not None:
197+ prev_tear_down(test)
198+ test._oops_capture.cleanUp()
199+
200+
201 def LayeredDocFileSuite(paths, id_extensions=None, **kw):
202 """Create a DocFileSuite, optionally applying a layer to it.
203
204@@ -119,29 +155,15 @@ def LayeredDocFileSuite(paths, id_extensions=None, **kw):
205 stdout_logging_level = kw.pop('stdout_logging_level', logging.INFO)
206
207 if stdout_logging:
208- kw_setUp = kw.get('setUp')
209-
210- def setUp(test):
211- if kw_setUp is not None:
212- kw_setUp(test)
213- log = StdoutHandler('')
214- log.setLoggerLevel(stdout_logging_level)
215- log.install()
216- test.globs['log'] = log
217- # Store as instance attribute so we can uninstall it.
218- test._stdout_logger = log
219-
220- kw['setUp'] = setUp
221-
222- kw_tearDown = kw.get('tearDown')
223-
224- def tearDown(test):
225- if kw_tearDown is not None:
226- kw_tearDown(test)
227- reset_logging()
228- test._stdout_logger.uninstall()
229-
230- kw['tearDown'] = tearDown
231+ kw['setUp'] = partial(
232+ setUpStdoutLogging, prev_set_up=kw.get('setUp'),
233+ stdout_logging_level=stdout_logging_level)
234+ kw['tearDown'] = partial(
235+ tearDownStdoutLogging, prev_tear_down=kw.get('tearDown'))
236+
237+ kw['setUp'] = partial(setUpOopsCapturing, prev_set_up=kw.get('setUp'))
238+ kw['tearDown'] = partial(
239+ tearDownOopsCapturing, prev_tear_down=kw.get('tearDown'))
240
241 layer = kw.pop('layer', None)
242 suite = doctest.DocFileSuite(*paths, **kw)

Subscribers

People subscribed via source and target branches