aminghadersohi commented on code in PR #41606: URL: https://github.com/apache/superset/pull/41606#discussion_r3589214107
########## tests/unit_tests/mcp_service/dashboard/tool/test_manage_dashboard_owners.py: ########## @@ -0,0 +1,408 @@ +# 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. + +"""Tests for the manage_dashboard_owners MCP tool. + +"Owners" are USER-type Subjects in the dashboard's ``editors`` list (the +Subject-based model apache/superset#38831 introduced, replacing the legacy +``owners`` relationship). +""" + +from collections.abc import Iterator +from unittest.mock import Mock, patch + +import pytest +from fastmcp import Client + +from superset.mcp_service.app import mcp +from superset.subjects.types import SubjectType +from superset.utils import json + +DAO_GET = "superset.daos.dashboard.DashboardDAO.get_by_id_or_slug" +GET_OR_CREATE_USER_SUBJECT = "superset.subjects.utils.get_or_create_user_subject" +POPULATE_SUBJECT_LIST = "superset.commands.utils.populate_subject_list" + + [email protected] +def mcp_server() -> object: + return mcp + + [email protected](autouse=True) +def mock_auth() -> Iterator[Mock]: + """Mock authentication for all tests in this module.""" + with patch("superset.mcp_service.auth.get_user_from_request") as mock_get_user: + with patch("superset.security_manager.raise_for_editorship"): + mock_user = Mock() + mock_user.id = 1 + mock_user.username = "admin" + mock_get_user.return_value = mock_user + yield mock_get_user + + +def _mock_subject(id: int, user_id: int, label: str = "user") -> Mock: + subject = Mock() + subject.id = id + subject.type = SubjectType.USER + subject.user_id = user_id + subject.role_id = None + subject.label = label + subject.active = True + return subject + + +def _mock_dashboard( + id: int = 42, + title: str = "Test Dashboard", + slug: str | None = "test-slug", + editors: list[Mock] | None = None, +) -> Mock: + dashboard = Mock() + dashboard.id = id + dashboard.dashboard_title = title + dashboard.slug = slug + dashboard.editors = ( + editors if editors is not None else [_mock_subject(100, 1, "admin")] + ) + return dashboard + + +class TestManageDashboardOwners: + @patch(POPULATE_SUBJECT_LIST) + @patch(GET_OR_CREATE_USER_SUBJECT) + @patch(DAO_GET) + @patch("superset.extensions.db.session") + @pytest.mark.asyncio + async def test_add_owner( + self, + mock_session: Mock, + mock_get: Mock, + mock_get_or_create: Mock, + mock_populate: Mock, + mcp_server: object, + ) -> None: + existing = _mock_subject(100, 1, "admin") Review Comment: Valid observation, but declining: those two blocks build different owner sets for different assertions (add vs. remove), and each test file is meant to be self-contained/readable standalone. Not extracting to avoid a premature shared-fixture abstraction for ~10 lines of setup. ########## tests/unit_tests/mcp_service/dashboard/tool/test_manage_dashboard_certification.py: ########## @@ -0,0 +1,283 @@ +# 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. + +"""Tests for the manage_dashboard_certification MCP tool.""" + +from collections.abc import Iterator +from unittest.mock import Mock, patch + +import pytest +from fastmcp import Client + +from superset.mcp_service.app import mcp +from superset.utils import json + +DAO_GET = "superset.daos.dashboard.DashboardDAO.get_by_id_or_slug" + + [email protected] +def mcp_server() -> object: + return mcp + + [email protected](autouse=True) +def mock_auth() -> Iterator[Mock]: + """Mock authentication for all tests in this module.""" + with patch("superset.mcp_service.auth.get_user_from_request") as mock_get_user: + with patch("superset.security_manager.raise_for_editorship"): + mock_user = Mock() + mock_user.id = 1 + mock_user.username = "admin" + mock_get_user.return_value = mock_user + yield mock_get_user + + +def _mock_dashboard( + id: int = 42, + title: str = "Test Dashboard", + slug: str | None = "test-slug", + certified_by: str | None = None, + certification_details: str | None = None, +) -> Mock: + dashboard = Mock() + dashboard.id = id + dashboard.dashboard_title = title + dashboard.slug = slug + dashboard.certified_by = certified_by + dashboard.certification_details = certification_details + return dashboard + + +class TestManageDashboardCertification: + @patch(DAO_GET) + @patch("superset.extensions.db.session") + @pytest.mark.asyncio + async def test_set_certification( + self, mock_session: Mock, mock_get: Mock, mcp_server: object + ) -> None: + dash = _mock_dashboard() + mock_get.return_value = dash + + async with Client(mcp_server) as client: + result = await client.call_tool( + "manage_dashboard_certification", + { + "request": { + "identifier": 42, + "certified_by": "Data Platform Team", + "certification_details": "Verified against source-of-truth.", + } + }, + ) + + assert dash.certified_by == "Data Platform Team" + assert dash.certification_details == "Verified against source-of-truth." + assert mock_session.commit.call_count >= 1 + payload = json.loads(result.content[0].text) + # Response text is wrapped for LLM context (mirrors the read-path + # sanitization on DashboardInfo.certified_by), so check substring. + assert "Data Platform Team" in payload["certified_by"] + assert set(payload["changed_fields"]) == { + "certified_by", + "certification_details", + } + + @patch(DAO_GET) + @patch("superset.extensions.db.session") + @pytest.mark.asyncio + async def test_set_certified_by_only( + self, mock_session: Mock, mock_get: Mock, mcp_server: object + ) -> None: + dash = _mock_dashboard(certification_details="Existing details") + mock_get.return_value = dash + + async with Client(mcp_server) as client: + result = await client.call_tool( + "manage_dashboard_certification", + {"request": {"identifier": 42, "certified_by": "Analytics Guild"}}, + ) + + assert dash.certified_by == "Analytics Guild" + assert dash.certification_details == "Existing details" + payload = json.loads(result.content[0].text) + assert payload["changed_fields"] == ["certified_by"] + + @patch(DAO_GET) + @patch("superset.extensions.db.session") + @pytest.mark.asyncio + async def test_clear_with_empty_string( + self, mock_session: Mock, mock_get: Mock, mcp_server: object + ) -> None: + dash = _mock_dashboard( + certified_by="Old Team", certification_details="Old details" + ) + mock_get.return_value = dash + + async with Client(mcp_server) as client: + result = await client.call_tool( + "manage_dashboard_certification", + { + "request": { + "identifier": 42, + "certified_by": "", + "certification_details": "", + } + }, + ) + + assert dash.certified_by is None + assert dash.certification_details is None + payload = json.loads(result.content[0].text) + assert payload["certified_by"] is None + assert payload["certification_details"] is None + + @patch(DAO_GET) + @pytest.mark.asyncio + async def test_no_fields_is_noop(self, mock_get: Mock, mcp_server: object) -> None: + dash = _mock_dashboard(certified_by="Existing") + mock_get.return_value = dash + + async with Client(mcp_server) as client: + result = await client.call_tool( + "manage_dashboard_certification", + {"request": {"identifier": 42}}, + ) + + assert dash.certified_by == "Existing" + payload = json.loads(result.content[0].text) + assert payload["changed_fields"] == [] + assert any("No fields provided" in w for w in payload["warnings"]) + + @patch(DAO_GET) + @pytest.mark.asyncio + async def test_dashboard_not_found( + self, mock_get: Mock, mcp_server: object + ) -> None: + from superset.commands.dashboard.exceptions import DashboardNotFoundError + + mock_get.side_effect = DashboardNotFoundError() + + async with Client(mcp_server) as client: + result = await client.call_tool( + "manage_dashboard_certification", + {"request": {"identifier": 999999, "certified_by": "Team"}}, Review Comment: Valid observation, but declining: this permission-denied scenario is intentionally repeated per tool (owners/roles/certification) since each test file is self-contained and exercises its own MCP tool end-to-end. A shared cross-file test utility/base class would be a bigger abstraction than the ~20 lines of duplication justifies. -- 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]
