sadpandajoe commented on code in PR #43842:
URL: https://github.com/apache/superset/pull/43842#discussion_r4409619343
##########
tests/unit_tests/initialization_test.py:
##########
@@ -238,6 +239,160 @@ def test_init_app_in_ctx_calls_sync_config_to_db(self,
mock_logger):
# Assert that sync_config_to_db was called on the app
mock_app.sync_config_to_db.assert_called_once()
+
@patch("superset.core.mcp.core_mcp_injection.initialize_core_mcp_host_tools")
+
@patch("superset.core.mcp.core_mcp_injection.initialize_core_mcp_decorators")
+
@patch("superset.core.api.core_api_injection.initialize_core_api_dependencies")
+ def test_init_core_dependencies_registers_host_tools_by_default(
+ self,
+ mock_init_api,
+ mock_init_decorators,
+ mock_init_host_tools,
+ ):
+ """Default config (flag absent): decorators and host tools both
register."""
+ mock_app = MagicMock()
+ mock_app.config = {}
+ app_initializer = SupersetAppInitializer(mock_app)
+
+ app_initializer.init_core_dependencies()
+
+ mock_init_api.assert_called_once()
+ # Decorators are always registered so extensions using @tool/@prompt at
+ # import time keep working in every process.
+ mock_init_decorators.assert_called_once()
+ # Host tools default to on.
+ mock_init_host_tools.assert_called_once()
+
+
@patch("superset.core.mcp.core_mcp_injection.initialize_core_mcp_host_tools")
+
@patch("superset.core.mcp.core_mcp_injection.initialize_core_mcp_decorators")
+
@patch("superset.core.api.core_api_injection.initialize_core_api_dependencies")
+ def test_init_core_dependencies_registers_host_tools_when_enabled(
+ self,
+ mock_init_api,
+ mock_init_decorators,
+ mock_init_host_tools,
+ ):
+ """Flag True: decorators and host tools both register (unchanged
behavior)."""
+ mock_app = MagicMock()
+ mock_app.config = {"CORE_MCP_HOST_TOOLS_ENABLED": True}
+ app_initializer = SupersetAppInitializer(mock_app)
+
+ app_initializer.init_core_dependencies()
+
+ mock_init_api.assert_called_once()
+ mock_init_decorators.assert_called_once()
+ mock_init_host_tools.assert_called_once()
+
+
@patch("superset.core.mcp.core_mcp_injection.initialize_core_mcp_host_tools")
+
@patch("superset.core.mcp.core_mcp_injection.initialize_core_mcp_decorators")
+
@patch("superset.core.api.core_api_injection.initialize_core_api_dependencies")
+ def test_init_core_dependencies_skips_host_tools_when_disabled(
+ self,
+ mock_init_api,
+ mock_init_decorators,
+ mock_init_host_tools,
+ ):
+ """Flag False: host-tool registration is skipped, but core init and the
+ decorator swap still run (so @tool/@prompt extensions don't break)."""
+ mock_app = MagicMock()
+ mock_app.config = {"CORE_MCP_HOST_TOOLS_ENABLED": False}
+ app_initializer = SupersetAppInitializer(mock_app)
+
+ app_initializer.init_core_dependencies()
+
+ # Core API deps and the decorator swap still run.
+ mock_init_api.assert_called_once()
+ mock_init_decorators.assert_called_once()
+ # The heavy MCP service app import is skipped.
+ mock_init_host_tools.assert_not_called()
+
+ def test_disabled_worker_decorators_do_not_import_service_app(self):
+ """A disabled worker only swaps the decorators; applying a real
+ @tool/@prompt decoration must not import the MCP service app.
+
+ This exercises the concrete decorators (not the initializer, which is
+ mocked in the tests above). Since a Celery worker with
+ CORE_MCP_HOST_TOOLS_ENABLED=False never calls
+ initialize_core_mcp_host_tools(), superset.mcp_service.app is absent,
and
+ the concrete @tool/@prompt decorators must stay no-ops so the heavy
+ host-tool stack is not loaded on demand.
+ """
+ from superset.core.mcp.core_mcp_injection import (
+ create_prompt_decorator,
+ create_tool_decorator,
+ )
+
+ # Simulate the worker: ensure the service app is not loaded, restoring
+ # whatever was there afterwards so we don't disturb other tests.
+ saved = sys.modules.pop("superset.mcp_service.app", None)
+ try:
+ assert "superset.mcp_service.app" not in sys.modules
+
+ @create_tool_decorator
+ def dummy_tool() -> int:
+ """A representative extension tool."""
+ return 1
+
+ @create_prompt_decorator
+ def dummy_prompt() -> str:
+ """A representative extension prompt."""
+ return "prompt"
+
+ # The heavy host-tool stack must stay unimported...
+ assert "superset.mcp_service.app" not in sys.modules
+ # ...and the decorators return the original functions untouched, so
+ # the extension keeps working.
+ assert dummy_tool() == 1
+ assert dummy_prompt() == "prompt"
+ finally:
+ if saved is not None:
+ sys.modules["superset.mcp_service.app"] = saved
+
+ @patch("superset.core.mcp.core_mcp_injection.logger")
+ def test_skipped_decoration_warns_once(self, mock_logger):
+ """A skipped @tool/@prompt registration is logged at WARNING, once per
+ registration, so a process that should serve MCP but has host tools
+ disabled does not silently lose extension tools/prompts."""
+ from superset.core.mcp import core_mcp_injection
+
+ saved = sys.modules.pop("superset.mcp_service.app", None)
+ try:
+ with patch.object(core_mcp_injection, "_skipped_registrations",
set()):
+
+ def dummy() -> int:
+ return 1
+
+ core_mcp_injection.create_tool_decorator(dummy)
+ core_mcp_injection.create_tool_decorator(dummy)
+
core_mcp_injection.create_prompt_decorator(name="my_prompt")(dummy)
+
+ assert "superset.mcp_service.app" not in sys.modules
+ # One warning for the tool (deduplicated) and one for the prompt.
+ assert mock_logger.warning.call_count == 2
+ logged = [c.args[1:3] for c in mock_logger.warning.call_args_list]
+ assert logged == [("tool", "dummy"), ("prompt", "my_prompt")]
+ finally:
+ if saved is not None:
+ sys.modules["superset.mcp_service.app"] = saved
+
+ @patch("superset.core.mcp.core_mcp_injection.logger")
+ def test_host_tools_quietly_skipped_without_fastmcp(self, mock_logger):
+ """Without the optional fastmcp dependency, host-tool registration is a
+ quiet skip (no ERROR log, no service app import), like the
decorators."""
+ from superset.core.mcp import core_mcp_injection
+
+ saved = sys.modules.pop("superset.mcp_service.app", None)
+ try:
+ with patch.object(
+ core_mcp_injection, "_fastmcp_available", return_value=False
+ ):
+ core_mcp_injection.initialize_core_mcp_dependencies()
Review Comment:
The missing-dependency test can still pass with the `_fastmcp_available()`
guard in `initialize_core_mcp_host_tools()` deleted. The module-scoped app
fixture has already imported the MCP service app, so popping
`sys.modules["superset.mcp_service.app"]` leaves `app` cached as an attribute
on the `superset.mcp_service` package; `from superset.mcp_service import app`
then succeeds without touching `sys.modules` and without logging an error, so
both assertions hold either way. Could this run in a state where the package
attribute is also cleared (for example
`monkeypatch.delattr(superset.mcp_service, "app", raising=False)`), or patch
the import so the test fails when the guard is absent?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
