Merge ~cjwatson/launchpad:remove-most-wsgi-intercept into launchpad:master
- Git
- lp:~cjwatson/launchpad
- remove-most-wsgi-intercept
- Merge into master
Proposed by
Colin Watson
Status: | Merged |
---|---|
Approved by: | Colin Watson |
Approved revision: | 8c7f5d03481e282c3b66ae62b9b2d1ed66a738d4 |
Merge reported by: | Otto Co-Pilot |
Merged at revision: | not available |
Proposed branch: | ~cjwatson/launchpad:remove-most-wsgi-intercept |
Merge into: | launchpad:master |
Prerequisite: | ~cjwatson/launchpad:simplify-appserverlayer-browser |
Diff against target: |
450 lines (+47/-113) 8 files modified
lib/launchpad_loggerhead/tests.py (+14/-45) lib/lp/registry/stories/webservice/xx-distribution-mirror.txt (+6/-7) lib/lp/services/webapp/tests/test_error.py (+10/-9) lib/lp/services/webservice/stories/xx-service.txt (+8/-8) lib/lp/soyuz/stories/webservice/xx-archive.txt (+8/-8) lib/lp/soyuz/stories/webservice/xx-person-createppa.txt (+0/-7) lib/lp/testing/fixture.py (+0/-25) lib/lp/testing/layers.py (+1/-4) |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Ioana Lasc (community) | Approve | ||
Review via email: mp+374881@code.launchpad.net |
Commit message
Avoid most uses of wsgi_intercept
zope.testbrowser 4.0.0 uses WebTest instead of wsgi_intercept, so we
also want to stop using wsgi_intercept for our own purposes. We can
point the WSGI test browser directly at an appropriate WSGI application
instead.
Description of the change
To post a comment you must log in.
Revision history for this message
Ioana Lasc (ilasc) : | # |
review:
Approve
Preview Diff
[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1 | diff --git a/lib/launchpad_loggerhead/tests.py b/lib/launchpad_loggerhead/tests.py |
2 | index f332518..31cb5a7 100644 |
3 | --- a/lib/launchpad_loggerhead/tests.py |
4 | +++ b/lib/launchpad_loggerhead/tests.py |
5 | @@ -1,7 +1,6 @@ |
6 | # Copyright 2010-2018 Canonical Ltd. This software is licensed under the |
7 | # GNU Affero General Public License version 3 (see the file LICENSE). |
8 | |
9 | -import lazr.uri |
10 | from paste.httpexceptions import HTTPExceptionHandler |
11 | import requests |
12 | from six.moves.urllib_parse import ( |
13 | @@ -11,12 +10,7 @@ from six.moves.urllib_parse import ( |
14 | import soupmatchers |
15 | from testtools.content import Content |
16 | from testtools.content_type import UTF8_TEXT |
17 | -import wsgi_intercept |
18 | -from wsgi_intercept.urllib2_intercept import ( |
19 | - install_opener, |
20 | - uninstall_opener, |
21 | - ) |
22 | -import wsgi_intercept.zope_testbrowser |
23 | +from zope.testbrowser.wsgi import Browser |
24 | from zope.security.proxy import removeSecurityProxy |
25 | |
26 | from launchpad_loggerhead.app import RootApp |
27 | @@ -50,12 +44,6 @@ def session_scribbler(app, test): |
28 | return scribble |
29 | |
30 | |
31 | -def dummy_destination(environ, start_response): |
32 | - """Return a fake response.""" |
33 | - start_response('200 OK', [('Content-type', 'text/plain')]) |
34 | - return ['This is a dummy destination.\n'] |
35 | - |
36 | - |
37 | class SimpleLogInRootApp(RootApp): |
38 | """A mock root app that doesn't require open id.""" |
39 | def _complete_login(self, environ, start_response): |
40 | @@ -63,53 +51,35 @@ class SimpleLogInRootApp(RootApp): |
41 | start_response('200 OK', [('Content-type', 'text/plain')]) |
42 | return ['\n'] |
43 | |
44 | + def __call__(self, environ, start_response): |
45 | + codebrowse_netloc = urlsplit( |
46 | + config.codehosting.secure_codebrowse_root).netloc |
47 | + if environ['HTTP_HOST'] == codebrowse_netloc: |
48 | + return RootApp.__call__(self, environ, start_response) |
49 | + else: |
50 | + # Return a fake response. |
51 | + start_response('200 OK', [('Content-type', 'text/plain')]) |
52 | + return ['This is a dummy destination.\n'] |
53 | + |
54 | |
55 | class TestLogout(TestCase): |
56 | layer = DatabaseFunctionalLayer |
57 | |
58 | - def intercept(self, uri, app): |
59 | - """Install wsgi interceptors for the uri, app tuple.""" |
60 | - if isinstance(uri, basestring): |
61 | - uri = lazr.uri.URI(uri) |
62 | - port = uri.port |
63 | - if port is None: |
64 | - if uri.scheme == 'http': |
65 | - port = 80 |
66 | - elif uri.scheme == 'https': |
67 | - port = 443 |
68 | - else: |
69 | - raise NotImplementedError(uri.scheme) |
70 | - else: |
71 | - port = int(port) |
72 | - wsgi_intercept.add_wsgi_intercept(uri.host, port, lambda: app) |
73 | - self.intercepted.append((uri.host, port)) |
74 | - |
75 | def setUp(self): |
76 | TestCase.setUp(self) |
77 | - self.intercepted = [] |
78 | self.session = None |
79 | - self.root = app = SimpleLogInRootApp(SESSION_VAR) |
80 | + app = SimpleLogInRootApp(SESSION_VAR) |
81 | app = session_scribbler(app, self) |
82 | app = HTTPExceptionHandler(app) |
83 | app = SessionHandler(app, SESSION_VAR, SECRET) |
84 | self.cookie_name = app.cookie_handler.cookie_name |
85 | - self.intercept(config.codehosting.secure_codebrowse_root, app) |
86 | - self.intercept(allvhosts.configs['mainsite'].rooturl, |
87 | - dummy_destination) |
88 | - install_opener() |
89 | - self.browser = wsgi_intercept.zope_testbrowser.WSGI_Browser() |
90 | + self.browser = Browser(wsgi_app=app) |
91 | # We want to pretend we are not a robot, or else mechanize will honor |
92 | # robots.txt. |
93 | self.browser.mech_browser.set_handle_robots(False) |
94 | self.browser.open( |
95 | config.codehosting.secure_codebrowse_root + '+login') |
96 | |
97 | - def tearDown(self): |
98 | - uninstall_opener() |
99 | - for host, port in self.intercepted: |
100 | - wsgi_intercept.remove_wsgi_intercept(host, port) |
101 | - TestCase.tearDown(self) |
102 | - |
103 | def testLoggerheadLogout(self): |
104 | # We start logged in as 'bob'. |
105 | self.assertEqual(self.session['user'], 'bob') |
106 | @@ -143,8 +113,7 @@ class TestLogout(TestCase): |
107 | # TestLoginAndLogout.test_CookieLogoutPage). |
108 | |
109 | # Here, we will have a more useless example of the basic machinery. |
110 | - dummy_root = 'http://dummy.dev/' |
111 | - self.intercept(dummy_root, dummy_destination) |
112 | + dummy_root = 'http://dummy.test/' |
113 | self.browser.open( |
114 | config.codehosting.secure_codebrowse_root + |
115 | '+logout?' + urlencode(dict(next_to=dummy_root + '+logout'))) |
116 | diff --git a/lib/lp/registry/stories/webservice/xx-distribution-mirror.txt b/lib/lp/registry/stories/webservice/xx-distribution-mirror.txt |
117 | index edcef0e..9e4c932 100644 |
118 | --- a/lib/lp/registry/stories/webservice/xx-distribution-mirror.txt |
119 | +++ b/lib/lp/registry/stories/webservice/xx-distribution-mirror.txt |
120 | @@ -165,15 +165,15 @@ Now trying to set the owner using Sample Person's webservice is not authorized. |
121 | ... } |
122 | >>> response = test_webservice.patch( |
123 | ... canonical_archive['self_link'], 'application/json', dumps(patch)) |
124 | - >>> print response.getheader('status') |
125 | - 401 Unauthorized |
126 | + >>> response.status |
127 | + 401 |
128 | |
129 | But if we use Karl, the mirror listing admin's, webservice, we can update the owner. |
130 | |
131 | >>> response = karl_webservice.patch( |
132 | ... canonical_archive['self_link'], 'application/json', dumps(patch)) |
133 | - >>> print response.getheader('status') |
134 | - 209 Content Returned |
135 | + >>> response.status |
136 | + 209 |
137 | |
138 | >>> patched_canonical_archive = response.jsonBody() |
139 | >>> print patched_canonical_archive['owner_link'] |
140 | @@ -193,7 +193,6 @@ Some attributes are read-only via the API: |
141 | ... canonical_releases['self_link'], 'application/json', dumps(patch)) |
142 | >>> print response |
143 | HTTP/1.1 400 Bad Request |
144 | - Status: 400 Bad Request |
145 | ... |
146 | enabled: You tried to modify a read-only attribute. |
147 | date_reviewed: You tried to modify a read-only attribute. |
148 | @@ -211,8 +210,8 @@ While others can be set with the appropriate authorization: |
149 | ... } |
150 | >>> response = test_webservice.patch( |
151 | ... canonical_releases['self_link'], 'application/json', dumps(patch)) |
152 | - >>> print response.getheader('status') |
153 | - 401 Unauthorized |
154 | + >>> response.status |
155 | + 401 |
156 | |
157 | >>> response = karl_webservice.patch( |
158 | ... canonical_releases['self_link'], 'application/json', dumps(patch)).jsonBody() |
159 | diff --git a/lib/lp/services/webapp/tests/test_error.py b/lib/lp/services/webapp/tests/test_error.py |
160 | index 7ad4e83..3170ae3 100644 |
161 | --- a/lib/lp/services/webapp/tests/test_error.py |
162 | +++ b/lib/lp/services/webapp/tests/test_error.py |
163 | @@ -12,6 +12,7 @@ import urllib2 |
164 | |
165 | from fixtures import FakeLogger |
166 | import psycopg2 |
167 | +from six.moves.urllib.error import HTTPError |
168 | from storm.exceptions import ( |
169 | DisconnectionError, |
170 | OperationalError, |
171 | @@ -26,6 +27,7 @@ from testtools.matchers import ( |
172 | import transaction |
173 | from zope.interface import Interface |
174 | from zope.publisher.interfaces.browser import IDefaultBrowserLayer |
175 | +from zope.testbrowser.wsgi import Browser |
176 | |
177 | from lp.services.webapp.error import ( |
178 | DisconnectionErrorView, |
179 | @@ -37,12 +39,12 @@ from lp.testing import TestCase |
180 | from lp.testing.fixture import ( |
181 | CaptureOops, |
182 | PGBouncerFixture, |
183 | - Urllib2Fixture, |
184 | ZopeAdapterFixture, |
185 | ) |
186 | from lp.testing.layers import ( |
187 | DatabaseLayer, |
188 | LaunchpadFunctionalLayer, |
189 | + wsgi_application, |
190 | ) |
191 | from lp.testing.matchers import Contains |
192 | |
193 | @@ -78,8 +80,8 @@ class TestDatabaseErrorViews(TestCase): |
194 | |
195 | def getHTTPError(self, url): |
196 | try: |
197 | - urllib2.urlopen(url) |
198 | - except urllib2.HTTPError as error: |
199 | + Browser(wsgi_app=wsgi_application).open(url) |
200 | + except HTTPError as error: |
201 | return error |
202 | else: |
203 | self.fail("We should have gotten an HTTP error") |
204 | @@ -103,13 +105,14 @@ class TestDatabaseErrorViews(TestCase): |
205 | def retryConnection(self, url, bouncer, retries=60): |
206 | """Retry to connect to *url* for *retries* times. |
207 | |
208 | - Return the file-like object returned by *urllib2.urlopen(url)*. |
209 | - Raise a TimeoutException if the connection can not be established. |
210 | + Raise a TimeoutException if the connection cannot be established. |
211 | """ |
212 | + browser = Browser(wsgi_app=wsgi_application) |
213 | for i in range(retries): |
214 | try: |
215 | - return urllib2.urlopen(url) |
216 | - except urllib2.HTTPError as e: |
217 | + browser.open(url) |
218 | + return |
219 | + except HTTPError as e: |
220 | if e.code != httplib.SERVICE_UNAVAILABLE: |
221 | raise |
222 | time.sleep(1) |
223 | @@ -122,7 +125,6 @@ class TestDatabaseErrorViews(TestCase): |
224 | def test_disconnectionerror_view_integration(self): |
225 | # Test setup. |
226 | self.useFixture(FakeLogger('SiteError', level=logging.CRITICAL)) |
227 | - self.useFixture(Urllib2Fixture()) |
228 | bouncer = PGBouncerFixture() |
229 | # XXX gary bug=974617, bug=1011847, bug=504291 2011-07-03: |
230 | # In parallel tests, we are rarely encountering instances of |
231 | @@ -230,7 +232,6 @@ class TestDatabaseErrorViews(TestCase): |
232 | def test_operationalerror_view_integration(self): |
233 | # Test setup. |
234 | self.useFixture(FakeLogger('SiteError', level=logging.CRITICAL)) |
235 | - self.useFixture(Urllib2Fixture()) |
236 | |
237 | class BrokenView(object): |
238 | """A view that raises an OperationalError""" |
239 | diff --git a/lib/lp/services/webservice/stories/xx-service.txt b/lib/lp/services/webservice/stories/xx-service.txt |
240 | index cfe2c32..ee15cd4 100644 |
241 | --- a/lib/lp/services/webservice/stories/xx-service.txt |
242 | +++ b/lib/lp/services/webservice/stories/xx-service.txt |
243 | @@ -54,8 +54,8 @@ consumer keys. |
244 | |
245 | >>> caller = LaunchpadWebServiceCaller(u'new-consumer', u'access-key') |
246 | >>> response = caller.get(root) |
247 | - >>> print response.getheader('status') |
248 | - 401 Unauthorized |
249 | + >>> response.status |
250 | + 401 |
251 | >>> print response.body |
252 | Unknown consumer (new-consumer). |
253 | |
254 | @@ -73,14 +73,14 @@ doesn't recognize the client. |
255 | |
256 | >>> caller = LaunchpadWebServiceCaller(u'another-new-consumer', u'') |
257 | >>> response = caller.get(root) |
258 | - >>> print response.getheader('status') |
259 | - 200 Ok |
260 | + >>> response.status |
261 | + 200 |
262 | |
263 | Anonymous requests can't access certain data. |
264 | |
265 | >>> response = anon_webservice.get(body['me_link']) |
266 | - >>> print response.getheader('status') |
267 | - 401 Unauthorized |
268 | + >>> response.status |
269 | + 401 |
270 | >>> print response.body |
271 | You need to be logged in to view this URL. |
272 | |
273 | @@ -90,8 +90,8 @@ Anonymous requests can't change the dataset. |
274 | >>> data = simplejson.dumps({'display_name' : "This won't work"}) |
275 | >>> response = anon_webservice.patch(root + "/~salgado", |
276 | ... 'application/json', data) |
277 | - >>> print response.getheader('status') |
278 | - 401 Unauthorized |
279 | + >>> response.status |
280 | + 401 |
281 | >>> print response.body |
282 | (<Person at...>, 'display_name', 'launchpad.Edit') |
283 | |
284 | diff --git a/lib/lp/soyuz/stories/webservice/xx-archive.txt b/lib/lp/soyuz/stories/webservice/xx-archive.txt |
285 | index 57bb1f5..7acd47c 100644 |
286 | --- a/lib/lp/soyuz/stories/webservice/xx-archive.txt |
287 | +++ b/lib/lp/soyuz/stories/webservice/xx-archive.txt |
288 | @@ -874,12 +874,12 @@ methods. |
289 | |
290 | >>> response = webservice.named_get( |
291 | ... cprov_archive['self_link'], 'getPublishedSources') |
292 | - >>> print(response.getheader('status')) |
293 | - 200 Ok |
294 | + >>> response.status |
295 | + 200 |
296 | >>> response = webservice.named_get( |
297 | ... cprov_archive['self_link'], 'getPublishedBinaries') |
298 | - >>> print(response.getheader('status')) |
299 | - 200 Ok |
300 | + >>> response.status |
301 | + 200 |
302 | |
303 | If either method is called with the version parameter, the name must |
304 | be specified too, otherwise it is considered a bad webservice |
305 | @@ -887,13 +887,13 @@ request. |
306 | |
307 | >>> response = webservice.named_get( |
308 | ... cprov_archive['self_link'], 'getPublishedSources', version='1.0') |
309 | - >>> print(response.getheader('status')) |
310 | - 400 Bad Request |
311 | + >>> response.status |
312 | + 400 |
313 | >>> response = webservice.named_get( |
314 | ... cprov_archive['self_link'], 'getPublishedBinaries', |
315 | ... version='1.0') |
316 | - >>> print(response.getheader('status')) |
317 | - 400 Bad Request |
318 | + >>> response.status |
319 | + 400 |
320 | |
321 | We don't have to specify any filters when getting published sources: |
322 | |
323 | diff --git a/lib/lp/soyuz/stories/webservice/xx-person-createppa.txt b/lib/lp/soyuz/stories/webservice/xx-person-createppa.txt |
324 | index 3f4f9a1..085d8c8 100644 |
325 | --- a/lib/lp/soyuz/stories/webservice/xx-person-createppa.txt |
326 | +++ b/lib/lp/soyuz/stories/webservice/xx-person-createppa.txt |
327 | @@ -22,7 +22,6 @@ |
328 | ... name='yay', displayname='My shiny new PPA', |
329 | ... description='Shinyness!')) |
330 | HTTP/1.1 201 Created |
331 | - Status: 201 |
332 | ... |
333 | Location: http://api.launchpad.test/.../+archive/ubuntu/yay |
334 | ... |
335 | @@ -32,7 +31,6 @@ |
336 | ... displayname='My shiny new PPA', description='Shinyness!', |
337 | ... )) |
338 | HTTP/1.1 400 Bad Request |
339 | - Status: 400 Bad Request |
340 | ... |
341 | A PPA cannot have the same name as its distribution. |
342 | |
343 | @@ -52,7 +50,6 @@ They aren't a commercial admin, so they cannot create private PPAs. |
344 | ... private=True, |
345 | ... )) |
346 | HTTP/1.1 400 Bad Request |
347 | - Status: 400 |
348 | ... |
349 | ... is not allowed to make private PPAs |
350 | |
351 | @@ -78,7 +75,6 @@ Once they have commercial access, they can create private PPAs: |
352 | ... private=True, |
353 | ... )) |
354 | HTTP/1.1 201 Created |
355 | - Status: 201 |
356 | ... |
357 | Location: http://api.launchpad.test/.../+archive/ubuntu/secret |
358 | ... |
359 | @@ -103,7 +99,6 @@ It's possible to create PPAs for all sorts of distributions. |
360 | ... ppa_owner['self_link'], 'createPPA', {}, distribution='/ubuntutest', |
361 | ... name='ppa')) |
362 | HTTP/1.1 201 Created |
363 | - Status: 201 |
364 | ... |
365 | Location: http://api.launchpad.test/.../+archive/ubuntutest/ppa |
366 | ... |
367 | @@ -114,7 +109,6 @@ But not for distributions that don't have PPAs enabled. |
368 | ... ppa_owner['self_link'], 'createPPA', {}, distribution='/redhat', |
369 | ... name='ppa')) |
370 | HTTP/1.1 400 Bad Request |
371 | - Status: 400 |
372 | ... |
373 | Red Hat does not support PPAs. |
374 | |
375 | @@ -128,7 +122,6 @@ respectively. |
376 | >>> print(ppa_owner_webservice.named_post( |
377 | ... ppa_owner['self_link'], 'createPPA', {})) |
378 | HTTP/1.1 201 Created |
379 | - Status: 201 |
380 | ... |
381 | Location: http://api.launchpad.test/.../+archive/ubuntu/ppa |
382 | ... |
383 | diff --git a/lib/lp/testing/fixture.py b/lib/lp/testing/fixture.py |
384 | index 557a385..45d3b50 100644 |
385 | --- a/lib/lp/testing/fixture.py |
386 | +++ b/lib/lp/testing/fixture.py |
387 | @@ -10,7 +10,6 @@ __all__ = [ |
388 | 'DisableTriggerFixture', |
389 | 'PGBouncerFixture', |
390 | 'PGNotReadyError', |
391 | - 'Urllib2Fixture', |
392 | 'ZopeAdapterFixture', |
393 | 'ZopeEventHandlerFixture', |
394 | 'ZopeUtilityFixture', |
395 | @@ -31,14 +30,6 @@ from lazr.restful.utils import get_current_browser_request |
396 | import oops |
397 | import oops_amqp |
398 | import pgbouncer.fixture |
399 | -from wsgi_intercept import ( |
400 | - add_wsgi_intercept, |
401 | - remove_wsgi_intercept, |
402 | - ) |
403 | -from wsgi_intercept.urllib2_intercept import ( |
404 | - install_opener, |
405 | - uninstall_opener, |
406 | - ) |
407 | from zope.component import ( |
408 | adapter, |
409 | getGlobalSiteManager, |
410 | @@ -254,22 +245,6 @@ class ZopeUtilityFixture(Fixture): |
411 | gsm.registerUtility, original, self.intf, self.name) |
412 | |
413 | |
414 | -class Urllib2Fixture(Fixture): |
415 | - """Let tests use urllib to connect to an in-process Launchpad. |
416 | - |
417 | - Initially this only supports connecting to launchpad.test because |
418 | - that is all that is needed. Later work could connect all |
419 | - sub-hosts (e.g. bugs.launchpad.test).""" |
420 | - |
421 | - def _setUp(self): |
422 | - # Work around circular import. |
423 | - from lp.testing.layers import wsgi_application |
424 | - add_wsgi_intercept('launchpad.test', 80, lambda: wsgi_application) |
425 | - self.addCleanup(remove_wsgi_intercept, 'launchpad.test', 80) |
426 | - install_opener() |
427 | - self.addCleanup(uninstall_opener) |
428 | - |
429 | - |
430 | class CaptureOops(Fixture): |
431 | """Capture OOPSes notified via zope event notification. |
432 | |
433 | diff --git a/lib/lp/testing/layers.py b/lib/lp/testing/layers.py |
434 | index 22a510f..984e87b 100644 |
435 | --- a/lib/lp/testing/layers.py |
436 | +++ b/lib/lp/testing/layers.py |
437 | @@ -1020,12 +1020,9 @@ def wsgi_application(environ, start_response): |
438 | request = zope.publisher.publish.publish( |
439 | request, handle_errors=handle_errors) |
440 | response = request.response |
441 | - # We sort these, and then put the status first, because |
442 | - # zope.app.testing.testbrowser does--and because it makes it easier to |
443 | - # write reliable tests. |
444 | + # We sort these because it makes it easier to write reliable tests. |
445 | headers = sorted(response.getHeaders()) |
446 | status = response.getStatusString() |
447 | - headers.insert(0, ('Status', status)) |
448 | # Start the WSGI server response. |
449 | start_response(status, headers) |
450 | # Return the result body iterable. |