[PR #114] [MERGED] Fix tests #252

New issue

Closed

opened 2026-02-27 20:23:58 +03:00 by kerem · 0 comments

kerem commented

2026-02-27 20:23:58 +03:00

Owner

📋 Pull Request Information

Original PR: https://github.com/ramsayleung/rspotify/pull/114
Author: @marioortizmanero
Created: 8/16/2020
Status: ✅ Merged
Merged: 8/16/2020
Merged by: @ramsayleung

Base: master ← Head: fix-tests

📝 Commits (3)

9859fde Fix tests
02a1bf2 cargo fmt
4197c41 Also implement for blocking module

📊 Changes

2 files changed (+18 additions, -16 deletions)

View changed files

📝 src/blocking/client.rs (+9 -8)
📝 src/client.rs (+9 -8)

📄 Description

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:

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.
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 github.com/ramsayleung/rspotify@b7b7c6f8b9

This should close #95.

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/ramsayleung/rspotify/pull/114 **Author:** [@marioortizmanero](https://github.com/marioortizmanero) **Created:** 8/16/2020 **Status:** ✅ Merged **Merged:** 8/16/2020 **Merged by:** [@ramsayleung](https://github.com/ramsayleung) **Base:** `master` ← **Head:** `fix-tests` --- ### 📝 Commits (3) - [`9859fde`](https://github.com/ramsayleung/rspotify/commit/9859fde4404d9c68d2109be2d34468224b9f6b33) Fix tests - [`02a1bf2`](https://github.com/ramsayleung/rspotify/commit/02a1bf28fed5fdc4b857f4efec9e4411283e8c06) cargo fmt - [`4197c41`](https://github.com/ramsayleung/rspotify/commit/4197c4193ee5ce0acd5c2289db9125524a7804a2) Also implement for blocking module ### 📊 Changes **2 files changed** (+18 additions, -16 deletions) <details> <summary>View changed files</summary> 📝 `src/blocking/client.rs` (+9 -8) 📝 `src/client.rs` (+9 -8) </details> ### 📄 Description 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 https://github.com/ramsayleung/rspotify/commit/b7b7c6f8b959724699eaf656a2b22de881e2e818 This should close #95. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>