From 7e91db29f9bba43492660a62a4c7faeec3749378 Mon Sep 17 00:00:00 2001 From: Quigley Malcolm Date: Wed, 15 May 2024 13:55:33 -0700 Subject: [PATCH 1/6] Mock global event manager in `test_setup_event_logger_specify_max_bytes` Previously we weren't mocking the global event manager in this test. This meant that we also had to clean up the global event manager afterward because changes we made to it would be carried over to other tests. Having to do so is tedious and somewhat annoying, which raised the frustration with writing unit tests that involve the event manager. Although mocking the event manager in this specific test isn't high impact, it is setting up a paradigm that allows us to more easily write unit tests involving the event manager. --- tests/unit/test_functions.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/unit/test_functions.py b/tests/unit/test_functions.py index e218178bf1c..c6294602c72 100644 --- a/tests/unit/test_functions.py +++ b/tests/unit/test_functions.py @@ -1,12 +1,13 @@ from argparse import Namespace import pytest +from pytest_mock import MockerFixture import dbt.flags as flags from dbt.adapters.events.types import AdapterDeprecationWarning from dbt.events.logging import setup_event_logger from dbt.events.types import NoNodesForSelectionCriteria -from dbt_common.events.event_manager_client import cleanup_event_logger +from dbt_common.events.event_manager import EventManager from dbt_common.events.functions import msg_to_dict, warn_or_error from dbt_common.events.types import InfoLevel, RetryExternalCall from dbt_common.exceptions import EventCompilationError @@ -85,7 +86,8 @@ def __init__(self): ), f"We expect `msg_to_dict` to gracefully handle exceptions, but it raised {exc}" -def test_setup_event_logger_specify_max_bytes(mocker): +def test_setup_event_logger_specify_max_bytes(mocker: MockerFixture) -> None: + mocker.patch("dbt_common.events.event_manager_client._EVENT_MANAGER", EventManager()) patched_file_handler = mocker.patch("dbt_common.events.logger.RotatingFileHandler") args = Namespace(log_file_max_bytes=1234567) flags.set_from_args(args, {}) @@ -93,5 +95,3 @@ def test_setup_event_logger_specify_max_bytes(mocker): patched_file_handler.assert_called_once_with( filename="logs/dbt.log", encoding="utf8", maxBytes=1234567, backupCount=5 ) - # XXX if we do not clean up event logger here we are going to affect other tests. - cleanup_event_logger() From 1d0f5f42985e8cb200a1cd9ed039b019a8025e09 Mon Sep 17 00:00:00 2001 From: Quigley Malcolm Date: Wed, 15 May 2024 15:11:53 -0700 Subject: [PATCH 2/6] Create test fixture `mock_global_event_manager` to mock global event manager We want to be able to modify the global event manager during unit tests. Previously to do so we had to modify the real global event manager and then clean it up at the end of the test. In the previous commit, we updated a test to mock the global event manager, which worked successfully and meant no more cleanup was necessary. This commit takes that work and creates a fixture. Now any test that takes in that fixture as a parameter will have the global event manager be mocked. --- tests/unit/conftest.py | 1 + tests/unit/test_functions.py | 6 +++--- tests/unit/utils/event_manager.py | 10 ++++++++++ 3 files changed, 14 insertions(+), 3 deletions(-) create mode 100644 tests/unit/utils/event_manager.py diff --git a/tests/unit/conftest.py b/tests/unit/conftest.py index b06dd39981e..6bad8e7eb85 100644 --- a/tests/unit/conftest.py +++ b/tests/unit/conftest.py @@ -6,6 +6,7 @@ # All manifest related fixtures. from tests.unit.utils.adapter import * # noqa +from tests.unit.utils.event_manager import * # noqa from tests.unit.utils.manifest import * # noqa from tests.unit.utils.project import * # noqa diff --git a/tests/unit/test_functions.py b/tests/unit/test_functions.py index c6294602c72..61866e67276 100644 --- a/tests/unit/test_functions.py +++ b/tests/unit/test_functions.py @@ -7,7 +7,6 @@ from dbt.adapters.events.types import AdapterDeprecationWarning from dbt.events.logging import setup_event_logger from dbt.events.types import NoNodesForSelectionCriteria -from dbt_common.events.event_manager import EventManager from dbt_common.events.functions import msg_to_dict, warn_or_error from dbt_common.events.types import InfoLevel, RetryExternalCall from dbt_common.exceptions import EventCompilationError @@ -86,8 +85,9 @@ def __init__(self): ), f"We expect `msg_to_dict` to gracefully handle exceptions, but it raised {exc}" -def test_setup_event_logger_specify_max_bytes(mocker: MockerFixture) -> None: - mocker.patch("dbt_common.events.event_manager_client._EVENT_MANAGER", EventManager()) +def test_setup_event_logger_specify_max_bytes( + mocker: MockerFixture, mock_global_event_manager +) -> None: patched_file_handler = mocker.patch("dbt_common.events.logger.RotatingFileHandler") args = Namespace(log_file_max_bytes=1234567) flags.set_from_args(args, {}) diff --git a/tests/unit/utils/event_manager.py b/tests/unit/utils/event_manager.py new file mode 100644 index 00000000000..d3352f30915 --- /dev/null +++ b/tests/unit/utils/event_manager.py @@ -0,0 +1,10 @@ +import pytest +from pytest_mock import MockerFixture + +from dbt_common.events.event_manager import EventManager + + +@pytest.fixture +def mock_global_event_manager(mocker: MockerFixture) -> None: + """Mocks the global _EVENT_MANAGER so that unit tests can safely modify it without worry about other tests.""" + mocker.patch("dbt_common.events.event_manager_client._EVENT_MANAGER", EventManager()) From 38bcde501e85ca6760ceffae7bfe2708f676f011 Mon Sep 17 00:00:00 2001 From: Quigley Malcolm Date: Wed, 15 May 2024 15:44:50 -0700 Subject: [PATCH 3/6] Add unit test to assert `setup_config_logger` clears the event manager state --- tests/unit/events/test_logging.py | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) create mode 100644 tests/unit/events/test_logging.py diff --git a/tests/unit/events/test_logging.py b/tests/unit/events/test_logging.py new file mode 100644 index 00000000000..a0ba8d8aec6 --- /dev/null +++ b/tests/unit/events/test_logging.py @@ -0,0 +1,26 @@ +from copy import deepcopy + +from dbt.events.logging import setup_event_logger +from dbt.flags import get_flags +from dbt_common.events.base_types import BaseEvent +from dbt_common.events.event_manager_client import get_event_manager +from dbt_common.events.logger import LoggerConfig +from tests.functional.utils import EventCatcher + + +class TestSetupEventLogger: + def test_clears_preexisting_event_manager_state(self, mock_global_event_manager) -> None: + manager = get_event_manager() + manager.add_logger(LoggerConfig(name="test_logger")) + manager.callbacks.append(EventCatcher(BaseEvent).catch) + assert len(manager.loggers) == 1 + assert len(manager.callbacks) == 1 + + flags = deepcopy(get_flags()) + # setting both of these to none guarantees that no logger will be added + object.__setattr__(flags, "LOG_LEVEL", "none") + object.__setattr__(flags, "LOG_LEVEL_FILE", "none") + + setup_event_logger(flags=flags) + assert len(manager.loggers) == 0 + assert len(manager.callbacks) == 0 From e08c4481db60763508db380f8eb797eab8b7af51 Mon Sep 17 00:00:00 2001 From: Quigley Malcolm Date: Wed, 15 May 2024 15:49:39 -0700 Subject: [PATCH 4/6] Move `setup_event_logger` tests from `test_functions.py` to `test_logging.py` --- tests/unit/events/test_logging.py | 18 +++++++++++++++++- tests/unit/test_functions.py | 14 -------------- 2 files changed, 17 insertions(+), 15 deletions(-) diff --git a/tests/unit/events/test_logging.py b/tests/unit/events/test_logging.py index a0ba8d8aec6..a49c9066ed9 100644 --- a/tests/unit/events/test_logging.py +++ b/tests/unit/events/test_logging.py @@ -1,7 +1,10 @@ +from argparse import Namespace from copy import deepcopy +from pytest_mock import MockerFixture + from dbt.events.logging import setup_event_logger -from dbt.flags import get_flags +from dbt.flags import get_flags, set_from_args from dbt_common.events.base_types import BaseEvent from dbt_common.events.event_manager_client import get_event_manager from dbt_common.events.logger import LoggerConfig @@ -24,3 +27,16 @@ def test_clears_preexisting_event_manager_state(self, mock_global_event_manager) setup_event_logger(flags=flags) assert len(manager.loggers) == 0 assert len(manager.callbacks) == 0 + + def test_specify_max_bytes( + self, + mocker: MockerFixture, + mock_global_event_manager, + ) -> None: + patched_file_handler = mocker.patch("dbt_common.events.logger.RotatingFileHandler") + args = Namespace(log_file_max_bytes=1234567) + set_from_args(args, {}) + setup_event_logger(get_flags()) + patched_file_handler.assert_called_once_with( + filename="logs/dbt.log", encoding="utf8", maxBytes=1234567, backupCount=5 + ) diff --git a/tests/unit/test_functions.py b/tests/unit/test_functions.py index 61866e67276..7d118dd7033 100644 --- a/tests/unit/test_functions.py +++ b/tests/unit/test_functions.py @@ -1,11 +1,9 @@ from argparse import Namespace import pytest -from pytest_mock import MockerFixture import dbt.flags as flags from dbt.adapters.events.types import AdapterDeprecationWarning -from dbt.events.logging import setup_event_logger from dbt.events.types import NoNodesForSelectionCriteria from dbt_common.events.functions import msg_to_dict, warn_or_error from dbt_common.events.types import InfoLevel, RetryExternalCall @@ -83,15 +81,3 @@ def __init__(self): assert ( False ), f"We expect `msg_to_dict` to gracefully handle exceptions, but it raised {exc}" - - -def test_setup_event_logger_specify_max_bytes( - mocker: MockerFixture, mock_global_event_manager -) -> None: - patched_file_handler = mocker.patch("dbt_common.events.logger.RotatingFileHandler") - args = Namespace(log_file_max_bytes=1234567) - flags.set_from_args(args, {}) - setup_event_logger(flags.get_flags()) - patched_file_handler.assert_called_once_with( - filename="logs/dbt.log", encoding="utf8", maxBytes=1234567, backupCount=5 - ) From bcb2efcf0f0f4bfb1a0c0e94ae7cdd329d5d4b68 Mon Sep 17 00:00:00 2001 From: Quigley Malcolm Date: Wed, 15 May 2024 15:54:56 -0700 Subject: [PATCH 5/6] Move `EventCatcher` to `tests.utils` for use in unit and functional tests --- .../configs/test_warn_error_options.py | 2 +- .../test_check_for_spaces_in_model_names.py | 2 +- tests/unit/events/test_logging.py | 2 +- tests/utils.py | 17 +++++++++++++++++ 4 files changed, 20 insertions(+), 3 deletions(-) create mode 100644 tests/utils.py diff --git a/tests/functional/configs/test_warn_error_options.py b/tests/functional/configs/test_warn_error_options.py index 4b6552b0143..d8e68fcf11d 100644 --- a/tests/functional/configs/test_warn_error_options.py +++ b/tests/functional/configs/test_warn_error_options.py @@ -6,7 +6,7 @@ from dbt.events.types import DeprecatedModel from dbt.tests.util import update_config_file from dbt_common.events.base_types import EventLevel -from tests.functional.utils import EventCatcher +from tests.utils import EventCatcher ModelsDictSpec = Dict[str, Union[str, "ModelsDictSpec"]] diff --git a/tests/functional/manifest_validations/test_check_for_spaces_in_model_names.py b/tests/functional/manifest_validations/test_check_for_spaces_in_model_names.py index 4ba25a530f9..543a4557eea 100644 --- a/tests/functional/manifest_validations/test_check_for_spaces_in_model_names.py +++ b/tests/functional/manifest_validations/test_check_for_spaces_in_model_names.py @@ -10,7 +10,7 @@ ) from dbt.tests.util import update_config_file from dbt_common.events.base_types import EventLevel -from tests.functional.utils import EventCatcher +from tests.utils import EventCatcher class TestSpacesInModelNamesHappyPath: diff --git a/tests/unit/events/test_logging.py b/tests/unit/events/test_logging.py index a49c9066ed9..532f55e428d 100644 --- a/tests/unit/events/test_logging.py +++ b/tests/unit/events/test_logging.py @@ -8,7 +8,7 @@ from dbt_common.events.base_types import BaseEvent from dbt_common.events.event_manager_client import get_event_manager from dbt_common.events.logger import LoggerConfig -from tests.functional.utils import EventCatcher +from tests.utils import EventCatcher class TestSetupEventLogger: diff --git a/tests/utils.py b/tests/utils.py new file mode 100644 index 00000000000..3f71cac38ef --- /dev/null +++ b/tests/utils.py @@ -0,0 +1,17 @@ +from dataclasses import dataclass, field +from typing import List + +from dbt_common.events.base_types import BaseEvent, EventMsg + + +@dataclass +class EventCatcher: + event_to_catch: BaseEvent + caught_events: List[EventMsg] = field(default_factory=list) + + def catch(self, event: EventMsg): + if event.info.name == self.event_to_catch.__name__: + self.caught_events.append(event) + + def flush(self) -> None: + self.caught_events = [] From 67c87fdb1434920640279ad7295ab51421f84334 Mon Sep 17 00:00:00 2001 From: Quigley Malcolm Date: Thu, 16 May 2024 15:31:13 -0700 Subject: [PATCH 6/6] Update fixture mocking global event manager to instead clear it Previously we had started _mocking_ the global event manager. We did this because we though that meant anything we did to the global event manager, including modifying it via things like `setup_event_logger`, would be automatically cleaned up at the end of any test using the fixture because the mock would go away. However, this understanding of fixtures and mocking was incorrect, and the global event manager wouldn't be properly isolated/reset. Thus we changed to fixture to instead cleanup the global event manager before any test that uses it and by using `autouse=True` in the fixture definition we made it so that every unit test uses the fixture. Note this will no longer be viabled if we every multi-thread our unit testing as the event manager isn't actually isolated, and thus two tests could both modify the event manager at the same time. --- tests/unit/events/test_logging.py | 3 +-- tests/unit/utils/event_manager.py | 10 ++++------ 2 files changed, 5 insertions(+), 8 deletions(-) diff --git a/tests/unit/events/test_logging.py b/tests/unit/events/test_logging.py index 532f55e428d..16441ad6de7 100644 --- a/tests/unit/events/test_logging.py +++ b/tests/unit/events/test_logging.py @@ -12,7 +12,7 @@ class TestSetupEventLogger: - def test_clears_preexisting_event_manager_state(self, mock_global_event_manager) -> None: + def test_clears_preexisting_event_manager_state(self) -> None: manager = get_event_manager() manager.add_logger(LoggerConfig(name="test_logger")) manager.callbacks.append(EventCatcher(BaseEvent).catch) @@ -31,7 +31,6 @@ def test_clears_preexisting_event_manager_state(self, mock_global_event_manager) def test_specify_max_bytes( self, mocker: MockerFixture, - mock_global_event_manager, ) -> None: patched_file_handler = mocker.patch("dbt_common.events.logger.RotatingFileHandler") args = Namespace(log_file_max_bytes=1234567) diff --git a/tests/unit/utils/event_manager.py b/tests/unit/utils/event_manager.py index d3352f30915..70415e36231 100644 --- a/tests/unit/utils/event_manager.py +++ b/tests/unit/utils/event_manager.py @@ -1,10 +1,8 @@ import pytest -from pytest_mock import MockerFixture -from dbt_common.events.event_manager import EventManager +from dbt_common.events.event_manager_client import cleanup_event_logger -@pytest.fixture -def mock_global_event_manager(mocker: MockerFixture) -> None: - """Mocks the global _EVENT_MANAGER so that unit tests can safely modify it without worry about other tests.""" - mocker.patch("dbt_common.events.event_manager_client._EVENT_MANAGER", EventManager()) +@pytest.fixture(autouse=True) +def always_clean_event_manager() -> None: + cleanup_event_logger()