yellow team mailing list archive

[Francesco Banconi <frankban@xxxxxxxxx>]

Francesco Banconi has proposed merging lp:~frankban/launchpad/bug-974585-unclean-reactor-error into lp:launchpad.

Requested reviews:
  Launchpad Yellow Squad (yellow)
Related bugs:
  Bug #974585 in Launchpad itself: "test_openFileInNonDirectory has UncleanReactorError"
  https://bugs.launchpad.net/launchpad/+bug/974585

For more details, see:
https://code.launchpad.net/~frankban/launchpad/bug-974585-unclean-reactor-error/+merge/102473

= Summary =

Some tests using twisted leave delayed calls. If, later, another test uses 
testtools an exception is raised if the reactor is not clean.


== Proposed fix ==

Use the clean_up_reactor function on the tearDown of tests leaving delayed
calls.


== Pre-implementation notes ==

I started a full test run temporary patching zope.testrunner to check and 
notify not cancelled delayed calls after each test.
This way I spotted  lp.buildmaster.tests.test_builder.TestSlaveConnectionTimeouts.test_connection_timeout as the only test leaving delayed calls. Hopefully, fixing that, we will not see the unclean reactor error again.


== Implementation details ==

- Move the `clean_up_reactor` function in a more generic place.
- And use it in 
  lp.buildmaster.tests.test_builder.TestSlaveConnectionTimeouts.tearDown.
- Lint updates for file `lib/lp/buildmaster/tests/test_builder.py`.


== Demo and Q/A ==

no-qa


== lint ==

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/buildmaster/tests/test_builder.py
  lib/lp/registry/tests/test_distributionmirror_prober.py
  lib/lp/testing/__init__.py

-- 
https://code.launchpad.net/~frankban/launchpad/bug-974585-unclean-reactor-error/+merge/102473
Your team Launchpad Yellow Squad is requested to review the proposed merge of lp:~frankban/launchpad/bug-974585-unclean-reactor-error into lp:launchpad.

=== modified file 'lib/lp/buildmaster/tests/test_builder.py'
--- lib/lp/buildmaster/tests/test_builder.py	2012-02-09 23:09:36 +0000
+++ lib/lp/buildmaster/tests/test_builder.py	2012-04-18 09:45:27 +0000
@@ -78,6 +78,7 @@
     BinaryPackageBuildBehavior,
     )
 from lp.testing import (
+    clean_up_reactor,
     TestCase,
     TestCaseWithFactory,
     )
@@ -150,11 +151,13 @@
         lostbuilding_builder = MockBuilder(
             'Lost Building Broken Slave', slave, behavior=CorruptBehavior())
         d = lostbuilding_builder.updateStatus(BufferLogger())
+
         def check_slave_status(failure):
             self.assertIn('abort', slave.call_log)
             # 'Fault' comes from the LostBuildingBrokenSlave, this is
             # just testing that the value is passed through.
             self.assertIsInstance(failure.value, xmlrpclib.Fault)
+
         return d.addBoth(check_slave_status)
 
     def test_resumeSlaveHost_nonvirtual(self):
@@ -176,8 +179,10 @@
 
         builder = self.factory.makeBuilder(virtualized=True, vm_host="pop")
         d = builder.resumeSlaveHost()
+
         def got_resume(output):
             self.assertEqual(('parp', ''), output)
+
         return d.addCallback(got_resume)
 
     def test_resumeSlaveHost_command_failed(self):
@@ -252,9 +257,11 @@
         removeSecurityProxy(builder)._findBuildCandidate = FakeMethod(
             result=candidate)
         d = builder.findAndStartJob()
+
         def check_build_started(candidate):
             self.assertEqual(candidate.builder, builder)
             self.assertEqual(BuildStatus.BUILDING, build.status)
+
         return d.addCallback(check_build_started)
 
     def test_virtual_job_dispatch_pings_before_building(self):
@@ -265,9 +272,11 @@
         removeSecurityProxy(builder)._findBuildCandidate = FakeMethod(
             result=candidate)
         d = builder.findAndStartJob()
