From 8cd8db2bb65b3580a98da140b4a60440c05821e8 Mon Sep 17 00:00:00 2001 From: Brendon Smith Date: Sat, 17 Apr 2021 21:17:40 -0400 Subject: [PATCH] Consolidate logging configuration method and tests br3ndonland/inboard#3 The `configure_logging()` method was previously located in `start.py`, but the logging configuration dictionary was in `logging_conf.py`. This organization was not ideal for readability and separation of concerns. It also created logistical complications, such as requiring the Gunicorn configuration file `gunicorn_conf.py` to import from start.py, so that it could configure logging for Gunicorn. The `start.py` script should be self-contained, and should import other modules and objects from the package, not the other way around. This commit will move `configure_logging()` to `logging_conf.py`, and move the tests to a corresponding module, `test_logging_conf.py`. This matches up nicely with the latest updates to the `gunicorn_conf.py`, by having a setup method at the top of the module, and the settings below. --- inboard/gunicorn_conf.py | 2 +- inboard/logging_conf.py | 34 +++++++++ inboard/start.py | 30 +------- tests/test_logging_conf.py | 148 +++++++++++++++++++++++++++++++++++++ tests/test_start.py | 135 --------------------------------- 5 files changed, 184 insertions(+), 165 deletions(-) create mode 100644 tests/test_logging_conf.py diff --git a/inboard/gunicorn_conf.py b/inboard/gunicorn_conf.py index 684711e..a4936f9 100644 --- a/inboard/gunicorn_conf.py +++ b/inboard/gunicorn_conf.py @@ -2,7 +2,7 @@ import os from typing import Optional -from inboard.start import configure_logging +from inboard.logging_conf import configure_logging def calculate_workers( diff --git a/inboard/logging_conf.py b/inboard/logging_conf.py index a82fc22..f5233c7 100644 --- a/inboard/logging_conf.py +++ b/inboard/logging_conf.py @@ -1,5 +1,39 @@ +import importlib.util +import logging +import logging.config import os import sys +from pathlib import Path + + +def configure_logging( + logger: logging.Logger = logging.getLogger(), + logging_conf: str = os.getenv("LOGGING_CONF", "inboard.logging_conf"), +) -> dict: + """Configure Python logging based on a path to a logging module or file.""" + try: + logging_conf_path = Path(logging_conf) + spec = ( + importlib.util.spec_from_file_location("confspec", logging_conf_path) + if logging_conf_path.is_file() and logging_conf_path.suffix == ".py" + else importlib.util.find_spec(logging_conf) + ) + if not spec: + raise ImportError(f"Unable to import {logging_conf}") + logging_conf_module = importlib.util.module_from_spec(spec) + spec.loader.exec_module(logging_conf_module) # type: ignore[union-attr] + if not hasattr(logging_conf_module, "LOGGING_CONFIG"): + raise AttributeError(f"No LOGGING_CONFIG in {logging_conf_module.__name__}") + logging_conf_dict = getattr(logging_conf_module, "LOGGING_CONFIG") + if not isinstance(logging_conf_dict, dict): + raise TypeError("LOGGING_CONFIG is not a dictionary instance") + logging.config.dictConfig(logging_conf_dict) + logger.debug(f"Logging dict config loaded from {logging_conf_path}.") + return logging_conf_dict + except Exception as e: + logger.error(f"Error when setting logging module: {e.__class__.__name__} {e}.") + raise + LOG_COLORS = ( True diff --git a/inboard/start.py b/inboard/start.py index 93dee2f..e355d42 100644 --- a/inboard/start.py +++ b/inboard/start.py @@ -1,7 +1,6 @@ #!/usr/bin/env python3 import importlib.util import logging -import logging.config import os import subprocess from pathlib import Path @@ -9,34 +8,7 @@ import uvicorn # type: ignore - -def configure_logging( - logger: logging.Logger = logging.getLogger(), - logging_conf: str = os.getenv("LOGGING_CONF", "inboard.logging_conf"), -) -> dict: - """Configure Python logging based on a path to a logging module or file.""" - try: - logging_conf_path = Path(logging_conf) - spec = ( - importlib.util.spec_from_file_location("confspec", logging_conf_path) - if logging_conf_path.is_file() and logging_conf_path.suffix == ".py" - else importlib.util.find_spec(logging_conf) - ) - if not spec: - raise ImportError(f"Unable to import {logging_conf}") - logging_conf_module = importlib.util.module_from_spec(spec) - spec.loader.exec_module(logging_conf_module) # type: ignore[union-attr] - if not hasattr(logging_conf_module, "LOGGING_CONFIG"): - raise AttributeError(f"No LOGGING_CONFIG in {logging_conf_module.__name__}") - logging_conf_dict = getattr(logging_conf_module, "LOGGING_CONFIG") - if not isinstance(logging_conf_dict, dict): - raise TypeError("LOGGING_CONFIG is not a dictionary instance") - logging.config.dictConfig(logging_conf_dict) - logger.debug(f"Logging dict config loaded from {logging_conf_path}.") - return logging_conf_dict - except Exception as e: - logger.error(f"Error when setting logging module: {e.__class__.__name__} {e}.") - raise +from inboard.logging_conf import configure_logging def run_pre_start_script(logger: logging.Logger = logging.getLogger()) -> str: diff --git a/tests/test_logging_conf.py b/tests/test_logging_conf.py new file mode 100644 index 0000000..6ca4158 --- /dev/null +++ b/tests/test_logging_conf.py @@ -0,0 +1,148 @@ +import os +from pathlib import Path + +import pytest +from pytest_mock import MockerFixture + +from inboard import logging_conf + + +class TestConfigureLogging: + """Test logging configuration method. + --- + """ + + def test_configure_logging_file( + self, logging_conf_file_path: Path, mocker: MockerFixture + ) -> None: + """Test logging configuration with correct logging config file path.""" + mock_logger = mocker.patch.object(logging_conf.logging, "root", autospec=True) + logging_conf.configure_logging( + logger=mock_logger, logging_conf=str(logging_conf_file_path) + ) + mock_logger.debug.assert_called_once_with( + f"Logging dict config loaded from {logging_conf_file_path}." + ) + + def test_configure_logging_module( + self, logging_conf_module_path: str, mocker: MockerFixture + ) -> None: + """Test logging configuration with correct logging config module path.""" + mock_logger = mocker.patch.object(logging_conf.logging, "root", autospec=True) + logging_conf.configure_logging( + logger=mock_logger, logging_conf=logging_conf_module_path + ) + mock_logger.debug.assert_called_once_with( + f"Logging dict config loaded from {logging_conf_module_path}." + ) + + def test_configure_logging_module_incorrect(self, mocker: MockerFixture) -> None: + """Test logging configuration with incorrect logging config module path.""" + mock_logger = mocker.patch.object(logging_conf.logging, "root", autospec=True) + mock_logger_error_msg = "Error when setting logging module" + with pytest.raises(ModuleNotFoundError): + logging_conf.configure_logging( + logger=mock_logger, logging_conf="no.module.here" + ) + assert mock_logger_error_msg in mock_logger.error.call_args.args[0] + assert "ModuleNotFoundError" in mock_logger.error.call_args.args[0] + + def test_configure_logging_tmp_file( + self, logging_conf_tmp_file_path: Path, mocker: MockerFixture + ) -> None: + """Test logging configuration with temporary logging config file path.""" + mock_logger = mocker.patch.object(logging_conf.logging, "root", autospec=True) + logging_conf_file = f"{logging_conf_tmp_file_path}/tmp_log.py" + logging_conf.configure_logging( + logger=mock_logger, logging_conf=logging_conf_file + ) + mock_logger.debug.assert_called_once_with( + f"Logging dict config loaded from {logging_conf_file}." + ) + + def test_configure_logging_tmp_file_incorrect_extension( + self, + logging_conf_tmp_path_incorrect_extension: Path, + mocker: MockerFixture, + ) -> None: + """Test logging configuration with incorrect temporary file type.""" + mock_logger = mocker.patch.object(logging_conf.logging, "root", autospec=True) + incorrect_logging_conf = logging_conf_tmp_path_incorrect_extension.joinpath( + "tmp_logging_conf" + ) + logger_error_msg = "Error when setting logging module" + import_error_msg = f"Unable to import {incorrect_logging_conf}" + with pytest.raises(ImportError) as e: + logging_conf.configure_logging( + logger=mock_logger, + logging_conf=str(incorrect_logging_conf), + ) + assert str(e.value) in import_error_msg + mock_logger.error.assert_called_once_with( + f"{logger_error_msg}: ImportError {import_error_msg}." + ) + with open(incorrect_logging_conf, "r") as f: + contents = f.read() + assert "This file doesn't have the correct extension" in contents + + def test_configure_logging_tmp_module( + self, + logging_conf_tmp_file_path: Path, + mocker: MockerFixture, + monkeypatch: pytest.MonkeyPatch, + ) -> None: + """Test logging configuration with temporary logging config path.""" + mock_logger = mocker.patch.object(logging_conf.logging, "root", autospec=True) + monkeypatch.syspath_prepend(logging_conf_tmp_file_path) + monkeypatch.setenv("LOGGING_CONF", "tmp_log") + assert os.getenv("LOGGING_CONF") == "tmp_log" + logging_conf.configure_logging(logger=mock_logger, logging_conf="tmp_log") + mock_logger.debug.assert_called_once_with( + "Logging dict config loaded from tmp_log." + ) + + def test_configure_logging_tmp_module_incorrect_type( + self, + logging_conf_tmp_path_incorrect_type: Path, + mocker: MockerFixture, + monkeypatch: pytest.MonkeyPatch, + ) -> None: + """Test logging configuration with temporary logging config path. + - Correct module name + - `LOGGING_CONFIG` object with incorrect type + """ + mock_logger = mocker.patch.object(logging_conf.logging, "root", autospec=True) + monkeypatch.syspath_prepend(logging_conf_tmp_path_incorrect_type) + monkeypatch.setenv("LOGGING_CONF", "incorrect_type") + logger_error_msg = "Error when setting logging module" + type_error_msg = "LOGGING_CONFIG is not a dictionary instance" + assert os.getenv("LOGGING_CONF") == "incorrect_type" + with pytest.raises(TypeError): + logging_conf.configure_logging( + logger=mock_logger, logging_conf="incorrect_type" + ) + mock_logger.error.assert_called_once_with( + f"{logger_error_msg}: TypeError {type_error_msg}." + ) + + def test_configure_logging_tmp_module_no_dict( + self, + logging_conf_tmp_path_no_dict: Path, + mocker: MockerFixture, + monkeypatch: pytest.MonkeyPatch, + ) -> None: + """Test logging configuration with temporary logging config path. + - Correct module name + - No `LOGGING_CONFIG` object + """ + mock_logger = mocker.patch.object(logging_conf.logging, "root", autospec=True) + monkeypatch.syspath_prepend(logging_conf_tmp_path_no_dict) + monkeypatch.setenv("LOGGING_CONF", "no_dict") + logger_error_msg = "Error when setting logging module" + attribute_error_msg = "No LOGGING_CONFIG in no_dict" + assert os.getenv("LOGGING_CONF") == "no_dict" + with pytest.raises(AttributeError): + logging_conf.configure_logging(logger=mock_logger, logging_conf="no_dict") + mock_logger.error.assert_called_once_with( + f"{logger_error_msg}: AttributeError {attribute_error_msg}." + ) diff --git a/tests/test_start.py b/tests/test_start.py index 39db313..cb5bc41 100644 --- a/tests/test_start.py +++ b/tests/test_start.py @@ -7,141 +7,6 @@ from inboard import start -class TestConfigureLogging: - """Test logging configuration methods. - --- - """ - - def test_configure_logging_file( - self, logging_conf_file_path: Path, mocker: MockerFixture - ) -> None: - """Test `start.configure_logging` with correct logging config file path.""" - mock_logger = mocker.patch.object(start.logging, "root", autospec=True) - start.configure_logging( - logger=mock_logger, logging_conf=str(logging_conf_file_path) - ) - mock_logger.debug.assert_called_once_with( - f"Logging dict config loaded from {logging_conf_file_path}." - ) - - def test_configure_logging_module( - self, logging_conf_module_path: str, mocker: MockerFixture - ) -> None: - """Test `start.configure_logging` with correct logging config module path.""" - mock_logger = mocker.patch.object(start.logging, "root", autospec=True) - start.configure_logging( - logger=mock_logger, logging_conf=logging_conf_module_path - ) - mock_logger.debug.assert_called_once_with( - f"Logging dict config loaded from {logging_conf_module_path}." - ) - - def test_configure_logging_module_incorrect(self, mocker: MockerFixture) -> None: - """Test `start.configure_logging` with incorrect logging config module path.""" - mock_logger = mocker.patch.object(start.logging, "root", autospec=True) - mock_logger_error_msg = "Error when setting logging module" - with pytest.raises(ModuleNotFoundError): - start.configure_logging(logger=mock_logger, logging_conf="no.module.here") - assert mock_logger_error_msg in mock_logger.error.call_args.args[0] - assert "ModuleNotFoundError" in mock_logger.error.call_args.args[0] - - def test_configure_logging_tmp_file( - self, logging_conf_tmp_file_path: Path, mocker: MockerFixture - ) -> None: - """Test `start.configure_logging` with temporary logging config file path.""" - mock_logger = mocker.patch.object(start.logging, "root", autospec=True) - logging_conf_file = f"{logging_conf_tmp_file_path}/tmp_log.py" - start.configure_logging(logger=mock_logger, logging_conf=logging_conf_file) - mock_logger.debug.assert_called_once_with( - f"Logging dict config loaded from {logging_conf_file}." - ) - - def test_configure_logging_tmp_file_incorrect_extension( - self, - logging_conf_tmp_path_incorrect_extension: Path, - mocker: MockerFixture, - ) -> None: - """Test `start.configure_logging` with incorrect temporary file type.""" - mock_logger = mocker.patch.object(start.logging, "root", autospec=True) - incorrect_logging_conf = logging_conf_tmp_path_incorrect_extension.joinpath( - "tmp_logging_conf" - ) - logger_error_msg = "Error when setting logging module" - import_error_msg = f"Unable to import {incorrect_logging_conf}" - with pytest.raises(ImportError) as e: - start.configure_logging( - logger=mock_logger, - logging_conf=str(incorrect_logging_conf), - ) - assert str(e.value) in import_error_msg - mock_logger.error.assert_called_once_with( - f"{logger_error_msg}: ImportError {import_error_msg}." - ) - with open(incorrect_logging_conf, "r") as f: - contents = f.read() - assert "This file doesn't have the correct extension" in contents - - def test_configure_logging_tmp_module( - self, - logging_conf_tmp_file_path: Path, - mocker: MockerFixture, - monkeypatch: pytest.MonkeyPatch, - ) -> None: - """Test `start.configure_logging` with temporary logging config path.""" - mock_logger = mocker.patch.object(start.logging, "root", autospec=True) - monkeypatch.syspath_prepend(logging_conf_tmp_file_path) - monkeypatch.setenv("LOGGING_CONF", "tmp_log") - assert os.getenv("LOGGING_CONF") == "tmp_log" - start.configure_logging(logger=mock_logger, logging_conf="tmp_log") - mock_logger.debug.assert_called_once_with( - "Logging dict config loaded from tmp_log." - ) - - def test_configure_logging_tmp_module_incorrect_type( - self, - logging_conf_tmp_path_incorrect_type: Path, - mocker: MockerFixture, - monkeypatch: pytest.MonkeyPatch, - ) -> None: - """Test `start.configure_logging` with temporary logging config path. - - Correct module name - - `LOGGING_CONFIG` object with incorrect type - """ - mock_logger = mocker.patch.object(start.logging, "root", autospec=True) - monkeypatch.syspath_prepend(logging_conf_tmp_path_incorrect_type) - monkeypatch.setenv("LOGGING_CONF", "incorrect_type") - logger_error_msg = "Error when setting logging module" - type_error_msg = "LOGGING_CONFIG is not a dictionary instance" - assert os.getenv("LOGGING_CONF") == "incorrect_type" - with pytest.raises(TypeError): - start.configure_logging(logger=mock_logger, logging_conf="incorrect_type") - mock_logger.error.assert_called_once_with( - f"{logger_error_msg}: TypeError {type_error_msg}." - ) - - def test_configure_logging_tmp_module_no_dict( - self, - logging_conf_tmp_path_no_dict: Path, - mocker: MockerFixture, - monkeypatch: pytest.MonkeyPatch, - ) -> None: - """Test `start.configure_logging` with temporary logging config path. - - Correct module name - - No `LOGGING_CONFIG` object - """ - mock_logger = mocker.patch.object(start.logging, "root", autospec=True) - monkeypatch.syspath_prepend(logging_conf_tmp_path_no_dict) - monkeypatch.setenv("LOGGING_CONF", "no_dict") - logger_error_msg = "Error when setting logging module" - attribute_error_msg = "No LOGGING_CONFIG in no_dict" - assert os.getenv("LOGGING_CONF") == "no_dict" - with pytest.raises(AttributeError): - start.configure_logging(logger=mock_logger, logging_conf="no_dict") - mock_logger.error.assert_called_once_with( - f"{logger_error_msg}: AttributeError {attribute_error_msg}." - ) - - class TestRunPreStartScript: """Run pre-start scripts using the method in `start.py`. ---