rusackas commented on code in PR #41250:
URL: https://github.com/apache/superset/pull/41250#discussion_r3461672861
##########
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:
Added a docstring.
##########
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:
Added a docstring.
##########
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:
Added a docstring.
##########
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:
Added a docstring.
##########
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:
Added a docstring.
##########
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:
Added a docstring.
##########
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:
Added a docstring.
--
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]
