✨ [Refactor] Improve app_module_setup decorator documentation and enhance test fixtures

Author: pcrespov

packages/service-library/src/servicelib/aiohttp/application_setup.py

@@ -1,7 +1,6 @@
 import functools
 import inspect
 import logging
-import warnings
 from collections.abc import Callable
 from contextlib import ContextDecorator
 from copy import deepcopy
@@ -258,27 +257,42 @@ def app_module_setup(
     config_section: str | None = None,
     config_enabled: str | None = None,
 ) -> Callable:
-    """Decorator marking a function as a module setup for an application.
-
-    Ensures one-time execution (idempotent) and tracks setup completion. For addons, setup can be
-    toggled via config [deprecated] or settings.
-
-    :param settings_name: Field name in app settings for this module
-    :raises DependencyError: If required dependent modules are not initialized
-    :raises ApplicationSetupError: If setup fails
-    :return: True if setup completed, False if skipped
+    """Decorator that marks a function as 'a setup function' for a given module in an application
+
+        - Marks a function as 'setup' of a given module in an application
+        - Ensures setup executed ONLY ONCE per app
+        - Addon modules:
+            - toggles run using 'enabled' entry in config file
+        - logs execution
+
+    See packages/service-library/tests/test_application_setup.py
+
+    :param module_name: typically __name__
+    :param depends: list of module_names that must be called first, defaults to None
+    :param config_section: explicit configuration section, defaults to None (i.e. the name of the module, or last entry of the name if dotted)
+    :param config_enabled: option in config to enable, defaults to None which is '$(module-section).enabled' (config_section and config_enabled are mutually exclusive)
+    :param settings_name: field name in the app's settings that corresponds to this module. Defaults to the name of the module with app prefix.
+    :raises DependencyError
+    :raises ApplicationSetupError
+    :return: True if setup was completed or False if setup was skipped
     :rtype: bool
 
     :Example:
+        from servicelib.aiohttp.application_setup import app_module_setup
+
         @app_module_setup('mysubsystem', ModuleCategory.SYSTEM, logger=log)
