WIP: Improvements for networkless testing (#2128 revamped)

[hseg]

Cache network responses more widely

Description

The network-based tests are missing saved API responses in some cases.
This makes running them hit the various API endpoints and suffer the resulting
rate-limiting. While #2142 is tracking the long-term fix, there's no reason not
to pick up this low-hanging fruit.

Where this is impossible, mark the tests so they can more easily be skipped if a
dev wants to avoid hitting the network.

Finally, while I'm generating the cassettes, I added pytest-vcr-delete-on-fail
as a dependency. It isn't strictly necessary afterwards, so I will be editing
this PR to remove the relevant commit at the end if so desired.

(Historical note: this PR used to be #2128, but I had made it hard to rebase on
top of master, so I'm reopening it. Plus, given the scope creep on that PR, I
thought a clean slate might make it easier to follow what's going on)

Newly-cached tests:

• tests/console/test_entry_point.py::test_download_song
• tests/console/test_entry_point.py::test_preload_song
• tests/test_init.py::test_download
• tests/test_init.py::test_get_urls
• tests/test_matching.py::test_ytmusic_matching
• tests/utils/test_ffmpeg.py::test_convert
• tests/utils/test_m3u.py::test_create_m3u_content
• tests/utils/test_m3u.py::test_create_m3u_file
• tests/utils/test_metadata.py::test_embed_metadata
• tests/utils/test_search.py::test_parse_query

Newly-marked tests:

• tests/utils/test_ffmpeg.py::test_download_ffmpeg

Related Issue

Closes: #2127
Closes: #2128

Motivation and Context

See above: this should make the testsuite less dependent on the network, making
it more robust both in the face of rate-limiting and in the face of flaky
networks.

How Has This Been Tested?

I am currently testing this in a clean systemd-nspawn container running Arch
Linux with the minimal dependencies needed to build the AUR package.

Screenshots (if appropriate)

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 CODE OF CONDUCT document
• I have added tests to cover my changes
• All new and existing tests passed

[hseg]

These are the remaining TODO items before I'd feel comfortable asking this to be merged:

• 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 EDIT: this seems to have been fixed in master
• 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.
• 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.

To generate more cassettes, we need to run pytest with --record-mode=once. The newly generated cassettes should enable the tests to succeed with --record-mode=none --block-network.

[hseg]

I'm having trouble adapting the guidance from #2142 to using my client_id/client_secret to download the VCRs. @xnetcat, you offered help?

[hseg]

OK, I think I managed -- just needed to insert them into the SpotifyClient.init line in tests/conftest.py. I don't know why it didn't work earlier, perhaps the token was too new.

[xnetcat]

Hey you need help with anything? I have some free time today. Let me know if you need help with something specific, if not I will try to work on the thing you mentioned above. ;)

[hseg]

The one thing I still need help with is figuring out why the yt-dlp invocations aren't frozen by simply using vcrpy and freezegun. Talking to the yt-dlp folks, they suggested this may be because yt-dlp does its own request library dispatch, and recommended building a mock RequestHandler for this, pointing me to yt_dlp.networking and YoutubeDL.urlopen() I haven't yet taken the time to try the suggestion and won't have time for it today, but if you could check it out it would be appreciated. El 2 de septiembre de 2024 16:35:38 GMT+03:00, kuba ***@***.***> escribió:

…

Hey you need help with anything? I have some free time today. Let me know if you need help with something specific, if not I will try to work on the thing you mentioned above. ;) -- Reply to this email directly or view it on GitHub: #2174 (comment) You are receiving this because you authored the thread. Message ID: ***@***.***>

[hseg]

Weirdly enough, the tests now pass. I've changed nothing in the past month, only
rebasing the PR onto master a couple weeks back. I also did end up generating
the cassettes with my own API key, which I accidentally leaked and removed.
But other than that, I don't know what changed on my end to make the tests pass,
so I'd appreciate a reproduction of success before merging.
@xnetcat, mind running the tests?

[hseg]

@xnetcat do you want me to rebase?

[xnetcat]

Tried running the tests, but I got a lot of errors.

Cassettes removed themselves, but didn't regenerate

[hseg]

Weird, but at least confirms situation was too good to be true.
Just to rule out a confounding variable, what's your yt-dlp? I'm on 2024.10.07.r12, revision cba7868

[hseg]

... 🤦 the script I was using to run was only checking tests/test_matching.py. That's what I get for returning to the project a month later, sorry for wasting your time. I'll see what I can do in the coming days.

[hseg]

