codeant-ai-for-open-source[bot] commented on code in PR #40994:
URL: https://github.com/apache/superset/pull/40994#discussion_r3433913966
##########
tests/integration_tests/cli_tests.py:
##########
@@ -364,3 +365,115 @@ def
test_re_encrypt_secrets_failure_exits_nonzero(app_context):
# The failure path must be handled by the CLI, not leaked as an
# uncaught exception.
assert response.exception is None or isinstance(response.exception,
SystemExit)
+
+
[email protected]("superset.cli.importexport.security_manager")
[email protected]("superset.examples.utils.load_configs_from_directory")
+def test_import_directory(
+ load_configs_mock: mock.MagicMock,
+ security_manager_mock: mock.MagicMock,
+ app_context: Any,
+ fs: Any,
+) -> None:
+ """
+ Test that import-directory calls load_configs_from_directory with the
+ correct arguments and assigns assets to the specified user.
+ """
+ # pylint: disable=reimported, redefined-outer-name
+ import superset.cli.importexport # noqa: F811
+
+ importlib.reload(superset.cli.importexport)
+
+ fake_user = mock.MagicMock()
+ fake_user.username = "admin"
+ security_manager_mock.find_user.return_value = fake_user
+
+ captured_user: list[Any] = []
+
+ def capture_g_user(**kwargs: Any) -> None:
+ captured_user.append(g.user)
+
+ load_configs_mock.side_effect = capture_g_user
+
+ fs.create_dir("/assets")
+ fs.create_file("/assets/metadata.yaml", contents="version: 1.0.0\n")
+
+ runner = current_app.test_cli_runner()
+ response = runner.invoke(
+ superset.cli.importexport.import_directory,
+ ("/assets", "-u", "admin"),
+ )
+
+ assert response.exit_code == 0
+ security_manager_mock.find_user.assert_called_once_with(username="admin")
+ load_configs_mock.assert_called_once()
+ call_kwargs = load_configs_mock.call_args
+ assert str(call_kwargs.kwargs["root"]) == "/assets"
+ assert call_kwargs.kwargs["overwrite"] is False
+ assert call_kwargs.kwargs["force_data"] is False
+ assert captured_user[0] is fake_user
+
+
[email protected]("superset.cli.importexport.security_manager")
+def test_import_directory_unknown_user(
+ security_manager_mock: mock.MagicMock,
+ app_context: Any,
+ fs: Any,
+) -> None:
+ """
+ Test that import-directory fails fast with a clear error when the
+ specified user does not exist.
+ """
+ # pylint: disable=reimported, redefined-outer-name
+ import superset.cli.importexport # noqa: F811
+
+ importlib.reload(superset.cli.importexport)
+
+ security_manager_mock.find_user.return_value = None
+
+ fs.create_dir("/assets")
+ fs.create_file("/assets/metadata.yaml", contents="version: 1.0.0\n")
+
+ runner = current_app.test_cli_runner()
+ response = runner.invoke(
+ superset.cli.importexport.import_directory,
+ ("/assets", "-u", "nonexistent"),
+ )
+
+ assert response.exit_code != 0
+ assert "nonexistent" in response.output
+
+
[email protected]("superset.cli.importexport.security_manager")
[email protected](
+ "superset.examples.utils.load_configs_from_directory",
+ side_effect=Exception("import failed"),
+)
+def test_failing_import_directory(
+ load_configs_mock: mock.MagicMock,
+ security_manager_mock: mock.MagicMock,
+ app_context: Any,
+ fs: Any,
+ caplog: Any,
+) -> None:
+ """
+ Test that a failure in import-directory is handled gracefully.
+ """
+ # pylint: disable=reimported, redefined-outer-name
+ import superset.cli.importexport # noqa: F811
+
+ importlib.reload(superset.cli.importexport)
Review Comment:
**Suggestion:** The patched `security_manager` is invalidated by reloading
`superset.cli.importexport`, so this failure-path test is not actually
controlling user resolution through `security_manager_mock`. This can make the
test flaky across environments and weakens what the test is verifying. Reload
before patching (or remove reload) to keep the mock active. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Failure-path test hits real security manager unexpectedly.
- ⚠️ Test determinism depends on external security configuration.
- ⚠️ Mocked user resolution is not actually exercised by test.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Run `pytest
tests/integration_tests/cli_tests.py::test_failing_import_directory` to
execute the failure-path test defined at
`tests/integration_tests/cli_tests.py:452-479`.
2. When the test file is imported, the decorator
`@mock.patch("superset.cli.importexport.security_manager")` at line `447`
replaces
`superset.cli.importexport.security_manager` with a `MagicMock`, and passes
that mock as
`security_manager_mock` into `test_failing_import_directory(...)`.
3. Inside the test, lines `463-465` re-import and reload the module: `import
superset.cli.importexport` then
`importlib.reload(superset.cli.importexport)`. This
re-executes `superset/cli/importexport.py`, including `from superset import
security_manager` at line `29`, which rebinds
`superset.cli.importexport.security_manager`
to the real security manager object and discards the patch, while
`security_manager_mock`
remains an un-used `MagicMock`.
4. The click runner at `tests/integration_tests/cli_tests.py:473-477` invokes
`superset.cli.importexport.import_directory`, whose implementation at
`superset/cli/importexport.py:58-76` now resolves the user with the real
`security_manager.find_user` rather than `security_manager_mock`. Although
the failure
condition is driven by the patched `load_configs_from_directory` (decorator
at lines
`448-451`), the user resolution path is no longer controlled by the test,
making the
failure-path verification partially dependent on external security
configuration instead
of a fully mocked environment.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=6508575f84ae4cdc92fa612358c1d9f3&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=6508575f84ae4cdc92fa612358c1d9f3&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/cli_tests.py
**Line:** 447:465
**Comment:**
*Logic Error: The patched `security_manager` is invalidated by
reloading `superset.cli.importexport`, so this failure-path test is not
actually controlling user resolution through `security_manager_mock`. This can
make the test flaky across environments and weakens what the test is verifying.
Reload before patching (or remove reload) to keep the mock active.
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%2F40994&comment_hash=30e28810b4b2cf25538db9c2b7c276222c1a3a0dc81e09f640784d2f406bbd6c&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40994&comment_hash=30e28810b4b2cf25538db9c2b7c276222c1a3a0dc81e09f640784d2f406bbd6c&reaction=dislike'>👎</a>
##########
tests/integration_tests/cli_tests.py:
##########
@@ -364,3 +365,115 @@ def
test_re_encrypt_secrets_failure_exits_nonzero(app_context):
# The failure path must be handled by the CLI, not leaked as an
# uncaught exception.
assert response.exception is None or isinstance(response.exception,
SystemExit)
+
+
[email protected]("superset.cli.importexport.security_manager")
[email protected]("superset.examples.utils.load_configs_from_directory")
+def test_import_directory(
+ load_configs_mock: mock.MagicMock,
+ security_manager_mock: mock.MagicMock,
+ app_context: Any,
+ fs: Any,
+) -> None:
+ """
+ Test that import-directory calls load_configs_from_directory with the
+ correct arguments and assigns assets to the specified user.
+ """
+ # pylint: disable=reimported, redefined-outer-name
+ import superset.cli.importexport # noqa: F811
+
+ importlib.reload(superset.cli.importexport)
Review Comment:
**Suggestion:** Reloading the module after applying
`@mock.patch("superset.cli.importexport.security_manager")` overwrites the
patched attribute with the real object, so this test no longer uses
`security_manager_mock`. That makes `assert_called_once_with` unreliable and
can fail by calling real user lookup instead of the mock. Remove the reload (or
patch after reloading) so the mocked dependency is actually exercised. [logic
error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ `test_import_directory` exercises real security manager dependency.
- ⚠️ Mock-based assertion on `find_user` may silently misrepresent behavior.
- ⚠️ Test reliability depends on real users in the test database.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Run `pytest tests/integration_tests/cli_tests.py::test_import_directory`
to execute the
test defined at `tests/integration_tests/cli_tests.py:372-415`.
2. During test module import, the decorator
`@mock.patch("superset.cli.importexport.security_manager")` at
`tests/integration_tests/cli_tests.py:370` replaces
`superset.cli.importexport.security_manager` with a `MagicMock`, and passes
that mock as
`security_manager_mock` into `test_import_directory(...)`.
3. Inside the test body, lines `383-385` re-import and then reload the
module: `import
superset.cli.importexport` followed by
`importlib.reload(superset.cli.importexport)`. This
re-executes `superset/cli/importexport.py`, including `from superset import
security_manager` at `superset/cli/importexport.py:29`, which rebinds
`superset.cli.importexport.security_manager` back to the real security
manager object,
discarding the earlier mock.
4. The click runner at `tests/integration_tests/cli_tests.py:401-405` invokes
`superset.cli.importexport.import_directory`, which now uses the real
`security_manager`
in `import_directory` (lines `superset/cli/importexport.py:58-76`) to call
`security_manager.find_user`. As a result, `security_manager_mock.find_user`
at
`tests/integration_tests/cli_tests.py:388-389` is never called, making the
assertion
`security_manager_mock.find_user.assert_called_once_with(username="admin")`
at line `408`
unreliable and causing the test to exercise the real dependency instead of
the intended
mock.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=1a873fe07b9d4dda9ce4b2bf6c8e899b&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=1a873fe07b9d4dda9ce4b2bf6c8e899b&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/cli_tests.py
**Line:** 370:385
**Comment:**
*Logic Error: Reloading the module after applying
`@mock.patch("superset.cli.importexport.security_manager")` overwrites the
patched attribute with the real object, so this test no longer uses
`security_manager_mock`. That makes `assert_called_once_with` unreliable and
can fail by calling real user lookup instead of the mock. Remove the reload (or
patch after reloading) so the mocked dependency is actually exercised.
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%2F40994&comment_hash=817d18bb0b66797cc77c92b854e6cb7de897191979d5a8f36caed3b180284d90&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40994&comment_hash=817d18bb0b66797cc77c92b854e6cb7de897191979d5a8f36caed3b180284d90&reaction=dislike'>👎</a>
##########
tests/integration_tests/cli_tests.py:
##########
@@ -364,3 +365,115 @@ def
test_re_encrypt_secrets_failure_exits_nonzero(app_context):
# The failure path must be handled by the CLI, not leaked as an
# uncaught exception.
assert response.exception is None or isinstance(response.exception,
SystemExit)
+
+
[email protected]("superset.cli.importexport.security_manager")
[email protected]("superset.examples.utils.load_configs_from_directory")
+def test_import_directory(
+ load_configs_mock: mock.MagicMock,
+ security_manager_mock: mock.MagicMock,
+ app_context: Any,
+ fs: Any,
+) -> None:
+ """
+ Test that import-directory calls load_configs_from_directory with the
+ correct arguments and assigns assets to the specified user.
+ """
+ # pylint: disable=reimported, redefined-outer-name
+ import superset.cli.importexport # noqa: F811
+
+ importlib.reload(superset.cli.importexport)
+
+ fake_user = mock.MagicMock()
+ fake_user.username = "admin"
+ security_manager_mock.find_user.return_value = fake_user
+
+ captured_user: list[Any] = []
+
+ def capture_g_user(**kwargs: Any) -> None:
+ captured_user.append(g.user)
+
+ load_configs_mock.side_effect = capture_g_user
+
+ fs.create_dir("/assets")
+ fs.create_file("/assets/metadata.yaml", contents="version: 1.0.0\n")
+
+ runner = current_app.test_cli_runner()
+ response = runner.invoke(
+ superset.cli.importexport.import_directory,
+ ("/assets", "-u", "admin"),
+ )
+
+ assert response.exit_code == 0
+ security_manager_mock.find_user.assert_called_once_with(username="admin")
+ load_configs_mock.assert_called_once()
+ call_kwargs = load_configs_mock.call_args
+ assert str(call_kwargs.kwargs["root"]) == "/assets"
+ assert call_kwargs.kwargs["overwrite"] is False
+ assert call_kwargs.kwargs["force_data"] is False
+ assert captured_user[0] is fake_user
+
+
[email protected]("superset.cli.importexport.security_manager")
+def test_import_directory_unknown_user(
+ security_manager_mock: mock.MagicMock,
+ app_context: Any,
+ fs: Any,
+) -> None:
+ """
+ Test that import-directory fails fast with a clear error when the
+ specified user does not exist.
+ """
+ # pylint: disable=reimported, redefined-outer-name
+ import superset.cli.importexport # noqa: F811
+
+ importlib.reload(superset.cli.importexport)
Review Comment:
**Suggestion:** This test has the same patching-order bug:
`security_manager` is patched and then immediately replaced by
`importlib.reload`, so the command runs against the real security manager
instead of the mock. That makes the test non-isolated and environment-dependent
for unknown-user behavior. Keep a single import without reload, or reload first
and then patch. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Unknown-user path test depends on real security manager.
- ⚠️ Behavior may vary with actual users in database.
- ⚠️ Test no longer isolates failure mode of `import_directory`.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Run `pytest
tests/integration_tests/cli_tests.py::test_import_directory_unknown_user`
to execute the test defined at
`tests/integration_tests/cli_tests.py:418-445`.
2. When the test module is imported, the decorator
`@mock.patch("superset.cli.importexport.security_manager")` at line `417`
patches
`superset.cli.importexport.security_manager` with a `MagicMock`, and
provides it as the
`security_manager_mock` argument to the test.
3. At the start of the test body, lines `428-430` execute `import
superset.cli.importexport` followed by
`importlib.reload(superset.cli.importexport)`,
which re-runs `superset/cli/importexport.py`. This re-imports the real
`security_manager`
via `from superset import security_manager` at
`superset/cli/importexport.py:29`,
overwriting the patched `superset.cli.importexport.security_manager` with
the real
implementation.
4. The CLI runner at `tests/integration_tests/cli_tests.py:437-441` invokes
`superset.cli.importexport.import_directory`, whose implementation at
`superset/cli/importexport.py:58-76` now uses the real
`security_manager.find_user(username="nonexistent")` instead of
`security_manager_mock`.
The test's expectations (`response.exit_code != 0` and `"nonexistent" in
response.output`
at lines `443-444`) are satisfied or not based on real environment state
(whether such a
user exists and how click formats the error), making the test behavior
environment-dependent and bypassing the intended mock isolation.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=31753eb213bf4f98a4c9999b10602f8a&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=31753eb213bf4f98a4c9999b10602f8a&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/cli_tests.py
**Line:** 417:430
**Comment:**
*Logic Error: This test has the same patching-order bug:
`security_manager` is patched and then immediately replaced by
`importlib.reload`, so the command runs against the real security manager
instead of the mock. That makes the test non-isolated and environment-dependent
for unknown-user behavior. Keep a single import without reload, or reload first
and then patch.
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%2F40994&comment_hash=dc090c87eb67039eecea45edffeeca3215d40618236ca0cd4f6a5846135b7cd3&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40994&comment_hash=dc090c87eb67039eecea45edffeeca3215d40618236ca0cd4f6a5846135b7cd3&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]
