Merge pull request #63 from cyiallou/fix/test-email

Improve notification error handling and update notebooks

Author: cyiallou

RELEASE_NOTES.md

@@ -6,8 +6,10 @@
 
 * Expose MicrogridData from microgrid namespace.
 * Upgrade `frequenz-client-reporting` to minimum `v0.16.0`.
+* Small updates to the Alerts Notebook and Solar Maintenance notebook.
 
 ## New Features
+* Notification service: Introduced `NotificationSendError` for structured retry failure handling and updated the logging in `send_test_email()` utility function.
 
 ## Bug Fixes
 

examples/Alerts Notebook.ipynb

@@ -466,18 +466,26 @@ def send_with_retry(
             backoff_factor: Delay factor for (linear) backoff calculation.
             max_sleep: Maximum sleep time in seconds.
             **kwargs: Keyword arguments for the send_func.
+
+        Raises:
+            NotificationSendError: If the notification fails after all retry attempts.
         """
         for attempt in range(retries + 1):
             try:
                 send_func(**kwargs)
                 _log.info("Successfully sent notification on attempt %d", attempt + 1)
                 return
             except Exception as e:  # pylint: disable=broad-except
+                last_exception = e
                 _log.error("Attempt %d failed: %s", attempt + 1, e)
                 if attempt < retries - 1:
                     linear_backoff = backoff_factor * (attempt + 1)
                     time.sleep(min(max_sleep, linear_backoff))
         _log.error("Failed to send notification after %d retries", retries)
+        raise NotificationSendError(
+            "Notification failed after all retry attempts.",
+            last_exception=last_exception,
+        )
 
     def start_scheduler(self) -> None:
         """Start the scheduler if configured."""
@@ -636,3 +644,24 @@ def _connect_and_send(
         except SMTPException as e:
             _log.error("Failed to send email: %s", e)
             raise
+
+
+class NotificationSendError(Exception):
+    """Raised when sending a notification fails after all retry attempts."""
+
+    def __init__(self, message: str, last_exception: Exception | None = None) -> None:
+        """Initialise the error with a message and optional last exception.
+
+        Args:
+            message: Error message.
+            last_exception: The last exception encountered during the send process.
+        """
+        super().__init__(message)
+        self.last_exception = last_exception
+
+    def __str__(self) -> str:
+        """Return a string representation of the error."""
+        base = super().__str__()
+        if self.last_exception:
+            return f"{base} (Caused by: {self.last_exception})"
+        return base

examples/Solar Maintenance.ipynb

@@ -8,28 +8,37 @@
 help users configure and debug their notification settings interactively.
 """
 import html
+import logging
 import os
 import re
 import smtplib
+import traceback
 from dataclasses import fields
 from datetime import UTC, datetime
 
-from frequenz.lib.notebooks.notification_service import EmailConfig, EmailNotification
+from frequenz.lib.notebooks.notification_service import (
+    EmailConfig,
+    EmailNotification,
+    NotificationSendError,
+)
+
+_log = logging.getLogger(__name__)
 
 EMAIL_REGEX = re.compile(r"^[^@\s]+@[^@\s]+\.[^@\s]+$")
 
 
-def send_test_email(config: EmailConfig) -> tuple[bool, str]:
+def send_test_email(config: EmailConfig, verbose: bool = True) -> bool:
     """Send a test email using the given EmailConfig.
 
     The email message is generated automatically based on the provided
     configuration and replaces the provided one.
 
     Args:
         config: An EmailConfig instance.
+        verbose: If True, prints the result to stdout.
 
     Returns:
-        Tuple of (success_flag, message).
+        True if the email was sent successfully, False otherwise.
     """
     if config.subject is None:
         config.subject = "Test Email"
@@ -65,9 +74,21 @@ def send_test_email(config: EmailConfig) -> tuple[bool, str]:
     try:
         email_notification = EmailNotification(config=config)
         email_notification.send()
-        return True, "✅ Test email sent successfully!"
-    except Exception as e:  # pylint: disable=broad-except
-        return False, f"❌ Error sending test email: {e}"
+        msg = "✅ Test email sent successfully!"
+        _log.info(msg)
+        if verbose:
+            print(msg)
+        return True
+    except NotificationSendError as e:
+        msg = f"❌ Error sending test email: {e}"
+        _log.error(msg)
+        if e.last_exception:
+            _log.debug(
+                "Traceback:\n%s", "".join(traceback.format_exception(e.last_exception))
+            )
+        if verbose:
+            print(msg)
+        return False
 
 
 def format_email_preview(

src/frequenz/lib/notebooks/notification_service.py

@@ -15,6 +15,7 @@
     EmailConfig,
     EmailNotification,
     FromDictMixin,
+    NotificationSendError,
     Scheduler,
     SchedulerConfig,
 )
@@ -140,13 +141,16 @@ def test_send_with_retry_failure(
     """Test that send_with_retry raises after all retries fail."""
     mock_send.side_effect = Exception("Always fails")
     with caplog.at_level(logging.ERROR):
-        BaseNotification.send_with_retry(
-            send_func=mock_send,
-            retries=3,
-            backoff_factor=1,
-            max_sleep=2,
-        )
+        with pytest.raises(NotificationSendError) as exc_info:
+            BaseNotification.send_with_retry(
+                send_func=mock_send,
+                retries=3,
+                backoff_factor=1,
+                max_sleep=2,
+            )
         assert "Failed to send notification after 3 retries" in caplog.text
+        assert "Always fails" in str(exc_info.value)
+        assert "notification failed" in str(exc_info.value).lower()
     assert mock_send.call_count == 4
 
 
@@ -168,12 +172,13 @@ def test_send_with_retry_backoff(mock_send: MagicMock, mock_sleep: MagicMock) ->
     retries = 3
     backoff_factor = 2
     max_sleep = 3
-    BaseNotification.send_with_retry(
-        send_func=mock_send,
-        retries=retries,
-        backoff_factor=backoff_factor,
-        max_sleep=max_sleep,
-    )
+    with pytest.raises(NotificationSendError):
+        BaseNotification.send_with_retry(
+            send_func=mock_send,
+            retries=retries,
+            backoff_factor=backoff_factor,
+            max_sleep=max_sleep,
+        )
 
     # Expected sleep durations for each retry
     expected_sleep_calls = [
@@ -344,3 +349,19 @@ class TestClass:
 
     with pytest.raises(AttributeError, match="has no attribute 'from_dict'"):
         TestClass.from_dict({})  # type: ignore
+
+
+# Tests for the NotificationSendError class
+def test_notification_send_error_str_with_cause() -> None:
+    """Test NotificationSendError string representation with a cause."""
+    original_error = ValueError("invalid email address")
+    exc = NotificationSendError(
+        "Failed to send notification", last_exception=original_error
+    )
+    assert str(exc) == "Failed to send notification (Caused by: invalid email address)"
+
+
+def test_notification_send_error_str_without_cause() -> None:
+    """Test NotificationSendError string representation without a cause."""
+    exc = NotificationSendError("Retry attempts exhausted")
+    assert str(exc) == "Retry attempts exhausted"

src/frequenz/lib/notebooks/notification_utils.py

@@ -6,9 +6,13 @@
 from unittest.mock import MagicMock, patch
 
 import pytest
+from _pytest.logging import LogCaptureFixture
 
 from frequenz.lib.notebooks import notification_utils
-from frequenz.lib.notebooks.notification_service import EmailConfig
+from frequenz.lib.notebooks.notification_service import (
+    EmailConfig,
+    NotificationSendError,
+)
 
 
 @pytest.fixture
@@ -177,19 +181,26 @@ def test_format_email_preview_without_attachments() -> None:
 
 @patch("frequenz.lib.notebooks.notification_service.EmailNotification.send")
 def test_send_test_email_success(basic_email_config: EmailConfig) -> None:
-    """Test that send_test_email reports success correctly."""
-    success, msg = notification_utils.send_test_email(basic_email_config)
+    """Test that send_test_email logs success correctly."""
+    success = notification_utils.send_test_email(basic_email_config, verbose=False)
     assert success is True
-    assert "successfully" in msg.lower()
 
 
 @patch("frequenz.lib.notebooks.notification_service.EmailNotification.send")
 def test_send_test_email_handles_failure(
-    mock_send: MagicMock, basic_email_config: EmailConfig
+    mock_send: MagicMock,
+    caplog: LogCaptureFixture,
+    basic_email_config: EmailConfig,
 ) -> None:
-    """Test that send_test_email catches and reports errors."""
-    mock_send.side_effect = Exception("Failed to send")
+    """Test that send_test_email logs error and traceback on failure."""
+    cause = RuntimeError("timeout")
+    exception = NotificationSendError("Retry failed", last_exception=cause)
+    mock_send.side_effect = exception  # Important: patch `send`, not `send_with_retry`
+
+    with caplog.at_level("DEBUG", logger="frequenz.lib.notebooks.notification_utils"):
+        success = notification_utils.send_test_email(basic_email_config, verbose=False)
 
-    success, msg = notification_utils.send_test_email(basic_email_config)
     assert success is False
-    assert "error" in msg.lower()
+    assert any("retry failed" in msg.lower() for msg in caplog.messages)
+    assert any("traceback" in msg.lower() for msg in caplog.messages)
+    assert any("timeout" in msg.lower() for msg in caplog.messages)