aminghadersohi commented on code in PR #41924:
URL: https://github.com/apache/superset/pull/41924#discussion_r3573718380
##########
tests/unit_tests/mcp_service/system/tool/test_health_check.py:
##########
@@ -53,3 +84,94 @@ def test_health_check_response_with_uptime():
)
assert response.uptime_seconds == 123.45
+
+
+# ---------------------------------------------------------------------------
+# Tool-level tests: health_check via MCP Client
+# ---------------------------------------------------------------------------
+
+
[email protected]
+async def test_health_check_success_via_client(mcp_server):
+ """Happy path: tool returns a healthy status with real system info."""
+ with patch.object(
+ health_check_module,
+ "get_version_metadata",
+ return_value={"version_string": "4.1.0"},
+ ):
+ async with Client(mcp_server) as client:
+ result = await client.call_tool("health_check", {})
+
+ data = json.loads(result.content[0].text)
+ assert data["status"] == "healthy"
+ assert data["service"] == "Superset MCP Service"
+ assert data["version"] == "4.1.0"
+ assert data["timestamp"] is not None
+ assert data["uptime_seconds"] is not None
+ assert data["uptime_seconds"] >= 0
+
+
[email protected]
+async def test_health_check_uses_configured_app_name(mcp_server, app):
+ """service name is derived from the APP_NAME config, not hardcoded."""
+ app.config["APP_NAME"] = "Acme Analytics"
+ try:
+ with patch.object(
+ health_check_module,
+ "get_version_metadata",
+ return_value={"version_string": "4.1.0"},
+ ):
+ async with Client(mcp_server) as client:
+ result = await client.call_tool("health_check", {})
+ finally:
+ app.config.pop("APP_NAME", None)
Review Comment:
Fixed in 8c643dd92d: the test records whether APP_NAME existed and restores
its exact prior value (or removes only a key that was originally absent) in
finally.
##########
tests/unit_tests/mcp_service/test_mcp_e2e_smoke.py:
##########
@@ -0,0 +1,214 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""End-to-end smoke test for the real MCP ASGI/HTTP stack.
+
+Every other test under ``tests/unit_tests/mcp_service/`` drives tools via
+FastMCP's in-process ``Client(mcp)``, which talks to the bare ``FastMCP``
+object through an in-memory transport (``FastMCPTransport``). That path
+never serializes a JSON-RPC message over HTTP and never runs through the
+Starlette ASGI app that ``superset.mcp_service.server.run_server()`` builds
+via ``mcp_instance.http_app(...)`` -- so the FastMCP-level middleware stack
+(``LoggingMiddleware``, ``GlobalErrorHandlerMiddleware``,
+``StructuredContentStripperMiddleware``, etc., see
+``build_middleware_list()`` in ``server.py``) is never actually exercised in
+CI.
+
+This module closes that gap with a single smoke test file that:
+
+1. Builds the *real* ASGI app the way ``run_server()`` does --
+ ``mcp.http_app(transport="streamable-http", stateless_http=True)`` --
+ with the production FastMCP-level middleware list attached.
+2. Serves it in-process over real MCP streamable-HTTP JSON-RPC using
+ ``httpx.ASGITransport`` (no real TCP socket, no real network).
+3. Drives it with FastMCP's own high-level ``Client``, proving the full
+ request/response wire protocol (headers, session negotiation, JSON-RPC
+ envelopes) round-trips correctly through the real transport.
+
+Deliberately out of scope: the Starlette-level ``BrowserHelloMiddleware``
+(added via ``_build_starlette_middleware()`` in ``server.py``) only
+intercepts ``GET``/``HEAD`` requests carrying a browser ``Accept`` header
+(see ``BrowserHelloMiddleware.dispatch`` in ``jwt_verifier.py``); it never
+touches the ``POST /mcp`` JSON-RPC path this test exercises, and wiring it
+up would require standing up real auth/config machinery for no additional
+coverage here.
+
+Global state note: the FastMCP middleware list lives on the *shared*
+``superset.mcp_service.app.mcp`` singleton that every other test file also
+imports (``mcp.middleware`` is a plain list mutated in place by
+``add_middleware()``). The ``_real_asgi_client`` helper below snapshots and
+restores that list so this file cannot leak middleware into other tests.
+"""
+
+import contextlib
+from collections.abc import AsyncIterator, Mapping
+from typing import Any
+from unittest.mock import Mock, patch
+
+import anyio
+import httpx
+import pytest
+from fastmcp import Client
+from fastmcp.client.transports import StreamableHttpTransport
+from starlette.applications import Starlette
+
+from superset.mcp_service.app import mcp
+from superset.mcp_service.server import build_middleware_list
+from superset.utils import json
+
+ASGIMessage = Mapping[str, Any]
+
+
[email protected]
+async def _run_asgi_lifespan(app: Starlette) -> AsyncIterator[None]:
+ """Manually drive the ASGI lifespan protocol for ``app``.
+
+ FastMCP's streamable-HTTP app only creates its
+ ``StreamableHTTPSessionManager`` (and starts its task group) inside the
+ app's ``lifespan`` context manager -- see ``create_streamable_http_app``
+ in ``fastmcp.server.http``. ``httpx.ASGITransport`` drives the ASGI
+ ``http`` scope but never sends ``lifespan`` events, so without this
+ helper every request would fail because the session manager was never
+ started. This is the same protocol libraries like ``asgi-lifespan``
+ implement; it's inlined here to avoid a new test-only dependency.
+ """
+ send_stream, receive_stream =
anyio.create_memory_object_stream[ASGIMessage](4)
+ startup_complete = anyio.Event()
+ shutdown_complete = anyio.Event()
+ startup_failure: list[str] = []
+
+ async def receive() -> ASGIMessage:
+ return await receive_stream.receive()
+
+ async def send(message: ASGIMessage) -> None:
+ if message["type"] == "lifespan.startup.complete":
+ startup_complete.set()
+ elif message["type"] == "lifespan.startup.failed":
+ startup_failure.append(message.get("message", "startup failed"))
+ startup_complete.set()
+ elif message["type"] == "lifespan.shutdown.complete":
+ shutdown_complete.set()
+ elif message["type"] == "lifespan.shutdown.failed":
+ shutdown_complete.set()
Review Comment:
Fixed in 8c643dd92d: shutdown failures are captured and re-raised after the
shutdown waiter is released, matching the startup-failure behavior.
--
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]
