codeant-ai-for-open-source[bot] commented on code in PR #37503:
URL: https://github.com/apache/superset/pull/37503#discussion_r2738494693
##########
tests/unit_tests/connectors/sqla/models_test.py:
##########
@@ -906,3 +907,42 @@ def test_sqla_table_link_escapes_url(mocker:
MockerFixture) -> None:
# Verify that special characters are escaped in both name and URL
assert "<script>" in str(link)
assert "<script>" not in str(link)
+
+
+def test_data_for_slices_handles_missing_datasource(mocker: MockerFixture) ->
None:
+ """
+ Test that data_for_slices gracefully handles a chart whose query_context
+ references a datasource that no longer exists.
+
+ When a chart's query_context references a deleted datasource,
get_query_context()
+ raises DatasourceNotFound. The fix ensures this exception is caught and
logged,
+ allowing the dashboard to load normally instead of returning a 404.
+ """
+ database = mocker.MagicMock()
+ database.id = 1
+
+ table = SqlaTable(
+ table_name="test_table",
+ database=database,
+ columns=[],
+ metrics=[],
+ )
+
+ # Create a mock slice whose get_query_context raises DatasourceNotFound
+ mock_slice = mocker.MagicMock()
+ mock_slice.id = 1
+ mock_slice.slice_name = "Test Chart"
+ mock_slice.form_data = {}
+ mock_slice.get_query_context.side_effect = DatasourceNotFound()
+
+ # Mock the columns and metrics properties to return empty lists
+ mocker.patch.object(SqlaTable, "columns", [])
+ mocker.patch.object(SqlaTable, "metrics", [])
Review Comment:
**Suggestion:** Patching the `SqlaTable` class attributes `columns` and
`metrics` can replace class-level descriptors/relationships and produce
unexpected behavior; set the instance attributes on the `table` object instead
to avoid interfering with the class/other tests. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Potentially causes other unit test flakiness.
- ⚠️ CI job test-suite instability for SqlaTable tests.
```
</details>
```suggestion
# Set instance-level columns and metrics to empty lists to avoid
patching class descriptors
table.columns = []
table.metrics = []
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Run the file's tests with pytest: pytest
tests/unit_tests/connectors/sqla/models_test.py. The test function in
question starts at
tests/unit_tests/connectors/sqla/models_test.py:912.
2. During test execution, at
tests/unit_tests/connectors/sqla/models_test.py:939-940 the
test replaces the SqlaTable class attributes:
`mocker.patch.object(SqlaTable, "columns",
[])` and `mocker.patch.object(SqlaTable, "metrics", [])`.
3. Because `SqlaTable.columns` / `metrics` are SQLAlchemy
relationship/descriptor-like
attributes on the class, replacing them at class scope could change behavior
for other
tests that run in the same process if the patching isn't properly scoped or
reverted.
Verify by running adjacent tests that access SqlaTable.columns after this
test — if patch
teardown fails, they may see an unexpected list instead of a relationship.
4. Reproducing the problematic behavior: add an assertion in a later test
that expects the
SQLAlchemy attribute behavior (e.g., relationship methods) and observe
unexpected
type/list value after the class-level patch. The safer reproduction is to
change the patch
to instance assignment (`table.columns = []`, `table.metrics = []`) at
tests/unit_tests/connectors/sqla/models_test.py:939-940 and rerun tests to
confirm
isolation.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/unit_tests/connectors/sqla/models_test.py
**Line:** 938:940
**Comment:**
*Logic Error: Patching the `SqlaTable` class attributes `columns` and
`metrics` can replace class-level descriptors/relationships and produce
unexpected behavior; set the instance attributes on the `table` object instead
to avoid interfering with the class/other tests.
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>
--
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]
