codeant-ai-for-open-source[bot] commented on code in PR #36896:
URL: https://github.com/apache/superset/pull/36896#discussion_r2723610585
##########
superset-frontend/scripts/check-custom-rules.js:
##########
@@ -25,9 +25,17 @@
const fs = require('fs');
const path = require('path');
-const glob = require('glob');
-const parser = require('@babel/parser');
-const traverse = require('@babel/traverse').default;
+
+let parser;
+let traverse;
+
+try {
+ parser = require('@babel/parser');
+ traverse = require('@babel/traverse').default;
+} catch (e) {
+ console.warn('\x1b[33m%s\x1b[0m', '⚠ Warning: @babel/parser or
@babel/traverse not found. Skipping custom rules check.');
+ process.exit(0);
Review Comment:
**Suggestion:** Calling process.exit(0) during module initialization will
terminate the entire process when this file is required (e.g., in tests or
other scripts). Remove the exit so importing the module won't kill the host
process; simply warn and let callers handle missing parser/traverse. [logic
error]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Requiring module kills host process unexpectedly.
- ❌ Test runners may terminate when importing this file.
- ⚠️ CI tasks importing utilities may abort silently.
```
</details>
```suggestion
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. In a test or script, require the module:
`require('./superset-frontend/scripts/check-custom-rules.js')`. The module
top-level
executes the try/catch at lines 29-38.
2. If @babel/parser or @babel/traverse are missing (common in lightweight
environments),
the catch branch runs at line 35 and executes `process.exit(0)` at line 37.
3. The call to process.exit terminates the entire Node process immediately,
causing the
caller (test runner, other script, or process hosting this require) to exit
with status 0
unexpectedly.
4. Observed outcome: importing the module from tests/CI will unexpectedly
stop the test
run or host process; replacing process.exit with a warning preserves safe
import behavior.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/scripts/check-custom-rules.js
**Line:** 37:37
**Comment:**
*Logic Error: Calling process.exit(0) during module initialization will
terminate the entire process when this file is required (e.g., in tests or
other scripts). Remove the exit so importing the module won't kill the host
process; simply warn and let callers handle missing parser/traverse.
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>
##########
tests/integration_tests/thumbnails_tests.py:
##########
@@ -257,17 +254,14 @@ def
test_get_async_dashboard_screenshot_as_current_user(self):
"""
username = "alpha"
self.login(username)
- with (
- patch.dict(
- "flask.current_app.config",
- {
- "THUMBNAIL_EXECUTORS": [ExecutorType.CURRENT_USER],
- },
- ),
- patch(
- "superset.thumbnails.digest._adjust_string_for_executor"
- ) as mock_adjust_string,
- ):
+ with patch.dict(
+ "flask.current_app.config",
Review Comment:
**Suggestion:** Passing the string "flask.current_app.config" into
patch.dict will make patch attempt to import "flask.current_app" as a module
and fail; use the actual config mapping `app.config` (imported in this test
module) so patch.dict can update the mapping. [possible bug]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Thumbnails current-user test errors during test run.
- ⚠️ CI test suite blocked by failing unit.
```
</details>
```suggestion
app.config,
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Run pytest including
TestThumbnails.test_get_async_dashboard_screenshot_as_current_user
in tests/integration_tests/thumbnails_tests.py.
2. Execution reaches the with patch.dict(...) context at hunk line 257
(tests/integration_tests/thumbnails_tests.py:257) inside
test_get_async_dashboard_screenshot_as_current_user.
3. patch.dict tries to import the module portion "flask.current_app"
(derived from the
string "flask.current_app.config").
4. The import fails with ModuleNotFoundError, causing the test to error out
before
mock_adjust_string is used at
tests/integration_tests/thumbnails_tests.py:265.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/integration_tests/thumbnails_tests.py
**Line:** 258:258
**Comment:**
*Possible Bug: Passing the string "flask.current_app.config" into
patch.dict will make patch attempt to import "flask.current_app" as a module
and fail; use the actual config mapping `app.config` (imported in this test
module) so patch.dict can update the mapping.
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>
##########
tests/integration_tests/thumbnails_tests.py:
##########
@@ -229,17 +229,14 @@ def
test_get_async_dashboard_screenshot_as_fixed_user(self):
Thumbnails: Simple get async dashboard screenshot as selenium user
"""
self.login(ALPHA_USERNAME)
- with (
- patch.dict(
- "flask.current_app.config",
- {
- "THUMBNAIL_EXECUTORS": [FixedExecutor(ADMIN_USERNAME)],
- },
- ),
- patch(
- "superset.thumbnails.digest._adjust_string_for_executor"
- ) as mock_adjust_string,
- ):
+ with patch.dict(
+ "flask.current_app.config",
Review Comment:
**Suggestion:** Using a dotted string "flask.current_app.config" as the
first argument to patch.dict is incorrect because patch.dict treats a string as
"module.attr" and will attempt to import "flask.current_app" as a module (which
doesn't exist), causing a failure; pass the actual mapping object (the app's
config) instead by using `app.config`. [possible bug]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Thumbnails integration tests error on setup.
- ⚠️ CI job fails when running thumbnails test suite.
- ❌ Blocks PR validation for related changes.
```
</details>
```suggestion
app.config,
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Run the thumbnails integration tests (e.g., pytest targeting
tests/integration_tests/thumbnails_tests.py).
2. Test runner enters function
TestThumbnails.test_get_async_dashboard_screenshot_as_fixed_user (file
tests/integration_tests/thumbnails_tests.py) and reaches the context manager
starting at
the hunk line 232 ("with patch.dict(...)" at
tests/integration_tests/thumbnails_tests.py:232).
3. unittest.mock.patch.dict attempts to treat the string target
"flask.current_app.config"
as a module path: it splits at the last dot and tries to import the module
portion
"flask.current_app" (this import occurs inside patch.dict).
4. Importing "flask.current_app" raises ModuleNotFoundError (No module named
'flask.current_app'), causing the test to error before the inner assertions
at
tests/integration_tests/thumbnails_tests.py:240 are executed.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/integration_tests/thumbnails_tests.py
**Line:** 233:233
**Comment:**
*Possible Bug: Using a dotted string "flask.current_app.config" as the
first argument to patch.dict is incorrect because patch.dict treats a string as
"module.attr" and will attempt to import "flask.current_app" as a module (which
doesn't exist), causing a failure; pass the actual mapping object (the app's
config) instead by using `app.config`.
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]
