codeant-ai-for-open-source[bot] commented on code in PR #41250:
URL: https://github.com/apache/superset/pull/41250#discussion_r3459863528
##########
tests/integration_tests/email_tests.py:
##########
@@ -281,6 +287,30 @@ def test_send_mime_dryrun(self, mock_smtp, mock_smtp_ssl):
assert not mock_smtp.called
assert not mock_smtp_ssl.called
+ @mock.patch("smtplib.SMTP_SSL")
+ @mock.patch("smtplib.SMTP")
+ def test_send_mime_respects_custom_timeout(
+ self, mock_smtp: mock.Mock, mock_smtp_ssl: mock.Mock
+ ) -> None:
+ # A missing timeout would block the worker forever when the SMTP server
+ # is unreachable, wedging the report schedule in WORKING (issue
#40047).
+ config = {**current_app.config, "SMTP_TIMEOUT": 7, "SMTP_SSL": False}
+ mock_smtp.return_value = mock.Mock()
+ utils.send_mime_email("from", ["to"], MIMEMultipart(), config,
dryrun=False)
+ assert mock_smtp.call_args.kwargs["timeout"] == 7
+
+ @mock.patch("smtplib.SMTP_SSL")
+ @mock.patch("smtplib.SMTP")
+ def test_send_mime_timeout_defaults_when_unset(
+ self, mock_smtp: mock.Mock, mock_smtp_ssl: mock.Mock
+ ) -> None:
Review Comment:
**Suggestion:** Add a docstring immediately under this new test method
definition to explain the default-timeout expectation. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This is also a newly added Python test method in the final file state, and
it lacks a docstring. That directly violates the rule requiring docstrings for
newly added Python functions and classes.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=d9ce79e641b24622856bfa1433f6154b&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=d9ce79e641b24622856bfa1433f6154b&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/email_tests.py
**Line:** 304:306
**Comment:**
*Custom Rule: Add a docstring immediately under this new test method
definition to explain the default-timeout expectation.
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%2F41250&comment_hash=d88d9dec9d008cd75473580b43b1ae56d4a575f5b8321e5a05c62065cb9bdba9&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41250&comment_hash=d88d9dec9d008cd75473580b43b1ae56d4a575f5b8321e5a05c62065cb9bdba9&reaction=dislike'>👎</a>
##########
tests/unit_tests/utils/csv_tests.py:
##########
@@ -103,7 +119,11 @@ def fake_get_chart_csv_data_valid(chart_url,
auth_cookies=None):
return json.dumps(fake_result).encode("utf-8")
Review Comment:
**Suggestion:** Add a concise docstring explaining that this helper returns
a valid non-temporal payload used to verify dataframe construction.
[custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This is a newly added function and it does not include a docstring.
It therefore violates the rule that new Python functions should be
documented inline.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=29d5764ff04d46a492936b3f5ff06e1d&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=29d5764ff04d46a492936b3f5ff06e1d&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/utils/csv_tests.py
**Line:** 102:119
**Comment:**
*Custom Rule: Add a concise docstring explaining that this helper
returns a valid non-temporal payload used to verify dataframe construction.
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%2F41250&comment_hash=e23fdc3c245439abdccb1ab6d0baeaaa44fcee214805e867a4d1b0de79f7e96f&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41250&comment_hash=e23fdc3c245439abdccb1ab6d0baeaaa44fcee214805e867a4d1b0de79f7e96f&reaction=dislike'>👎</a>
##########
tests/integration_tests/email_tests.py:
##########
@@ -281,6 +287,30 @@ def test_send_mime_dryrun(self, mock_smtp, mock_smtp_ssl):
assert not mock_smtp.called
assert not mock_smtp_ssl.called
+ @mock.patch("smtplib.SMTP_SSL")
+ @mock.patch("smtplib.SMTP")
+ def test_send_mime_respects_custom_timeout(
+ self, mock_smtp: mock.Mock, mock_smtp_ssl: mock.Mock
+ ) -> None:
Review Comment:
**Suggestion:** Add a docstring immediately under this new test method
definition to describe the behavior being validated. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This is a newly added Python test method in the updated file, and it has no
docstring immediately under the definition. The custom rule requires newly
added Python functions and classes to include docstrings, so the suggestion
matches a real violation.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=94d2c3a2fd974fd6bf9f36dcdddfe8ad&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=94d2c3a2fd974fd6bf9f36dcdddfe8ad&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/email_tests.py
**Line:** 292:294
**Comment:**
*Custom Rule: Add a docstring immediately under this new test method
definition to describe the behavior being validated.
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%2F41250&comment_hash=3c7a0d68773154e57ce63bd3f7c928e90db10366cbd29b5b6c537cf0921c1c9f&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41250&comment_hash=3c7a0d68773154e57ce63bd3f7c928e90db10366cbd29b5b6c537cf0921c1c9f&reaction=dislike'>👎</a>
##########
tests/unit_tests/utils/csv_tests.py:
##########
@@ -138,7 +162,11 @@ def fake_get_chart_csv_data_hierarchical(chart_url,
auth_cookies=None):
return json.dumps(fake_result).encode("utf-8")
Review Comment:
**Suggestion:** Add a function-level docstring summarizing that this helper
returns hierarchical-column mock data for MultiIndex-related assertions.
[custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This newly added helper has no docstring in the final code.
The rule applies to new Python functions, so the suggestion correctly
identifies a real violation.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=cdcf9f0e027946c9a6fa5ed238ce11a8&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=cdcf9f0e027946c9a6fa5ed238ce11a8&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/utils/csv_tests.py
**Line:** 145:162
**Comment:**
*Custom Rule: Add a function-level docstring summarizing that this
helper returns hierarchical-column mock data for MultiIndex-related assertions.
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%2F41250&comment_hash=3870cf97daacc3f57e011366f1bbc5bcb891e14e765a1befdd492f3f3590d39d&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41250&comment_hash=3870cf97daacc3f57e011366f1bbc5bcb891e14e765a1befdd492f3f3590d39d&reaction=dislike'>👎</a>
##########
tests/unit_tests/utils/csv_tests.py:
##########
@@ -75,19 +79,31 @@ def test_escape_value():
assert result == "'\rfoo"
-def fake_get_chart_csv_data_none(chart_url, auth_cookies=None):
+def fake_get_chart_csv_data_none(
+ chart_url: str,
+ auth_cookies: dict[str, str] | None = None,
+ timeout: float | None = None,
+) -> bytes | None:
return None
-def fake_get_chart_csv_data_empty(chart_url, auth_cookies=None):
+def fake_get_chart_csv_data_empty(
+ chart_url: str,
+ auth_cookies: dict[str, str] | None = None,
+ timeout: float | None = None,
+) -> bytes | None:
# Return JSON with empty data so that the resulting DataFrame is empty
- fake_result = {
+ fake_result: dict[str, Any] = {
"result": [{"data": {}, "coltypes": [], "colnames": [], "indexnames":
[]}]
}
return json.dumps(fake_result).encode("utf-8")
Review Comment:
**Suggestion:** Add a function docstring that explains this helper returns
an encoded empty-result payload for dataframe-empty test scenarios.
[custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This newly added function has no docstring. The presence of an inline
comment does not satisfy the rule,
which specifically requires docstrings for new Python functions and classes.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=d75e4030774348d59ded915bdfe45476&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=d75e4030774348d59ded915bdfe45476&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/utils/csv_tests.py
**Line:** 90:99
**Comment:**
*Custom Rule: Add a function docstring that explains this helper
returns an encoded empty-result payload for dataframe-empty test scenarios.
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%2F41250&comment_hash=4d5af085199171c7ecefa6423e8d9a86a7792a735968cfaf98fe7edc5ef43a0f&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41250&comment_hash=4d5af085199171c7ecefa6423e8d9a86a7792a735968cfaf98fe7edc5ef43a0f&reaction=dislike'>👎</a>
##########
tests/unit_tests/utils/csv_tests.py:
##########
@@ -75,19 +79,31 @@ def test_escape_value():
assert result == "'\rfoo"
-def fake_get_chart_csv_data_none(chart_url, auth_cookies=None):
+def fake_get_chart_csv_data_none(
+ chart_url: str,
+ auth_cookies: dict[str, str] | None = None,
+ timeout: float | None = None,
+) -> bytes | None:
return None
Review Comment:
**Suggestion:** Add a short docstring describing the purpose of this helper
and what `None` represents in the mocked response path. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This is a newly added Python function and it has no docstring in the final
file state.
That matches the custom rule requiring newly added functions to be
documented inline.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=572eca0d7b584517b31b544db1f67528&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=572eca0d7b584517b31b544db1f67528&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/utils/csv_tests.py
**Line:** 82:87
**Comment:**
*Custom Rule: Add a short docstring describing the purpose of this
helper and what `None` represents in the mocked response path.
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%2F41250&comment_hash=12c1d386e531f26234c4aef983eb7b14df1f6f3462f074190c0bdb8b21c0b335&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41250&comment_hash=12c1d386e531f26234c4aef983eb7b14df1f6f3462f074190c0bdb8b21c0b335&reaction=dislike'>👎</a>
##########
tests/unit_tests/utils/csv_tests.py:
##########
@@ -380,3 +408,40 @@ def test_get_chart_dataframe_preserves_na_string_values(
last_name_values = df[("last_name",)].values
assert last_name_values[0] == "Smith"
assert last_name_values[1] == "NA"
+
+
+def test_get_chart_csv_data_passes_timeout_to_opener(
+ monkeypatch: pytest.MonkeyPatch,
+) -> None:
+ """The timeout argument must reach the urllib opener's open() call."""
+ # Without a timeout the request blocks forever when the webserver is
+ # unreachable, wedging the report schedule in WORKING (issue #40047).
+ mock_response = mock.Mock()
+ mock_response.read.return_value = b"data"
+ mock_response.getcode.return_value = 200
+ mock_opener = mock.Mock()
+ mock_opener.open.return_value = mock_response
+ mock_opener.addheaders = []
+ monkeypatch.setattr(
+ "urllib.request.build_opener", mock.Mock(return_value=mock_opener)
+ )
+
+ get_chart_csv_data("http://dummy-url";, auth_cookies={"session": "x"},
timeout=42)
+
+ mock_opener.open.assert_called_once_with("http://dummy-url";, timeout=42)
+
+
+def test_get_chart_dataframe_forwards_timeout(monkeypatch: pytest.MonkeyPatch)
-> None:
+ captured: dict[str, float | None] = {}
+
+ def fake(
+ chart_url: str,
+ auth_cookies: dict[str, str] | None = None,
+ timeout: float | None = None,
+ ) -> bytes | None:
+ captured["timeout"] = timeout
+ return None
+
+ monkeypatch.setattr(csv, "get_chart_csv_data", fake)
+ get_chart_dataframe("http://dummy-url";, timeout=99)
Review Comment:
**Suggestion:** Add a test docstring describing that this test verifies
timeout propagation from dataframe retrieval to the lower-level CSV fetch
function. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This is a newly added Python function and it does not have a docstring.
That directly violates the custom rule requiring new functions to include
docstrings.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=2e466fb9ad204e91a952d181a80cb944&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=2e466fb9ad204e91a952d181a80cb944&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/utils/csv_tests.py
**Line:** 434:446
**Comment:**
*Custom Rule: Add a test docstring describing that this test verifies
timeout propagation from dataframe retrieval to the lower-level CSV fetch
function.
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%2F41250&comment_hash=e4df42183bdf7a6744eaa703dd4fb9c49b4ef2f8194322d030cf3d0236a448ba&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41250&comment_hash=e4df42183bdf7a6744eaa703dd4fb9c49b4ef2f8194322d030cf3d0236a448ba&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]
