From fc49902f3a0c15e9ab32357bbb3971775d7d3145 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C5=A0ar=C5=ABnas=20Nejus?= Date: Wed, 2 Oct 2024 19:25:34 +0100 Subject: [PATCH] Refactor lyrics backend tests to use pytest fixtures - Replaced unittest.mock with pytest fixtures for better test isolation and readability. - Simplified test cases by using parameterized tests. - Added `requests-mock` dependency to `pyproject.toml` and `poetry.lock`. - Removed redundant helper functions and classes. --- .github/workflows/ci.yaml | 4 +- CONTRIBUTING.rst | 2 +- poetry.lock | 21 ++- pyproject.toml | 1 + test/plugins/test_lyrics.py | 310 ++++++++++++++---------------------- 5 files changed, 139 insertions(+), 199 deletions(-) diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index 4e948aab6..cad13598f 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -44,13 +44,13 @@ jobs: - if: ${{ env.IS_MAIN_PYTHON != 'true' }} name: Test without coverage run: | - poetry install --extras=autobpm + poetry install --extras=autobpm --extras=lyrics poe test - if: ${{ env.IS_MAIN_PYTHON == 'true' }} name: Test with coverage run: | - poetry install --extras=autobpm --extras=docs --extras=replaygain --extras=reflink + poetry install --extras=autobpm --extras=lyrics --extras=docs --extras=replaygain --extras=reflink poe docs poe test-with-coverage diff --git a/CONTRIBUTING.rst b/CONTRIBUTING.rst index f44e89b6e..1c82050cb 100644 --- a/CONTRIBUTING.rst +++ b/CONTRIBUTING.rst @@ -382,7 +382,7 @@ to get a basic view on how tests are written. Since we are currently migrating the tests from `unittest`_ to `pytest`_, new tests should be written using `pytest`_. Contributions migrating existing tests are welcome! -External API requests under test should be mocked with `requests_mock`_, +External API requests under test should be mocked with `requests-mock`_, However, we still want to know whether external APIs are up and that they return expected responses, therefore we test them weekly with our `integration test`_ suite. diff --git a/poetry.lock b/poetry.lock index 61413bf4e..8fa603a13 100644 --- a/poetry.lock +++ b/poetry.lock @@ -1,4 +1,4 @@ -# This file is automatically @generated by Poetry 1.8.3 and should not be changed by hand. +# This file is automatically @generated by Poetry 1.8.5 and should not be changed by hand. [[package]] name = "accessible-pygments" @@ -2577,6 +2577,23 @@ urllib3 = ">=1.21.1,<3" socks = ["PySocks (>=1.5.6,!=1.5.7)"] use-chardet-on-py3 = ["chardet (>=3.0.2,<6)"] +[[package]] +name = "requests-mock" +version = "1.12.1" +description = "Mock out responses from the requests package" +optional = false +python-versions = ">=3.5" +files = [ + {file = "requests-mock-1.12.1.tar.gz", hash = "sha256:e9e12e333b525156e82a3c852f22016b9158220d2f47454de9cae8a77d371401"}, + {file = "requests_mock-1.12.1-py2.py3-none-any.whl", hash = "sha256:b1e37054004cdd5e56c84454cc7df12b25f90f382159087f4b6915aaeef39563"}, +] + +[package.dependencies] +requests = ">=2.22,<3" + +[package.extras] +fixture = ["fixtures"] + [[package]] name = "requests-oauthlib" version = "2.0.0" @@ -3285,4 +3302,4 @@ web = ["flask", "flask-cors"] [metadata] lock-version = "2.0" python-versions = ">=3.9,<4" -content-hash = "2edbbe1f3488fb9d3a05e2d60c23d3fd1afaa8154d2a71ffad83b34476ceac78" +content-hash = "d609e83f7ffeefc12e28d627e5646aa5c1a6f5a56d7013bb649a468069550dba" diff --git a/pyproject.toml b/pyproject.toml index 3d91a59b4..d985c54ea 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -92,6 +92,7 @@ python3-discogs-client = ">=2.3.15" py7zr = "*" pyxdg = "*" rarfile = "*" +requests-mock = ">=1.12.1" requests_oauthlib = "*" responses = ">=0.3.0" diff --git a/test/plugins/test_lyrics.py b/test/plugins/test_lyrics.py index 7d89b833f..b29143873 100644 --- a/test/plugins/test_lyrics.py +++ b/test/plugins/test_lyrics.py @@ -16,18 +16,14 @@ import itertools import os -import re import unittest +from functools import partial from unittest.mock import patch import pytest -import requests -from bs4 import BeautifulSoup, SoupStrainer from beets.library import Item -from beets.test import _common from beets.test.helper import PluginMixin -from beets.util import bytestring_path from beetsplug import lyrics PHRASE_BY_TITLE = { @@ -175,33 +171,9 @@ class LyricsPluginTest(unittest.TestCase): assert lyrics._scrape_merge_paragraphs(text) == "one\ntwo\nthree" -def url_to_filename(url): - url = re.sub(r"https?://|www.", "", url) - url = re.sub(r".html", "", url) - fn = "".join(x for x in url if (x.isalnum() or x == "/")) - fn = fn.split("/") - fn = os.path.join( - LYRICS_ROOT_DIR, - bytestring_path(fn[0]), - bytestring_path(fn[-1] + ".txt"), - ) - return fn - - -class MockFetchUrl: - def __init__(self, pathval="fetched_path"): - self.pathval = pathval - self.fetched = None - - def __call__(self, url, filename=None): - self.fetched = url - fn = url_to_filename(url) - with open(fn, encoding="utf8") as f: - content = f.read() - return content - - -LYRICS_ROOT_DIR = os.path.join(_common.RSRC, b"lyrics") +@pytest.fixture(scope="module") +def lyrics_root_dir(pytestconfig: pytest.Config): + return pytestconfig.rootpath / "test" / "rsrc" / "lyrics" class LyricsBackendTest(PluginMixin): @@ -221,6 +193,12 @@ class LyricsBackendTest(PluginMixin): lyrics_plugin = lyrics.LyricsPlugin() return lyrics_plugin.backends[0] + @pytest.fixture + def lyrics_html(self, lyrics_root_dir, file_name): + return (lyrics_root_dir / f"{file_name}.txt").read_text( + encoding="utf-8" + ) + @pytest.mark.integration_test def test_backend_source(self, backend): """Test default backends with a song known to exist in respective @@ -228,21 +206,16 @@ class LyricsBackendTest(PluginMixin): """ title = "Lady Madonna" - res = backend.fetch("The Beatles", title) + lyrics = backend.fetch("The Beatles", title) - assert res - assert PHRASE_BY_TITLE[title] in res.lower() + assert lyrics + assert PHRASE_BY_TITLE[title] in lyrics.lower() class TestGoogleLyrics(LyricsBackendTest): """Test scraping heuristics on a fake html page.""" - source = dict( - url="http://www.example.com", - artist="John Doe", - title="Beets song", - path="/lyrics/beetssong", - ) + TITLE = "Beets song" @pytest.fixture(scope="class") def backend_name(self): @@ -252,6 +225,10 @@ class TestGoogleLyrics(LyricsBackendTest): def plugin_config(self): return {"google_API_key": "test"} + @pytest.fixture(scope="class") + def file_name(self): + return "examplecom/beetssong" + @pytest.mark.integration_test @pytest.mark.parametrize( "title, url", @@ -292,80 +269,55 @@ class TestGoogleLyrics(LyricsBackendTest): assert backend.is_lyrics(result) assert PHRASE_BY_TITLE[title] in result - @patch.object(lyrics.Backend, "fetch_url", MockFetchUrl()) - def test_mocked_source_ok(self, backend): + def test_mocked_source_ok(self, backend, lyrics_html): """Test that lyrics of the mocked page are correctly scraped""" - url = self.source["url"] + self.source["path"] - res = lyrics.scrape_lyrics_from_html(backend.fetch_url(url)) - assert backend.is_lyrics(res), url - assert PHRASE_BY_TITLE[self.source["title"]] in res.lower() + result = lyrics.scrape_lyrics_from_html(lyrics_html).lower() - @patch.object(lyrics.Backend, "fetch_url", MockFetchUrl()) - def test_is_page_candidate_exact_match(self, backend): - """Test matching html page title with song infos -- when song infos are - present in the title. - """ - s = self.source - url = str(s["url"] + s["path"]) - html = backend.fetch_url(url) - soup = BeautifulSoup( - html, "html.parser", parse_only=SoupStrainer("title") + assert result + assert backend.is_lyrics(result) + assert PHRASE_BY_TITLE[self.TITLE] in result + + @pytest.mark.parametrize( + "url_title, artist, should_be_candidate", + [ + ("John Doe - beets song Lyrics", "John Doe", True), + ("example.com | Beats song by John doe", "John Doe", True), + ("example.com | seets bong lyrics by John doe", "John Doe", False), + ("foo", "Sun O)))", False), + ], + ) + def test_is_page_candidate( + self, backend, lyrics_html, url_title, artist, should_be_candidate + ): + result = backend.is_page_candidate( + "http://www.example.com/lyrics/beetssong", + url_title, + self.TITLE, + artist, ) - assert backend.is_page_candidate( - url, soup.title.string, s["title"], s["artist"] - ), url + assert bool(result) == should_be_candidate - def test_is_page_candidate_fuzzy_match(self, backend): - """Test matching html page title with song infos -- when song infos are - not present in the title. - """ - s = self.source - url = s["url"] + s["path"] - url_title = "example.com | Beats song by John doe" - - # very small diffs (typo) are ok eg 'beats' vs 'beets' with same artist - assert backend.is_page_candidate( - url, url_title, s["title"], s["artist"] - ), url - # reject different title - url_title = "example.com | seets bong lyrics by John doe" - assert not backend.is_page_candidate( - url, url_title, s["title"], s["artist"] - ), url - - def test_is_page_candidate_special_chars(self, backend): - """Ensure that `is_page_candidate` doesn't crash when the artist - and such contain special regular expression characters. - """ - # https://github.com/beetbox/beets/issues/1673 - s = self.source - url = s["url"] + s["path"] - url_title = "foo" - - backend.is_page_candidate(url, url_title, s["title"], "Sunn O)))") - - def test_is_lyrics(self, backend): - texts = ["LyricsMania.com - Copyright (c) 2013 - All Rights Reserved"] - texts += [ + @pytest.mark.parametrize( + "lyrics", + [ + "LyricsMania.com - Copyright (c) 2013 - All Rights Reserved", """All material found on this site is property\n - of mywickedsongtext brand""" - ] - for t in texts: - assert not backend.is_lyrics(t) - - def test_slugify(self, backend): - text = "http://site.com/\xe7afe-au_lait(boisson)" - assert backend.slugify(text) == "http://site.com/cafe_au_lait" - - def test_missing_lyrics(self, backend): - lyrics = """ + of mywickedsongtext brand""", + """ Lyricsmania staff is working hard for you to add $TITLE lyrics as soon as they'll be released by $ARTIST, check back soon! In case you have the lyrics to $TITLE and want to send them to us, fill out the following form. -""" +""", + ], + ) + def test_bad_lyrics(self, backend, lyrics): assert not backend.is_lyrics(lyrics) + def test_slugify(self, backend): + text = "http://site.com/\xe7afe-au_lait(boisson)" + assert backend.slugify(text) == "http://site.com/cafe_au_lait" + class TestGeniusLyrics(LyricsBackendTest): @pytest.fixture(scope="class") @@ -377,31 +329,18 @@ class TestGeniusLyrics(LyricsBackendTest): def test_backend_source(self, backend): super().test_backend_source(backend) - def test_no_lyrics_div(self, backend): - """Ensure we don't crash when the scraping the html for a genius page - doesn't contain
- """ - # https://github.com/beetbox/beets/issues/3535 - # expected return value None - url = "https://genius.com/sample" - mock = MockFetchUrl() - assert backend._scrape_lyrics_from_html(mock(url)) is None + @pytest.mark.parametrize( + "file_name, expected_line_count", + [ + ("geniuscom/2pacalleyezonmelyrics", 134), + ("geniuscom/Ttngchinchillalyrics", 29), + ("geniuscom/sample", 0), # see https://github.com/beetbox/beets/issues/3535 + ], + ) # fmt: skip + def test_scrape(self, backend, lyrics_html, expected_line_count): + result = backend._scrape_lyrics_from_html(lyrics_html) or "" - def test_good_lyrics(self, backend): - """Ensure we are able to scrape a page with lyrics""" - url = "https://genius.com/Ttng-chinchilla-lyrics" - mock = MockFetchUrl() - lyrics = backend._scrape_lyrics_from_html(mock(url)) - assert lyrics is not None - assert lyrics.count("\n") == 28 - - def test_good_lyrics_multiple_divs(self, backend): - """Ensure we are able to scrape a page with lyrics""" - url = "https://genius.com/2pac-all-eyez-on-me-lyrics" - mock = MockFetchUrl() - lyrics = backend._scrape_lyrics_from_html(mock(url)) - assert lyrics is not None - assert lyrics.count("\n") == 133 + assert len(result.splitlines()) == expected_line_count @patch.object(lyrics.Genius, "_scrape_lyrics_from_html") @patch.object(lyrics.Backend, "fetch_url", return_value=True) @@ -455,22 +394,18 @@ class TestTekstowoLyrics(LyricsBackendTest): def backend_name(self): return "tekstowo" - def test_good_lyrics(self, backend): - """Ensure we are able to scrape a page with lyrics""" - url = "https://www.tekstowo.pl/piosenka,24kgoldn,city_of_angels_1.html" - mock = MockFetchUrl() - assert backend.extract_lyrics(mock(url)) - - def test_no_lyrics(self, backend): - """Ensure we don't crash when the scraping the html for a Tekstowo page - doesn't contain lyrics - """ - url = ( - "https://www.tekstowo.pl/piosenka,beethoven," - "beethoven_piano_sonata_17_tempest_the_3rd_movement.html" - ) - mock = MockFetchUrl() - assert not backend.extract_lyrics(mock(url)) + @pytest.mark.parametrize( + "file_name, expecting_lyrics", + [ + ("tekstowopl/piosenka24kgoldncityofangels1", True), + ( + "tekstowopl/piosenkabeethovenbeethovenpianosonata17tempestthe3rdmovement", # noqa: E501 + False, + ), + ], + ) + def test_scrape(self, backend, lyrics_html, expecting_lyrics): + assert bool(backend.extract_lyrics(lyrics_html)) == expecting_lyrics class TestLRCLibLyrics(LyricsBackendTest): @@ -479,64 +414,51 @@ class TestLRCLibLyrics(LyricsBackendTest): return "lrclib" @pytest.fixture - def mock_get(self): - with patch("beetsplug.lyrics.requests.get") as mock: - yield mock + def fetch_lyrics(self, backend, requests_mock, response_data): + requests_mock.get(lyrics.LRCLib.base_url, json=response_data) + + return partial(backend.fetch, "la", "la", "la") @pytest.mark.parametrize( - "plugin_config, expected_lyrics_type", + "response_data", [ - ({"synced": True}, "syncedLyrics"), - ({"synced": False}, "plainLyrics"), + { + "syncedLyrics": "[00:00.00] la la la", + "plainLyrics": "la la la", + } ], ) - def test_synced_config_option( - self, backend, mock_get, expected_lyrics_type - ): - mock_response = { - "syncedLyrics": "[00:00.00] la la la", - "plainLyrics": "la la la", - } - mock_get.return_value.json.return_value = mock_response - mock_get.return_value.status_code = 200 - - lyrics = backend.fetch("la", "la", "la", 999) - assert lyrics == mock_response[expected_lyrics_type] + @pytest.mark.parametrize( + "plugin_config, expected_lyrics", + [ + ({"synced": True}, "[00:00.00] la la la"), + ({"synced": False}, "la la la"), + ], + ) + def test_synced_config_option(self, fetch_lyrics, expected_lyrics): + assert fetch_lyrics() == expected_lyrics @pytest.mark.parametrize( - "plugin_config", [({"synced": True}), ({"synced": False})] + "response_data, expected_lyrics", + [ + pytest.param( + {"syncedLyrics": "", "plainLyrics": "la la la"}, + "la la la", + id="pick plain lyrics", + ), + pytest.param( + { + "statusCode": 404, + "error": "Not Found", + "message": "Failed to find specified track", + }, + None, + id="not found", + ), + ], ) - def test_fetch_plain_lyrics(self, backend, mock_get): - mock_response = { - "syncedLyrics": "", - "plainLyrics": "la la la", - } - mock_get.return_value.json.return_value = mock_response - mock_get.return_value.status_code = 200 - - lyrics = backend.fetch("la", "la", "la", 999) - - assert lyrics == mock_response["plainLyrics"] - - def test_fetch_not_found(self, backend, mock_get): - mock_response = { - "statusCode": 404, - "error": "Not Found", - "message": "Failed to find specified track", - } - mock_get.return_value.json.return_value = mock_response - mock_get.return_value.status_code = 404 - - lyrics = backend.fetch("la", "la", "la", 999) - - assert lyrics is None - - def test_fetch_exception(self, backend, mock_get): - mock_get.side_effect = requests.RequestException - - lyrics = backend.fetch("la", "la", "la", 999) - - assert lyrics is None + def test_fetch_lyrics(self, fetch_lyrics, expected_lyrics): + assert fetch_lyrics() == expected_lyrics # test utilities