launchpad-reviewers team mailing list archive

[Colin Watson <mp+429586@xxxxxxxxxxxxxxxxxx>]

Colin Watson has proposed merging ~cjwatson/launchpad:excessive-logger-cleanup into launchpad:master.

Commit message:
Fix excessive logger cleanup in reset_logging

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/429586

We reset the logging system between tests to avoid test isolation issues.  However, we overdo it slightly by removing any loggers we don't recognize from `Manager.loggerDict`.  This used to be OK, but as of Python 3.7 the return value of `Logger.isEnabledFor` is cached, and the mechanism for invalidating the cache when setting the log level relies on being able to iterate over all loggers in `Manager.loggerDict`.

This only seemed to affect some webhook scheduling tests (depending on test ordering, since it was a test isolation issue), but I suspect that's mostly because they happen to be particularly pedantic about their expected log output.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:excessive-logger-cleanup into launchpad:master.

diff --git a/lib/lp/testing/__init__.py b/lib/lp/testing/__init__.py
index c7a926c..b250fb0 100644
--- a/lib/lp/testing/__init__.py
+++ b/lib/lp/testing/__init__.py
@@ -186,7 +186,7 @@ def reset_logging():
     Currently, defaults means 'the way the Z3 testrunner sets it up'
     plus customizations made in lp_sitecustomize
     """
-    # Remove all handlers from non-root loggers, and remove the loggers too.
+    # Remove all handlers from non-root loggers.
     loggerDict = logging.Logger.manager.loggerDict
     for name, logger in list(loggerDict.items()):
         if name == "pagetests-access":
@@ -195,7 +195,6 @@ def reset_logging():
         if not isinstance(logger, logging.PlaceHolder):
             for handler in list(logger.handlers):
                 logger.removeHandler(handler)
-        del loggerDict[name]
 
     # Remove all handlers from the root logger
     root = logging.getLogger("")