launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/tachandler-debug-sql into lp:launchpad.

Commit message:
Ensure that LP_DEBUG_SQL is not set when setting up TacTestFixture, otherwise the fixture will always fail.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/tachandler-debug-sql/+merge/217228

If you try to use LP_DEBUG_SQL=1 with tests that need to start a .tac fixture (such as anything involving LibrarianLayer), the fixture setup will fail with "unclean stdout/err".  This squashes LP_DEBUG_SQL for the .tac file so that it can still be used for tests.
-- 
https://code.launchpad.net/~cjwatson/launchpad/tachandler-debug-sql/+merge/217228
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/tachandler-debug-sql into lp:launchpad.

=== modified file 'lib/lp/services/daemons/tachandler.py'
--- lib/lp/services/daemons/tachandler.py	2012-02-28 08:25:30 +0000
+++ lib/lp/services/daemons/tachandler.py	2014-04-25 12:37:52 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2011 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2014 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Test harness for TAC (Twisted Application Configuration) files."""
@@ -21,7 +21,10 @@
 
 import lp
 from lp.services.daemons import readyservice
-from lp.services.osutils import remove_if_exists
+from lp.services.osutils import (
+    override_environ,
+    remove_if_exists,
+    )
 
 
 twistd_script = os.path.abspath(os.path.join(
@@ -42,9 +45,10 @@
         # hard-to-diagnose race conditions. Delete the logfile to make sure
         # this does not happen.
         remove_if_exists(self.logfile)
-        TacTestFixture.setUp(self,
-            python_path=sys.executable,
-            twistd_script=twistd_script)
+        with override_environ(LP_DEBUG_SQL=None):
+            TacTestFixture.setUp(self,
+                python_path=sys.executable,
+                twistd_script=twistd_script)
 
     def _hasDaemonStarted(self):
         """Has the daemon started?

=== modified file 'lib/lp/services/daemons/tests/okay.tac'
--- lib/lp/services/daemons/tests/okay.tac	2011-12-24 19:06:36 +0000
+++ lib/lp/services/daemons/tests/okay.tac	2014-04-25 12:37:52 +0000
@@ -1,4 +1,4 @@
-# Copyright 2011 Canonical Ltd.  This software is licensed under the
+# Copyright 2011-2014 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """
@@ -9,11 +9,24 @@
 __metaclass__ = type
 
 from twisted.application import service
+from zope.component import getUtility
 
+from lp.services.database.interfaces import (
+    DEFAULT_FLAVOR,
+    IStoreSelector,
+    MAIN_STORE,
+    )
 from lp.services.daemons import readyservice
-
+from lp.services.scripts import execute_zcml_for_scripts
+
+
+execute_zcml_for_scripts()
 
 application = service.Application('Okay')
 
 # Service that announces when the daemon is ready
 readyservice.ReadyService().setServiceParent(application)
+
+# Do some trivial database work that will show up in an SQL debug log.
+store = getUtility(IStoreSelector).get(MAIN_STORE, DEFAULT_FLAVOR)
+store.execute("SELECT 1").get_one()

=== modified file 'lib/lp/services/daemons/tests/test_tachandler.py'
--- lib/lp/services/daemons/tests/test_tachandler.py	2011-12-24 19:06:36 +0000
+++ lib/lp/services/daemons/tests/test_tachandler.py	2014-04-25 12:37:52 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2011 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2014 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Tests for lp.services.daemons.tachandler"""
@@ -26,7 +26,11 @@
     TacException,
     TacTestSetup,
     )
-from lp.services.osutils import get_pid_from_file
+from lp.services.osutils import (
+    get_pid_from_file,
+    override_environ,
+    )
+from lp.testing.layers import DatabaseLayer
 
 
 class SimpleTac(TacTestSetup):
@@ -70,6 +74,8 @@
 class TacTestSetupTestCase(testtools.TestCase):
     """Some tests for the error handling of TacTestSetup."""
 
+    layer = DatabaseLayer
+
     def test_okay(self):
         """TacTestSetup sets up and runs a simple service."""
         tempdir = self.useFixture(TempDir()).path
@@ -84,6 +90,21 @@
         # No warnings are emitted.
         self.assertEqual([], warnings_log)
 
+    def test_debug_sql(self):
+        """TacTestSetup works even under LP_DEBUG_SQL=1."""
+        tempdir = self.useFixture(TempDir()).path
+        fixture = SimpleTac("okay", tempdir)
+
+        # Fire up the fixture, capturing warnings.
+        with override_environ(LP_DEBUG_SQL="1"):
+            with warnings.catch_warnings(record=True) as warnings_log:
+                with fixture:
+                    self.assertThat(fixture, IsRunning())
+                self.assertThat(fixture, Not(IsRunning()))
+
+        # No warnings are emitted.
+        self.assertEqual([], warnings_log)
+
     def test_missingTac(self):
         """TacTestSetup raises TacException if the tacfile doesn't exist"""
         fixture = SimpleTac("missing", "/file/does/not/exist")