launchpad-reviewers team mailing list archive

[Jonathan Lange <jml@xxxxxxxxx>]

Jonathan Lange has proposed merging lp:~jml/launchpad/failing-tests-bug-711209 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  #711209 When running Twisted tests, failures are not reported
  https://bugs.launchpad.net/bugs/711209

For more details, see:
https://code.launchpad.net/~jml/launchpad/failing-tests-bug-711209/+merge/48184

This branch address bug 711209.  Some Twisted tests in our suite were not failing when they ought to have been failing.  This was because of a band-aid to solve a very similar problem, that turned out to be self-defeating once we were running tests without the underlying wound. i.e. the bandaid helps Trial tests, but not testtools Twisted tests.

The branch "solves" this problem by moving the bandaid into the TwistedLayer, which is only used for Trial tests.  It has the pleasant side-effect of removing an import side-effect.

I verified that the fix works by adding a failing test to test_builder.py and watching it fail.

Note that fixing this bug might expose other tests that have been failing silently. Best thing to do is try to land this then watch the failures that come back from the server.
-- 
https://code.launchpad.net/~jml/launchpad/failing-tests-bug-711209/+merge/48184
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jml/launchpad/failing-tests-bug-711209 into lp:launchpad.

=== modified file 'lib/canonical/testing/layers.py'
--- lib/canonical/testing/layers.py	2011-01-28 19:28:40 +0000
+++ lib/canonical/testing/layers.py	2011-02-01 16:03:56 +0000
@@ -56,7 +56,6 @@
 import gc
 import logging
 import os
-import shutil
 import signal
 import socket
 import subprocess
@@ -69,7 +68,10 @@
 from unittest import TestCase, TestResult
 from urllib import urlopen
 
-from fixtures import Fixture
+from fixtures import (
+    Fixture,
+    MonkeyPatch,
+    )
 import psycopg2
 from storm.zope.interfaces import IZStorm
 import transaction
@@ -595,8 +597,8 @@
             time.sleep(0.1)
 
         # Store the pidfile for other processes to kill.
-        pidfile = MemcachedLayer.getPidFile()
-        open(pidfile, 'w').write(str(MemcachedLayer._memcached_process.pid))
+        pid_file = MemcachedLayer.getPidFile()
+        open(pid_file, 'w').write(str(MemcachedLayer._memcached_process.pid))
 
     @classmethod
     @profiled
@@ -1219,6 +1221,14 @@
         TwistedLayer._save_signals()
         from twisted.internet import interfaces, reactor
         from twisted.python import threadpool
+        # zope.exception demands more of frame objects than
+        # twisted.python.failure provides in its fake frames.  This is enough
+        # to make it work with them as of 2009-09-16.  See
+        # https://bugs.launchpad.net/bugs/425113.
+        cls._patch = MonkeyPatch(
+            'twisted.python.failure._Frame.f_locals',
+            property(lambda self: {}))
+        cls._patch.setUp()
         if interfaces.IReactorThreads.providedBy(reactor):
             pool = getattr(reactor, 'threadpool', None)
             # If the Twisted threadpool has been obliterated (probably by
@@ -1240,6 +1250,7 @@
             if pool is not None:
                 reactor.threadpool.stop()
                 reactor.threadpool = None
+        cls._patch.cleanUp()
         TwistedLayer._restore_signals()
 
 

=== modified file 'lib/lp/testing/__init__.py'
--- lib/lp/testing/__init__.py	2011-01-17 22:31:30 +0000
+++ lib/lp/testing/__init__.py	2011-02-01 16:03:56 +0000
@@ -87,10 +87,6 @@
 from testtools.content import Content
 from testtools.content_type import UTF8_TEXT
 import transaction
-# zope.exception demands more of frame objects than twisted.python.failure
-# provides in its fake frames.  This is enough to make it work with them
-# as of 2009-09-16.  See https://bugs.launchpad.net/bugs/425113.
-from twisted.python.failure import _Frame
 from windmill.authoring import WindmillTestClient
 from zope.component import (
     adapter,
@@ -163,9 +159,6 @@
 from lp.testing.matchers import Provides
 
 
-_Frame.f_locals = property(lambda self: {})
-
-
 class FakeTime:
     """Provides a controllable implementation of time.time().