WIP: Use vcrpy more widely, regenerate broken cassettes

[hseg]

Note

The scope of this PR has creeped since the initial posting.
Additions are listed at #2128 (comment)

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

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)

Checklist

• My code follows the code style of this project

• My change requires a change to the documentation

• I have updated the documentation accordingly

• I have read the CONTRIBUTING document

• I have read the CORE VALUES document

  It doesn't exist?

• I have added tests to cover my changes

• All new and existing tests passed

  I am still getting these four failures, but these seem to be a bug in the YTMusic provider code (I suspect the "lookup by ISRC" code is dropping some matches) rather than a problem with the cassettes

FAILED tests/test_matching.py::test_ytmusic_matching[https://open.spotify.com/track/6fyzS9YbkUhmEmJ52s19Ob-expected18] - AttributeError: 'NoneType' object has no attribute 'split'
FAILED tests/test_matching.py::test_ytmusic_matching[https://open.spotify.com/track/4Ga9D6SHCVUNsOLPVSZf9v-expected19] - AttributeError: 'NoneType' object has no attribute 'split'
FAILED tests/test_matching.py::test_ytmusic_matching[https://open.spotify.com/track/2w8C31hIPvJMPD4MDdNTro-expected28] - AttributeError: 'NoneType' object has no attribute 'split'
FAILED tests/test_matching.py::test_ytmusic_matching[https://open.spotify.com/track/1ZEsqzNBQqyC7VLRTUDopj-expected33] - AttributeError: 'NoneType' object has no attribute 'split'

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]

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

• tests/test_init.py::test_get_urls
• tests/utils/test_m3u.py::test_create_m3u_content
• tests/utils/test_m3u.py::test_create_m3u_file
• tests/utils/test_search.py::test_parse_query

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]

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:

• 4 from the broken cases in tests/test_matching.py::test_ytmusic_matching
• 5 due to tests/utils/test_config.py wasn't updated for XDG support #2058 (only on my system, not in chroot, since the chroot isn't configured for XDG)
• 1 due to a failed assertion in tests/utils/test_search.py::test_parse_yt_link (conjencturally, due to a failed network request, with the specific error masked by the assertion failure)
• 2 due to SystemExit: 1 in tests/console/test_entry_point.py (only on my system, not in chroot. No idea why)
• 2 tests for which I haven't yet managed to get a working cassette yet (am retrying once an hour until I'll get working cassettes, will force-update the "Regenerate cassettes" commit once I get the working cassettes)

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

[hseg]

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]

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]

Yet another function found:

• tests/test_init.py::test_download

[hseg]

This is as far as I'm able to take it for now -- work is ramping up and won't be able to keep pushing on this in the near future.

If anyone picks this up, the remaining items that need to be dealt with for this PR are:

• Get valid cassettes for tests/types/test_artist.py::test_artist_from_string and tests/types/test_playlist.py::test_playlist_from_string -- these should be squashed into the Regenerate cassettes commit.
• Diagnose why tests/test_matching.py::test_ytmusic_matching fails to find the matching YTMusic entries for https://open.spotify.com/track/6fyzS9YbkUhmEmJ52s19Ob, https://open.spotify.com/track/4Ga9D6SHCVUNsOLPVSZf9v, https://open.spotify.com/track/2w8C31hIPvJMPD4MDdNTro and https://open.spotify.com/track/1ZEsqzNBQqyC7VLRTUDopj
• Diagnose why (at least in my pkgctl build VM), tests/test_matching.py::test_ytmusic_matching gets YT-DLP download errors on https://open.spotify.com/track/1zi7xx7UVEFkmKfv06H8x0, https://open.spotify.com/track/6nO3tr47nr2P7f3hXb8JIo and https://open.spotify.com/track/4EqPIm07a55pJQRSZw2Z4X. Hypothesis: the youtube responses encode expiry times for their responses, so sufficiently old cassettes will prompt yt-dlp to attempt to refresh the cassette, causing trouble. I guess the VM sets its time consistently (or to a different timezone?) for reproducible builds. Possible workaround: use freezegun to pin the clock as well.
• Github has been sending me warnings about the credentials in the cassettes -- this needs to be reviewed to make sure we're not leaking problematic data, potentially using https://vcrpy.readthedocs.io/en/latest/advanced.html#filter-sensitive-data-from-the-request to strip the credentials from the cassettes.
• Find a better solution for tests/utils/test_ffmpeg.py::test_download_ffmpeg than just commenting it out -- it should at the very least be disabled when running with --block-network, and an issue should be raised around whether we even want this feature moving forward, as opposed to just shipping releases with the ffmpeg binary bundled in. Done, created the novcr mark for those tests we can't vcr
• If desired, drop the pytest-vcr-delete-on-fail commit -- while it's useful for ensuring only successful cassettes are recorded, it's not necessarily a dependency we want to permanently pick up.
• Recapitulate the PR message in view of the changes accreted over time -- the Use vcrpy for other relevant tests commit has the list of additional vcr'd tests, and the stat for the Regenerate cassettes commit has the list of cassettes that were broken enough to regenerate.

