diff --git a/services/lyrics_api.py b/services/lyrics_api.py index 33b262e..50e4950 100644 --- a/services/lyrics_api.py +++ b/services/lyrics_api.py @@ -15,13 +15,30 @@ log = logging.getLogger("loopify.lyrics") +# Genius is reached from a thread, so a lookup that never returns pins that +# thread — and a pinned executor thread delays the event loop's shutdown. The +# library's own default is 5s; this trades a little patience for the occasional +# slow response while staying well inside the shutdown budget (see #35). +GENIUS_TIMEOUT = 10.0 + @lru_cache(maxsize=1) def _client() -> lyricsgenius.Genius: - """One client for the process — rebuilding it per query buys nothing.""" - genius = lyricsgenius.Genius(GENIUS_TOKEN, quiet=True, skip_non_songs=True) - genius.remove_section_headers = False - return genius + """ + One client for the process — rebuilding it per query buys nothing. + + There is deliberately no verbosity argument. ``quiet`` belonged to + lyricsgenius 2.x and raises ``TypeError`` on 3.x, which is what broke + ``!lyrics`` entirely; 3.x prints nothing of its own, so there is nothing left + to silence. The other two are passed because their behaviour is relied on, + not because the defaults differ. + """ + return lyricsgenius.Genius( + GENIUS_TOKEN, + skip_non_songs=True, # tracklists and credits pages aren't lyrics + remove_section_headers=False, # keep [Chorus] and friends in the embed + timeout=GENIUS_TIMEOUT, + ) async def fetch(title: str, artist: str = "", *, loop=None) -> Optional[dict]: @@ -29,13 +46,22 @@ async def fetch(title: str, artist: str = "", *, loop=None) -> Optional[dict]: Search Genius for lyrics. Returns a dict with ``title``, ``artist``, ``lyrics`` and ``url``, or None - when there is no match or Genius is unreachable — a missing lyric is never - a reason to take the bot down. + when there is no match, no token, or Genius is unreachable — a missing lyric + is never a reason to take the bot down. """ + if not GENIUS_TOKEN: + # Constructing a client without one makes lyricsgenius fall back to + # $GENIUS_ACCESS_TOKEN and raise KeyError. config.validate() already + # warned about this at startup. + log.debug("No GENIUS_TOKEN configured; skipping lookup for %r", title) + return None + loop = loop or asyncio.get_event_loop() def _search(): - return _client().search_song(title, artist) if artist else _client().search_song(title) + client = _client() + # An empty artist matches worse than no artist at all. + return client.search_song(title, artist) if artist else client.search_song(title) try: song = await loop.run_in_executor(None, _search) diff --git a/tests/test_lyrics_api.py b/tests/test_lyrics_api.py new file mode 100644 index 0000000..2360683 --- /dev/null +++ b/tests/test_lyrics_api.py @@ -0,0 +1,170 @@ +""" +The Genius client, built against the lyricsgenius that is actually installed. + +Issue #37: `!lyrics` had never worked with the pinned lyricsgenius. The client +was constructed with `quiet=True`, an argument from the 2.x series, so every +lookup died in the constructor with a TypeError and the command answered +"Couldn't find lyrics" — which reads like a failed search, not a broken build. + +Nothing here talks to Genius. The point is the wiring: that the constructor +accepts what we pass it, that a missing token is handled before it can raise, +and that the call is bounded. +""" + +import threading +from unittest.mock import MagicMock + +import lyricsgenius +import pytest + +from services import lyrics_api + +TOKEN = "not-a-real-token" + + +@pytest.fixture +def token(monkeypatch): + """ + A token present in the module, and a client cache cleared around it. + + The real cached function is held onto rather than looked up again on the way + out: tests that replace ``_client`` outright would otherwise have the + teardown reach for ``cache_clear`` on their own stand-in. + """ + cached = lyrics_api._client + cached.cache_clear() + monkeypatch.setattr(lyrics_api, "GENIUS_TOKEN", TOKEN) + yield TOKEN + cached.cache_clear() + + +def _recording_client(calls: list) -> MagicMock: + """A client whose ``search_song`` records the positional arguments it got.""" + def search_song(*args): + calls.append(args) + return FakeSong() + return MagicMock(search_song=search_song) + + +class FakeSong: + title = "Bohemian Rhapsody" + artist = "Queen" + lyrics = "Is this the real life?" + url = "https://genius.invalid/song" + + +# -- building the client ----------------------------------------------- + +def test_the_client_is_accepted_by_the_installed_lyricsgenius(token): + """ + The regression itself: this raised + ``TypeError: Genius.__init__() got an unexpected keyword argument 'quiet'``. + """ + assert isinstance(lyrics_api._client(), lyricsgenius.Genius) + + +def test_the_client_is_reused(token): + assert lyrics_api._client() is lyrics_api._client() + + +def test_section_headers_are_kept(token): + """`[Chorus]` and friends are wanted in the embed, so they stay.""" + assert lyrics_api._client().remove_section_headers is False + + +def test_non_songs_are_skipped(token): + """Genius indexes tracklists and credits pages; those are not lyrics.""" + assert lyrics_api._client().skip_non_songs is True + + +def test_the_lookup_is_bounded(token): + """ + An unbounded lookup pins an executor thread, and a pinned executor thread + delays the event loop's shutdown — the ceiling that #35 was about. + """ + assert 0 < lyrics_api._client().timeout <= 30 + + +# -- fetching ---------------------------------------------------------- + +async def test_a_missing_token_gives_up_before_building_a_client(monkeypatch): + """ + Without a token lyricsgenius falls back to $GENIUS_ACCESS_TOKEN and raises + KeyError, so the guard has to come first. config.validate() already warns + about this at startup; a user typing !lyrics should just get an answer. + """ + lyrics_api._client.cache_clear() + monkeypatch.setattr(lyrics_api, "GENIUS_TOKEN", None) + monkeypatch.setattr(lyrics_api, "_client", + MagicMock(side_effect=AssertionError("must not be built"))) + + assert await lyrics_api.fetch("Bohemian Rhapsody") is None + + +async def test_a_found_song_becomes_a_result(token, monkeypatch): + monkeypatch.setattr(lyrics_api, "_client", + lambda: MagicMock(search_song=lambda *a, **k: FakeSong())) + + result = await lyrics_api.fetch("Bohemian Rhapsody", "Queen") + + assert result == { + "title": "Bohemian Rhapsody", + "artist": "Queen", + "lyrics": "Is this the real life?", + "url": "https://genius.invalid/song", + } + + +async def test_no_match_is_not_an_error(token, monkeypatch): + monkeypatch.setattr(lyrics_api, "_client", + lambda: MagicMock(search_song=lambda *a, **k: None)) + + assert await lyrics_api.fetch("asdkjhaskdjh") is None + + +async def test_an_unreachable_genius_is_not_an_error(token, monkeypatch): + """A missing lyric is never a reason to take the bot down.""" + def explode(*args, **kwargs): + raise ConnectionError("genius is down") + + monkeypatch.setattr(lyrics_api, "_client", + lambda: MagicMock(search_song=explode)) + + assert await lyrics_api.fetch("Bohemian Rhapsody") is None + + +async def test_the_artist_is_used_when_given(token, monkeypatch): + calls = [] + monkeypatch.setattr(lyrics_api, "_client", lambda: _recording_client(calls)) + + await lyrics_api.fetch("Bohemian Rhapsody", "Queen") + + assert calls == [("Bohemian Rhapsody", "Queen")] + + +async def test_a_bare_title_does_not_pass_an_empty_artist(token, monkeypatch): + """Genius matches worse against an empty artist than against none at all.""" + calls = [] + monkeypatch.setattr(lyrics_api, "_client", lambda: _recording_client(calls)) + + await lyrics_api.fetch("Bohemian Rhapsody") + + assert calls == [("Bohemian Rhapsody",)] + + +async def test_the_search_never_runs_on_the_event_loop(token, monkeypatch): + """ + lyricsgenius is blocking and synchronous. Calling it on the loop would freeze + every other guild's playback for the length of an HTTP round trip. + """ + threads = [] + + def record(*args): + threads.append(threading.get_ident()) + return FakeSong() + + monkeypatch.setattr(lyrics_api, "_client", lambda: MagicMock(search_song=record)) + + await lyrics_api.fetch("Bohemian Rhapsody") + + assert threads and threads[0] != threading.get_ident(), "the blocking call ran on the event loop thread"