Merge lp:~mbp/launchpad/use-txfixtures into lp:launchpad

[Martin Pool]

[r=bac][no-qa] split out and depend upon txfixtures

This builds on <https://code.launchpad.net/~mbp/launchpad/800295-buildd-split/+merge/81111> to do more separation of the buildd code from the Launchpad tree.

The only remaining code dependency of the buildds is that they need a test fixture to run a Twisted application out-of-process. I have extracted out that code into lp:txfixtures.

This patch then:

 * makes Launchpad depend on txfixtures
 * makes txfixtures.TacTestFixture the base for Launchpad's own TacTestSetup
 * corrects in passing a test that counts on raising a DeprecationWarning for what's actually an operational warning (stale pid file) and that can be flaky on pythons where that is automatically suppressed
 * changes the buildd code to use txfixtures directly rather than canonical.launchpad.daemons.tachandler, so it no longer needs anything from the lp tree

I thought about changing buildd to use only relative imports so that it obviously didn't use anything from the rest of the tree and it could be more easily moved, but in the light of <https://code.launchpad.net/~abentley/launchpad/absolute-oops-import/+merge/71423> I decided not to.

This doesn't actually move the buildd out of tree but that should now be a small step.

I've left most of the tac fixture tests in the tree - they are somewhat redundant with tests now done inside txfixtures, but they're cheap and I think the integration testing is worthwhile, at least for now.

Launchpad's TacTestSetup could probably be slimmed down a bit more or even deleted entirely.

[Martin Pool (mbp)]

At least one more thing is needed before this can land: package txfixtures
and make that a dpkg dependency of buildd.

[Brad Crittenden (bac)]

Hi Martin,

Thanks for the work you're doing here to untangle things. Your changes look good.

review: Approve (code)

[Robert Collins (lifeless)]

FTR you don't need to debian package txfixtures.

Add the egg to the download cache, and we're GTG.

[Martin Pool (mbp)]

On 7 November 2011 18:28, Robert Collins <email address hidden> wrote:
> FTR you don't need to debian package txfixtures.
>
> Add the egg to the download cache, and we're GTG.

For deployment into lp itself, yes. But this will also need to be
installed alongside buildds, which are installed from a package. So
we either need txfixtures packaged, or at least to get lamont & co to
agree to install it some other way.

[Martin Pool (mbp)]

I needed some additional changes in txfixtures to make this work on python<2.7, since they don't understand PYTHONWARNINGS and deprecation warnings make things fail.

=== modified file 'lib/canonical/buildd/tests/harness.py'
--- lib/canonical/buildd/tests/harness.py	2011-11-04 00:43:11 +0000
+++ lib/canonical/buildd/tests/harness.py	2011-11-07 07:50:31 +0000
@@ -11,11 +11,9 @@
 import unittest
 from ConfigParser import SafeConfigParser
 
-import canonical
-import canonical.buildd
+from txfixtures.tachandler import TacTestFixture
 
 from canonical.buildd.slave import BuildDSlave
-from canonical.launchpad.daemons.tachandler import TacTestSetup
 
 from lp.services.osutils import remove_tree
 
@@ -58,7 +56,7 @@
         f.close()
 
 
-class BuilddSlaveTestSetup(TacTestSetup):
+class BuilddSlaveTestSetup(TacTestFixture):
     r"""Setup BuildSlave for use by functional tests
 
     >>> fixture = BuilddSlaveTestSetup()
@@ -119,7 +117,8 @@
     @property
     def tacfile(self):
         return os.path.abspath(os.path.join(
-            os.path.dirname(canonical.buildd.__file__),
+            os.path.dirname(__file__),
+            os.path.pardir,
             'buildd-slave.tac'
             ))
 
 
=== modified file 'lib/canonical/launchpad/daemons/tachandler.py'
--- lib/canonical/launchpad/daemons/tachandler.py	2011-11-04 00:43:11 +0000
+++ lib/canonical/launchpad/daemons/tachandler.py	2011-11-07 07:50:31 +0000
@@ -21,6 +21,11 @@
 
 from fixtures import Fixture
 
+from txfixtures.tachandler import (
+    TacException,
+    TacTestFixture,
+    )
+
 from canonical.launchpad.daemons import readyservice
 from lp.services.osutils import (
     get_pid_from_file,
@@ -36,74 +41,22 @@
     os.pardir, os.pardir, os.pardir, os.pardir, 'bin', 'twistd'))
 
 
-class TacException(Exception):
-    """Error raised by TacTestSetup."""
-
-
-class TacTestSetup(Fixture):
+class TacTestSetup(TacTestFixture):
     """Setup an TAC file as daemon for use by functional tests.
 
     You must override setUpRoot to set up a root directory for the daemon.
     """
 
     def setUp(self, spew=False, umask=None):
-        Fixture.setUp(self)
-        if get_pid_from_file(self.pidfile):
-            # An attempt to run while there was an existing live helper
-            # was made. Note that this races with helpers which use unique
-            # roots, so when moving/eliminating this code check subclasses
-            # for workarounds and remove those too.
-            pid = get_pid_from_file(self.pidfile)
-            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 the pid file still exists, it may indicate that the process
-            # respawned itself, or that two processes were started (race?) and
-            # one is still running while the other has ended, or the process
-            # was killed but it didn't remove the pid file (bug), or the
-            # machine was hard-rebooted and the pid file was not cleaned up
-            # (bug again). In other words, it's not safe to assume that a
-            # stale pid file is safe to delete without human intervention.
-            if get_pid_from_file(self.pidfile):
-                raise TacException(
-                    "Could not kill stale process %s." % (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
         # readyservice.LOG_MAGIC string and return immediately, provoking
         # hard-to-diagnose race conditions. Delete the logfile to make sure
         # this does not happen.
         remove_if_exists(self.logfile)
-
-        self.setUpRoot()
-        args = [sys.executable,
-                # XXX: 2010-04-26, Salgado, bug=570246: Deprecation warnings
-                # in Twisted are not our problem.  They also aren't easy to
-                # suppress, and cause test failures due to spurious stderr
-                # output.  Just shut the whole bloody mess up.
-                '-Wignore::DeprecationWarning',
-                twistd_script, '-o', '-y', self.tacfile,
-                '--pidfile', self.pidfile, '--logfile', self.logfile]
-        if spew:
-            args.append('--spew')
-        if umask is not None:
-            args.extend(('--umask', umask))
-
-        # Run twistd, and raise an error if the return value is non-zero or
-        # stdout/stderr are written to.
-        proc = subprocess.Popen(args, stdout=subprocess.PIPE,
-                                stderr=subprocess.STDOUT)
-        self.addCleanup(self.killTac)
-        stdout = until_no_eintr(10, proc.stdout.read)
-        if stdout:
-            raise TacException('Error running %s: unclean stdout/err: %s'
-                               % (args, stdout))
-        rv = proc.wait()
-        if rv != 0:
-            raise TacException('Error %d running %s' % (rv, args))
-
-        self._waitForDaemonStartup()
+        TacTestFixture.setUp(self,
+            python_path=sys.executable,
+            twistd_script=twistd_script)
 
     def _hasDaemonStarted(self):
         """Has the daemon started?
