From 341803d78454f26932267b4c5da361c83556e96a Mon Sep 17 00:00:00 2001 From: Quigley Malcolm Date: Thu, 16 May 2024 15:52:19 -0700 Subject: [PATCH] Standardize event catching during repository unit tests (#10153) * Add unit test to assert `setup_config_logger` clears the event manager state * Move `setup_event_logger` tests from `test_functions.py` to `test_logging.py` * Move `EventCatcher` to `tests.utils` for use in unit and functional tests * 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. --- .../configs/test_warn_error_options.py | 2 +- .../test_check_for_spaces_in_model_names.py | 2 +- tests/unit/conftest.py | 1 + tests/unit/events/test_logging.py | 41 +++++++++++++++++++ tests/unit/test_functions.py | 14 ------- tests/unit/utils/event_manager.py | 8 ++++ tests/utils.py | 17 ++++++++ 7 files changed, 69 insertions(+), 16 deletions(-) create mode 100644 tests/unit/events/test_logging.py create mode 100644 tests/unit/utils/event_manager.py 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/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/events/test_logging.py b/tests/unit/events/test_logging.py new file mode 100644 index 00000000000..16441ad6de7 --- /dev/null +++ b/tests/unit/events/test_logging.py @@ -0,0 +1,41 @@ +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, 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 +from tests.utils import EventCatcher + + +class TestSetupEventLogger: + 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) + 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 + + def test_specify_max_bytes( + self, + mocker: MockerFixture, + ) -> 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 e218178bf1c..7d118dd7033 100644 --- a/tests/unit/test_functions.py +++ b/tests/unit/test_functions.py @@ -4,9 +4,7 @@ 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.functions import msg_to_dict, warn_or_error from dbt_common.events.types import InfoLevel, RetryExternalCall from dbt_common.exceptions import EventCompilationError @@ -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): - 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 - ) - # XXX if we do not clean up event logger here we are going to affect other tests. - cleanup_event_logger() diff --git a/tests/unit/utils/event_manager.py b/tests/unit/utils/event_manager.py new file mode 100644 index 00000000000..70415e36231 --- /dev/null +++ b/tests/unit/utils/event_manager.py @@ -0,0 +1,8 @@ +import pytest + +from dbt_common.events.event_manager_client import cleanup_event_logger + + +@pytest.fixture(autouse=True) +def always_clean_event_manager() -> None: + cleanup_event_logger() 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 = []