-
Notifications
You must be signed in to change notification settings - Fork 64
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
Comments
Hey could you assign this issue to me, i would like to give it a try |
I am getting this error when trying to run after building the script. ❯ spotify_to_tidal
^^^^^^^^^^^^^^^^^^^^^^^^^
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? |
Thanks, gimme a couple of minutes, looks like I accidentally introduced a bug the other day, this is exactly why we need CI! |
Done |
I will check it out, thank you. |
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 |
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 |
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. for unit test however we are good by using mock data. |
There are a few problems with this:
|
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? |
I will start with the unit test and CI integration for now and maybe try the following:-
|
I added basic circle CI config with this PR #59 |
maybe i could perhaps take this up in another PR ? |
@timrae can you check the PR, I have cleaned up the commit history. |
Hey @timrae , I pulled from I will fix it and create another PR, also would you like me to add ruff or flake8 for linting? |
Just make small targeted PR's is best for me |
I have fixed the test here #68, let me know when the tests pass in circle-ci |
Hey @timrae, hopefully the tests are working now in the circle-ci dashboard |
@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 |
@timrae sure. I will write more tests to cover all the unit test cases and hopefully also cover up integration tests. |
Would be great if someone were to setup some basic integration tests with Circle CI or something
The text was updated successfully, but these errors were encountered: