Copilot commented on code in PR #62409:
URL: https://github.com/apache/airflow/pull/62409#discussion_r3066475189
##########
providers/smtp/src/airflow/providers/smtp/hooks/smtp.py:
##########
@@ -118,7 +118,7 @@ def get_conn(self) -> SmtpHook:
except AirflowNotFoundException:
raise AirflowException("SMTP connection is not found.")
- for attempt in range(1, self.smtp_retry_limit + 1):
+ for attempt in range(0, self.smtp_retry_limit + 1):
Review Comment:
Using `range(0, limit + 1)` is equivalent to `range(limit + 1)` and the
explicit `0` can read like the loop is intended to expose 0-based attempt
numbers. Consider simplifying to `range(self.smtp_retry_limit + 1)` to reduce
noise and avoid implying 0-based attempts (apply similarly to the other updated
loops in this file).
```suggestion
for attempt in range(self.smtp_retry_limit + 1):
```
##########
airflow-core/src/airflow/utils/email.py:
##########
@@ -257,7 +257,7 @@ def send_mime_email(
log.debug("No user/password found for SMTP, so logging in with no
authentication.")
if not dryrun:
- for attempt in range(1, smtp_retry_limit + 1):
+ for attempt in range(0, smtp_retry_limit + 1):
log.info("Email alerting: attempt %s", str(attempt))
Review Comment:
With the loop now starting at 0, the log line will emit `attempt 0` for the
first attempt, which is user-facing and likely confusing (and may break any
downstream log parsing/alerting keyed on attempt numbers). Consider keeping the
internal loop as-is but logging `attempt + 1`, or iterate attempts as
`1..(smtp_retry_limit + 1)` while still treating `smtp_retry_limit` as “retries
after the first attempt.”
```suggestion
log.info("Email alerting: attempt %s", str(attempt + 1))
```
##########
providers/smtp/tests/unit/smtp/hooks/test_smtp.py:
##########
@@ -143,8 +156,10 @@ def test_connect_and_disconnect(
"""Test sync connection with different configurations."""
mock_conn = _create_fake_smtp(mock_smtplib, use_ssl=use_ssl)
- with SmtpHook(smtp_conn_id=conn_id):
- pass
+ smtp_hook = SmtpHook(smtp_conn_id=conn_id)
+ assert smtp_hook._smtp_client is None
+ with smtp_hook:
+ assert smtp_hook._smtp_client is not None
Review Comment:
These assertions rely on the private `_smtp_client` attribute, which makes
the test more brittle to refactors (e.g., if the implementation changes to
lazily open or to encapsulate the client differently). If feasible, prefer
asserting behavior via public API / mocked calls (e.g., that the SMTP
constructor was invoked within the context and that `close()`/`quit()` is
called on exit), while keeping the `retry_limit=0` case covered.
```suggestion
mock_smtplib.SMTP.assert_not_called()
mock_smtplib.SMTP_SSL.assert_not_called()
with smtp_hook:
if create_context:
mock_smtplib.SMTP_SSL.assert_called_once()
mock_smtplib.SMTP.assert_not_called()
else:
mock_smtplib.SMTP.assert_called_once()
mock_smtplib.SMTP_SSL.assert_not_called()
```
--
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]
