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.

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

[WIP] Monkeypatch tests #448

Merged
merged 9 commits into from
Dec 26, 2018
Merged

[WIP] Monkeypatch tests #448

merged 9 commits into from
Dec 26, 2018

Conversation

ritiek
Copy link
Member

@ritiek ritiek commented Dec 19, 2018

This PR mainly monkeypatches some of our tests, especially the ones that search on YouTube (these have been failing very frequently).

Changes:

  • test_with_metadata.py now makes tests on this track https://open.spotify.com/track/3SipFlNddvL0XNZRLXvdZD (Janji - Heroes). I've saved the HTML content from this track's YouTube's search results to this Gist. It would fetch the raw content of this gist in the tests test_with_metadata.{test_youtube_url, test_youtube_title}. Yipee!!

  • Also monkeypatched Pafy.download in our tests, so we don't eat heavy network resources. Instead, this call now either generates 1 sec blank audio (using FFmpeg) or does nothing depending on whether we need to perform further operations (like embedding metadata) on the track.

  • The function convert.song now also returns the FFmpeg command which was invoked to conversion.

  • youtube_tools.generate_m3u now returns a list of YouTube video URLs it wrote in the .m3u file.

  • Some minor grammatical changes in variable and function names.

  • Moved test cases from test_list.py to test_spotify_tools.py and test_youtube_tools.py as appropriate.

  • Change FFmpeg to use the built-in encoder aac instead of 3rd party libfdk-aac which does not ship with the apt package (we might as well just be able to sudo apt install ffmpeg in .travis.yml!).

However, there are a couple of more ideas I have for this PR like skipping FFmpeg tests if the user doesn't have it installed and if anything else interesting comes to me, so let's wait a bit before merging this.

EDIT: Let's just keep it mandatory to install FFmpeg instead of skipping tests. Skipping tests = no good.

@ritiek
Copy link
Member Author

ritiek commented Dec 19, 2018

I don't know what else we might want to monkeypatch for now. The tests seem to be passing on Travis with this PR! In case the other tests fail in the future (the ones not monkeypatched) due to dynamic web content, we could probably monkeypatch them at that time in the future.

@ritiek
Copy link
Member Author

ritiek commented Dec 19, 2018

Note to self: Fix these warnings showing up on Travis:

test/test_with_metadata.py::test_youtube_url
  /home/travis/build/ritiek/spotify-downloader/spotdl/youtube_tools.py:158: RemovedInPytest4Warning: Fixture "monkeypatch_youtube_search_page" called directly. Fixtures are not meant to be called directly, are created automatically when test functions request them as parameters. See https://docs.pytest.org/en/latest/fixture.html for more information.
    url = url_fetch.scrape()
test/test_with_metadata.py::test_youtube_title
  /home/travis/build/ritiek/spotify-downloader/spotdl/youtube_tools.py:158: RemovedInPytest4Warning: Fixture "monkeypatch_youtube_search_page" called directly. Fixtures are not meant to be called directly, are created automatically when test functions request them as parameters. See https://docs.pytest.org/en/latest/fixture.html for more information.
    url = url_fetch.scrape()

EDIT: Done!

@ritiek
Copy link
Member Author

ritiek commented Dec 22, 2018

The code should be ready for review (hella lot of changes I know :). I'll update CHANGES.md soon.

Copy link
Contributor

@linusg linusg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome! Really lots of changes, so I may have missed something. But except for some indentations black would handle differently, this LGTM 👍

@linusg
Copy link
Contributor

linusg commented Dec 26, 2018

Also, fetching the HTML from a Gist is pretty neat! 😄

@ritiek
Copy link
Member Author

ritiek commented Dec 26, 2018

Thanks for the review @linusg! I'll merge this. The master branch certainly needs some passing builds! :D

@ritiek ritiek merged commit 51b01fc into master Dec 26, 2018
@ritiek ritiek deleted the monkeypatch-tests branch December 26, 2018 11:46
@ritiek ritiek added this to the v1.1.1 milestone Dec 26, 2018
@linusg
Copy link
Contributor

linusg commented Dec 26, 2018

The master branch certainly needs some passing builds! :D

Yes indeed!

https://travis-ci.org/ritiek/spotify-downloader/builds/472316484

@ritiek ritiek mentioned this pull request Jan 15, 2019
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants