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

Fix tests #114

Merged
merged 3 commits into from
Aug 16, 2020
Merged

Fix tests #114

merged 3 commits into from
Aug 16, 2020

Conversation

marioortizmanero
Copy link
Collaborator

@marioortizmanero marioortizmanero commented Aug 16, 2020

I think the issue with the tests is that Rspotify had a bug for multithreaded requests, which is needed for Rust tests. The reqwest client (CLIENT) was a global with no thread safety at all (for some reason Rust didn't detect that). Thus, there are two possible solutions:

  1. Keep the global client, but wrap it with an Arc<Mutex<T>> so that it's thread-safe. But global state is usually a bad idea IMO.
  2. Move the client inside the Spotify struct itself. This means that it can no longer derive Serialize and Deserialize, but I don't see why these were needed anyway, and it makes sense to me to have a client for each Spotify object.

By the way, I would like to know why Serialize and Deserialize are needed for structs such as Spotify and ApiError. Shouldn't these be cleaned up? Okay I've just seen this commit b7b7c6f

This should close #95.

@ramsayleung
Copy link
Owner

Awesome! merged :)

@ramsayleung ramsayleung merged commit c1f2e1d into master Aug 16, 2020
@ramsayleung ramsayleung deleted the fix-tests branch August 16, 2020 13:46
@ramsayleung
Copy link
Owner

ramsayleung commented Aug 16, 2020

Learned from this case, I think we could just avoid Singleton, and make tokio::Runtime as a field of Struct Spotify ?

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