launchpad-reviewers team mailing list archive

[Benji York <benji@xxxxxxxxxxxxx>]

Benji York has proposed merging lp:~benji/launchpad/bug-963463 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #963463 in Launchpad itself: "test_defaultBannerSentOnlyOnce fails intermittently in parallel tests"
  https://bugs.launchpad.net/launchpad/+bug/963463

For more details, see:
https://code.launchpad.net/~benji/launchpad/bug-963463/+merge/100514

Bug 963463 is about tests failing intermittently when run in groups (in
parallel, but that doesn't matter).

The problem is that there are tests that use twisted and leave
DelayedCalls, uncalled in the reactor.  Tests using testtools later
detect the dirty reactor and generates an error.

This branch fixes several (all?) instances of those tests by giving them
a tearDown that clears out the reactor.

Lint: "make lint" reported several "E301 expected 1 blank line, found 0"
in ./lib/lp/registry/tests/test_distributionmirror_prober.py which I
fixed.

QA: since this is a testing bug, there is no QA to do.
-- 
https://code.launchpad.net/~benji/launchpad/bug-963463/+merge/100514
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~benji/launchpad/bug-963463 into lp:launchpad.

=== modified file 'lib/lp/registry/tests/test_distributionmirror_prober.py'
--- lib/lp/registry/tests/test_distributionmirror_prober.py	2012-02-28 05:36:01 +0000
+++ lib/lp/registry/tests/test_distributionmirror_prober.py	2012-04-02 20:37:24 +0000
@@ -142,16 +142,20 @@
         connect to the given host.
         """
         prober = ProberFactory(url)
+
         def fakeConnect(host, port, factory):
             factory.connecting_to = host
             factory.succeeded('200')
+
         prober.connecting_to = None
         orig_connect = reactor.connectTCP
         reactor.connectTCP = fakeConnect
+
         def restore_connect(result, orig_connect):
             self.failUnlessEqual(prober.connecting_to, host)
             reactor.connectTCP = orig_connect
             return None
+
         deferred = prober.probe()
         return deferred.addCallback(restore_connect, orig_connect)
 
@@ -179,11 +183,13 @@
         self.failUnless(prober.redirection_count == 0)
         self.failUnless(prober.url == url)
         deferred = prober.probe()
+
         def got_result(result):
             self.failUnless(prober.redirection_count == 1)
             new_url = 'http://localhost:%s/valid-mirror/file' % self.port
             self.failUnless(prober.url == new_url)
             self.failUnless(result == str(httplib.OK))
+
         return deferred.addCallback(got_result)
 
     def test_redirectawareprober_detects_infinite_loop(self):
@@ -200,26 +206,32 @@
 
     def test_200(self):
         d = self._createProberAndProbe(self.urls['200'])
+
         def got_result(result):
             self.failUnless(
                 result == str(httplib.OK),
                 "Expected a '200' status but got '%s'" % result)
+
         return d.addCallback(got_result)
 
     def test_success_cancel_timeout_call(self):
         prober = ProberFactory(self.urls['200'])
         deferred = prober.probe()
         self.failUnless(prober.timeoutCall.active())
+
         def check_timeout_call(result):
             self.failIf(prober.timeoutCall.active())
+
         return deferred.addCallback(check_timeout_call)
 
     def test_failure_cancel_timeout_call(self):
         prober = ProberFactory(self.urls['500'])
         deferred = prober.probe()
         self.failUnless(prober.timeoutCall.active())
+
         def check_timeout_call(result):
             self.failIf(prober.timeoutCall.active())
+
         return deferred.addErrback(check_timeout_call)
 
     def test_notfound(self):
@@ -299,6 +311,9 @@
         # Restore the globals that our tests fiddle with.
         distributionmirror_prober.host_requests = self.orig_host_requests
         distributionmirror_prober.host_timeouts = self.orig_host_timeouts
+        # We need to remove any DelayedCalls that didn't actually get called.
+        for delayed_call in reactor.getDelayedCalls():
+            delayed_call.cancel()
         super(
             TestProberFactoryRequestTimeoutRatioWithoutTwisted,
             self).tearDown()
@@ -402,22 +417,26 @@
         host = 'localhost'
         d = self._createProberAndProbe(
             u'http://%s:%s/timeout' % (host, self.port))
+
         def got_error(error):
             self.failUnlessEqual(
                 {host: 1}, distributionmirror_prober.host_requests)
             self.failUnlessEqual(
                 {host: 1}, distributionmirror_prober.host_timeouts)
+
         return d.addErrback(got_error)
 
     def test_non_timeout_is_recorded(self):
         host = 'localhost'
         d = self._createProberAndProbe(
             u'http://%s:%s/valid-mirror' % (host, self.port))
+
         def got_result(result):
             self.failUnlessEqual(
                 {host: 1}, distributionmirror_prober.host_requests)
             self.failUnlessEqual(
                 {host: 0}, distributionmirror_prober.host_timeouts)
+
         return d.addCallback(got_result)
 
     def test_failure_after_too_many_timeouts(self):
@@ -518,6 +537,12 @@
 
 class TestRedirectAwareProberFactoryAndProtocol(TestCase):
 
+    def tearDown(self):
+        # We need to remove any DelayedCalls that didn't actually get called.
+        for delayed_call in reactor.getDelayedCalls():
+            delayed_call.cancel()
+        super(TestRedirectAwareProberFactoryAndProtocol, self).tearDown()
+
     def test_redirect_resets_timeout(self):
         prober = RedirectAwareProberFactory('http://foo.bar')
         prober.timeoutCall = FakeTimeOutCall()
@@ -532,16 +557,20 @@
         prober = RedirectAwareProberFactory(url)
         prober.timeoutCall = FakeTimeOutCall()
         prober.connectCalled = False
+
         def connect():
             prober.connectCalled = True
+
         prober.connect = connect
         return prober
 
     def test_raises_error_if_redirected_to_different_file(self):
         prober = self._createFactoryAndStubConnectAndTimeoutCall(
             'http://foo.bar/baz/boo/package.deb')
+
         def failed(error):
             prober.has_failed = True
+
         prober.failed = failed
         prober.redirect('http://foo.bar/baz/boo/notfound?file=package.deb')
         self.failUnless(prober.has_failed)
@@ -622,8 +651,10 @@
     def getLogger(self):
         logger = logging.getLogger('distributionmirror-prober')
         logger.errorCalled = False
+
         def error(msg):
             logger.errorCalled = True
+
         logger.error = error
         return logger
 
@@ -739,13 +770,16 @@
         # not a ProberTimeout, a BadResponseCode or a ConnectionSkipped.
         d = defer.Deferred()
         d.addErrback(callbacks.deleteMirrorSeries)
+        ok = []
+
         def got_result(result):
             self.fail(
                 "Any failure that's not a timeout/bad-response/skipped "
                 "should be propagated.")
-        ok = []
+
         def got_failure(failure):
             ok.append(1)
+
         d.addCallbacks(got_result, got_failure)
         d.errback(Failure(ZeroDivisionError()))
         self.assertEqual([1], ok)
@@ -879,6 +913,12 @@
 
 class TestLoggingMixin(TestCase):
 
+    def tearDown(self):
+        # We need to remove any DelayedCalls that didn't actually get called.
+        for delayed_call in reactor.getDelayedCalls():
+            delayed_call.cancel()
+        super(TestLoggingMixin, self).tearDown()
+
     def _fake_gettime(self):
         # Fake the current time.
         fake_time = datetime(2004, 10, 20, 12, 00, 00, 000000)