+
         def check_build_started(candidate):
             self.assertIn(
                 ('echo', 'ping'), removeSecurityProxy(builder.slave).call_log)
+
         return d.addCallback(check_build_started)
 
     def test_slave(self):
@@ -286,8 +295,10 @@
         builder = MockBuilder("mock_builder", aborted_slave)
         builder.currentjob = None
         d = builder.rescueIfLost()
+
         def check_slave_calls(ignored):
             self.assertIn('clean', aborted_slave.call_log)
+
         return d.addCallback(check_slave_calls)
 
     def test_recovery_of_aborted_nonvirtual_slave(self):
@@ -300,9 +311,11 @@
         builder.virtualized = False
         builder.builderok = True
         d = builder.rescueIfLost()
+
         def check_failed(ignored):
             self.assertFalse(builder.builderok)
             self.assertNotIn('clean', aborted_slave.call_log)
+
         return d.addCallback(check_failed)
 
     def test_recover_ok_slave(self):
@@ -310,9 +323,11 @@
         slave = OkSlave()
         builder = MockBuilder("mock_builder", slave, TrivialBehavior())
         d = builder.rescueIfLost()
+
         def check_slave_calls(ignored):
             self.assertNotIn('abort', slave.call_log)
             self.assertNotIn('clean', slave.call_log)
+
         return d.addCallback(check_slave_calls)
 
     def test_recover_waiting_slave_with_good_id(self):
@@ -321,9 +336,11 @@
         waiting_slave = WaitingSlave()
         builder = MockBuilder("mock_builder", waiting_slave, TrivialBehavior())
         d = builder.rescueIfLost()
+
         def check_slave_calls(ignored):
             self.assertNotIn('abort', waiting_slave.call_log)
             self.assertNotIn('clean', waiting_slave.call_log)
+
         return d.addCallback(check_slave_calls)
 
     def test_recover_waiting_slave_with_bad_id(self):
@@ -335,31 +352,39 @@
         waiting_slave = WaitingSlave()
         builder = MockBuilder("mock_builder", waiting_slave, CorruptBehavior())
         d = builder.rescueIfLost()
+
         def check_slave_calls(ignored):
             self.assertNotIn('abort', waiting_slave.call_log)
             self.assertIn('clean', waiting_slave.call_log)
+
         return d.addCallback(check_slave_calls)
 
     def test_recover_building_slave_with_good_id(self):
         # rescueIfLost does not attempt to abort or clean a builder that is
         # BUILDING.
         building_slave = BuildingSlave()
-        builder = MockBuilder("mock_builder", building_slave, TrivialBehavior())
+        builder = MockBuilder(
+            "mock_builder", building_slave, TrivialBehavior())
         d = builder.rescueIfLost()
+
         def check_slave_calls(ignored):
             self.assertNotIn('abort', building_slave.call_log)
             self.assertNotIn('clean', building_slave.call_log)
+
         return d.addCallback(check_slave_calls)
 
     def test_recover_building_slave_with_bad_id(self):
         # If a slave is BUILDING with a build id we don't recognize, then we
         # abort the build, thus stopping it in its tracks.
         building_slave = BuildingSlave()
-        builder = MockBuilder("mock_builder", building_slave, CorruptBehavior())
+        builder = MockBuilder(
+            "mock_builder", building_slave, CorruptBehavior())
         d = builder.rescueIfLost()
+
         def check_slave_calls(ignored):
             self.assertIn('abort', building_slave.call_log)
             self.assertNotIn('clean', building_slave.call_log)
+
         return d.addCallback(check_slave_calls)
 
     def test_recover_building_slave_with_job_that_finished_elsewhere(self):
@@ -384,10 +409,12 @@
         self.layer.txn.commit()
         builder = getUtility(IBuilderSet)[builder.name]
         d = builder.rescueIfLost()
+
         def check_builder(ignored):
             self.assertIsInstance(
                 removeSecurityProxy(builder.current_build_behavior),
                 IdleBuildBehavior)
+
         return d.addCallback(check_builder)
 
 
