fix(M10): apply FIXME_M10 corrections (transport, dry_run, format, security)
Fix all 8 findings from the independent review (FIXME_M10.md): #1 Transport compatible with slixmpp 1.17.0 (D5): - Use real ClientXMPP type (remove Any), JID with resource - connect(host, port) explicit, no use_tls kwarg - enable_direct_tls/enable_starttls configured before connect - Single timeout via asyncio.Future for session_start/failed_auth/disconnected - Remove premature 'starttls' in features check, remove auto_reconnect - try/finally guarantees disconnect on all paths (#4) #2 Factory dry_run no longer bypassed (D6): - Single send() entry point in SyncXmppChannel - dry_run check before any ClientXMPP creation - Remove XmppChannel.send() dual implementation #3 Thread daemon removed — single asyncio.run(), documented limitation #5 Richer message format: - Target date header, change type [Ajouté/Supprimé/Modifié] - Lesson times, homework due date, message author - No pronote_messages duplication (external_info = blog + other_info only) #6 Error contract unified (D6): - Channel.send() -> bool never raises PipelineWarning - Errors logged with redaction, returns False - PipelineWarning(step='xmpp') will be created by pipeline M11 #7 Tests faithful to slixmpp 1.17.0 API: - FakeClientXMPP with real connect(host,port)/disconnect() signatures - Assertions on host, port, resource, mtype='chat' - No RuntimeWarning from unawaited coroutines #8 .secrets.baseline restored from main Coverage: 96.44% on channels/, 600 tests pass, pre-commit all-files green. Co-authored-by: opencode/coder <coder@agents.invalid> Co-authored-by: opencode/test-engineer <test-engineer@agents.invalid>
This commit is contained in:
File diff suppressed because it is too large
Load Diff
@@ -19,6 +19,7 @@ Spécification (D2) :
|
||||
from __future__ import annotations
|
||||
|
||||
from typing import Any
|
||||
from unittest.mock import patch
|
||||
|
||||
import pytest
|
||||
from pydantic import SecretStr
|
||||
@@ -286,6 +287,26 @@ class TestGetChannelDryRun:
|
||||
assert result is not None
|
||||
assert isinstance(result, Channel)
|
||||
|
||||
def test_get_channel_dry_run_no_connection(self) -> None:
|
||||
"""Vérifie que dry_run=True ne crée pas de ClientXMPP.
|
||||
|
||||
:return: None
|
||||
:rtype: None
|
||||
"""
|
||||
settings = XmppSettings(
|
||||
enabled=True,
|
||||
jid="bot@example.com",
|
||||
password=SecretStr("pass"),
|
||||
host="example.com",
|
||||
to="parent@example.com",
|
||||
)
|
||||
with patch("pronote_sync.channels.xmpp.ClientXMPP") as mock_cls:
|
||||
result = get_channel(settings, dry_run=True)
|
||||
assert result is not None
|
||||
assert isinstance(result, Channel)
|
||||
# ClientXMPP ne doit pas être instancié en dry_run
|
||||
assert not mock_cls.called
|
||||
|
||||
|
||||
class TestChannelImportsFromInit:
|
||||
"""Tests pour vérifier que les exports depuis __init__.py fonctionnent."""
|
||||
|
||||
@@ -9,19 +9,17 @@ ou de slixmpp, et vérifient le comportement de l'envoi synchrone selon la déci
|
||||
Conformément à D4, SyncXmppChannel.send() utilise asyncio.run() directement sans
|
||||
créer de nouvelle event loop inutilement. Le comportement est :
|
||||
- Pas de boucle en cours → asyncio.run(channel.send_async(message))
|
||||
- Boucle en cours détectée → thread daemon avec asyncio.run() + join(timeout)
|
||||
- Retourne True en cas de succès, False en cas d'erreur (attrape toute exception)
|
||||
- Aucun secret dans les logs.
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
import asyncio
|
||||
import threading
|
||||
from datetime import date
|
||||
from unittest.mock import AsyncMock, patch
|
||||
from unittest.mock import MagicMock, patch
|
||||
|
||||
import pytest
|
||||
from pydantic import SecretStr
|
||||
|
||||
from pronote_sync.channels.protocol import Channel
|
||||
from pronote_sync.channels.xmpp import SyncXmppChannel, XmppMessage
|
||||
@@ -40,8 +38,6 @@ def xmpp_settings() -> XmppSettings:
|
||||
:return: Instance de XmppSettings avec des valeurs par défaut valides.
|
||||
:rtype: XmppSettings
|
||||
"""
|
||||
from pydantic import SecretStr
|
||||
|
||||
return XmppSettings(
|
||||
enabled=True,
|
||||
jid="bot@example.com",
|
||||
@@ -77,45 +73,46 @@ class TestSyncXmppChannelSend:
|
||||
booléen (True/False) sans lever d'exception.
|
||||
"""
|
||||
|
||||
@patch("pronote_sync.channels.xmpp.asyncio.run")
|
||||
def test_sync_adapter_send_returns_true_on_success(
|
||||
self, xmpp_settings: XmppSettings, xmpp_message: XmppMessage
|
||||
self, mock_asyncio_run: MagicMock, xmpp_settings: XmppSettings, xmpp_message: XmppMessage
|
||||
) -> None:
|
||||
"""Test que send retourne True en cas de succès.
|
||||
|
||||
:param mock_asyncio_run: Mock de asyncio.run
|
||||
:param xmpp_settings: Paramètres XMPP valides.
|
||||
:param xmpp_message: Message XMPP minimal.
|
||||
"""
|
||||
# Mock XmppChannel.send_async pour simuler un succès
|
||||
with patch("pronote_sync.channels.xmpp.XmppChannel") as MockChannel:
|
||||
mock_instance = MockChannel.return_value
|
||||
mock_instance.send_async = AsyncMock(return_value=True)
|
||||
mock_instance.settings = xmpp_settings
|
||||
|
||||
adapter = SyncXmppChannel(xmpp_settings)
|
||||
result = adapter.send(xmpp_message)
|
||||
|
||||
assert result is True
|
||||
mock_asyncio_run.side_effect = lambda coro: coro.close() or True
|
||||
channel = SyncXmppChannel(xmpp_settings)
|
||||
result = channel.send(xmpp_message)
|
||||
assert result is True
|
||||
|
||||
@patch("pronote_sync.channels.xmpp.asyncio.run")
|
||||
def test_sync_adapter_send_returns_false_on_error(
|
||||
self, xmpp_settings: XmppSettings, xmpp_message: XmppMessage
|
||||
self, mock_run: MagicMock, xmpp_settings: XmppSettings, xmpp_message: XmppMessage
|
||||
) -> None:
|
||||
"""Test que send retourne False en cas d'erreur.
|
||||
|
||||
Vérifie que la méthode ne lève pas d'exception non gérée et retourne False.
|
||||
|
||||
:param mock_run: Mock de asyncio.run
|
||||
:param xmpp_settings: Paramètres XMPP valides.
|
||||
:param xmpp_message: Message XMPP minimal.
|
||||
"""
|
||||
# Mock XmppChannel.send_async pour simuler une erreur
|
||||
with patch("pronote_sync.channels.xmpp.XmppChannel") as MockChannel:
|
||||
mock_instance = MockChannel.return_value
|
||||
mock_instance.send_async = AsyncMock(side_effect=RuntimeError("Connexion impossible"))
|
||||
mock_instance.settings = xmpp_settings
|
||||
|
||||
adapter = SyncXmppChannel(xmpp_settings)
|
||||
result = adapter.send(xmpp_message)
|
||||
# Simuler une erreur dans asyncio.run
|
||||
def _run_with_error(coro: object) -> bool:
|
||||
"""Ferme la coroutine non exécutée puis lève l'erreur simulée."""
|
||||
close = getattr(coro, "close", None)
|
||||
if close is not None:
|
||||
close()
|
||||
raise RuntimeError("Connexion impossible")
|
||||
|
||||
assert result is False
|
||||
mock_run.side_effect = _run_with_error
|
||||
channel = SyncXmppChannel(xmpp_settings)
|
||||
result = channel.send(xmpp_message)
|
||||
assert result is False
|
||||
|
||||
def test_sync_adapter_satisfies_channel_protocol(self, xmpp_settings: XmppSettings) -> None:
|
||||
"""Test que SyncXmppChannel satisfait le protocole Channel.
|
||||
@@ -127,155 +124,56 @@ class TestSyncXmppChannelSend:
|
||||
channel = SyncXmppChannel(xmpp_settings)
|
||||
assert isinstance(channel, Channel)
|
||||
|
||||
|
||||
class TestSyncXmppChannelAsyncioRun:
|
||||
"""Tests pour la gestion de l'event loop dans SyncXmppChannel selon D4.
|
||||
|
||||
Ces tests vérifient que SyncXmppChannel utilise asyncio.run() directement
|
||||
sans créer de nouvelle event loop inutilement.
|
||||
"""
|
||||
|
||||
def test_sync_adapter_uses_asyncio_run_when_no_loop(
|
||||
def test_sync_adapter_dry_run_does_not_create_client(
|
||||
self, xmpp_settings: XmppSettings, xmpp_message: XmppMessage
|
||||
) -> None:
|
||||
"""Test que send utilise asyncio.run() quand aucune boucle n'est en cours.
|
||||
|
||||
Vérifie que asyncio.run est appelé avec le coroutine send_async.
|
||||
"""Test que dry_run=True ne crée jamais ClientXMPP.
|
||||
|
||||
:param xmpp_settings: Paramètres XMPP valides.
|
||||
:param xmpp_message: Message XMPP minimal.
|
||||
"""
|
||||
# Mock XmppChannel.send_async pour simuler un succès
|
||||
with patch("pronote_sync.channels.xmpp.XmppChannel") as MockChannel:
|
||||
mock_instance = MockChannel.return_value
|
||||
mock_instance.send_async = AsyncMock(return_value=True)
|
||||
mock_instance.settings = xmpp_settings
|
||||
channel = SyncXmppChannel(xmpp_settings, dry_run=True)
|
||||
with patch("pronote_sync.channels.xmpp.ClientXMPP") as mock_cls:
|
||||
result = channel.send(xmpp_message)
|
||||
assert result is True
|
||||
# ClientXMPP ne doit pas être instancié en dry_run
|
||||
assert not mock_cls.called
|
||||
|
||||
adapter = SyncXmppChannel(xmpp_settings)
|
||||
|
||||
# Patch asyncio.run pour vérifier son appel
|
||||
with patch("pronote_sync.channels.xmpp.asyncio.run") as mock_asyncio_run:
|
||||
mock_asyncio_run.return_value = True
|
||||
|
||||
result = adapter.send(xmpp_message)
|
||||
|
||||
# Vérifier que asyncio.run a été appelé
|
||||
mock_asyncio_run.assert_called_once()
|
||||
# Vérifier que le résultat est True
|
||||
assert result is True
|
||||
|
||||
def test_sync_adapter_send_returns_false_when_asyncio_run_fails(
|
||||
def test_sync_adapter_dry_run_returns_true(
|
||||
self, xmpp_settings: XmppSettings, xmpp_message: XmppMessage
|
||||
) -> None:
|
||||
"""Test que send retourne False quand asyncio.run échoue.
|
||||
|
||||
Vérifie que toute exception dans asyncio.run est capturée et retourne False.
|
||||
"""Test que dry_run=True retourne True sans se connecter.
|
||||
|
||||
:param xmpp_settings: Paramètres XMPP valides.
|
||||
:param xmpp_message: Message XMPP minimal.
|
||||
"""
|
||||
# Mock XmppChannel.send_async pour simuler un succès
|
||||
with patch("pronote_sync.channels.xmpp.XmppChannel") as MockChannel:
|
||||
mock_instance = MockChannel.return_value
|
||||
mock_instance.send_async = AsyncMock(return_value=True)
|
||||
mock_instance.settings = xmpp_settings
|
||||
channel = SyncXmppChannel(xmpp_settings, dry_run=True)
|
||||
result = channel.send(xmpp_message)
|
||||
assert result is True
|
||||
|
||||
adapter = SyncXmppChannel(xmpp_settings)
|
||||
|
||||
# Patch asyncio.run pour lever une exception
|
||||
with patch("pronote_sync.channels.xmpp.asyncio.run") as mock_asyncio_run:
|
||||
mock_asyncio_run.side_effect = RuntimeError("Erreur asyncio")
|
||||
|
||||
result = adapter.send(xmpp_message)
|
||||
|
||||
# Vérifier que le résultat est False
|
||||
assert result is False
|
||||
|
||||
def test_sync_adapter_send_with_running_loop_uses_daemon_thread(
|
||||
self, xmpp_settings: XmppSettings, xmpp_message: XmppMessage
|
||||
@patch("pronote_sync.channels.xmpp.asyncio.run")
|
||||
def test_sync_adapter_never_raises(
|
||||
self, mock_run: MagicMock, xmpp_settings: XmppSettings, xmpp_message: XmppMessage
|
||||
) -> None:
|
||||
"""Test que send bascule dans un thread démon quand une event loop est active.
|
||||
|
||||
Simule un appel depuis une boucle déjà en cours d'exécution : l'envoi
|
||||
passe alors par le thread daemon joint avec un timeout, et retourne
|
||||
True en cas de succès.
|
||||
"""Test que send ne lève jamais d'exception.
|
||||
|
||||
:param mock_run: Mock de asyncio.run
|
||||
:param xmpp_settings: Paramètres XMPP valides.
|
||||
:param xmpp_message: Message XMPP minimal.
|
||||
"""
|
||||
|
||||
async def _inner() -> None:
|
||||
with patch("pronote_sync.channels.xmpp.XmppChannel") as MockChannel:
|
||||
mock_instance = MockChannel.return_value
|
||||
mock_instance.send_async = AsyncMock(return_value=True)
|
||||
mock_instance.settings = xmpp_settings
|
||||
# Simuler une erreur quelconque
|
||||
def _run_with_error(coro: object) -> bool:
|
||||
"""Ferme la coroutine non exécutée puis lève l'erreur simulée."""
|
||||
close = getattr(coro, "close", None)
|
||||
if close is not None:
|
||||
close()
|
||||
raise Exception("Any error")
|
||||
|
||||
adapter = SyncXmppChannel(xmpp_settings)
|
||||
result = adapter.send(xmpp_message)
|
||||
|
||||
assert result is True
|
||||
|
||||
asyncio.run(_inner())
|
||||
|
||||
def test_sync_adapter_send_with_running_loop_error_returns_false(
|
||||
self, xmpp_settings: XmppSettings, xmpp_message: XmppMessage
|
||||
) -> None:
|
||||
"""Test que send retourne False quand le thread démon lève une exception.
|
||||
|
||||
Une exception survenue dans le thread daemon est capturée par la cible
|
||||
du thread puis journalisée sans secret ; send retourne False sans lever.
|
||||
|
||||
:param xmpp_settings: Paramètres XMPP valides.
|
||||
:param xmpp_message: Message XMPP minimal.
|
||||
"""
|
||||
|
||||
async def _inner() -> None:
|
||||
with patch("pronote_sync.channels.xmpp.XmppChannel") as MockChannel:
|
||||
mock_instance = MockChannel.return_value
|
||||
mock_instance.send_async = AsyncMock(
|
||||
side_effect=RuntimeError("Connexion impossible")
|
||||
)
|
||||
mock_instance.settings = xmpp_settings
|
||||
|
||||
adapter = SyncXmppChannel(xmpp_settings)
|
||||
result = adapter.send(xmpp_message)
|
||||
|
||||
assert result is False
|
||||
|
||||
asyncio.run(_inner())
|
||||
|
||||
def test_sync_adapter_send_with_running_loop_timeout_returns_false(
|
||||
self, xmpp_settings: XmppSettings, xmpp_message: XmppMessage
|
||||
) -> None:
|
||||
"""Test que send retourne False quand le thread démon dépasse le timeout.
|
||||
|
||||
Le thread démon reste bloqué au-delà du délai de jointure : send doit
|
||||
journaliser un avertissement et retourner False sans lever.
|
||||
|
||||
:param xmpp_settings: Paramètres XMPP valides.
|
||||
:param xmpp_message: Message XMPP minimal.
|
||||
"""
|
||||
blocker = threading.Event()
|
||||
|
||||
async def _blocking_send(message: XmppMessage) -> bool:
|
||||
while not blocker.is_set():
|
||||
await asyncio.sleep(0.01)
|
||||
return True
|
||||
|
||||
async def _inner() -> None:
|
||||
settings = xmpp_settings.model_copy(update={"timeout": 1})
|
||||
with patch("pronote_sync.channels.xmpp.XmppChannel") as MockChannel:
|
||||
mock_instance = MockChannel.return_value
|
||||
mock_instance.send_async = _blocking_send
|
||||
mock_instance.settings = settings
|
||||
|
||||
adapter = SyncXmppChannel(settings)
|
||||
result = adapter.send(xmpp_message)
|
||||
|
||||
assert result is False
|
||||
|
||||
asyncio.run(_inner())
|
||||
blocker.set()
|
||||
mock_run.side_effect = _run_with_error
|
||||
channel = SyncXmppChannel(xmpp_settings)
|
||||
result = channel.send(xmpp_message)
|
||||
assert result is False
|
||||
|
||||
|
||||
class TestSyncXmppChannelSecurity:
|
||||
@@ -294,34 +192,37 @@ class TestSyncXmppChannelSecurity:
|
||||
:param xmpp_settings: Paramètres XMPP valides.
|
||||
"""
|
||||
# Créer des settings avec sentinelles
|
||||
from pydantic import SecretStr
|
||||
|
||||
settings = XmppSettings(
|
||||
enabled=True,
|
||||
jid=BOT_SENTINEL_JID,
|
||||
password=SecretStr(PASS_SENTINEL_123),
|
||||
host="xmpp.example.com",
|
||||
host="localhost",
|
||||
port=5222,
|
||||
to=RECIPIENT_SENTINEL,
|
||||
use_tls=True,
|
||||
use_tls=False,
|
||||
)
|
||||
|
||||
# Mock XmppChannel.send_async pour lever une erreur
|
||||
with patch("pronote_sync.channels.xmpp.XmppChannel") as MockChannel:
|
||||
mock_instance = MockChannel.return_value
|
||||
mock_instance.send_async = AsyncMock(side_effect=RuntimeError("Connexion impossible"))
|
||||
mock_instance.settings = settings
|
||||
channel = SyncXmppChannel(settings)
|
||||
msg = XmppMessage(
|
||||
target_date=date(2025, 9, 7),
|
||||
synthesis=None,
|
||||
homeworks=(),
|
||||
changes=(),
|
||||
messages=(),
|
||||
external_info=None,
|
||||
)
|
||||
|
||||
channel = SyncXmppChannel(settings)
|
||||
msg = XmppMessage(
|
||||
target_date=date(2025, 9, 7),
|
||||
synthesis=None,
|
||||
homeworks=(),
|
||||
changes=(),
|
||||
messages=(),
|
||||
external_info=None,
|
||||
)
|
||||
# Simuler une erreur dans asyncio.run
|
||||
with patch("pronote_sync.channels.xmpp.asyncio.run") as mock_run:
|
||||
|
||||
# Appeler send qui doit retourner False sans lever d'exception
|
||||
def _run_with_error(coro: object) -> bool:
|
||||
"""Ferme la coroutine non exécutée puis lève l'erreur simulée."""
|
||||
close = getattr(coro, "close", None)
|
||||
if close is not None:
|
||||
close()
|
||||
raise RuntimeError("Connexion impossible")
|
||||
|
||||
mock_run.side_effect = _run_with_error
|
||||
result = channel.send(msg)
|
||||
|
||||
# Vérifier que le résultat est False
|
||||
|
||||
Reference in New Issue
Block a user