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
=== modified file 'lib/lp/services/webapp/tests/test_error.py'
--- lib/lp/services/webapp/tests/test_error.py 2012-04-13 02:56:20 +0000
+++ lib/lp/services/webapp/tests/test_error.py 2012-04-24 18:23:27 +0000
@@ -9,7 +9,6 @@
9 DisconnectionError,9 DisconnectionError,
10 OperationalError,10 OperationalError,
11 )11 )
12import time
13import transaction12import transaction
14import urllib213import urllib2
1514
@@ -112,31 +111,15 @@
112 bouncer.stop()111 bouncer.stop()
113 url = 'http://launchpad.dev/'112 url = 'http://launchpad.dev/'
114 error = self.getHTTPError(url)113 error = self.getHTTPError(url)
115 self.assertEqual(503, error.code)114 self.assertEqual(httplib.SERVICE_UNAVAILABLE, error.code)
116 self.assertThat(error.read(),115 self.assertThat(error.read(),
117 Contains(OperationalErrorView.reason))116 Contains(OperationalErrorView.reason))
118 # We keep seeing the correct exception on subsequent requests.117 # We keep seeing the correct exception on subsequent requests.
119 self.assertEqual(503, self.getHTTPError(url).code)118 self.assertEqual(httplib.SERVICE_UNAVAILABLE,
119 self.getHTTPError(url).code)
120 # When the database is available again, requests succeed.120 # When the database is available again, requests succeed.
121 bouncer.start()121 bouncer.start()
122 # bouncer.start() can sometimes return before the service is actually122 urllib2.urlopen(url)
123 # available for use. To be defensive, let's retry a few times. See
124 # bug 974617.
125 retries = 5
126 for i in xrange(retries):
127 try:
128 urllib2.urlopen(url)
129 except urllib2.HTTPError as e:
130 if e.code != httplib.SERVICE_UNAVAILABLE:
131 raise
132 else:
133 break
134 time.sleep(0.5)
135 else:
136 raise TimeoutException(
137 "bouncer did not come up after {} attempts.".format(retries))
138
139
140123
141 def test_operationalerror_view(self):124 def test_operationalerror_view(self):
142 request = LaunchpadTestRequest()125 request = LaunchpadTestRequest()
143126
=== modified file 'lib/lp/testing/fixture.py'
--- lib/lp/testing/fixture.py 2012-01-18 07:35:31 +0000
+++ lib/lp/testing/fixture.py 2012-04-24 18:23:27 +0000
@@ -8,6 +8,7 @@
8 'CaptureOops',8 'CaptureOops',
9 'DemoMode',9 'DemoMode',
10 'PGBouncerFixture',10 'PGBouncerFixture',
11 'PGNotReadyError',
11 'Urllib2Fixture',12 'Urllib2Fixture',
12 'ZopeAdapterFixture',13 'ZopeAdapterFixture',
13 'ZopeEventHandlerFixture',14 'ZopeEventHandlerFixture',
@@ -23,6 +24,7 @@
23 EnvironmentVariableFixture,24 EnvironmentVariableFixture,
24 Fixture,25 Fixture,
25 )26 )
27import itertools
26from lazr.restful.utils import get_current_browser_request28from lazr.restful.utils import get_current_browser_request
27import oops29import oops
28import oops_amqp30import oops_amqp
@@ -56,6 +58,10 @@
56from lp.services.webapp.errorlog import ErrorReportEvent58from lp.services.webapp.errorlog import ErrorReportEvent
5759
5860
61class PGNotReadyError(Exception):
62 pass
63
64
59class PGBouncerFixture(pgbouncer.fixture.PGBouncerFixture):65class PGBouncerFixture(pgbouncer.fixture.PGBouncerFixture):
60 """Inserts a controllable pgbouncer instance in front of PostgreSQL.66 """Inserts a controllable pgbouncer instance in front of PostgreSQL.
6167
@@ -121,6 +127,16 @@
121 if is_ca_available():127 if is_ca_available():
122 reconnect_stores()128 reconnect_stores()
123129
130 def start(self, retries=20, sleep=0.5):
131 """Simply return to simulate an error starting PGBouncer."""
132 super(PGBouncerFixture, self).start()
133 for i in itertools.count(1):
134 if self.is_running:
135 return
136 if i == retries:
137 raise PGNotReadyError("Not ready after %d attempts." % i)
138 time.sleep(sleep)
139
124140
125class ZopeAdapterFixture(Fixture):141class ZopeAdapterFixture(Fixture):
126 """A fixture to register and unregister an adapter."""142 """A fixture to register and unregister an adapter."""