launchpad-reviewers team mailing list archive

[Brad Crittenden <bac@xxxxxxxxxxxxx>]

Brad Crittenden has proposed merging lp:~bac/launchpad/bug-974617-2 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #974617 in Launchpad itself: "test_operationalerror_view_integration fails intermittently in parallel tests"
  https://bugs.launchpad.net/launchpad/+bug/974617

For more details, see:
https://code.launchpad.net/~bac/launchpad/bug-974617-2/+merge/103339

= 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
-- 
https://code.launchpad.net/~bac/launchpad/bug-974617-2/+merge/103339
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~bac/launchpad/bug-974617-2 into lp:launchpad.

=== 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:13:25 +0000
@@ -9,7 +9,6 @@
     DisconnectionError,
     OperationalError,
     )
-import time
 import transaction
 import urllib2
 
@@ -112,31 +111,15 @@
         bouncer.stop()
         url = 'http://launchpad.dev/'
         error = self.getHTTPError(url)
-        self.assertEqual(503, error.code)
+        self.assertEqual(httplib.SERVICE_UNAVAILABLE, error.code)
         self.assertThat(error.read(),
                         Contains(OperationalErrorView.reason))
         # We keep seeing the correct exception on subsequent requests.
-        self.assertEqual(503, self.getHTTPError(url).code)
+        self.assertEqual(httplib.SERVICE_UNAVAILABLE,
+                         self.getHTTPError(url).code)
         # When the database is available again, requests succeed.
         bouncer.start()
-        # bouncer.start() can sometimes return before the service is actually
-        # available for use.  To be defensive, let's retry a few times.  See
-        # bug 974617.
-        retries = 5
-        for i in xrange(retries):
-            try:
-                urllib2.urlopen(url)
-            except urllib2.HTTPError as e:
-                if e.code != httplib.SERVICE_UNAVAILABLE:
-                    raise
-            else:
-                break
-            time.sleep(0.5)
-        else:
-            raise TimeoutException(
-                "bouncer did not come up after {} attempts.".format(retries))
-
-
+        urllib2.urlopen(url)
 
     def test_operationalerror_view(self):
         request = LaunchpadTestRequest()

=== 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:13:25 +0000
@@ -8,6 +8,7 @@
     'CaptureOops',
     'DemoMode',
     'PGBouncerFixture',
+    'PGNotReadyError',
     'Urllib2Fixture',
     'ZopeAdapterFixture',
     'ZopeEventHandlerFixture',
@@ -23,6 +24,7 @@
     EnvironmentVariableFixture,
     Fixture,
     )
+import itertools
 from lazr.restful.utils import get_current_browser_request
 import oops
 import oops_amqp
@@ -56,6 +58,10 @@
 from lp.services.webapp.errorlog import ErrorReportEvent
 
 
+class PGNotReadyError(Exception):
+    pass
+
+
 class PGBouncerFixture(pgbouncer.fixture.PGBouncerFixture):
     """Inserts a controllable pgbouncer instance in front of PostgreSQL.
 
@@ -121,6 +127,16 @@
         if is_ca_available():
             reconnect_stores()
 
+    def start(self, retries=20, sleep=0.5):
+        """Simply return to simulate an error starting PGBouncer."""
+        super(PGBouncerFixture, self).start()
+        for i in itertools.count(1):
+            if self.is_running:
+                return
+            if i == retries:
+                raise PGNotReadyError("Not ready after %d attempts." % i)
+            time.sleep(sleep)
+
 
 class ZopeAdapterFixture(Fixture):
     """A fixture to register and unregister an adapter."""