codeant-ai-for-open-source[bot] commented on code in PR #28008:
URL: https://github.com/apache/superset/pull/28008#discussion_r3394919147
##########
tests/integration_tests/datasets/commands_tests.py:
##########
@@ -273,6 +273,42 @@ def test_export_dataset_command_no_related(self, mock_g):
f"datasets/examples/energy_usage_{example_dataset.id}.yaml",
]
+ @patch("superset.security.manager.g")
+ def test_export_dataset_command_unicode_chars(self, mock_g) -> None:
+ mock_g.user = security_manager.find_user("admin")
+ examples_db = get_example_database()
+ with examples_db.get_sqla_engine() as engine:
+ engine.execute("DROP TABLE IF EXISTS 中文")
+ engine.execute("CREATE TABLE 中文 AS SELECT 2 as col")
+ # scope cleanup to the example database so datasets with the same name
+ # on other databases are left untouched
+ stale = db.session.query(SqlaTable).filter_by(
+ table_name="中文", database_id=examples_db.id
+ )
+ if stale.count():
+ stale.delete()
+ with override_user(security_manager.find_user("admin")):
+ example_dataset = CreateDatasetCommand(
+ {
+ "table_name": "中文",
+ "database": examples_db.id,
+ }
+ ).run()
+
+ command = ExportDatasetsCommand([example_dataset.id],
export_related=False)
+ contents = dict(command.run())
+
+ path = f"datasets/examples/{example_dataset.id}.yaml"
+ assert path in set(contents.keys())
+ yaml_content = contents[path]()
+ assert "table_name: 中文" in yaml_content
+
+ db.session.delete(example_dataset)
+ db.session.commit()
+ with examples_db.get_sqla_engine() as engine:
+ engine.execute("DROP TABLE 中文")
+ db.session.commit()
Review Comment:
**Suggestion:** The dataset/table teardown runs only on the happy path; if
any assertion above fails, cleanup is skipped and leaves persistent DB
artifacts that can break later tests. Wrap the body in `try/finally` and move
deletion/drop logic into the `finally` block. [missing cleanup]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Failed unicode dataset test leaves dataset \"中文\" persisted.
- ⚠️ Failed test leaves physical table \"中文\" in examples DB.
- ⚠️ Later tests see unexpected tables in shared example database.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. The test `test_export_dataset_command_unicode_chars` in
`tests/integration_tests/datasets/commands_tests.py:27-61` creates a
physical table named
`中文` in the example database via
`examples_db.get_sqla_engine().execute("CREATE TABLE 中文
AS SELECT 2 as col")` at lines 31-33, and then creates a corresponding
`SqlaTable` dataset
via `CreateDatasetCommand(...).run()` at lines 41-47.
2. The test exports this dataset using
`ExportDatasetsCommand([example_dataset.id],
export_related=False)` at line 49, builds `path =
f"datasets/examples/{example_dataset.id}.yaml"` at line 52, and asserts the
YAML contains
`table_name: 中文` at line 55.
3. If any of the operations or assertions from lines 49-55 fail (for example
due to a
regression in dataset export formatting or path construction), Python raises
an exception
and the test function exits before executing the cleanup block at lines 57-61
(`db.session.delete(example_dataset)`, `db.session.commit()`, `DROP TABLE
中文`,
`db.session.commit()`).
4. Because there is no surrounding `try/finally`, in that failure scenario
the `SqlaTable`
row for `table_name="中文"` and the physical table `中文` remain in the shared
example
database returned by `get_example_database()`
(`superset/utils/database.py`), unlike the
analogous database-unicode test in
`tests/integration_tests/databases/commands_tests.py:19-41` which wraps
cleanup in
`finally`; these leaked artifacts can then be seen by subsequent tests that
rely on the
example database contents, breaking assumptions about test isolation.
```
</details>
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=f9ca5eafd5974494bce636f3ad2eee8f&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
| [Fix in VSCode
Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=f9ca5eafd5974494bce636f3ad2eee8f&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/integration_tests/datasets/commands_tests.py
**Line:** 298:310
**Comment:**
*Missing Cleanup: The dataset/table teardown runs only on the happy
path; if any assertion above fails, cleanup is skipped and leaves persistent DB
artifacts that can break later tests. Wrap the body in `try/finally` and move
deletion/drop logic into the `finally` block.
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.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F28008&comment_hash=32d6da6f4e7cd8ef226d8bcd6239033755427f709f1d25db62383dedc88f74d4&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F28008&comment_hash=32d6da6f4e7cd8ef226d8bcd6239033755427f709f1d25db62383dedc88f74d4&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]
