spotDL / spotify-downloader

Download your Spotify playlists and songs along with album art and metadata (from YouTube if a match is found).
https://spotdl.readthedocs.io/en/latest/
MIT License
15.55k stars 1.48k forks source link

WIP: Use vcrpy more widely, regenerate broken cassettes #2128

Open hseg opened 3 days ago

hseg commented 3 days ago

[!NOTE] The scope of this PR has creeped since the initial posting. Additions are listed at https://github.com/spotDL/spotify-downloader/pull/2128#issuecomment-2208708687

test_matching: Use vcrpy to avoid being rate-limited by Spotify

Description

Among the network-based tests, test_matching has not been updated to use vcrpy. This makes it hit the Spotify API with every run, making the tests flaky. This patch series enables vcrpy on the test and uploads the cassettes that were generated with it.

Also, update test expectations for some tracks that now have more matches.

Related Issue

Closes: #2127

Motivation and Context

See above.

How Has This Been Tested?

Tests are running on a clean chroot with the following config:

platform linux -- Python 3.12.4, pytest-8.2.2, pluggy-1.5.0
rootdir: /build/spotdl/src/spotdl
configfile: pyproject.toml
plugins: typeguard-4.3.0, subprocess-1.5.0, mock-3.14.0, cov-5.0.0, asyncio-0.23.7, anyio-4.4.0, pyfakefs-5.5.0, recording-0.13.0
asyncio: mode=Mode.AUTO

(Specifically, I'm using Arch Linux's pkgctl to create a systemd-nspawn container with only the minimal dependencies needed to build the AUR package installed.)

Types of Changes

Checklist

Moreover, when I run in a chroot, on the first run I get errors due to attempts to overwrite cassettes (even when running with --recording-mode=none --block-network) pytest-initial-chroot, pytestdebug-initial-chroot, and on subsequent runs I get YT-DLP download error in some cases pytest-later-chroot, pytestdebug-later-chroot. Outside the chroot, I sometimes get cassette-overwriting attempts pytest-cassette-system, pytestdebug-cassette-system, and sometimes only the four YTMusic errors above pytest-ytmusic-system, pytestdebug-ytmusic-system.

I don't know what's wrong with my setup that I'm getting these inconsistent results, but this is as far as my knowledge carries me.

hseg commented 2 days ago

Attempting to ignore the broken tests, I see the following are also not vcr'd:

Besides those, I'm getting vcr.errors.CannotOverwriteExistingCassetteException on tests I haven't touched (eg in tests/utils/test_search.py) and possibly relatedly I'm getting requests.exceptions.ContentDecodingError due to incorrect gzip headers.

There are a couple more errors, but they're in a different vein, and hopefully by the time we get these errors settled the others will resolve themselves.

Logs: pytest-system pytestdebug-system pytest-chroot pytestdebug-chroot

hseg commented 1 day ago

In view of the above, I've added vcrpy invocations to the listed calls that are missing them. I've also regenerated the cassettes that were causing errors.

On my system, this reduces the total number of failed tests to 14:

pytest-chroot.log pytestdebug-chroot.log pytest-system.log pytestdebug-system.log

hseg commented 1 day ago

One cassette down, two to go. Though for some reason I've picked up a test failure in tests/utils/test_ffmpeg.py::test_convert on my system and flakily have picked up another in tests/utils/test_metadata.py::test_embed_metadata in my chroot -- not clear what's going on there.

hseg commented 1 day ago

Turns out tests/utils/test_ffmpeg.py::test_download_ffmpeg makes network requests as well. The cassette it generates is 110M, which is larger than Github's default maximal file size. I've commented it out for now to support users who are testing with --block-network, though this should probably be set to skip with --block-network set.

Moreover, I'm puzzled by the fact we have this function in the first place. If we're so concerned users won't be able to install ffmpeg properly on their machines, shouldn't we just provide binaries with ffmpeg bundled in? (leaving aside the problematic nature of such private dependencies). This might have GPL implications, though -- we'd need to check if it's enough to just attribute and link to the ffmpeg source or if we need anything more extensive.

hseg commented 1 day ago

Yet another function found: