Copilot commented on code in PR #40164:
URL: https://github.com/apache/superset/pull/40164#discussion_r3247835792
##########
tests/unit_tests/mcp_service/chart/test_preview_utils.py:
##########
@@ -19,6 +19,29 @@
Tests for preview_utils query context column building.
"""
+import ast
+from pathlib import Path
+
+from superset.mcp_service.chart.preview_utils import
SUPPORTED_FORM_DATA_PREVIEW_FORMATS
+
+
+def test_preview_utils_does_not_top_level_import_chart_data_command():
+ """preview_utils constants should stay safe to import before app setup."""
+ source = Path("superset/mcp_service/chart/preview_utils.py").read_text()
Review Comment:
This test reads the module source via
`Path("superset/mcp_service/chart/preview_utils.py")`, which depends on the
current working directory being the repo root. That can make the test flaky
when running pytest from a different CWD (e.g. some IDE runners). Prefer
resolving the path relative to the installed module (e.g. `importlib.resources`
/ `inspect.getsourcefile`) or relative to `__file__`, and specify an explicit
encoding when reading text.
##########
tests/unit_tests/mcp_service/chart/test_preview_utils.py:
##########
@@ -19,6 +19,29 @@
Tests for preview_utils query context column building.
"""
+import ast
+from pathlib import Path
+
+from superset.mcp_service.chart.preview_utils import
SUPPORTED_FORM_DATA_PREVIEW_FORMATS
+
+
+def test_preview_utils_does_not_top_level_import_chart_data_command():
+ """preview_utils constants should stay safe to import before app setup."""
+ source = Path("superset/mcp_service/chart/preview_utils.py").read_text()
+ tree = ast.parse(source)
+ top_level_imports = [
+ node for node in tree.body if isinstance(node, (ast.Import,
ast.ImportFrom))
+ ]
+
+ assert SUPPORTED_FORM_DATA_PREVIEW_FORMATS == frozenset(
+ {"ascii", "table", "vega_lite"}
+ )
+ assert not any(
+ isinstance(node, ast.ImportFrom)
+ and node.module == "superset.commands.chart.data.get_data_command"
Review Comment:
The import-guard assertion only checks `ast.ImportFrom` nodes. A top-level
`import superset.commands.chart.data.get_data_command` (or importing the module
under a different name) would still violate the intent but would not be caught
by this test. Consider extending the check to also cover `ast.Import` nodes
that import that module/package.
--
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]
