bito-code-review[bot] commented on code in PR #44950:
URL: https://github.com/apache/superset/pull/44950#discussion_r4180318479
##########
tests/unit_tests/mcp_service/system/tool/test_get_schema.py:
##########
@@ -643,3 +644,17 @@ async def
test_resource_scope_is_enforced_when_rbac_disabled(self, app, mcp_serv
can_access.assert_not_called()
scope_allows.assert_called_once_with("read", "Chart")
+
+
[email protected]
+async def test_get_schema_chart_datasource_id(mcp_server: Any) -> None:
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Test placed outside class convention</b></div>
<div id="fix">
Every other test in this file lives in a class
(`TestGetSchemaToolViaClient`, `TestGetSchemaEdgeCases`,
`TestSchemaDiscoveryConstants`, ...); this chart-discovery test sits alone at
module level below `TestGetSchemaPermissionMap`. Moving it into
`TestGetSchemaToolViaClient` keeps all client-path `get_schema` tests
discoverable in one place.
</div>
</div>
<small><i>Code Review Run #c425f9</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
##########
tests/unit_tests/mcp_service/system/tool/test_get_schema.py:
##########
@@ -643,3 +644,17 @@ async def
test_resource_scope_is_enforced_when_rbac_disabled(self, app, mcp_serv
can_access.assert_not_called()
scope_allows.assert_called_once_with("read", "Chart")
+
+
[email protected]
+async def test_get_schema_chart_datasource_id(mcp_server: Any) -> None:
+ """Chart discovery advertises the supported dataset ID operators."""
+ with patch.object(
+ get_schema_module, "user_can_view_data_model_metadata",
return_value=True
+ ):
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Redundant duplicate permission patch</b></div>
<div id="fix">
The autouse fixture `allow_data_model_metadata` (lines 69-77) already
patches `get_schema_module.user_can_view_data_model_metadata` to True for every
test in this module, so this identical `patch.object` wrapper is redundant.
Dropping it keeps the test focused on the `datasource_id` contract and avoids a
second patch site to maintain.
</div>
</div>
<small><i>Code Review Run #c425f9</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]
