codeant-ai-for-open-source[bot] commented on code in PR #41974:
URL: https://github.com/apache/superset/pull/41974#discussion_r3566359056
##########
tests/unit_tests/commands/importers/v1/utils_test.py:
##########
@@ -117,3 +124,62 @@ def test_warning_count_excludes_preexisting_nulls(self) ->
None:
call_args = mock_logger.warning.call_args[0]
assert call_args[1] == 2 # 2 out-of-bounds, 1 pre-existing null
+
+
+class TestLoadConfigs:
+ def test_load_configs_caught_exception_and_log_redacted_configs(self) ->
None:
+ logging.basicConfig(level=logging.DEBUG)
Review Comment:
**Suggestion:** Calling `logging.basicConfig` inside a unit test mutates
global logging configuration for the whole test process and can leak state into
unrelated tests. Remove this call or confine logging level changes to a local
logger/mocked logger to avoid cross-test interference. [logic error]
<details>
<summary><b>Severity Level:</b> Minor π§Ή</summary>
```mdx
β οΈ Test mutates global logging configuration shared by suite.
β οΈ Later tests may see unexpected debug-level logging output.
```
</details>
<details>
<summary><b>Steps of Reproduction β
</b></summary>
```mdx
1. Open `tests/unit_tests/commands/importers/v1/utils_test.py` and locate
`TestLoadConfigs.test_load_configs_caught_exception_and_log_redacted_configs`
starting at
line 129.
2. At line 131 inside this test, observe the call to
`logging.basicConfig(level=logging.DEBUG)` before any mocks or assertions
are set up.
3. In Pythonβs logging module, `logging.basicConfig` configures the
process-wide root
logger; calling it here mutates global logging settings (level and handlers)
for all
subsequent code, including other tests in the same pytest run.
4. Run the test file with pytest; this test will execute and permanently
adjust the root
logging configuration, so later tests inherit debug-level logging and any
handlers
configured here, creating shared state that can affect logging behavior or
noise outside
this specific test.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=db21446de3f84f9e92863ce39e78490a&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=db21446de3f84f9e92863ce39e78490a&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/unit_tests/commands/importers/v1/utils_test.py
**Line:** 131:131
**Comment:**
*Logic Error: Calling `logging.basicConfig` inside a unit test mutates
global logging configuration for the whole test process and can leak state into
unrelated tests. Remove this call or confine logging level changes to a local
logger/mocked logger to avoid cross-test interference.
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%2F41974&comment_hash=d7a2528141b84dfb0357e40594cbf94388f5b51841dfc81190928506d3a55616&reaction=like'>π</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41974&comment_hash=d7a2528141b84dfb0357e40594cbf94388f5b51841dfc81190928506d3a55616&reaction=dislike'>π</a>
##########
tests/unit_tests/commands/importers/v1/utils_test.py:
##########
@@ -117,3 +124,62 @@ def test_warning_count_excludes_preexisting_nulls(self) ->
None:
call_args = mock_logger.warning.call_args[0]
assert call_args[1] == 2 # 2 out-of-bounds, 1 pre-existing null
+
+
+class TestLoadConfigs:
+ def test_load_configs_caught_exception_and_log_redacted_configs(self) ->
None:
+ logging.basicConfig(level=logging.DEBUG)
+ from superset.commands.importers.v1.utils import (
+ load_configs,
+ )
+
+ with (
+ patch("superset.db") as mock_db,
Review Comment:
**Suggestion:** The DB patch targets `superset.db`, but `load_configs` uses
the module-local import from `superset.commands.importers.v1.utils`. Because
the function is imported before entering the patch context, this mock does not
intercept database calls and the test can hit the real DB/session instead of an
isolated fake. Patch the symbol where it is used
(`superset.commands.importers.v1.utils.db`) so the test is deterministic.
[incorrect variable usage]
<details>
<summary><b>Severity Level:</b> Major β οΈ</summary>
```mdx
β Unit test touches real database session instead of mock.
β οΈ Test behavior depends on external database state and contents.
```
</details>
<details>
<summary><b>Steps of Reproduction β
</b></summary>
```mdx
1. Open `superset/superset/commands/importers/v1/utils.py` and note the
module-level
import `from superset import db` at line 27, and the use of
`db.session.query(...).all()`
inside `load_configs` at lines 121-143.
2. Open `tests/unit_tests/commands/importers/v1/utils_test.py` and locate
`TestLoadConfigs.test_load_configs_caught_exception_and_log_redacted_configs`
starting at
line 129, where `load_configs` is imported from
`superset.commands.importers.v1.utils` at
lines 132-134 (triggering the module import and binding its local `db` name).
3. In the same test, observe the patch context at lines 136-139, which
patches
`"superset.db"` (line 137) and
`"superset.commands.importers.v1.utils.logger"`. The patch
replaces the `superset.db` attribute but does not modify the
already-imported `db` symbol
in `superset.commands.importers.v1.utils`.
4. Run this test (e.g., `pytest
tests/unit_tests/commands/importers/v1/utils_test.py::TestLoadConfigs::test_load_configs_caught_exception_and_log_redacted_configs`);
during `load_configs` execution, the `db.session.query(...).all()` calls in
`utils.py`
still hit the real SQLAlchemy `db` object, not the mocked `mock_db`, so the
test interacts
with the live database session instead of an isolated fake.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=5b9a094367794cf7a707a13a85dcb2bf&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=5b9a094367794cf7a707a13a85dcb2bf&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/unit_tests/commands/importers/v1/utils_test.py
**Line:** 137:137
**Comment:**
*Incorrect Variable Usage: The DB patch targets `superset.db`, but
`load_configs` uses the module-local import from
`superset.commands.importers.v1.utils`. Because the function is imported before
entering the patch context, this mock does not intercept database calls and the
test can hit the real DB/session instead of an isolated fake. Patch the symbol
where it is used (`superset.commands.importers.v1.utils.db`) so the test is
deterministic.
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%2F41974&comment_hash=807f821bd9c5180fd0d266d4c3b57fd35235990ab4aaa73550a6b3429991f638&reaction=like'>π</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41974&comment_hash=807f821bd9c5180fd0d266d4c3b57fd35235990ab4aaa73550a6b3429991f638&reaction=dislike'>π</a>
##########
tests/unit_tests/commands/importers/v1/utils_test.py:
##########
@@ -117,3 +124,62 @@ def test_warning_count_excludes_preexisting_nulls(self) ->
None:
call_args = mock_logger.warning.call_args[0]
assert call_args[1] == 2 # 2 out-of-bounds, 1 pre-existing null
+
+
+class TestLoadConfigs:
+ def test_load_configs_caught_exception_and_log_redacted_configs(self) ->
None:
+ logging.basicConfig(level=logging.DEBUG)
+ from superset.commands.importers.v1.utils import (
+ load_configs,
+ )
+
+ with (
+ patch("superset.db") as mock_db,
+ patch("yaml.safe_load") as mock_yaml_safe_load,
+ patch("superset.commands.importers.v1.utils.logger") as
mock_logger,
+ ):
+ mock_yaml_safe_load.return_value = {
+ "masked_encrypted_extra": json.dumps(
+ {"oauth2_client_info": {"secret": "MASKED"}}
+ ),
+ "ssh_tunnel": {},
+ }
+ mock_db.session.query.return_value = {"uuid-1":
"SECRET_TO_BE_SEEN"}
Review Comment:
**Suggestion:** The mocked query return is a plain dict, but production code
calls `.all()` on each `db.session.query(...)` result. This mock does not match
the query API contract and will fail with an attribute error once the DB patch
target is corrected. Return a mock object (or chain) that implements `.all()`
with the expected tuple rows for each query. [api mismatch]
<details>
<summary><b>Severity Level:</b> Major β οΈ</summary>
```mdx
β DB mock would crash when `.all()` is invoked.
β οΈ Prevents isolating `load_configs` database queries via mocking.
```
</details>
<details>
<summary><b>Steps of Reproduction β
</b></summary>
```mdx
1. Inspect `load_configs` in
`superset/superset/commands/importers/v1/utils.py` (lines
108-143) and note that it builds several dictionaries by calling
`db.session.query(...).all()` at lines 121-124, 126-129, 131-135, and
138-142, expecting
the query object to implement an `.all()` method returning iterable rows.
2. In `tests/unit_tests/commands/importers/v1/utils_test.py`, locate the
same test at
lines 129-185 and see that it patches `"superset.db"` at line 137 and
configures the DB
mock at line 147 with `mock_db.session.query.return_value = {"uuid-1":
"SECRET_TO_BE_SEEN"}`, i.e., a plain dict.
3. A plain dict does not provide an `.all()` method, so a correctly wired
mock that
replaces the `db` used in `load_configs` would cause
`db.session.query(...).all()` in
`utils.py` to call `.all()` on that dict and raise `AttributeError` instead
of returning
rows.
4. This demonstrates that the testβs DB mock does not conform to the query
API contract
used by `load_configs`; it is currently unused due to the wrong patch target
(see
suggestion 1), but as soon as the patch is corrected to
`superset.commands.importers.v1.utils.db`, the mismatch will immediately
break the test.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=795b936e62734fa0a68af8dbfb9c7626&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=795b936e62734fa0a68af8dbfb9c7626&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/unit_tests/commands/importers/v1/utils_test.py
**Line:** 147:147
**Comment:**
*Api Mismatch: The mocked query return is a plain dict, but production
code calls `.all()` on each `db.session.query(...)` result. This mock does not
match the query API contract and will fail with an attribute error once the DB
patch target is corrected. Return a mock object (or chain) that implements
`.all()` with the expected tuple rows for each query.
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%2F41974&comment_hash=3e108cc4814f774beabe23cdd42071b1e732e8453ac88d7cf2766f96620d2d94&reaction=like'>π</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41974&comment_hash=3e108cc4814f774beabe23cdd42071b1e732e8453ac88d7cf2766f96620d2d94&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]
