Merge lp:~frankban/launchpad/bug-993510 into lp:launchpad

[Francesco Banconi]

= Summary =

Log handlers are removed before each test because tests should not depend on global logging config.

== Changes ==

Iterate over a copy of the handler list, so that the original list is not affected by the .remove calls and all handlers are correctly removed.

NO QA

== Lint ==

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/testing/__init__.py

== Tests ==

$ bin/test -vvc --load-list ../s6
Running tests at level 1
Running lp.testing.layers.LaunchpadZopelessLayer tests:
  Set up lp.testing.layers.BaseLayer in 0.052 seconds.
  Set up lp.testing.layers.ZopelessLayer in 3.405 seconds.
  Set up lp.testing.layers.DatabaseLayer in 1.217 seconds.
  Set up lp.testing.layers.LibrarianLayer in 5.545 seconds.
  Set up lp.testing.layers.MemcachedLayer in 0.113 seconds.
  Set up lp.testing.layers.RabbitMQLayer in 1.501 seconds.
  Set up lp.testing.layers.LaunchpadLayer in 0.000 seconds.
  Set up lp.testing.layers.LaunchpadScriptLayer in 0.003 seconds.
  Set up lp.testing.layers.LaunchpadZopelessLayer in 0.000 seconds.
  Running:
 lp.archivepublisher.tests.test_publish_ftpmaster.TestPublishFTPMasterScript.test_script_is_happy_with_no_publications
 lp.services.job.tests.test_runner.TestJobRunner.test_runJobHandleErrors_oops_generated_user_notify_failsNo handlers could be found for logger "root"

  Ran 2 tests with 0 failures and 0 errors in 2.726 seconds.
Tearing down left over layers:
  Tear down lp.testing.layers.LaunchpadZopelessLayer in 0.000 seconds.
  Tear down lp.testing.layers.LaunchpadScriptLayer in 0.001 seconds.
  Tear down lp.testing.layers.ZopelessLayer ... not supported
  Tear down lp.testing.layers.LaunchpadLayer in 0.000 seconds.
  Tear down lp.testing.layers.LibrarianLayer in 0.137 seconds.
  Tear down lp.testing.layers.MemcachedLayer in 0.001 seconds.
  Tear down lp.testing.layers.RabbitMQLayer ... not supported
  Tear down lp.testing.layers.DatabaseLayer in 0.162 seconds.
  Tear down lp.testing.layers.BaseLayer in 0.001 seconds.

[Benji York (benji)]

Looks good. I see why this would be a problem.

I wonder if we should have a test for this behaviour.

review: Approve (code)

[Francesco Banconi (frankban)]

Thanks Benji, added a test as you suggested.

$ bin/test -cvvt lp.testing.tests.test_testcase.TestRemoveLoggingHandlers
Running tests at level 1
Running zope.testing.testrunner.layer.UnitTests tests:
  Set up zope.testing.testrunner.layer.UnitTests in 0.000 seconds.
  Running:
 lp.testing.tests.test_testcase.TestRemoveLoggingHandlers.test_handlers_list_is_empty
  Ran 1 tests with 0 failures and 0 errors in 0.001 seconds.
Tearing down left over layers:
  Tear down zope.testing.testrunner.layer.UnitTests in 0.000 seconds.

[Benji York (benji)]

The test looks good. It is sort of funny (in a good way) how the test machinery is part of the test itself.

review: Approve (code)

=== modified file 'lib/lp/testing/__init__.py'
--- lib/lp/testing/__init__.py	2012-05-28 08:44:41 +0000
+++ lib/lp/testing/__init__.py	2012-05-28 15:45:24 +0000
@@ -653,7 +653,7 @@
         # Remove all log handlers, tests should not depend on global logging
         # config but should make their own config instead.
         logger = logging.getLogger()
-        for handler in logger.handlers:
+        for handler in list(logger.handlers):
             logger.removeHandler(handler)
 
     def assertStatementCount(self, expected_count, function, *args, **kwargs):

=== modified file 'lib/lp/testing/tests/test_testcase.py'
--- lib/lp/testing/tests/test_testcase.py	2012-01-01 18:15:02 +0000
+++ lib/lp/testing/tests/test_testcase.py	2012-05-28 15:45:24 +0000
@@ -5,6 +5,7 @@
 
 __metaclass__ = type
 
+import logging
 from StringIO import StringIO
 import sys
 
@@ -16,6 +17,7 @@
 from lp.services.webapp import errorlog
 from lp.testing import (
     record_statements,
+    TestCase,
     TestCaseWithFactory,
     )
 from lp.testing.layers import (
@@ -102,3 +104,18 @@
         # rfc822 serializer is lossy).
         oops_report = self.oopses[0]
         self.assertEqual(from_details['id'], oops_report['id'])
+
+
+class TestRemoveLoggingHandlers(TestCase):
+
+    def setUp(self):
+        self.logger = logging.getLogger()
+        # Add 2 handlers.
+        self.logger.addHandler(logging.Handler())
+        self.logger.addHandler(logging.Handler())
+        # `TestCase.setUp()` removes the handlers just added.
+        super(TestRemoveLoggingHandlers, self).setUp()
+
+    def test_handlers_list_is_empty(self):
+        # Ensure `TestCase.setUp()` correctly removed all logging handlers.
+        self.assertEqual(0, len(self.logger.handlers))