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

Continuous integration #50

Open
timrae opened this issue Jun 2, 2024 · 20 comments
Open

Continuous integration #50

timrae opened this issue Jun 2, 2024 · 20 comments
Assignees
Labels
enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed

Comments

@timrae
Copy link
Collaborator

timrae commented Jun 2, 2024

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

@timrae timrae added enhancement New feature or request help wanted Extra attention is needed good first issue Good for newcomers labels Jun 2, 2024
@xerexcoded
Copy link
Contributor

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

@xerexcoded
Copy link
Contributor

xerexcoded commented Jun 3, 2024

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

timrae commented Jun 3, 2024

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

@timrae
Copy link
Collaborator Author

timrae commented Jun 3, 2024

Done

@xerexcoded
Copy link
Contributor

I will check it out, thank you.

@xerexcoded
Copy link
Contributor

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:

  • Write basic unit tests for all the files.
  • Write integration tests to verify the synchronization flow.
  • Use pytest as the testing module and pytest-mock for mocking dependencies.

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

Looking forward to your input.

Best regards

@timrae
Copy link
Collaborator Author

timrae commented Jun 5, 2024

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

xerexcoded commented Jun 6, 2024

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

timrae commented Jun 6, 2024

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

c0ball commented Jun 6, 2024

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

xerexcoded commented Jun 6, 2024

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

I added basic circle CI config with this PR #59

@xerexcoded
Copy link
Contributor

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

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

@xerexcoded
Copy link
Contributor

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 54526a0 , 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
Copy link
Collaborator Author

timrae commented Jun 10, 2024

Just make small targeted PR's is best for me

@xerexcoded
Copy link
Contributor

I have fixed the test here #68, let me know when the tests pass in circle-ci

@xerexcoded
Copy link
Contributor

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

@timrae
Copy link
Collaborator Author

timrae commented Jun 11, 2024

@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
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

3 participants