The remaining test failures in the logs I've posted are due to #2058 and should be addressed there. The other failures should be addressed by the (newly/re)generated cassettes -- they no longer cause failures either my machine or my chroot with --record-mode=none --block-network.

[xnetcat]

Great work, if you need help with anything let me know. @ me, and I will review this <3

[hseg]

Hadn't created a separate branch for this pr, this made rebasing to the latest master more difficult.
I'm closing this PR and reopening it with a proper branch.
Sorry for the mess.

[hseg]

Not sure what this merge signals, shouldn't the PR have been closed instead? El 2 de septiembre de 2024 21:55:20 GMT+03:00, kuba ***@***.***> escribió:

…

Merged #2128 into dev. -- Reply to this email directly or view it on GitHub: #2128 (comment) You are receiving this because you authored the thread. Message ID: ***@***.***>

[xnetcat]

Not sure what this merge signals, shouldn't the PR have been closed instead? El 2 de septiembre de 2024 21:55:20 GMT+03:00, kuba @.> escribió:
…
Merged #2128 into dev. -- Reply to this email directly or view it on GitHub: #2128 (comment) You are receiving this because you authored the thread. Message ID: @.>

I didn't merge this? Not sure what happened here, there's no trace of your code on the dev/master branch.

[hseg]

Huh, weird. Got an email notification saying you merged it. El 2 de septiembre de 2024 23:04:11 GMT+03:00, kuba ***@***.***> escribió:

…

> Not sure what this merge signals, shouldn't the PR have been closed instead? El 2 de septiembre de 2024 21:55:20 GMT+03:00, kuba ***@***.***> escribió: > […](#) > Merged #2128 into dev. -- Reply to this email directly or view it on GitHub: [#2128 (comment)](#2128 (comment)) You are receiving this because you authored the thread. Message ID: ***@***.***> I didn't merge this? Not sure what happened here, there's no trace of your code on the dev/master branch. -- Reply to this email directly or view it on GitHub: #2128 (comment) You are receiving this because you authored the thread. Message ID: ***@***.***>

[xnetcat]

Huh, weird. Got an email notification saying you merged it. El 2 de septiembre de 2024 23:04:11 GMT+03:00, kuba @.***> escribió:
…

Not sure what this merge signals, shouldn't the PR have been closed instead? El 2 de septiembre de 2024 21:55:20 GMT+03:00, kuba @.> escribió: > … > Merged #2128 into dev. -- Reply to this email directly or view it on GitHub: [#2128 (comment)](#2128 (comment)) You are receiving this because you authored the thread. Message ID: @.> I didn't merge this? Not sure what happened here, there's no trace of your code on the dev/master branch. -- Reply to this email directly or view it on GitHub: #2128 (comment) You are receiving this because you authored the thread. Message ID: @.***>

Yeah, I am also seeing a message that I merged this. Even though I haven't pressed a button. You can consider this PR closed.

Maybe the fact that I merged master into dev, merged this PR automatically? idk

[hseg]

Ah, perhaps I confused it by having this PR be based off my master (which is the entire reason I forked this PR), and then resynced my master to yours. El 2 de septiembre de 2024 23:11:25 GMT+03:00, kuba ***@***.***> escribió:

…

> Huh, weird. Got an email notification saying you merged it. El 2 de septiembre de 2024 23:04:11 GMT+03:00, kuba ***@***.***> escribió: > […](#) > > Not sure what this merge signals, shouldn't the PR have been closed instead? El 2 de septiembre de 2024 21:55:20 GMT+03:00, kuba ***@***.***> escribió: > […](#) > Merged #2128 into dev. -- Reply to this email directly or view it on GitHub: [#2128 (comment)]([#2128 (comment)](#2128 (comment))) You are receiving this because you authored the thread. Message ID: ***@***.***> I didn't merge this? Not sure what happened here, there's no trace of your code on the dev/master branch. -- Reply to this email directly or view it on GitHub: [#2128 (comment)](#2128 (comment)) You are receiving this because you authored the thread. Message ID: ***@***.***> Yeah, I am also seeing a message that I merged this. Even though I haven't pressed a button. You can consider this PR closed. Maybe the fact that I merged master into dev, merged this PR automatically? idk -- Reply to this email directly or view it on GitHub: #2128 (comment) You are receiving this because you authored the thread. Message ID: ***@***.***>