perf: servir les statiques par nginx, et dire ce que le bot n a pas livre
PERF-006. Le bloc location /static/ etait commente : 59 Ko de CSS et de JS
passaient par Waitress a chaque page. L activer tel quel aurait ete une
regression : ces URL ne changent jamais, donc un cache de 30 jours sert une
feuille de style vieille d un mois apres chaque deploiement, sans moyen de
l invalider. url_for('static') estampille maintenant chaque URL du mtime du
fichier ; c est ce qui rend le immutable vrai et pas seulement rapide.
Deux pieges nginx consignes dans le fichier : un add_header dans un location
annule tous les add_header herites du server (nosniff disparaissait du
JavaScript), et un statique manquant doit renvoyer 404 plutot que retomber
sur Flask, sinon un deploiement casse se cache derriere une page qui marche.
PERF-005. Les objets utilisateur Discord sont mis en cache. A etre precis
sur le gain : un envoi coute deux appels reseau, resoudre puis envoyer, et
seul le premier est economise — un premier match a vingt joueurs fait
toujours vingt resolutions. Ce qui est gagne l est entre notifications, la
ou le bot ecrit aux memes personnes soir apres soir.
Chaque message dit desormais ce qu il est devenu, avec le destinataire et
la raison. Les trois echecs ne se ressemblent pas et ne se lisent plus
pareil : une boite fermee est definitive et ne se retente pas, une erreur
HTTP est passagere, un identifiant sans proprietaire est un compte a
corriger. Le lot quotidien annonce son propre deficit.
Piege trouve en ecrivant les tests : configure_logging met propagate=False
sur le logger 'app', et le handler de caplog est sur la racine. Les
assertions sur les journaux passaient seules et echouaient dans la suite
complete, ou une application avait deja ete construite — elles lisaient un
journal vide, pas un bot silencieux.
417 tests.
This commit is contained in:
@@ -0,0 +1,272 @@
|
||||
"""Resolving Discord users, and saying what became of each message.
|
||||
|
||||
PERF-005. A direct message costs two sequential API calls — resolve the
|
||||
snowflake, then send — and the first one is identical every time for the
|
||||
same person. The bot writes to the same roster over and over, so resolution
|
||||
is cached across notifications.
|
||||
|
||||
The other half of the constat was visibility: a batch of twenty reminders in
|
||||
which three bounced produced no line saying three had bounced. Failures are
|
||||
now named, per recipient, and the daily batch reports its own shortfall.
|
||||
|
||||
Nothing here talks to Discord. The coroutines are driven with asyncio.run
|
||||
rather than pytest-asyncio, which the project does not depend on.
|
||||
"""
|
||||
|
||||
import asyncio
|
||||
import logging
|
||||
|
||||
import pytest
|
||||
from discord import Forbidden, HTTPException, NotFound
|
||||
|
||||
from app.discord_bot import USER_CACHE_MAX, TeamTryoutsBot
|
||||
|
||||
|
||||
class FakeUser:
|
||||
"""Stands in for a discord.User. Records what it was asked to send."""
|
||||
|
||||
def __init__(self, uid, name=None, raises=None):
|
||||
self.id = uid
|
||||
self.name = name or f'user{uid}'
|
||||
self.raises = raises
|
||||
self.sent = []
|
||||
|
||||
async def send(self, message):
|
||||
if self.raises is not None:
|
||||
raise self.raises
|
||||
self.sent.append(message)
|
||||
return FakeMessage(1000 + len(self.sent))
|
||||
|
||||
|
||||
class FakeMessage:
|
||||
def __init__(self, mid):
|
||||
self.id = mid
|
||||
self.reactions = []
|
||||
|
||||
async def add_reaction(self, emoji):
|
||||
self.reactions.append(emoji)
|
||||
|
||||
|
||||
def _response(status):
|
||||
"""A minimal object with the attributes discord's exceptions read."""
|
||||
|
||||
class _R:
|
||||
def __init__(self):
|
||||
self.status = status
|
||||
self.reason = 'test'
|
||||
|
||||
return _R()
|
||||
|
||||
|
||||
def forbidden():
|
||||
return Forbidden(_response(403), 'Cannot send messages to this user')
|
||||
|
||||
|
||||
def http_error():
|
||||
return HTTPException(_response(500), 'Internal Server Error')
|
||||
|
||||
|
||||
def not_found():
|
||||
return NotFound(_response(404), 'Unknown User')
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def logs(caplog):
|
||||
"""Let the bot's records reach caplog.
|
||||
|
||||
configure_logging sets propagate = False on the 'app' logger so records
|
||||
are not written twice, and caplog's handler sits on the root logger. So
|
||||
these tests passed on their own and failed in the full suite, where some
|
||||
earlier test had already built an application — the assertions were
|
||||
reading an empty log, not a silent bot.
|
||||
|
||||
Restored afterwards. The production setting is right; it is only in the
|
||||
way here.
|
||||
"""
|
||||
package_logger = logging.getLogger('app')
|
||||
previous = package_logger.propagate
|
||||
package_logger.propagate = True
|
||||
caplog.set_level(logging.INFO, logger='app.discord_bot')
|
||||
yield caplog
|
||||
package_logger.propagate = previous
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def bot():
|
||||
"""A bot object with nothing but the state the delivery path needs.
|
||||
|
||||
__new__ rather than the constructor: TeamTryoutsBot.__init__ builds a
|
||||
real discord.py client, which wants an event loop and a token.
|
||||
"""
|
||||
instance = TeamTryoutsBot.__new__(TeamTryoutsBot)
|
||||
instance._user_cache = {}
|
||||
instance.pending_requests = {}
|
||||
instance.fetch_calls = []
|
||||
|
||||
# The library's own cache. Empty by default: the members intent was
|
||||
# dropped in OPS-014, so in production it almost always misses.
|
||||
instance.library_cache = {}
|
||||
instance.get_user = instance.library_cache.get
|
||||
|
||||
async def fetch_user(uid):
|
||||
instance.fetch_calls.append(uid)
|
||||
if uid in instance.fetch_failures:
|
||||
raise instance.fetch_failures[uid]
|
||||
return FakeUser(uid)
|
||||
|
||||
instance.fetch_failures = {}
|
||||
instance.fetch_user = fetch_user
|
||||
return instance
|
||||
|
||||
|
||||
class TestResolution:
|
||||
def test_the_same_recipient_is_fetched_once(self, bot):
|
||||
first = asyncio.run(bot._resolve_user(42))
|
||||
second = asyncio.run(bot._resolve_user(42))
|
||||
|
||||
assert first is second
|
||||
assert bot.fetch_calls == [42], 'the second lookup should have come from the cache'
|
||||
|
||||
def test_distinct_recipients_are_each_fetched(self, bot):
|
||||
"""The cache pays off across notifications, not within one.
|
||||
|
||||
Twenty players in one match are still twenty lookups. Claiming
|
||||
otherwise in the commit message would have been the easy lie.
|
||||
"""
|
||||
for uid in range(10, 20):
|
||||
asyncio.run(bot._resolve_user(uid))
|
||||
|
||||
assert bot.fetch_calls == list(range(10, 20))
|
||||
|
||||
def test_the_library_cache_is_consulted_before_the_api(self, bot):
|
||||
known = FakeUser(7)
|
||||
bot.library_cache[7] = known
|
||||
|
||||
assert asyncio.run(bot._resolve_user(7)) is known
|
||||
assert bot.fetch_calls == []
|
||||
|
||||
def test_a_string_snowflake_resolves(self, bot):
|
||||
"""Every caller reads discord_user_id off a model, where it is text."""
|
||||
asyncio.run(bot._resolve_user('42'))
|
||||
asyncio.run(bot._resolve_user(42))
|
||||
|
||||
assert bot.fetch_calls == [42], 'the string and the int must be the same cache entry'
|
||||
|
||||
def test_a_malformed_id_is_rejected_without_a_call(self, bot):
|
||||
assert asyncio.run(bot._resolve_user('not-a-snowflake')) is None
|
||||
assert asyncio.run(bot._resolve_user(None)) is None
|
||||
assert bot.fetch_calls == []
|
||||
|
||||
def test_the_cache_is_bounded(self, bot):
|
||||
for uid in range(USER_CACHE_MAX + 25):
|
||||
asyncio.run(bot._resolve_user(uid))
|
||||
|
||||
assert len(bot._user_cache) <= USER_CACHE_MAX
|
||||
|
||||
def test_an_unknown_account_is_not_cached(self, bot):
|
||||
"""A snowflake nobody owns must stay retryable.
|
||||
|
||||
Caching the miss would mean that fixing the id on the account has no
|
||||
effect until the process restarts.
|
||||
"""
|
||||
bot.fetch_failures[99] = not_found()
|
||||
|
||||
assert asyncio.run(bot._resolve_user(99)) is None
|
||||
assert asyncio.run(bot._resolve_user(99)) is None
|
||||
assert bot.fetch_calls == [99, 99]
|
||||
|
||||
def test_a_transport_failure_says_which_kind_it_was(self, bot, logs):
|
||||
bot.fetch_failures[99] = not_found()
|
||||
bot.fetch_failures[98] = http_error()
|
||||
|
||||
with logs.at_level(logging.WARNING, logger='app.discord_bot'):
|
||||
asyncio.run(bot._resolve_user(99))
|
||||
asyncio.run(bot._resolve_user(98))
|
||||
|
||||
text = logs.text
|
||||
assert 'does not exist' in text, 'a bad id is an account to fix, and must read that way'
|
||||
assert 'Could not resolve' in text
|
||||
|
||||
|
||||
class TestDelivery:
|
||||
def test_a_delivered_message_is_logged_with_its_recipient(self, bot, logs):
|
||||
with logs.at_level(logging.INFO, logger='app.discord_bot'):
|
||||
sent = asyncio.run(bot._send_dm(5, 'hello', purpose='match reminder', recipient='ana'))
|
||||
|
||||
assert sent is not None
|
||||
assert 'match reminder' in logs.text
|
||||
assert 'ana' in logs.text
|
||||
|
||||
def test_closed_dms_are_named_and_not_confused_with_an_outage(self, bot, logs):
|
||||
bot.library_cache[5] = FakeUser(5, raises=forbidden())
|
||||
|
||||
with logs.at_level(logging.WARNING, logger='app.discord_bot'):
|
||||
sent = asyncio.run(bot._send_dm(5, 'hello', purpose='match reminder', recipient='ana'))
|
||||
|
||||
assert sent is None
|
||||
assert 'ana' in logs.text
|
||||
assert 'Retrying will not help' in logs.text, (
|
||||
'a closed inbox is permanent; reporting it like a transient error '
|
||||
'sends someone chasing an outage that is not there'
|
||||
)
|
||||
|
||||
def test_a_transient_failure_is_an_error_not_a_warning(self, bot, logs):
|
||||
bot.library_cache[5] = FakeUser(5, raises=http_error())
|
||||
|
||||
with logs.at_level(logging.WARNING, logger='app.discord_bot'):
|
||||
sent = asyncio.run(bot._send_dm(5, 'x', purpose='match reminder', recipient='ana'))
|
||||
|
||||
assert sent is None
|
||||
levels = {record.levelno for record in logs.records}
|
||||
assert logging.ERROR in levels
|
||||
|
||||
def test_an_unreachable_recipient_still_produces_a_line(self, bot, logs):
|
||||
bot.fetch_failures[5] = not_found()
|
||||
|
||||
with logs.at_level(logging.WARNING, logger='app.discord_bot'):
|
||||
sent = asyncio.run(bot._send_dm(5, 'x', purpose='tryout reminder', recipient='ana'))
|
||||
|
||||
assert sent is None
|
||||
assert 'not delivered' in logs.text
|
||||
assert 'tryout reminder' in logs.text
|
||||
|
||||
|
||||
class TestReminderOutcomes:
|
||||
"""The daily batch counts what it delivered, so it needs a real answer.
|
||||
|
||||
These three return values feed `delivered += await …` in
|
||||
_send_daily_reminders_impl. A reminder that returned None on both paths
|
||||
would make a wholly failed batch report as a wholly successful one.
|
||||
"""
|
||||
|
||||
class FakePlayer:
|
||||
username = 'ana'
|
||||
discord_user_id = '5'
|
||||
|
||||
class FakeMatch:
|
||||
title = 'Finals'
|
||||
location = 'Arena'
|
||||
|
||||
from datetime import date, time
|
||||
|
||||
date = date(2026, 8, 12)
|
||||
start_time = time(18, 0)
|
||||
end_time = time(20, 0)
|
||||
|
||||
class FakeTryout:
|
||||
title = 'Open tryout'
|
||||
location = 'Arena'
|
||||
|
||||
from datetime import date
|
||||
|
||||
date = date(2026, 8, 12)
|
||||
|
||||
def test_a_delivered_reminder_reports_true(self, bot):
|
||||
assert asyncio.run(bot.send_match_reminder(self.FakePlayer(), self.FakeMatch())) is True
|
||||
assert asyncio.run(bot.send_tryout_reminder(self.FakePlayer(), self.FakeTryout())) is True
|
||||
|
||||
def test_a_bounced_reminder_reports_false(self, bot):
|
||||
bot.library_cache[5] = FakeUser(5, raises=forbidden())
|
||||
|
||||
assert asyncio.run(bot.send_match_reminder(self.FakePlayer(), self.FakeMatch())) is False
|
||||
assert asyncio.run(bot.send_tryout_reminder(self.FakePlayer(), self.FakeTryout())) is False
|
||||
Reference in New Issue
Block a user