launchpad-reviewers team mailing list archive

[Gavin Panella <gavin.panella@xxxxxxxxxxxxx>]

Gavin Panella has proposed merging lp:~allenap/launchpad/tac-test-setup-killkillkill-bug-828164 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #828164 in Launchpad itself: "TacTestSetup breaks when trying to kill an already dead process"
  https://bugs.launchpad.net/launchpad/+bug/828164

For more details, see:
https://code.launchpad.net/~allenap/launchpad/tac-test-setup-killkillkill-bug-828164/+merge/71945

This changes the TacTestSetup fixture as detailed in the bug, and
spends a lot of time demonstrating that it works! I've also improved
the existing tests.

-- 
https://code.launchpad.net/~allenap/launchpad/tac-test-setup-killkillkill-bug-828164/+merge/71945
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~allenap/launchpad/tac-test-setup-killkillkill-bug-828164 into lp:launchpad.

=== modified file 'lib/canonical/launchpad/daemons/tachandler.py'
--- lib/canonical/launchpad/daemons/tachandler.py	2011-06-21 17:13:47 +0000
+++ lib/canonical/launchpad/daemons/tachandler.py	2011-08-17 20:29:25 +0000
@@ -24,7 +24,6 @@
     get_pid_from_file,
     kill_by_pidfile,
     remove_if_exists,
-    two_stage_kill,
     until_no_eintr,
     )
 
@@ -55,10 +54,7 @@
             warnings.warn("Attempt to start Tachandler with an existing "
                 "instance (%d) running in %s." % (pid, self.pidfile),
                 DeprecationWarning, stacklevel=2)
-            two_stage_kill(pid)
-            if get_pid_from_file(self.pidfile):
-                raise TacException(
-                    "Could not kill stale process %s." % (self.pidfile,))
+            kill_by_pidfile(self.pidfile)
 
         # setUp() watches the logfile to determine when the daemon has fully
         # started. If it sees an old logfile, then it will find the

=== modified file 'lib/canonical/launchpad/daemons/tests/cannotlisten.tac'
--- lib/canonical/launchpad/daemons/tests/cannotlisten.tac	2010-10-20 18:43:29 +0000
+++ lib/canonical/launchpad/daemons/tests/cannotlisten.tac	2011-08-17 20:29:25 +0000
@@ -8,7 +8,10 @@
 
 __metaclass__ = type
 
-from twisted.application import service, internet, strports
+from twisted.application import (
+    internet,
+    service,
+    )
 from twisted.internet import protocol
 
 from canonical.launchpad.daemons import readyservice
@@ -26,4 +29,3 @@
 
 # Just in case we can, try listening on port 1 *again*.  This will fail.
 internet.TCPServer(1, protocol.Factory()).setServiceParent(serviceCollection)
-

=== added file 'lib/canonical/launchpad/daemons/tests/okay.tac'
--- lib/canonical/launchpad/daemons/tests/okay.tac	1970-01-01 00:00:00 +0000
+++ lib/canonical/launchpad/daemons/tests/okay.tac	2011-08-17 20:29:25 +0000
@@ -0,0 +1,19 @@
+# Copyright 2011 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""
+This TAC is used for the TacTestSetupTestCase.test_pidForNotRunningProcess
+test case in test_tachandler.py.  It simply starts up correctly.
+"""
+
+__metaclass__ = type
+
+from twisted.application import service
+
+from canonical.launchpad.daemons import readyservice
+
+
+application = service.Application('Okay')
+
+# Service that announces when the daemon is ready
+readyservice.ReadyService().setServiceParent(application)

=== modified file 'lib/canonical/launchpad/daemons/tests/test_tachandler.py'
--- lib/canonical/launchpad/daemons/tests/test_tachandler.py	2011-08-12 11:19:40 +0000
+++ lib/canonical/launchpad/daemons/tests/test_tachandler.py	2011-08-17 20:29:25 +0000
@@ -6,40 +6,96 @@
 __metaclass__ = type
 
 import os.path
-import tempfile
-import unittest
+import subprocess
+import warnings
+
+from fixtures import TempDir
+import testtools
 
 from canonical.launchpad.daemons.tachandler import (
     TacException,
     TacTestSetup,
     )
-
-
-class TacTestSetupTestCase(unittest.TestCase):
+from lp.services.osutils import get_pid_from_file
+
+
+class TacTestSetupTestCase(testtools.TestCase):
     """Some tests for the error handling of TacTestSetup."""
 
     def test_missingTac(self):
         """TacTestSetup raises TacException if the tacfile doesn't exist"""
+
         class MissingTac(TacTestSetup):
+
             root = '/'
             tacfile = '/file/does/not/exist'
             pidfile = tacfile
             logfile = tacfile
+
             def setUpRoot(self):
                 pass
 
-        self.assertRaises(TacException, MissingTac().setUp)
+        fixture = MissingTac()
+        try:
+            self.assertRaises(TacException, fixture.setUp)
+        finally:
+            fixture.cleanUp()
 
     def test_couldNotListenTac(self):
-        """If the tac fails due to not being able to listen on the needed port,
-        TacTestSetup will fail.
+        """If the tac fails due to not being able to listen on the needed
+        port, TacTestSetup will fail.
         """
+        tempdir = self.useFixture(TempDir()).path
+
         class CouldNotListenTac(TacTestSetup):
+
             root = os.path.dirname(__file__)
             tacfile = os.path.join(root, 'cannotlisten.tac')
-            pidfile = os.path.join(tempfile.gettempdir(), 'cannotlisten.pid')
-            logfile = os.path.join(tempfile.gettempdir(), 'cannotlisten.log')
-            def setUpRoot(self):
-                pass
-
-        self.assertRaises(TacException, CouldNotListenTac().setUp)
+            pidfile = os.path.join(tempdir, 'cannotlisten.pid')
+            logfile = os.path.join(tempdir, 'cannotlisten.log')
+
+            def setUpRoot(self):
+                pass
+
+        fixture = CouldNotListenTac()
+        try:
+            self.assertRaises(TacException, fixture.setUp)
+        finally:
+            fixture.cleanUp()
+
+    def test_pidForNotRunningProcess(self):
+        """TacTestSetup copes fine if the pidfile contains a stale pid."""
+        tempdir = self.useFixture(TempDir()).path
+
+        class OkayTac(TacTestSetup):
+
+            root = os.path.dirname(__file__)
+            tacfile = os.path.join(root, 'okay.tac')
+            pidfile = os.path.join(tempdir, 'okay.pid')
+            logfile = os.path.join(tempdir, 'okay.log')
+
+            def setUpRoot(self):
+                pass
+
+        # Run a short-lived process with the intention of using its pid in the
+        # next step. Linux uses pids sequentially (from the information I've
+        # been able to discover) so this approach is safe as long as we don't
+        # delay until pids wrap... which should be a very long time unless the
+        # machine is seriously busy.
+        process = subprocess.Popen("true")
+        process.wait()
+
+        # Put the (now bogus) pid in the pid file.
+        with open(OkayTac.pidfile, "wb") as pidfile:
+            pidfile.write(str(process.pid))
+
+        # Fire up the fixture, capturing warnings.
+        with warnings.catch_warnings(record=True) as warnings_log:
+            with OkayTac() as fixture:
+                self.assertNotEqual(
+                    get_pid_from_file(fixture.pidfile),
+                    process.pid)
+
+        # One deprecation warning is emitted.
+        self.assertEqual(1, len(warnings_log))
+        self.assertIs(DeprecationWarning, warnings_log[0].category)