Hi Jonathan, I love the test simplifications (LoggingManagerMixin). The only things I noticed were: 1. Some missing imports (as per our chat), 2. Some tests not asserting the intermediate state (but you said that other tests in the module already do this, but will check (I didn't)), and 3. A duplicate test. Great stuff! IRC chat: {{{ 11:11 < noodles> jml: any reason that you're not importing adapter and using @adapter, rather than @zope.component.adapter? Is there a preference? 11:12 < jml> noodles: just me being lazy. I have to import zope.component.event anyway 11:13 < noodles> jml: ok, while you're there, provideHandler too I guess. 11:13 < jml> (as a side-effect, z.c.event import starts up the event system) 11:35 < noodles> jml: there are some tests that setup the logging manager, and then immediately tear it down and make an assertion about the end state, without asserting the intermediate state (ie. showing that it is *not* the same as the endstate). Is that not worthwhile? (if not, I'll stop doing it :) ). 11:36 < noodles> oops, wrong channel, but anyway... 11:36 < jml> noodles: I'm on a call atm. 11:36 < noodles> np. 11:36 < jml> noodles: I think you'll find that the intermediate state is tested in other tests. 11:37 < noodles> Ah, right. 11:37 * jml isn't sure though }}} Notes: > === modified file 'lib/lp/codehosting/sshserver/accesslog.py' > --- lib/lp/codehosting/sshserver/accesslog.py 2010-04-14 19:38:29 +0000 > +++ lib/lp/codehosting/sshserver/accesslog.py 2010-04-14 19:38:33 +0000 > @@ -25,70 +14,72 @@ > > # This non-standard import is necessary to hook up the event system. > import zope.component.event > -from zope.interface import Attribute, implements, Interface > > -from canonical.config import config > from canonical.launchpad.scripts import WatchedFileHandler > +from lp.codehosting.sshserver.events import ILoggingEvent > from lp.services.utils import synchronize > -from lp.services.twistedsupport.loggingsupport import set_up_oops_reporting > > > class LoggingManager: > """Class for managing codehosting logging.""" > > - def setUp(self, configure_oops_reporting=False): > + def __init__(self, main_log, access_log, access_log_path): > + """Construct the logging manager. > + > + :param main_log: The main log. Twisted will log to this. > + :param access_log: The access log object. > + :param access_log_path: The path to the file where access log > + messages go. > + """ > + self._main_log = main_log > + self._access_log = access_log > + self._access_log_path = access_log_path > + self._is_set_up = False > + > + def setUp(self): > """Set up logging for the smart server. > > This sets up a debugging handler on the 'codehosting' logger, makes > sure that things logged there won't go to stderr (necessary because of > bzrlib.trace shenanigans) and then returns the 'codehosting' logger. > - > - :param configure_oops_reporting: If True, install a Twisted log > - observer that ensures unhandled exceptions get reported as OOPSes. > """ > - log = get_codehosting_logger() > + log = self._main_log > self._orig_level = log.level > self._orig_handlers = list(log.handlers) > self._orig_observers = list(tplog.theLogPublisher.observers) > log.setLevel(logging.INFO) > log.addHandler(_NullHandler()) > - access_log = get_access_logger() > - handler = WatchedFileHandler(config.codehosting.access_log) > + handler = WatchedFileHandler(self._access_log_path) > handler.setFormatter( > logging.Formatter("%(asctime)s %(levelname)s %(message)s")) > - access_log.addHandler(handler) > - if configure_oops_reporting: > - set_up_oops_reporting('codehosting') > + self._access_log.addHandler(handler) > # Make sure that our logging event handler is there, ready to receive > # logging events. > - zope.component.provideHandler(_log_event) > + zope.component.provideHandler(self._log_event) Is that preferred? Normally we'd just import provideHandler right? > + self._is_set_up = True > + > + @zope.component.adapter(ILoggingEvent) Same here? > + def _log_event(self, event): > + """Log 'event' to the codehosting logger.""" > + self._access_log.log(event.level, event.message) > > def tearDown(self): > - log = get_codehosting_logger() > + if not self._is_set_up: > + return > + log = self._main_log > log.level = self._orig_level > synchronize( > log.handlers, self._orig_handlers, log.addHandler, > log.removeHandler) > - access_log = get_access_logger() > synchronize( > - access_log.handlers, self._orig_handlers, access_log.addHandler, > - access_log.removeHandler) > + self._access_log.handlers, self._orig_handlers, > + self._access_log.addHandler, self._access_log.removeHandler) > synchronize( > tplog.theLogPublisher.observers, self._orig_observers, > tplog.addObserver, tplog.removeObserver) > - zope.component.getGlobalSiteManager().unregisterHandler(_log_event) > - > - > -def get_codehosting_logger(): > - """Return the codehosting logger.""" > - # This is its own function to avoid spreading the string 'codehosting' > - # everywhere and to avoid duplicating information about how log objects > - # are acquired. > - return logging.getLogger('codehosting') > - > - > -def get_access_logger(): > - return logging.getLogger('codehosting.access') > + zope.component.getGlobalSiteManager().unregisterHandler( And here? > + self._log_event) > + self._is_set_up = False > > > class _NullHandler(logging.Handler): > === added file 'lib/lp/codehosting/sshserver/tests/test_events.py' > --- lib/lp/codehosting/sshserver/tests/test_events.py 1970-01-01 00:00:00 +0000 > +++ lib/lp/codehosting/sshserver/tests/test_events.py 2010-04-14 19:38:33 +0000 > @@ -0,0 +1,91 @@ I know this is just a copy/paste, but if you're keen, there is a similar lack of imports in this file (provideHandler, adapter etc.) > === modified file 'lib/lp/codehosting/sshserver/tests/test_logging.py' > --- lib/lp/codehosting/sshserver/tests/test_logging.py 2010-01-22 03:27:59 +0000 > +++ lib/lp/codehosting/sshserver/tests/test_logging.py 2010-04-14 19:38:33 +0000 > @@ -55,30 +75,21 @@ > # Once logging setup is called, any messages logged to the > # codehosting logger should *not* be logged to stderr. If they are, > # they will appear on the user's terminal. > - manager = LoggingManager() > - manager.setUp() > - self.addCleanup(manager.tearDown) > + log = self.makeLogger() > + self.installLoggingManager(log) > > # Make sure that a logged message does not go to stderr. > - get_codehosting_logger().info('Hello hello') > + log.info('Hello hello') > self.assertEqual(sys.stderr.getvalue(), '') > > > -class TestLoggingManager(TestCase): > - > - def test_returns_codehosting_logger(self): > - # get_codehosting_logger returns the 'codehosting' logger. > - self.assertIs( > - logging.getLogger('codehosting'), get_codehosting_logger()) > - > - def test_codehosting_handlers(self): > - # There needs to be at least one handler for the codehosting root > - # logger. > - manager = LoggingManager() > - manager.setUp() > - self.addCleanup(manager.tearDown) > - handlers = get_codehosting_logger().handlers > - self.assertNotEqual([], handlers) > +class TestLoggingManager(TestCase, LoggingManagerMixin): > + > + def test_main_log_handlers(self): > + # There needs to be at least one handler for the root logger. > + log = self.makeLogger() It's not necessary here to assert that log.handlers == [] before installing? > + self.installLoggingManager(log) > + self.assertNotEqual([], log.handlers) > > def _get_handlers(self): > registrations = ( > @@ -87,127 +98,50 @@ > registration.factory > for registration in registrations] > > - def test_log_event_handler_not_registered(self): > - self.assertNotIn(_log_event, self._get_handlers()) > - > def test_set_up_registers_event_handler(self): > - manager = LoggingManager() > - manager.setUp() > - self.addCleanup(manager.tearDown) > - self.assertIn(_log_event, self._get_handlers()) > + manager = self.installLoggingManager() > + self.assertIn(manager._log_event, self._get_handlers()) > > def test_teardown_restores_event_handlers(self): > handlers = self._get_handlers() > - manager = LoggingManager() > - manager.setUp() > + manager = self.installLoggingManager() Similar here, it's not worth asserting handlers != self._get_handlers() first? Or is that the same as the previous test. > manager.tearDown() > self.assertEqual(handlers, self._get_handlers()) > > def test_teardown_restores_level(self): > - log = get_codehosting_logger() > + log = self.makeLogger() > old_level = log.level > - manager = LoggingManager() > - manager.setUp() > + manager = self.installLoggingManager(log) And here too. > manager.tearDown() > self.assertEqual(old_level, log.level) > > def test_teardown_restores_handlers(self): > - log = get_codehosting_logger() > + log = self.makeLogger() > handlers = list(log.handlers) > - manager = LoggingManager() > - manager.setUp() > + manager = self.installLoggingManager(log) Ditto. OK, we chatted and you said that other tests are testing the setup (but you'll check). > manager.tearDown() > self.assertEqual(handlers, log.handlers) > > - def test_teardown_restores_twisted_observers(self): > - observers = list(tplog.theLogPublisher.observers) > - manager = LoggingManager() > - manager.setUp(True) > - manager.tearDown() > - self.assertEqual(observers, list(tplog.theLogPublisher.observers)) > - > def test_access_handlers(self): > # The logging setup installs a rotatable log handler that logs output > # to config.codehosting.access_log. > - manager = LoggingManager() > - manager.setUp() > - self.addCleanup(manager.tearDown) > - [handler] = get_access_logger().handlers > + directory = self.makeTemporaryDirectory() > + access_log = self.makeLogger() > + access_log_path = os.path.join(directory, 'access.log') > + self.installLoggingManager( > + access_log=access_log, > + access_log_path=access_log_path) > + [handler] = access_log.handlers > self.assertIsInstance(handler, WatchedFileHandler) > - self.assertEqual(config.codehosting.access_log, handler.baseFilename) > + self.assertEqual(access_log_path, handler.baseFilename) > > def test_teardown_restores_access_handlers(self): > - log = get_access_logger() > + log = self.makeLogger() > handlers = list(log.handlers) > - manager = LoggingManager() > - manager.setUp() > + manager = self.installLoggingManager(access_log=log) > manager.tearDown() > self.assertEqual(handlers, log.handlers) Erm, this seems identical to test_teardown_restores_handlers above? Great, thanks!