@@ -117,61 +70,6 @@
         else:
             return False
 
-    def _isPortListening(self, host, port):
-        """True if a tcp port is accepting connections.
-
-        This can be used by subclasses overriding _hasDaemonStarted, if they
-        want to check the port is up rather than for the contents of the log
-        file.
-        """
-        try:
-            s = socket.socket()
-            s.settimeout(2.0)
-            s.connect((host, port))
-            s.close()
-            return True
-        except socket.error, e:
-            if e.errno == errno.ECONNREFUSED:
-                return False
-            else:
-                raise
-
-    def _waitForDaemonStartup(self):
-        """ Wait for the daemon to fully start.
-
-        Times out after 20 seconds.  If that happens, the log file content
-        will be included in the exception message for debugging purpose.
-
-        :raises TacException: Timeout.
-        """
-        # Watch the log file for readyservice.LOG_MAGIC to signal that startup
-        # has completed.
-        now = time.time()
-        deadline = now + 20
-        while now < deadline and not self._hasDaemonStarted():
-            time.sleep(0.1)
-            now = time.time()
-
-        if now >= deadline:
-            raise TacException('Unable to start %s. Content of %s:\n%s' % (
-                self.tacfile, self.logfile, open(self.logfile).read()))
-
-    def tearDown(self):
-        # For compatibility - migrate to cleanUp.
-        self.cleanUp()
-
-    def killTac(self):
-        """Kill the TAC file if it is running."""
-        pidfile = self.pidfile
-        kill_by_pidfile(pidfile)
-
-    def sendSignal(self, sig):
-        """Send the given signal to the tac process."""
-        pid = get_pid_from_file(self.pidfile)
-        if pid is None:
-            return
-        os.kill(pid, sig)
-
     def truncateLog(self):
         """Truncate the log file.
 
@@ -198,19 +96,3 @@
         test failure (e.g. log files might contain helpful tracebacks).
         """
         raise NotImplementedError
-
-    @property
-    def root(self):
-        raise NotImplementedError
-
-    @property
-    def tacfile(self):
-        raise NotImplementedError
-
-    @property
-    def pidfile(self):
-        raise NotImplementedError
-
-    @property
-    def logfile(self):
-        raise NotImplementedError
 
=== modified file 'lib/canonical/launchpad/daemons/tests/test_tachandler.py'
--- lib/canonical/launchpad/daemons/tests/test_tachandler.py	2011-08-19 15:36:31 +0000
+++ lib/canonical/launchpad/daemons/tests/test_tachandler.py	2011-11-07 07:50:31 +0000
@@ -132,7 +132,7 @@
 
         # One deprecation warning is emitted.
         self.assertEqual(1, len(warnings_log))
-        self.assertIs(DeprecationWarning, warnings_log[0].category)
+        self.assertIs(UserWarning, warnings_log[0].category)
 
     def test_truncateLog(self):
         """
 
=== modified file 'setup.py'
--- setup.py	2011-11-03 23:03:26 +0000
+++ setup.py	2011-11-07 07:50:31 +0000
@@ -86,6 +86,7 @@
         'timeline',
         'transaction',
         'Twisted',
+        'txfixtures',
         'txlongpollfixture',
         'wadllib',
         'z3c.pt',
 
=== modified file 'versions.cfg'
--- versions.cfg	2011-11-07 03:44:49 +0000
+++ versions.cfg	2011-11-07 07:50:31 +0000
@@ -98,6 +98,7 @@
 timeline = 0.0.1
 transaction = 1.0.0
 txamqp = 0.4
+txfixtures = 0.1.4
 txlongpoll = 0.2.9
 txlongpollfixture = 0.1.3
 Twisted = 11.1.0pre1