-        def setup_mysubsystem(app: web.Application):
+        def setup(app: web.Application):
             ...
     """
+    # TODO: resilience to failure. if this setup fails, then considering dependencies, is it fatal or app can start?
+    # TODO: enforce signature as def setup(app: web.Application, **kwargs) -> web.Application
+
     module_name, depends, section, config_enabled = _parse_and_validate_arguments(
         module_name, depends, config_section, config_enabled
     )
 
-    def _get_metadata() -> SetupMetadataDict:
+    # metadata info
+    def _setup_metadata() -> SetupMetadataDict:
         return SetupMetadataDict(
             module_name=module_name,
             dependencies=depends,
@@ -303,11 +317,7 @@ def _wrapper(app: web.Application, *args, **kargs) -> bool:
 
                 if settings_name is None:
                     # Fall back to config if settings_name is not explicitly defined
-                    warnings.warn(
-                        f"Using config-based enabling/disabling for addon '{module_name}' is deprecated. Please use settings instead.",
-                        DeprecationWarning,
-                        stacklevel=2,
-                    )
+                    # TODO: deprecate
                     cfg = app[APP_CONFIG_KEY]
                     is_enabled = _is_addon_enabled_from_config(
                         cfg, config_enabled, section
@@ -357,7 +367,7 @@ def _wrapper(app: web.Application, *args, **kargs) -> bool:
             _wrapper.__wrapped__ == setup_func
         ), "this is added by functools.wraps decorator"  # nosec
 
-        setattr(_wrapper, "metadata", _get_metadata)  # noqa: B010
+        setattr(_wrapper, "metadata", _setup_metadata)  # noqa: B010
         setattr(_wrapper, "mark_as_simcore_servicelib_setup_func", True)  # noqa: B010
 
         return _wrapper

packages/service-library/tests/aiohttp/test_application_setup.py

@@ -1,52 +1,29 @@
-# pylint:disable=unused-variable
-# pylint:disable=unused-argument
-# pylint:disable=redefined-outer-name
+# pylint: disable=protected-access
+# pylint: disable=redefined-outer-name
+# pylint: disable=too-many-arguments
+# pylint: disable=unused-argument
+# pylint: disable=unused-variable
 
-from unittest.mock import Mock
+
+from collections.abc import Callable
 
 import pytest
 from aiohttp import web
+from pytest_mock import MockerFixture, MockType
 from servicelib.aiohttp.application_keys import APP_CONFIG_KEY
 from servicelib.aiohttp.application_setup import (
     DependencyError,
     ModuleCategory,
     SkipModuleSetupError,
     app_module_setup,
+    ensure_single_setup,
     is_setup_completed,
 )
 
-log = Mock()
-
-
-@app_module_setup("package.bar", ModuleCategory.ADDON, logger=log)
-def setup_bar(app: web.Application, arg1, *, raise_skip: bool = False):
-    return True
-
-
-@app_module_setup("package.foo", ModuleCategory.ADDON, logger=log)
-def setup_foo(app: web.Application, arg1, kargs=33, *, raise_skip: bool = False):
-    if raise_skip:
-        raise SkipModuleSetupError(reason="explicit skip")
-    return True
 
-
-@app_module_setup(
-    "package.zee", ModuleCategory.ADDON, config_enabled="main.zee_enabled", logger=log
-)
-def setup_zee(app: web.Application, arg1, kargs=55):
-    return True
-
-
-@app_module_setup(
-    "package.needs_foo",
-    ModuleCategory.SYSTEM,
-    depends=[
-        "package.foo",
-    ],
-    logger=log,
-)
-def setup_needs_foo(app: web.Application, arg1, kargs=55):
-    return True
+@pytest.fixture
+def mock_logger(mocker: MockerFixture) -> MockType:
+    return mocker.patch("logging.getLogger", autospec=True)
 
 
 @pytest.fixture
@@ -59,55 +36,156 @@ def app_config() -> dict:
 
 
 @pytest.fixture
-def app(app_config):
+def app(app_config) -> web.Application:
     _app = web.Application()
     _app[APP_CONFIG_KEY] = app_config
     return _app
 
 
-def test_setup_config_enabled(app_config, app):
-    assert setup_zee(app, 1)
+@pytest.fixture
+def create_setup_bar() -> Callable[[web.Application, str, bool], bool]:
+    @app_module_setup("package.bar", ModuleCategory.ADDON)
+    def _setup_bar(
+        app: web.Application, arg1: str, *, raise_skip: bool = False
+    ) -> bool:
+        return True
+
+    return _setup_bar
+
+
+@pytest.fixture
+def create_setup_foo() -> Callable[[web.Application, str, bool], bool]:
+    @app_module_setup("package.foo", ModuleCategory.ADDON)
+    def _setup_foo(
+        app: web.Application, arg1: str, *, raise_skip: bool = False, **kwargs
+    ) -> bool:
+        if raise_skip:
+            raise SkipModuleSetupError(reason="explicit skip")
+        return True
+
+    return _setup_foo
+
+
+@pytest.fixture
+def create_setup_zee() -> Callable[[web.Application, str, int], bool]:
+    @app_module_setup(
+        "package.zee", ModuleCategory.ADDON, config_enabled="main.zee_enabled"
+    )
+    def _setup_zee(app: web.Application, arg1: str, kargs: int = 55) -> bool:
+        return True
 
+    return _setup_zee
+
+
+@pytest.fixture
+def create_setup_needs_foo() -> Callable[[web.Application, str, int], bool]:
+    @app_module_setup(
+        "package.needs_foo",
+        ModuleCategory.SYSTEM,
+        depends=[
+            "package.foo",
+        ],
+    )
+    def _setup_needs_foo(app: web.Application, arg1: str, kargs: int = 55) -> bool:
+        return True
+
+    return _setup_needs_foo
+
+
+def setup_basic(app: web.Application) -> bool:
+    return True
+
+
+def setup_that_raises(app: web.Application) -> bool:
+    error_msg = "Setup failed"
+    raise ValueError(error_msg)
+
+
+def test_setup_config_enabled(
+    app_config: dict, app: web.Application, create_setup_zee: Callable
+):
+    setup_zee = create_setup_zee()
+
+    assert setup_zee(app, 1)
     assert setup_zee.metadata()["config_enabled"] == "main.zee_enabled"
     app_config["main"]["zee_enabled"] = False
     assert not setup_zee(app, 2)
 
 
-def test_setup_dependencies(app_config, app):
+def test_setup_dependencies(
+    app: web.Application,
+    create_setup_needs_foo: Callable,
+    create_setup_foo: Callable,
+) -> None:
+    setup_needs_foo = create_setup_needs_foo()
+    setup_foo = create_setup_foo()
 
     with pytest.raises(DependencyError):
-        setup_needs_foo(app, 1)
+        setup_needs_foo(app, "1")
 
-    assert setup_foo(app, 1)
-    assert setup_needs_foo(app, 2)
+    assert setup_foo(app, "1")
+    assert setup_needs_foo(app, "2")
 
     assert setup_needs_foo.metadata()["dependencies"] == [
         setup_foo.metadata()["module_name"],
     ]
 
 
-def test_marked_setup(app_config, app):
-    assert setup_foo(app, 1)
+def test_marked_setup(
+    app_config: dict, app: web.Application, create_setup_foo: Callable
+):
+    setup_foo = create_setup_foo()
 
+    assert setup_foo(app, 1)
     assert setup_foo.metadata()["module_name"] == "package.foo"
     assert is_setup_completed(setup_foo.metadata()["module_name"], app)
 
     app_config["foo"]["enabled"] = False
     assert not setup_foo(app, 2)
 
 
-def test_skip_setup(app_config, app):
-    try:
-        log.reset_mock()
+def test_skip_setup(
+    app: web.Application, mock_logger: MockType, create_setup_foo: Callable
+):
+    setup_foo = create_setup_foo()
+    assert not setup_foo(app, 1, raise_skip=True)
+    assert setup_foo(app, 1)
+    mock_logger.info.assert_called()
+
+
+def test_ensure_single_setup_runs_once(app: web.Application, mock_logger: Mock) -> None:
+    decorated = ensure_single_setup("test.module", logger=mock_logger)(setup_basic)
+
+    # First call succeeds
+    assert decorated(app)
+    assert is_setup_completed("test.module", app)
+
+    # Second call skips
+    assert not decorated(app)
+    mock_logger.info.assert_called_with(
+        "Skipping '%s' setup: %s",
+        "test.module",
+        "'test.module' was already initialized in <Application. Avoid logging objects like this>. Setup can only be executed once per app.",
+    )
+
+
+def test_ensure_single_setup_error_handling(
+    app: web.Application, mock_logger: Mock
+) -> None:
+    decorated = ensure_single_setup("test.error", logger=mock_logger)(setup_that_raises)
+
+    with pytest.raises(ValueError, match="Setup failed"):
+        decorated(app)
+    assert not is_setup_completed("test.error", app)
 
-        assert not setup_foo(app, 1, raise_skip=True)
 
-        # FIXME: mock logger
-        # assert log.warning.called
-        # warn_msg = log.warning.call_args()[0]
-        # assert "package.foo" in warn_msg
-        # assert "explicit skip" in warn_msg
+def test_ensure_single_setup_multiple_modules(
+    app: web.Application, mock_logger: Mock
+) -> None:
+    decorated1 = ensure_single_setup("module1", logger=mock_logger)(setup_basic)
+    decorated2 = ensure_single_setup("module2", logger=mock_logger)(setup_basic)
 
-        assert setup_foo(app, 1)
-    finally:
-        log.reset_mock()
+    assert decorated1(app)
+    assert decorated2(app)
+    assert is_setup_completed("module1", app)
+    assert is_setup_completed("module2", app)