Merge lp:~bac/launchpad/bug-974617-2 into lp:launchpad

Proposed by Brad Crittenden on 2012-04-24
Status: Merged
Approved by: Brad Crittenden on 2012-04-24
Approved revision: no longer in the source branch.
Merged at revision: 15157
Proposed branch: lp:~bac/launchpad/bug-974617-2
Merge into: lp:launchpad
Diff against target: 95 lines (+20/-21)
2 files modified
lib/lp/services/webapp/tests/test_error.py (+4/-21)
lib/lp/testing/fixture.py (+16/-0)
To merge this branch: bzr merge lp:~bac/launchpad/bug-974617-2
Reviewer Review Type Date Requested Status
Benji York (community) code 2012-04-24 Approve on 2012-04-24
Review via email: mp+103339@code.launchpad.net

Commit message

Wait for is_running to be true before returning from the PGBouncerTestFixture start() method.

Description of the change

= Summary =

Our previous attempt to fix this bug, suspected to be caused by the PG
bouncer not really being available even after the PID is written,
didn't work. This approach adds a start method to the test fixture
that waits for the is_running property to be true, instead of
attempting to fetch an URL.

== Proposed fix ==

As above.

== Pre-implementation notes ==

Talks with Gary.

== Implementation details ==

As above.

== Tests ==

bin/test -vvt test_operationalerror_view_integration

== Demo and Q/A ==

No QA.

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/services/webapp/tests/test_error.py
  lib/lp/testing/fixture.py

To post a comment you must log in.
Benji York (benji) wrote :

This branch looks good. It does look like the docstring for start in
fixture.py needs reconciling with reality.

review: Approve (code)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/lp/services/webapp/tests/test_error.py'
2--- lib/lp/services/webapp/tests/test_error.py 2012-04-13 02:56:20 +0000
3+++ lib/lp/services/webapp/tests/test_error.py 2012-04-24 18:23:27 +0000
4@@ -9,7 +9,6 @@
5 DisconnectionError,
6 OperationalError,
7 )
8-import time
9 import transaction
10 import urllib2
11
12@@ -112,31 +111,15 @@
13 bouncer.stop()
14 url = 'http://launchpad.dev/'
15 error = self.getHTTPError(url)
16- self.assertEqual(503, error.code)
17+ self.assertEqual(httplib.SERVICE_UNAVAILABLE, error.code)
18 self.assertThat(error.read(),
19 Contains(OperationalErrorView.reason))
20 # We keep seeing the correct exception on subsequent requests.
21- self.assertEqual(503, self.getHTTPError(url).code)
22+ self.assertEqual(httplib.SERVICE_UNAVAILABLE,
23+ self.getHTTPError(url).code)
24 # When the database is available again, requests succeed.
25 bouncer.start()
26- # bouncer.start() can sometimes return before the service is actually
27- # available for use. To be defensive, let's retry a few times. See
28- # bug 974617.
29- retries = 5
30- for i in xrange(retries):
31- try:
32- urllib2.urlopen(url)
33- except urllib2.HTTPError as e:
34- if e.code != httplib.SERVICE_UNAVAILABLE:
35- raise
36- else:
37- break
38- time.sleep(0.5)
39- else:
40- raise TimeoutException(
41- "bouncer did not come up after {} attempts.".format(retries))
42-
43-
44+ urllib2.urlopen(url)
45
46 def test_operationalerror_view(self):
47 request = LaunchpadTestRequest()
48
49=== modified file 'lib/lp/testing/fixture.py'
50--- lib/lp/testing/fixture.py 2012-01-18 07:35:31 +0000
51+++ lib/lp/testing/fixture.py 2012-04-24 18:23:27 +0000
52@@ -8,6 +8,7 @@
53 'CaptureOops',
54 'DemoMode',
55 'PGBouncerFixture',
56+ 'PGNotReadyError',
57 'Urllib2Fixture',
58 'ZopeAdapterFixture',
59 'ZopeEventHandlerFixture',
60@@ -23,6 +24,7 @@
61 EnvironmentVariableFixture,
62 Fixture,
63 )
64+import itertools
65 from lazr.restful.utils import get_current_browser_request
66 import oops
67 import oops_amqp
68@@ -56,6 +58,10 @@
69 from lp.services.webapp.errorlog import ErrorReportEvent
70
71
72+class PGNotReadyError(Exception):
73+ pass
74+
75+
76 class PGBouncerFixture(pgbouncer.fixture.PGBouncerFixture):
77 """Inserts a controllable pgbouncer instance in front of PostgreSQL.
78
79@@ -121,6 +127,16 @@
80 if is_ca_available():
81 reconnect_stores()
82
83+ def start(self, retries=20, sleep=0.5):
84+ """Simply return to simulate an error starting PGBouncer."""
85+ super(PGBouncerFixture, self).start()
86+ for i in itertools.count(1):
87+ if self.is_running:
88+ return
89+ if i == retries:
90+ raise PGNotReadyError("Not ready after %d attempts." % i)
91+ time.sleep(sleep)
92+
93
94 class ZopeAdapterFixture(Fixture):
95 """A fixture to register and unregister an adapter."""