bito-code-review[bot] commented on code in PR #40343: URL: https://github.com/apache/superset/pull/40343#discussion_r3292311241
########## tests/unit_tests/mcp_service/theme/tool/test_theme_tools.py: ########## @@ -0,0 +1,235 @@ +# 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. + +import logging +from unittest.mock import MagicMock, patch + +import pytest +from fastmcp import Client +from fastmcp.exceptions import ToolError +from pydantic import ValidationError + +from superset.mcp_service.app import mcp +from superset.mcp_service.theme.schemas import ( + ListThemesRequest, + ThemeFilter, +) +from superset.utils import json + +logging.basicConfig(level=logging.DEBUG) +logger = logging.getLogger(__name__) + + +class TestThemeFilterSchema: + """Tests for ThemeFilter schema — filterable columns.""" + + def test_invalid_filter_column_rejected(self): + """Columns not in the Literal set must be rejected.""" + with pytest.raises(ValidationError): + ThemeFilter(col="not_a_real_column", opr="eq", value="test") + + def test_valid_filter_column_accepted(self): + """theme_name is a valid filter column.""" + f = ThemeFilter(col="theme_name", opr="eq", value="my_theme") + assert f.col == "theme_name" + + def test_json_data_column_not_filterable(self): + """json_data is not a public filter column.""" + with pytest.raises(ValidationError): + ThemeFilter(col="json_data", opr="eq", value="{}") + + +def create_mock_theme( + theme_id: int = 1, + theme_name: str = "light_theme", + json_data: str = '{"primaryColor": "#1890ff"}', + uuid: str = "test-theme-uuid-1", +) -> MagicMock: + """Factory function to create mock theme objects.""" + theme = MagicMock() + theme.id = theme_id + theme.theme_name = theme_name + theme.json_data = json_data + theme.uuid = uuid + theme.is_system = False + theme.is_system_default = False + theme.is_system_dark = False + theme.changed_on = None + theme.created_on = None + return theme + + [email protected] +def mcp_server(): + return mcp + + [email protected](autouse=True) +def mock_auth(): + """Mock authentication for all tests.""" + from unittest.mock import Mock, patch + + with patch("superset.mcp_service.auth.get_user_from_request") as mock_get_user: + mock_user = Mock() + mock_user.id = 1 + mock_user.username = "admin" + mock_get_user.return_value = mock_user + yield mock_get_user + + +@patch("superset.daos.theme.ThemeDAO.list") [email protected] +async def test_list_themes_basic(mock_list, mcp_server): + """Test basic theme listing functionality.""" + theme = create_mock_theme() + mock_list.return_value = ([theme], 1) + + async with Client(mcp_server) as client: + request = ListThemesRequest(page=1, page_size=10) + result = await client.call_tool( + "list_themes", {"request": request.model_dump()} + ) + assert result.content is not None + data = json.loads(result.content[0].text) + assert data["themes"] is not None + assert len(data["themes"]) == 1 + assert data["themes"][0]["id"] == 1 + assert data["themes"][0]["theme_name"] == "light_theme" + assert data["themes"][0]["uuid"] == "test-theme-uuid-1" + + +@patch("superset.daos.theme.ThemeDAO.list") [email protected] +async def test_list_themes_default_columns_include_uuid(mock_list, mcp_server): + """Test that uuid is included in default columns for themes.""" + theme = create_mock_theme() + mock_list.return_value = ([theme], 1) + + async with Client(mcp_server) as client: + result = await client.call_tool("list_themes", {}) + data = json.loads(result.content[0].text) + + assert data["columns_requested"] == ["id", "theme_name", "uuid"] + assert data["themes"][0]["uuid"] == "test-theme-uuid-1" + + +@patch("superset.daos.theme.ThemeDAO.list") [email protected] +async def test_list_themes_with_search(mock_list, mcp_server): + """Test theme listing with search functionality.""" + theme = create_mock_theme(theme_name="dark_theme") + mock_list.return_value = ([theme], 1) + + async with Client(mcp_server) as client: + request = ListThemesRequest(page=1, page_size=10, search="dark") + result = await client.call_tool( + "list_themes", {"request": request.model_dump()} + ) + data = json.loads(result.content[0].text) + assert len(data["themes"]) == 1 + assert data["themes"][0]["theme_name"] == "dark_theme" + + +@patch("superset.daos.theme.ThemeDAO.list") [email protected] +async def test_list_themes_with_filters(mock_list, mcp_server): + """Test theme listing with filters.""" + theme = create_mock_theme(theme_name="custom_theme") + mock_list.return_value = ([theme], 1) + + async with Client(mcp_server) as client: + request = ListThemesRequest( + page=1, + page_size=10, + filters=[{"col": "theme_name", "opr": "eq", "value": "custom_theme"}], + ) + result = await client.call_tool( + "list_themes", {"request": request.model_dump()} + ) + data = json.loads(result.content[0].text) + assert len(data["themes"]) == 1 + + +@patch("superset.daos.theme.ThemeDAO.list") [email protected] +async def test_list_themes_api_error(mock_list, mcp_server): + """Test error handling when DAO raises an exception.""" + mock_list.side_effect = ToolError("Theme error") + + async with Client(mcp_server) as client: + request = ListThemesRequest(page=1, page_size=10) + with pytest.raises(ToolError) as excinfo: # noqa: PT012 + await client.call_tool("list_themes", {"request": request.model_dump()}) + assert "Theme error" in str(excinfo.value) + + +@patch("superset.daos.theme.ThemeDAO.find_by_id") [email protected] +async def test_get_theme_info_basic(mock_find, mcp_server): + """Test basic get theme info functionality.""" + theme = create_mock_theme() + mock_find.return_value = theme + + async with Client(mcp_server) as client: + result = await client.call_tool( + "get_theme_info", {"request": {"identifier": 1}} + ) + assert result.content is not None + data = json.loads(result.content[0].text) + assert data["id"] == 1 + assert data["theme_name"] == "light_theme" + assert data["uuid"] == "test-theme-uuid-1" + assert data["json_data"] == '{"primaryColor": "#1890ff"}' + + +@patch("superset.daos.theme.ThemeDAO.find_by_id") [email protected] +async def test_get_theme_info_by_uuid(mock_find, mcp_server): + """Test get theme info by UUID.""" + theme = create_mock_theme(uuid="a1b2c3d4-5678-90ab-cdef-1234567890ab") + mock_find.return_value = theme + + async with Client(mcp_server) as client: + result = await client.call_tool( + "get_theme_info", + {"request": {"identifier": "a1b2c3d4-5678-90ab-cdef-1234567890ab"}}, + ) + data = json.loads(result.content[0].text) + assert data["id"] == 1 + assert data["uuid"] == "a1b2c3d4-5678-90ab-cdef-1234567890ab" + + +@patch("superset.daos.theme.ThemeDAO.find_by_id") [email protected] +async def test_get_theme_info_not_found(mock_find, mcp_server): + """Test get theme info when theme does not exist.""" + mock_find.return_value = None + + async with Client(mcp_server) as client: + result = await client.call_tool( + "get_theme_info", {"request": {"identifier": 999}} + ) + assert result.data["error_type"] == "not_found" + Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Incorrect result attribute in not_found assertion</b></div> <div id="fix"> `result.data` does not exist on `ToolResult` — FastMCP middleware stores Pydantic error schemas (like `ThemeError`) as JSON in `content[0].text`. All five comparable tests in this repo (`test_database_tools:338`, `test_dashboard_tools:617`, `test_dataset_tools:1056`, `test_css_template_tools:228`, `test_middleware:548-549`) use `json.loads(result.content[0].text)` for error assertions. This test will raise `AttributeError` at runtime. </div> <details> <summary> <b>Code suggestion</b> </summary> <blockquote>Check the AI-generated fix before applying</blockquote> <div id="code"> ``` --- a/tests/unit_tests/mcp_service/theme/tool/test_theme_tools.py +++ b/tests/unit_tests/mcp_service/theme/tool/test_theme_tools.py @@ -223,5 +223,5 @@ async with Client(mcp_server) as client: result = await client.call_tool( "get_theme_info", {"request": {"identifier": 999}} ) - assert result.data["error_type"] == "not_found" + assert json.loads(result.content[0].text)["error_type"] == "not_found" ``` </div> </details> </div> <small><i>Code Review Run #ad353e</i></small> </div> --- Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>) - [ ] Yes, avoid them -- 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]
