Refactor lyrics tests, do not search for empty metadata #5452
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
snejus
force-pushed
the
lyrics-refactor-tests
branch
4 times, most recently
from
626bf91
to
273e39b
Compare
bal-e
reviewed
Oct 5, 2024
Comment on lines
+47
to
+48
| beetsplug/lyrics.py | ||
| test/plugins/test_lyrics.py |
There was a problem hiding this comment.
I'm not entirely comfortable with this mode of testing-as-needed. These files necessarily reference many other parts of the codebase and it's always possible that they can break tests (even if they're pretty independent of the rest of the codebase right now, that can and probably will change in the future).
There was a problem hiding this comment.
Currently these integration tests only run once a week, and no one finds out about failures 😟
snejus
force-pushed
the
lyrics-refactor-tests
branch
3 times, most recently
from
200cd19
to
9e67a56
Compare
snejus
force-pushed
the
lyrics-refactor-tests
branch
2 times, most recently
from
b4d78da
to
038ca48
Compare
bal-e
reviewed
Oct 12, 2024
Two google sources failed to return the expected output. I looked into each case why parsing failed: - lyrics on musica.com contain <aside> Google Ads - each lyrics line on lacoccinelle.net is wrapped within alternating <em> and <strong> tags Thus remove these tags as part of the HTML cleanup logic.
snejus
force-pushed
the
lyrics-refactor-tests
branch
2 times, most recently
from
2fb6682
to
d02e017
Compare
Create 'helpers.ConfigMixin' which sets up testing configuration. This is helpful for tests (e.g. test_lyrics.py) that only need the configuration and do not require temp dir. (#5102) Refactor lyrics tests to fix the issue global beets config issue. Additionally, add 'integration_test' mark that can be used to mark tests that should only run once a week.
- 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.
The test for GeniusLyrics was heavily patched and no longer provided useful coverage. It has been removed to clean up the test suite.
- Consolidated multiple test cases into parameterized tests for better readability and maintainability. - Simplified assertions by comparing lists of actual and expected artists/titles. - Added `unexpected_empty_artist` marker to handle cases which unexpectedly return an empty artist. This seems to be happen when `artist_sort` field is empty.
Modified `search_pairs` function in `lyrics.py` to: * Firstly strip each of `artist`, `artist_sort` and `title` fields * Only generate alternatives if both `artist` and `title` are not empty * Ensure that `artist_sort` is not empty and not equal to artist (ignoring case) before appending it to the artists Extended tests to cover the changes.
Since at least one Backend requires album` and `duration` arguments (`LRCLib`), the caller (`LyricsPlugin.fetch_item_lyrics`) must always provide them. Since they need to provided, we need to enforce this by defining them as positional arguments. Why is this important? I found that integrated `LRCLib` tests have been passing, but they called `LRCLib.fetch` with values for `artist` and `title` fields only, while the actual functionality *always* provides values for `album` and `duration` fields too. When I adjusted the test to provide values for the missing fields, I found that it failed. This makes sense: Lib `album` and `duration` filters are strict on LRCLib, so I was not surprised the lyrics could not be found. Thus I adjusted `LRCLib` backend implementation to only filter by each of these fields when their values are truthy.
snejus
force-pushed
the
lyrics-refactor-tests
branch
2 times, most recently
from
54f06f9
to
19dff1c
Compare
|
@bal-e added a pair of additional commits as I noticed that LRCLib integrated tests have been testing no-op logic and failed once I configured it correctly. I also added lyrics texts that we're expecting to receive from each of the sources we're testing. These will be helpful to have when changes are made in the backends' scraping logic. |
Add explicit checks for lyrics texts fetched from the tested sources. - Introduced `LyricsPage` class to represent lyrics pages for integrated tests. - Configured expected lyrics for each of the URLs that are being fetched. - Consolidated integrated tests in a new `TestLyricsSources` class. - Mocked Google Search API to return the lyrics page under test.
snejus
force-pushed
the
lyrics-refactor-tests
branch
from
19dff1c
to
e70f61b
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Fixes #2635
Fixes #5133
I realised that #5406 has gotten too big, thus I'm splitting it into several smaller PRs.
This PR refactors lyrics plugin tests and fixes an empty metadata issue in the lyrics logic.
CI
--extras=lyricsto the Poetry install command to include the lyrics plugin dependencies.LYRICS_UPDATEDenvironment variable based on changes detected in the lyrics files.Test setup
ConfigMixinto centralize configuration setup for tests, reducing redundancy. This can be used by tests based onpytest.Lyrics logic
item.title,item.artist, anditem.artist_sortinsearch_pairsfunction._scrape_strip_cruftfunction to remove Google Ads tags and unnecessary HTML tags.Lyrics tests overhaul
pytestfor better isolation and configuration management.LYRICS_UPDATEDis set from the CI) using theon_lyrics_updatemarker.Documentation and Dependencies
requests-mockversion1.12.1topyproject.tomlandpoetry.lockfor mocking HTTP requests in tests.setup.cfgto include a new markeron_lyrics_update.