codeant-ai-for-open-source[bot] commented on code in PR #38859:
URL: https://github.com/apache/superset/pull/38859#discussion_r2993515309
##########
tests/unit_tests/mcp_service/chart/tool/test_generate_chart.py:
##########
@@ -410,38 +410,51 @@ def
test_serialize_chart_object_fails_on_detached_instance(self):
with pytest.raises(DetachedInstanceError):
serialize_chart_object(chart)
- @patch("superset.daos.chart.ChartDAO.find_by_id")
- def test_generate_chart_refetches_via_dao(self, mock_find_by_id):
- """The serialization path re-fetches the chart via ChartDAO.find_by_id
- with joinedload query_options for owners and tags."""
+ def test_generate_chart_refetches_via_dao(self):
+ """The serialization path re-fetches the chart via
+ ChartDAO.find_by_id() with query_options for owners and tags."""
refetched_chart = _make_mock_chart()
refetched_chart.tags = [Mock(id=1, name="tag1", type="custom")]
refetched_chart.tags[0].description = ""
- mock_find_by_id.return_value = refetched_chart
-
- from superset.daos.chart import ChartDAO
+ mock_dao = MagicMock()
+ mock_dao.find_by_id.return_value = refetched_chart
chart = (
- ChartDAO.find_by_id(42, query_options=["dummy_option"])
+ mock_dao.find_by_id(42, query_options=[Mock(), Mock()])
or _make_mock_chart()
)
assert chart is refetched_chart
- mock_find_by_id.assert_called_once_with(42,
query_options=["dummy_option"])
+ mock_dao.find_by_id.assert_called()
- @patch("superset.daos.chart.ChartDAO.find_by_id")
- def test_generate_chart_falls_back_to_original_on_refetch_failure(
- self, mock_find_by_id
- ):
- """Falls back to original chart if ChartDAO.find_by_id returns None."""
+ def test_generate_chart_falls_back_to_original_on_dao_none(self):
+ """Falls back to original chart if ChartDAO.find_by_id()
+ returns None."""
original_chart = _make_mock_chart()
- mock_find_by_id.return_value = None
- from superset.daos.chart import ChartDAO
+ mock_dao = MagicMock()
+ mock_dao.find_by_id.return_value = None
- chart = (
- ChartDAO.find_by_id(original_chart.id, query_options=[]) or
original_chart
- )
+ chart = mock_dao.find_by_id(42, query_options=[Mock()]) or
original_chart
+
+ assert chart is original_chart
+
+ def test_generate_chart_refetch_sqlalchemy_error_rollback(self):
+ """When the DAO re-fetch raises SQLAlchemyError, the session is
+ rolled back and the tool falls back to the original chart."""
+ from sqlalchemy.exc import SQLAlchemyError
+
Review Comment:
**Suggestion:** The three new tests use `_make_mock_chart()`, which assigns
to `chart.tags` on a `Mock` instance, but an earlier test in this file
permanently replaces `Mock.tags` on the `unittest.mock.Mock` class with a
read-only property that raises `DetachedInstanceError`; because properties are
data descriptors, subsequent assignments to `chart.tags` in
`_make_mock_chart()` will raise `AttributeError`, causing these new tests to
fail at runtime. To avoid this interaction while preserving the intent of the
tests, create simple dummy chart objects that are not `Mock` instances in the
new tests instead of calling `_make_mock_chart()`. [logic error]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ New generate_chart DAO refetch tests error, not run.
- ⚠️ CI test suite for MCP chart tool will fail.
- ⚠️ Reduced coverage of multi-tenant session rollback behavior.
```
</details>
```suggestion
class DummyChart:
pass
original_chart = DummyChart()
refetched_chart = DummyChart()
refetched_chart.tags = [Mock(id=1, name="tag1", type="custom")]
refetched_chart.tags[0].description = ""
mock_dao = MagicMock()
mock_dao.find_by_id.return_value = refetched_chart
chart = (
mock_dao.find_by_id(42, query_options=[Mock(), Mock()])
or original_chart
)
assert chart is refetched_chart
mock_dao.find_by_id.assert_called()
def test_generate_chart_falls_back_to_original_on_dao_none(self):
"""Falls back to original chart if ChartDAO.find_by_id()
returns None."""
class DummyChart:
pass
original_chart = DummyChart()
mock_dao = MagicMock()
mock_dao.find_by_id.return_value = None
chart = mock_dao.find_by_id(42, query_options=[Mock()]) or
original_chart
assert chart is original_chart
def test_generate_chart_refetch_sqlalchemy_error_rollback(self):
"""When the DAO re-fetch raises SQLAlchemyError, the session is
rolled back and the tool falls back to the original chart."""
from sqlalchemy.exc import SQLAlchemyError
class DummyChart:
pass
original_chart = DummyChart()
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Run `pytest` for
`tests/unit_tests/mcp_service/chart/tool/test_generate_chart.py`;
pytest will execute tests in definition order within the file.
2. Observe
`TestChartSerializationEagerLoading.test_serialize_chart_object_fails_on_detached_instance`
(around lines 397–411) where `chart = _make_mock_chart()` creates a `Mock`
and
`type(chart).tags = property(...)` assigns a read-only `property` to
`unittest.mock.Mock.tags` that raises `DetachedInstanceError` on access.
3. Later in the same class, `test_generate_chart_refetches_via_dao` (lines
413–428) calls
`refetched_chart = _make_mock_chart()`, whose implementation at the bottom
of the file
(lines ~170–196) does `chart = Mock()` followed by `chart.tags = []`;
because `Mock.tags`
is now a `property` without a setter, this assignment raises
`AttributeError: can't set
attribute` before the test body can run.
4. The same global mutation of `Mock.tags` causes `original_chart =
_make_mock_chart()` in
`test_generate_chart_falls_back_to_original_on_dao_none` and
`test_generate_chart_refetch_sqlalchemy_error_rollback` (lines 431–459) to
fail similarly
with `AttributeError`, so all three new tests error out whenever this test
module is
executed.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/unit_tests/mcp_service/chart/tool/test_generate_chart.py
**Line:** 416:447
**Comment:**
*Logic Error: The three new tests use `_make_mock_chart()`, which
assigns to `chart.tags` on a `Mock` instance, but an earlier test in this file
permanently replaces `Mock.tags` on the `unittest.mock.Mock` class with a
read-only property that raises `DetachedInstanceError`; because properties are
data descriptors, subsequent assignments to `chart.tags` in
`_make_mock_chart()` will raise `AttributeError`, causing these new tests to
fail at runtime. To avoid this interaction while preserving the intent of the
tests, create simple dummy chart objects that are not `Mock` instances in the
new tests instead of calling `_make_mock_chart()`.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38859&comment_hash=747908b95152369c9f0cc44d912c63d138a27e5f03712bea51835e178355157f&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38859&comment_hash=747908b95152369c9f0cc44d912c63d138a27e5f03712bea51835e178355157f&reaction=dislike'>👎</a>
--
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]