On my end, with --block-network -m 'not test_config' -m 'not novcr' (I'm skipping test_config due to #2058), am getting in chroot:

FAILED tests/console/test_entry_point.py::test_download_song - SystemExit: 1
FAILED tests/console/test_entry_point.py::test_preload_song - SystemExit: 1
FAILED tests/test_init.py::test_get_urls - vcr.errors.CannotOverwriteExisting...
FAILED tests/test_init.py::test_download - AssertionError: assert None not in...
FAILED tests/utils/test_ffmpeg.py::test_convert - yt_dlp.utils.DownloadError:...
FAILED tests/utils/test_metadata.py::test_embed_metadata[mp3] - yt_dlp.utils....
FAILED tests/utils/test_metadata.py::test_embed_metadata[flac] - yt_dlp.utils...
FAILED tests/utils/test_metadata.py::test_embed_metadata[opus] - yt_dlp.utils...
FAILED tests/utils/test_metadata.py::test_embed_metadata[ogg] - yt_dlp.utils....
FAILED tests/utils/test_metadata.py::test_embed_metadata[m4a] - yt_dlp.utils....

and on system

FAILED tests/console/test_entry_point.py::test_download_song - SystemExit: 1
FAILED tests/console/test_entry_point.py::test_preload_song - SystemExit: 1
FAILED tests/test_init.py::test_get_urls - vcr.errors.CannotOverwriteExistingCassetteException: Can't overwrite existing cassette ('/home/gesh/a...
FAILED tests/utils/test_ffmpeg.py::test_convert - AssertionError: assert (False, {'arg...ffmpeg', ...}) == (True, None)
FAILED tests/utils/test_metadata.py::test_embed_metadata[mp3] - AssertionError: assert (False, {'arg...ffmpeg', ...}) == (True, None)
FAILED tests/utils/test_metadata.py::test_embed_metadata[flac] - AssertionError: assert (False, {'arg...ffmpeg', ...}) == (True, None)
FAILED tests/utils/test_metadata.py::test_embed_metadata[opus] - AssertionError: assert (False, {'arg...ffmpeg', ...}) == (True, None)
FAILED tests/utils/test_metadata.py::test_embed_metadata[ogg] - AssertionError: assert (False, {'arg...ffmpeg', ...}) == (True, None)
FAILED tests/utils/test_metadata.py::test_embed_metadata[m4a] - AssertionError: assert (False, {'arg...ffmpeg', ...}) == (True, None)

[hseg]

OK, the test_metadata failures seem to be because ffmpeg isn't seeing the vcr -- it tries querying rr4---sn-nhpax-ua8d.googlevideo.com, which gives 403 rn (presumably this is a dynamically-generated URL)

[hseg]

oops, forgot to squash first

[hseg]

Of the errors we had:

• Solved tests/utils/test_ffmpeg.py::test_convert and tests/utils/test_metadata.py::test_embed_metadata by manually downloading the files (so that their download would be covered by vcrpy).
• At a guess (am currently testing) that the SystemExit: 1 in tests/console/test_entry_point.py::test_download_song and tests/console/test_entry_point.py::test_preload_song is due to an underlying yt-dlp cache invalidation, wrapped them in freezegun
• tests/test_init.py::test_get_urls is still a little weird to me -- why isn't it using the same API credentials as all the other tests? And why does test_init.py::test_download succeed with its credentials? And why doesn't test_get_urls succeed when given test_init.py::test_download's credentials?

Progress, though!

[hseg]

BTW, I've noticed the testsuites take a long while to run on CI -- ~5h in the latest run. Any idea what's going on with that?

[xnetcat]

BTW, I've noticed the testsuites take a long while to run on CI -- ~5h in the latest run. Any idea what's going on with that?

No idea. I cant even finish our current tests on my machine. They just get stuck forever.

But I think I've investigated it once and it could be caused by Spotify api not returning the response and and raising an exception even with the lowest timeout in the spotipy library

[hseg]

Wait, checking the failures from the latest CI run, I don't understand the purpose of the `test-vcr` workflow. Surely we're not testing the recording capabilities of vcrpy? A step generating new cassettes makes sense as part of release engineering or bugfixing, but surely it doesn't need to be run on every CI run? Incidentally, I notice we're on the deprecated v1 of setup-ffmpeg. Is anything blocking the upgrade? El 14 de octubre de 2024 19:12:19 GMT+03:00, kuba ***@***.***> escribió:

…

> BTW, I've noticed the testsuites take a long while to run on CI -- ~5h in the latest run. Any idea what's going on with that? No idea. I cant even finish our current tests on my machine. They just get stuck forever. But I think I've investigated it once and it could be caused by Spotify api not returning the response and and raising an exception even with the lowest timeout in the spotipy library -- Reply to this email directly or view it on GitHub: #2174 (comment) You are receiving this because you authored the thread. Message ID: ***@***.***>

[xnetcat]

Wait, checking the failures from the latest CI run, I don't understand the purpose of the test-vcr workflow. Surely we're not testing the recording capabilities of vcrpy? A step generating new cassettes makes sense as part of release engineering or bugfixing, but surely it doesn't need to be run on every CI run?

Incidentally, I notice we're on the deprecated v1 of setup-ffmpeg. Is anything blocking the upgrade?

El 14 de octubre de 2024 19:12:19 GMT+03:00, kuba @.***> escribió:

BTW, I've noticed the testsuites take a long while to run on CI -- ~5h in the latest run. Any idea what's going on with that?

No idea. I cant even finish our current tests on my machine. They just get stuck forever.

But I think I've investigated it once and it could be caused by Spotify api not returning the response and and raising an exception even with the lowest timeout in the spotipy library

--

Reply to this email directly or view it on GitHub:

#2174 (comment)

You are receiving this because you authored the thread.

Message ID: @.***>

Some tests are run using the vcr and some are run with real networking to verify that the responses didn't change too drastically and that the underlying code still works.

[hseg]

I've decided to leave tests/utils/test_ffmpeg.py::test_convert out of scope -- it requires developing a vcrpy-backed proxy for yt-dlp, which would be better suited in a separate project anyway. Design sketch for whoever ends up working on this:
yt_dlp.networking.common has a class RequestHandler and a decorator register_rh. Given that the API requires registering a class, I think we'd need it to be dynamically-defined per-test, i.e. something like

def genVCRpyRH(vcr):
    class VCRpyRH(RequestHandler):
        ...
        def _send(req):
            return vcr.play_response(req)

    return VCRpyRH

@pytest.mark.vcr()
def test_convert(vcr, ...)
    rh = genVCRpyRH(vcr)
    register_rh(rh)
    ...
    del _REQUEST_HANDLERS[rh.RH_KEY]

[hseg]

Some tests are run using the vcr and some are run with real networking to verify that the responses didn't change too drastically and that the underlying code still works.

I guess that's fair. Have you given thought to using pytest-timeout to ensure we stay under CI time limits?

[hseg]

(Just rebased for 4.2.9, I'm busy with other stuff right now)
(forgot for a moment to rebase on dev, not master)

[hseg]

Rebased for 4.2.10, still finding me too busy to get this over the finish line.
Will get back to this one of these days.

[Silverarmor]

Hmm. I'm not an expert at tests. If you look at the actions tab, for some reason the tests are taking hours. I tried to start fixing this in #2433 but I'm confused. See also test.yml

I'm not sure why this is set to --disable-recording

spotify-downloader/.github/workflows/tests.yml

Line 80 in dce07ef

uv run pytest -vvv --record-mode=none --disable-recording --ignore tests/providers/lyrics --ignore tests/utils/test_github.py --ignore tests/utils/test_ffmpeg.py --ignore tests/utils/test_metadata.py --ignore tests/test_matching.py --ignore tests/providers/audio/test_youtube.py --ignore tests/console/test_entry_point.py --ignore tests/test_init.py

Any thoughts?

[hseg]

Iiuc, `test-vcr` runs the tests against the recorded VCRs, while `test` regenerates the VCRs. The rationale I heard for this was that this is how we catch shifting APIs. However, I'm not sure that's wise - this will definitely run into spotify's rate limits, it will be heavy on network resources, and I imagine Spotify have _some_ way of querying the current API schema if one really wants to check. Instead, I'd recommend regenerating the VCRs as part of validating the code before release, and potentially as part of PRs making heavy changes to the dependencies of tests. That is, as a dev step, not as a CI step

[Silverarmor]

Cheers thanks.
I agree, we should only test against the VCRs - that's what they're there for. The GitHub actions get called so many times that it is not wise to be making API requests.
Or perhaps, we should configure the CI to only test w/ API queries on pushes to master.

However, I'm still confused at the current state of testing, they seem to complete on the CI really slowly, even though I haven't made any changes to master yet. Not sure if you have any insights on this.

[hseg]

Rebased for 4.4.1, but that's as much time as I have for this right now

[hseg]

Tried regenerating cassettes, but something must still be leaking, since

python -m pytest --record-mode=rewrite
python -m pytest --record-mode=none

still produces failures (especially in test_matching.py).

Not sure what's needed to get this to be reproducible, I've tried using freezegun to remove time sensitivity, and vcr-delete-on-fail to make sure we only have working cassettes, and it still doesn't work.

Last-ditch attempt -- I'm currently regenerating the cassettes with tight control on what pytest plugins are running, using:

PYTEST_DISABLE_PLUGIN_AUTOLOAD=1 python -m pytest --record-mode=rewrite \
    -p pytest_asyncio \
    -p pytest_cov \
    -p pytest_mock \
    -p pytest_subprocess \
    -p pytest_vcr_delete_on_fail \
    -p recording

Hopefully this attempt works.

[hseg]

It seems to have worked, but I'm not holding my breath on this. Here's hoping this reproduces.