Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

WIP: Improvements for networkless testing (#2128 revamped) #2174

Draft
wants to merge 7 commits into
base: dev
Choose a base branch
from
Draft

WIP: Improvements for networkless testing (#2128 revamped) #2174

wants to merge 7 commits into from

Conversation

hseg
Copy link

@hseg hseg commented Aug 27, 2024 •
edited
Loading

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
Copy link
Author

hseg commented Aug 27, 2024 •
edited
Loading

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

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
Copy link
Author

hseg commented Aug 27, 2024

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
Copy link
Author

hseg commented Aug 27, 2024

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.

@hseg hseg force-pushed the fix-vcrs branch 6 times, most recently from 4252057 to 3d8492e Compare August 28, 2024 14:37
@xnetcat
Copy link
Member

xnetcat commented Sep 2, 2024

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
Copy link
Author

hseg commented Sep 2, 2024 via email

@hseg hseg marked this pull request as ready for review October 13, 2024 13:23
@hseg
Copy link
Author

hseg commented Oct 13, 2024

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?

@xnetcat xnetcat changed the base branch from master to dev October 13, 2024 13:33
@hseg
Copy link
Author

hseg commented Oct 13, 2024

@xnetcat do you want me to rebase?

@xnetcat
Copy link
Member

xnetcat commented Oct 13, 2024

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

image

Cassettes removed themselves, but didn't regenerate

@hseg
Copy link
Author

hseg commented Oct 13, 2024

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
Copy link
Author

hseg commented Oct 13, 2024

... 🤦 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
Copy link
Author

hseg commented Oct 13, 2024 •
edited
Loading

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
Copy link
Author

hseg commented Oct 13, 2024

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
Copy link
Author

hseg commented Oct 14, 2024

oops, forgot to squash first

@hseg hseg force-pushed the fix-vcrs branch 3 times, most recently from f83bac0 to 8871741 Compare October 14, 2024 14:25
@hseg
Copy link
Author

hseg commented Oct 14, 2024

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
Copy link
Author

hseg commented Oct 14, 2024

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?

@hseg
Copy link
Author

hseg commented Oct 14, 2024 •
edited
Loading

Hrm. Weirdly enough, on chroot:

FAILED tests/console/test_entry_point.py::test_download_song - assert 'Downlo...
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.networ...
FAILED tests/utils/test_metadata.py::test_embed_metadata[flac] - yt_dlp.netwo...
FAILED tests/utils/test_metadata.py::test_embed_metadata[opus] - yt_dlp.netwo...
FAILED tests/utils/test_metadata.py::test_embed_metadata[ogg] - yt_dlp.networ...
FAILED tests/utils/test_metadata.py::test_embed_metadata[m4a] - yt_dlp.networ...

but on system:

FAILED tests/utils/test_ffmpeg.py::test_convert - vcr.errors.CannotOverwriteExistingCassetteException: Can't overwrite existing cassette ('/home/gesh/a...

@xnetcat
Copy link
Member

xnetcat commented Oct 14, 2024

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
Copy link
Author

hseg commented Oct 15, 2024 via email

@xnetcat
Copy link
Member

xnetcat commented Oct 15, 2024

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
Copy link
Author

hseg commented Oct 15, 2024

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 hseg marked this pull request as draft October 15, 2024 18:58
@hseg
Copy link
Author

hseg commented Oct 15, 2024

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
Copy link
Author

hseg commented Nov 17, 2024 •
edited
Loading

(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
Use vcrpy in more tests
699f27d 
Cache network responses to focus on testing our consumption of the API.
This makes the tests more robust to API rate-limiting and denoises them
from network problems.

- 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
@hseg
Add pytest-freezegun
6ebb628 
Having trouble getting yt-dlp invocations to work nicely with vcrpy,
suspect this is due to it receiving responses that are time-limited

Affected tests:
- tests/console/test_entry_point.py::test_download_song
- tests/console/test_entry_point.py::test_preload_song
- tests/test_init.py::test_get_urls
- tests/test_init.py::test_download
- tests/utils/test_ffmpeg.py::test_convert
- tests/utils/test_metadata.py::test_embed_metadata
@hseg
test_matching: Update expected results
c1863ba
@hseg
Manually download urls passed to ffmpeg
008af93 
This enables them to be picked up by vcrpy
@hseg
Mark unvcrable tests that still need network
8c8abf8 
This is necessary to allow testing with --block-network -- just pass -m
'not novcr' as well to disable these tests.

Tests affected:

- tests/utils/test_ffmpeg.py::test_download_ffmpeg

  However, I doubt whether this function is even necessary -- shouldn't
  we be providing binaries with ffmpeg bundled instead for those users
  who can't install it conveniently on their machines?
@hseg
Add pytest-vcr-delete-on-fail
c929126 
To smooth out cassette regeneration workflow -- am constantly getting
HTTP 401/429 responses, which is masking the cause of test failures.
@hseg
Regenerate cassettes
ca28b62 
Some of these were forgotten -- marked, but never generated.
Some would raise errors, indicating cached broken responses.
Some of these needed to be regenerated so that the timestamp given to
freezegun would be withing tolerable distance of their simulated request
times -- given we're using a single freeze time for the entire project,
all externally-facing cassettes (in our case, facing yt-dlp) need to be
regenerated on each freezegun shift.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

test_matching: Use vcrpy
2 participants
@hseg @xnetcat