launchpad-reviewers team mailing list archive

[Stuart Bishop <stuart.bishop@xxxxxxxxxxxxx>]

Stuart Bishop has proposed merging lp:~stub/launchpad/pgbouncer-fixture-noca into lp:launchpad with lp:~stub/launchpad/pgbouncer-fixture as a prerequisite.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~stub/launchpad/pgbouncer-fixture-noca/+merge/73525

= Summary =

PGBouncerFixture would not work in layers without the Component Architecture loaded, such as DatabaseLayer, because it would attempt to reset the Storm Stores and these were not registered.

== Proposed fix ==

Only reset the Storm Stores if they are registered with the Component Architecture.
-- 
https://code.launchpad.net/~stub/launchpad/pgbouncer-fixture-noca/+merge/73525
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~stub/launchpad/pgbouncer-fixture-noca into lp:launchpad.

=== modified file 'lib/lp/testing/fixture.py'
--- lib/lp/testing/fixture.py	2011-08-31 13:18:42 +0000
+++ lib/lp/testing/fixture.py	2011-08-31 13:18:42 +0000
@@ -91,8 +91,7 @@
 
         # reconnect_store cleanup added first so it is run last, after
         # the environment variables have been reset.
-        from canonical.testing.layers import reconnect_stores
-        self.addCleanup(reconnect_stores)
+        self.addCleanup(self._maybe_reconnect_stores)
 
         # Abuse the PGPORT environment variable to get things connecting
         # via pgbouncer. Otherwise, we would need to temporarily
@@ -100,7 +99,21 @@
         self.useFixture(EnvironmentVariableFixture('PGPORT', str(self.port)))
 
         # Reset database connections so they go through pgbouncer.
-        reconnect_stores()
+        self._maybe_reconnect_stores()
+
+    def _maybe_reconnect_stores(self):
+        """Force Storm Stores to reconnect if they are registered.
+
+        This is a noop if the Component Architecture is not loaded,
+        as we are using a test layer that doesn't provide database
+        connections.
+        """
+        from canonical.testing.layers import (
+            reconnect_stores,
+            is_ca_available,
+            )
+        if is_ca_available():
+            reconnect_stores()
 
 
 class ZopeAdapterFixture(Fixture):

=== modified file 'lib/lp/testing/tests/test_fixture.py'
--- lib/lp/testing/tests/test_fixture.py	2011-08-31 13:18:42 +0000
+++ lib/lp/testing/tests/test_fixture.py	2011-08-31 13:18:42 +0000
@@ -8,6 +8,7 @@
 from textwrap import dedent
 
 from fixtures import EnvironmentVariableFixture
+import psycopg2
 from storm.exceptions import DisconnectionError
 from zope.component import (
     adapts,
@@ -18,8 +19,13 @@
     Interface,
     )
 
+from canonical.config import dbconfig
 from canonical.launchpad.interfaces.lpstorm import IMasterStore
-from canonical.testing.layers import BaseLayer, LaunchpadZopelessLayer
+from canonical.testing.layers import (
+    BaseLayer,
+    DatabaseLayer,
+    LaunchpadZopelessLayer,
+    )
 from lp.registry.model.person import Person
 from lp.testing import TestCase
 from lp.testing.fixture import (
@@ -95,7 +101,11 @@
         self.assertIs(None, queryAdapter(context, IBar))
 
 
-class TestPGBouncerFixture(TestCase):
+class TestPGBouncerFixtureWithCA(TestCase):
+    """PGBouncerFixture reconnect tests for Component Architecture layers.
+
+    Registered Storm Stores should be reconnected through pgbouncer.
+    """
     layer = LaunchpadZopelessLayer
 
     def is_connected(self):
@@ -149,3 +159,49 @@
 
         # Database is working again.
         assert self.is_connected()
+
+
+class TestPGBouncerFixtureWithoutCA(TestCase):
+    """PGBouncerFixture tests for non-Component Architecture layers."""
+    layer = DatabaseLayer
+
+    def is_db_available(self):
+        # Direct connection to the DB.
+        con_str = dbconfig.rw_main_master + ' user=launchpad_main'
+        try:
+            con = psycopg2.connect(con_str)
+            cur = con.cursor()
+            cur.execute("SELECT id FROM Person LIMIT 1")
+            con.close()
+            return True
+        except psycopg2.OperationalError:
+            return False
+
+    def test_install_fixture(self):
+        self.assert_(self.is_db_available())
+
+        with PGBouncerFixture() as pgbouncer:
+            self.assert_(self.is_db_available())
+
+            pgbouncer.stop()
+            self.assert_(not self.is_db_available())
+
+        # This confirms that we are again connecting directly to the
+        # database, as the pgbouncer process was shutdown.
+        self.assert_(self.is_db_available())
+
+    def test_install_fixture_with_restart(self):
+        self.assert_(self.is_db_available())
+
+        with PGBouncerFixture() as pgbouncer:
+            self.assert_(self.is_db_available())
+
+            pgbouncer.stop()
+            self.assert_(not self.is_db_available())
+
+            pgbouncer.start()
+            self.assert_(self.is_db_available())
+
+        # Note that because pgbouncer was left running, we can't confirm
+        # that we are now connecting directly to the database.
+        self.assert_(self.is_db_available())