Fix modified logger in add_file_handler, add tests

It appears that the `logger` parameter was previously unused, and that
the function just used `colcon_logger` directly. As it happens, the only
use of `add_file_handler` passes `colcon_logger`, so the mistake had no
effect.

This fixes the bug and adds a simple test for that code.

colcon_core/logging.py

@@ -95,15 +95,15 @@ def filter(self, record): # noqa: A003
  if isinstance(handler, logging.StreamHandler):
  formatter = handler.formatter
  # filter colcon specific log messages from default stream handler
- handler.addFilter(Filter(colcon_logger.name))
+ handler.addFilter(Filter(logger.name))
 
  # add a stream handler replacing the one filtered on the root logger
  handler = logging.StreamHandler()
  if formatter:
  # use same formatter as for stream handler
  handler.setFormatter(formatter)
- handler.setLevel(colcon_logger.getEffectiveLevel())
- colcon_logger.addHandler(handler)
+ handler.setLevel(logger.getEffectiveLevel())
+ logger.addHandler(handler)
 
  # add a file handler writing all log levels
  handler = logging.FileHandler(str(path))
@@ -121,9 +121,9 @@ def format_message_with_relative_time(record):
  # use same formatter as for stream handler
  handler.setFormatter(formatter)
  handler.setLevel(1)
- colcon_logger.addHandler(handler)
+ logger.addHandler(handler)
 
  # change the logger to handle all levels
- colcon_logger.setLevel(1)
+ logger.setLevel(1)
 
  return handler

test/spell_check.words

@@ -130,6 +130,7 @@ testcase
 testsfailed
 testsuite
 thomas
+tmpdir
 todo
 traceback
 tryfirst

test/test_logging.py

@@ -2,8 +2,10 @@
 # Licensed under the Apache License, Version 2.0
 
 import logging
+from pathlib import Path
 from unittest.mock import Mock
 
+from colcon_core.logging import add_file_handler
 from colcon_core.logging import get_numeric_log_level
 from colcon_core.logging import set_logger_level_from_env
 import pytest
@@ -56,3 +58,21 @@ def test_get_numeric_log_level():
  with pytest.raises(ValueError) as e:
  get_numeric_log_level('-1')
  assert str(e.value).endswith('numeric log levels must be positive')
+
+
+def test_add_file_handler(tmpdir):
+ log_path = Path(tmpdir) / 'test_add_file_handler.log'
+ log_path.touch()
+ logger = logging.getLogger('test_add_file_handler')
+ try:
+ logger.setLevel(logging.WARN)
+ add_file_handler(logger, log_path)
+ assert logger.getEffectiveLevel() != logging.WARN
+ logger.info('test_add_file_handler')
+ finally:
+ for handler in logger.handlers:
+ logger.removeHandler(handler)
+ handler.close()
+
+ # check only that we logged SOMETHING to the file
+ assert log_path.stat().st_size > 10