@@ -930,10 +957,12 @@
         build_id = 'status-build-id'
         d = self.slave_helper.triggerGoodBuild(slave, build_id)
         d.addCallback(lambda ignored: slave.status())
+
         def check_status(status):
             self.assertEqual([BuilderStatus.BUILDING, build_id], status[:2])
             [log_file] = status[2:]
             self.assertIsInstance(log_file, xmlrpclib.Binary)
+
         return d.addCallback(check_status)
 
     def test_ensurepresent_not_there(self):
@@ -964,9 +993,11 @@
         slave = self.slave_helper.getClientSlave()
         self.slave_helper.makeCacheFile(tachandler, 'blahblah')
         d = slave.sendFileToSlave('blahblah', None, None, None)
+
         def check_present(ignored):
             d = slave.ensurepresent('blahblah', None, None, None)
             return d.addCallback(self.assertEqual, [True, 'No URL'])
+
         d.addCallback(check_present)
         return d
 
@@ -1103,6 +1134,11 @@
         self.slave = self.slave_helper.getClientSlave(
             reactor=self.clock, proxy=self.proxy)
 
+    def tearDown(self):
+        # We need to remove any DelayedCalls that didn't actually get called.
+        clean_up_reactor()
+        super(TestSlaveConnectionTimeouts, self).tearDown()
+
     def test_connection_timeout(self):
         # The default timeout of 30 seconds should not cause a timeout,
         # only the config value should.
@@ -1167,9 +1203,11 @@
         slave = self.slave_helper.getClientSlave()
         d = slave.ensurepresent(
             lf.content.sha1, lf.http_url, "", "")
+
         def check_file(ignored):
             d = getPage(expected_url.encode('utf8'))
             return d.addCallback(self.assertEqual, content)
+
         return d.addCallback(check_file)
 
     def test_getFiles(self):
@@ -1186,7 +1224,6 @@
         def got_files(ignored):
             # Called back when getFiles finishes.  Make sure all the
             # content is as expected.
-            got_contents = []
             for sha1 in filemap:
                 local_file = filemap[sha1]
                 file = open(local_file)

=== modified file 'lib/lp/registry/tests/test_distributionmirror_prober.py'
--- lib/lp/registry/tests/test_distributionmirror_prober.py	2012-04-02 20:56:02 +0000
+++ lib/lp/registry/tests/test_distributionmirror_prober.py	2012-04-18 09:45:27 +0000
@@ -60,6 +60,7 @@
 from lp.services.config import config
 from lp.services.daemons.tachandler import TacTestSetup
 from lp.testing import (
+    clean_up_reactor,
     TestCase,
     TestCaseWithFactory,
     )
@@ -69,14 +70,6 @@
     )
 
 
-def clean_up_reactor():
-    # XXX: JonathanLange 2010-11-22: These tests leave stacks of delayed
-    # calls around.  They need to be updated to use Twisted correctly.
-    # For the meantime, just blat the reactor.
-    for delayed_call in reactor.getDelayedCalls():
-        delayed_call.cancel()
-
-
 class HTTPServerTestSetup(TacTestSetup):
 
     def setUpRoot(self):

=== modified file 'lib/lp/testing/__init__.py'
--- lib/lp/testing/__init__.py	2012-03-26 07:04:44 +0000
+++ lib/lp/testing/__init__.py	2012-04-18 09:45:27 +0000
@@ -15,6 +15,7 @@
     'BrowserTestCase',
     'build_yui_unittest_suite',
     'celebrity_logged_in',
+    'clean_up_reactor',
     'ExpectedException',
     'extract_lp_cache',
     'FakeAdapterMixin',
@@ -1527,3 +1528,12 @@
             self.addCleanup(
                 site_manager.registerUtility, current_commponent,
                 for_interface, name)
+
+
+def clean_up_reactor():
+    # XXX: JonathanLange 2010-11-22: These tests leave stacks of delayed
+    # calls around.  They need to be updated to use Twisted correctly.
+    # For the meantime, just blat the reactor.
+    from twisted.internet import reactor
+    for delayed_call in reactor.getDelayedCalls():
+        delayed_call.cancel()