bito-code-review[bot] commented on code in PR #40358: URL: https://github.com/apache/superset/pull/40358#discussion_r3330879456
########## tests/unit_tests/mcp_service/tag/tool/test_create_tag.py: ########## @@ -0,0 +1,243 @@ +# 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. + +from unittest.mock import MagicMock, patch + +import pytest +from fastmcp import Client + +from superset.mcp_service.app import mcp +from superset.mcp_service.tag.schemas import CreateTagRequest +from superset.utils import json + + [email protected] +def mcp_server(): + return mcp + + [email protected](autouse=True) +def mock_auth(): + """Mock authentication for all tests in this module.""" + from unittest.mock import Mock + + 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 + + +def _make_tag( + tag_id: int = 42, name: str = "my-tag", description: str = "" +) -> MagicMock: + tag = MagicMock() + tag.id = tag_id + tag.name = name + tag.description = description + return tag + + [email protected] +async def test_create_tag_success(mcp_server) -> None: + """Happy path: tag is created and ID is returned.""" + mock_tag = _make_tag(42, "my-tag", "A test tag") + + with ( + patch( + "superset.commands.tag.create.CreateCustomTagWithRelationshipsCommand.run", + return_value=(set(), set()), + ), + patch( + "superset.daos.tag.TagDAO.find_by_name", + return_value=mock_tag, + ), + ): + async with Client(mcp_server) as client: + result = await client.call_tool( + "create_tag", + {"request": {"name": "my-tag", "description": "A test tag"}}, + ) + + data = json.loads(result.content[0].text) + assert data["id"] == 42 + assert data["name"] == "my-tag" + assert data["description"] == "A test tag" + assert data["objects_tagged"] == [] + assert data["objects_skipped"] == [] + assert data["error"] is None + + [email protected] +async def test_create_tag_with_objects(mcp_server) -> None: + """Tag is created and objects_tagged is populated and sorted.""" + mock_tag = _make_tag(10, "org-tag", "") + + with ( + patch( + "superset.commands.tag.create.CreateCustomTagWithRelationshipsCommand.run", + return_value=({("chart", 2), ("dashboard", 1)}, set()), + ), + patch( + "superset.daos.tag.TagDAO.find_by_name", + return_value=mock_tag, + ), + ): + async with Client(mcp_server) as client: + result = await client.call_tool( + "create_tag", + { + "request": { + "name": "org-tag", + "objects_to_tag": [["chart", 2], ["dashboard", 1]], + } + }, + ) + + data = json.loads(result.content[0].text) + assert data["id"] == 10 + assert data["error"] is None + # Response lists must be deterministically sorted + assert data["objects_tagged"] == sorted([["chart", 2], ["dashboard", 1]]) + + [email protected] +async def test_create_tag_objects_skipped(mcp_server) -> None: + """Skipped objects (insufficient ownership) are reported separately.""" + mock_tag = _make_tag(7, "skip-tag", "") + + with ( + patch( + "superset.commands.tag.create.CreateCustomTagWithRelationshipsCommand.run", + return_value=(set(), {("chart", 5)}), + ), + patch( + "superset.daos.tag.TagDAO.find_by_name", + return_value=mock_tag, + ), + ): + async with Client(mcp_server) as client: + result = await client.call_tool( + "create_tag", + { + "request": { + "name": "skip-tag", + "objects_to_tag": [["chart", 5]], + } + }, + ) + + data = json.loads(result.content[0].text) + assert data["id"] == 7 + assert data["objects_tagged"] == [] + assert data["objects_skipped"] == [["chart", 5]] + assert data["error"] is None + + [email protected] +async def test_create_tag_strips_whitespace_from_name(mcp_server) -> None: + """Leading/trailing whitespace is stripped from the tag name.""" + mock_tag = _make_tag(1, "trimmed", "") + + with ( + patch( + "superset.commands.tag.create.CreateCustomTagWithRelationshipsCommand.run", + return_value=(set(), set()), + ), + patch( + "superset.daos.tag.TagDAO.find_by_name", + return_value=mock_tag, + ), + ): + async with Client(mcp_server) as client: + result = await client.call_tool( + "create_tag", + {"request": {"name": " trimmed "}}, + ) + + data = json.loads(result.content[0].text) + assert data["id"] == 1 + assert data["name"] == "trimmed" + + [email protected] +async def test_create_tag_invalid_error_returns_structured_response( + mcp_server, +) -> None: + """TagInvalidError is caught and returned as a structured error.""" + from superset.commands.tag.exceptions import TagInvalidError + + with patch( + "superset.commands.tag.create.CreateCustomTagWithRelationshipsCommand.run", + side_effect=TagInvalidError(), + ): + async with Client(mcp_server) as client: + result = await client.call_tool( + "create_tag", + {"request": {"name": "bad-tag"}}, + ) + + data = json.loads(result.content[0].text) + assert data["id"] is None + assert data["error"] is not None Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Incomplete error assertion in test</b></div> <div id="fix"> The test `test_create_tag_invalid_error_returns_structured_response` at lines 179-197 only asserts `data['error'] is not None` but does not validate the error message content. Per BITO rule [6262], tests should verify actual behavior — `TagInvalidError.normalized_messages()` is called at `create_tag.py:96` but the test never inspects its output. Add assertions like `assert 'Tag parameters are invalid' in data['error'] or validate the dict structure returned by normalized_messages()`. </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/tag/tool/test_create_tag.py +++ b/tests/unit_tests/mcp_service/tag/tool/test_create_tag.py @@ -194,6 +194,8 @@ async def test_create_tag_invalid_error_returns_structured_response( data = json.loads(result.content[0].text) assert data["id"] is None assert data["error"] is not None + # Verify error message content is present and non-empty + assert len(data["error"]) > 0 ``` </div> </details> </div> <small><i>Code Review Run #f44f48</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]
