spotify2tidal / spotify_to_tidal

A command line tool for importing your Spotify playlists into Tidal
GNU Affero General Public License v3.0
346 stars 61 forks source link

Continuous integration #50

Open timrae opened 6 months ago

timrae commented 6 months ago

Would be great if someone were to setup some basic integration tests with Circle CI or something

xerexcoded commented 5 months ago

Hey could you assign this issue to me, i would like to give it a try

xerexcoded commented 5 months ago

I am getting this error when trying to run after building the script.

❯ spotify_to_tidal

Traceback (most recent call last): File "", line 198, in _run_module_as_main File "", line 88, in _run_code File "C:\Users\arnav\AppData\Local\Packages\PythonSoftwareFoundation.Python.3.12_qbz5n2kfra8p0\LocalCache\local-packages\Python312\Scripts\spotify_to_tidal.exe__main.py", line 4, in File "C:\Users\arnav\OSS contri\spotify_to_tidal\src\spotify_to_tidal\main__.py", line 5, in from . import sync as _sync File "C:\Users\arnav\OSS contri\spotify_to_tidal\src\spotify_to_tidal\sync.py", line 157, in async def get_tracks_from_spotify_playlist(spotify_session: spotipy.Spotify, spotify_playlist: t_spotify.SpotifyPlaylist):

^^^^^^^^^^^^^^^^^^^^^^^^^
AttributeError: module 'spotify_to_tidal.type.spotify' has no attribute 'SpotifyPlaylist'. Did you mean: 'SpotifyArtist'?

am i using the CLI wrong or is it something else?

timrae commented 5 months ago

Thanks, gimme a couple of minutes, looks like I accidentally introduced a bug the other day, this is exactly why we need CI!

timrae commented 5 months ago

Done

xerexcoded commented 5 months ago

I will check it out, thank you.

xerexcoded commented 5 months ago

Hello,

I apologize for the delayed response; I was unwell.

I would like to outline my thought process for your feedback.

I plan to take the following steps:

After this, I will integrate the tests to work with CircleCI.

Looking forward to your input.

Best regards

timrae commented 5 months ago

Sounds great. How would the integration tests work though? I guess we'd need to have test accounts for Spotify and Tidal in order to authorise

xerexcoded commented 5 months ago

yes that is correct we would require accounts from both spotify and tidal to test the synchronisation flow.

In my country tidal is not available. Could you create dummy accounts maybe which we can use for the tests.

for unit test however we are good by using mock data.

timrae commented 5 months ago

There are a few problems with this:

  1. Currently the authorization flow requires user intervention to authorize the app via the browser, so it's not suitable for a CI
  2. While Spotify has a free version which we could perhaps use for integration testing, Tidal currently does not. It may or may not be possible to modify playlists without an active subscription, this is something we could try I guess, but we still have problem 1.
c0ball commented 5 months ago

Wouldn't this also be a good time to establish a style guide? So to add a stage with a style guide enforce like flake8 or ruff?

xerexcoded commented 5 months ago

I will start with the unit test and CI integration for now and maybe try the following:-

  1. Automate Authorization Flow:

    • Use OAuth 2.0 Authorization Code Flow with PKCE for Spotify.
    • Pre-authorize the app and use a refresh token to automate token renewal in CI environments.
  2. Mock API for Tidal:

    • Create a mock server to simulate Tidal's API responses.
    • Use tools like responses to mock API endpoints for integration testing.
xerexcoded commented 5 months ago

I added basic circle CI config with this PR https://github.com/spotify2tidal/spotify_to_tidal/pull/59

xerexcoded commented 5 months ago

Wouldn't this also be a good time to establish a style guide? So to add a stage with a style guide enforce like flake8 or ruff?

maybe i could perhaps take this up in another PR ?

xerexcoded commented 5 months ago

@timrae can you check the PR, I have cleaned up the commit history.

xerexcoded commented 5 months ago

Hey @timrae , I pulled from upstream, one test method was failing test_auth.py, because my test methods did not cover the user-library-read scope. As this scope was added in https://github.com/spotify2tidal/spotify_to_tidal/commit/54526a0306d463c7c4b0c060b7b5504a17d8b682 , it was added after I forked.

I will fix it and create another PR, also would you like me to add ruff or flake8 for linting?

timrae commented 5 months ago

Just make small targeted PR's is best for me

xerexcoded commented 5 months ago

I have fixed the test here https://github.com/spotify2tidal/spotify_to_tidal/pull/68, let me know when the tests pass in circle-ci

xerexcoded commented 5 months ago

Hey @timrae, hopefully the tests are working now in the circle-ci dashboard

timrae commented 5 months ago

@xerexcoded yes thank you, there is a green tick on the commit in GitHub also. It would be great to have some more unit tests if you still feel like working on this. Especially the main sync playlist function, and coming up with various test cases for the matching algorithms

xerexcoded commented 5 months ago

@timrae sure. I will write more tests to cover all the unit test cases and hopefully also cover up integration tests.