From 9026ec591d25d58346d51706d487392372cd2374 Mon Sep 17 00:00:00 2001 From: Nick Steel Date: Sun, 22 May 2022 23:45:27 +0100 Subject: [PATCH 01/21] Remove pyspotify: playlists and tracks --- mopidy_spotify/backend.py | 131 +--------- mopidy_spotify/browse.py | 6 +- mopidy_spotify/library.py | 2 +- mopidy_spotify/lookup.py | 35 +-- mopidy_spotify/playback.py | 266 -------------------- mopidy_spotify/playlists.py | 21 +- mopidy_spotify/web.py | 16 ++ tests/conftest.py | 1 - tests/test_backend.py | 265 ++------------------ tests/test_playback.py | 474 +----------------------------------- tests/test_playlists.py | 79 +----- 11 files changed, 85 insertions(+), 1211 deletions(-) delete mode 100644 mopidy_spotify/playback.py diff --git a/mopidy_spotify/backend.py b/mopidy_spotify/backend.py index 7cefb489..59a6a467 100644 --- a/mopidy_spotify/backend.py +++ b/mopidy_spotify/backend.py @@ -6,39 +6,24 @@ from mopidy import backend, httpclient import spotify -from mopidy_spotify import Extension, library, playback, playlists, web +from mopidy_spotify import Extension, library, playlists, web logger = logging.getLogger(__name__) -BITRATES = { - 96: spotify.Bitrate.BITRATE_96k, - 160: spotify.Bitrate.BITRATE_160k, - 320: spotify.Bitrate.BITRATE_320k, -} - - class SpotifyBackend(pykka.ThreadingActor, backend.Backend): - _logged_in = threading.Event() - _logged_out = threading.Event() - _logged_out.set() - def __init__(self, config, audio): super().__init__() self._config = config self._audio = audio self._actor_proxy = None - self._session = None - self._event_loop = None - self._bitrate = None + self._bitrate = config["spotify"]["bitrate"] self._web_client = None self.library = library.SpotifyLibraryProvider(backend=self) - self.playback = playback.SpotifyPlaybackProvider( - audio=audio, backend=self - ) + self.playback = SpotifyPlaybackProvider(audio=audio, backend=self) if config["spotify"]["allow_playlists"]: self.playlists = playlists.SpotifyPlaylistsProvider(backend=self) else: @@ -47,15 +32,6 @@ def __init__(self, config, audio): def on_start(self): self._actor_proxy = self.actor_ref.proxy() - self._session = self._get_session(self._config) - - self._event_loop = spotify.EventLoop(self._session) - self._event_loop.start() - - self._session.login( - self._config["spotify"]["username"], - self._config["spotify"]["password"], - ) self._web_client = web.SpotifyOAuthClient( client_id=self._config["spotify"]["client_id"], @@ -67,99 +43,8 @@ def on_start(self): if self.playlists is not None: self.playlists.refresh() - def on_stop(self): - logger.debug("Logging out of Spotify") - self._session.logout() - self._logged_out.wait() - self._event_loop.stop() - - def _get_session(self, config): - session = spotify.Session(self._get_spotify_config(config)) - - session.connection.allow_network = config["spotify"]["allow_network"] - - self._bitrate = config["spotify"]["bitrate"] - session.preferred_bitrate = BITRATES[self._bitrate] - session.volume_normalization = config["spotify"]["volume_normalization"] - - backend_actor_proxy = self._actor_proxy - session.on( - spotify.SessionEvent.CONNECTION_STATE_UPDATED, - on_connection_state_changed, - self._logged_in, - self._logged_out, - backend_actor_proxy, - ) - session.on( - spotify.SessionEvent.PLAY_TOKEN_LOST, - on_play_token_lost, - backend_actor_proxy, - ) - - return session - - def _get_spotify_config(self, config): - ext = Extension() - spotify_config = spotify.Config() - - spotify_config.load_application_key_file( - pathlib.Path(__file__).parent / "spotify_appkey.key" - ) - - if config["spotify"]["allow_cache"]: - spotify_config.cache_location = bytes(ext.get_cache_dir(config)) - else: - spotify_config.cache_location = None - - spotify_config.settings_location = bytes(ext.get_data_dir(config)) - - proxy_uri = httpclient.format_proxy(config["proxy"], auth=False) - if proxy_uri is not None: - logger.debug(f"Connecting to Spotify through proxy: {proxy_uri}") - - spotify_config.proxy = proxy_uri - spotify_config.proxy_username = config["proxy"].get("username") - spotify_config.proxy_password = config["proxy"].get("password") - - return spotify_config - - def on_logged_in(self): - if self._config["spotify"]["private_session"]: - logger.info("Spotify private session activated") - self._session.social.private_session = True - - def on_play_token_lost(self): - if self._session.player.state == spotify.PlayerState.PLAYING: - self.playback.pause() - logger.warning( - "Spotify has been paused because your account is " - "being used somewhere else." - ) - - -def on_connection_state_changed( - session, logged_in_event, logged_out_event, backend -): - - # Called from the pyspotify event loop, and not in an actor context. - if session.connection.state is spotify.ConnectionState.LOGGED_OUT: - logger.debug("Logged out of Spotify") - logged_in_event.clear() - logged_out_event.set() - elif session.connection.state is spotify.ConnectionState.LOGGED_IN: - logger.info("Logged in to Spotify in online mode") - logged_in_event.set() - logged_out_event.clear() - backend.on_logged_in() - elif session.connection.state is spotify.ConnectionState.DISCONNECTED: - logger.info("Disconnected from Spotify") - elif session.connection.state is spotify.ConnectionState.OFFLINE: - logger.info("Logged in to Spotify in offline mode") - logged_in_event.set() - logged_out_event.clear() - - -def on_play_token_lost(session, backend): - # Called from the pyspotify event loop, and not in an actor context. - logger.debug("Spotify play token lost") - backend.on_play_token_lost() +class SpotifyPlaybackProvider(backend.PlaybackProvider): + def translate_uri(self, uri): + username = self.backend._config["spotify"]["username"] + password = self.backend._config["spotify"]["password"] + return f"{uri}?username={username}&password={password}" diff --git a/mopidy_spotify/browse.py b/mopidy_spotify/browse.py index 749360d7..d8f31690 100644 --- a/mopidy_spotify/browse.py +++ b/mopidy_spotify/browse.py @@ -57,7 +57,7 @@ def browse(*, config, session, web_client, uri): elif uri == _PLAYLISTS_DIR.uri: return _PLAYLISTS_DIR_CONTENTS elif uri.startswith("spotify:user:") or uri.startswith("spotify:playlist:"): - return _browse_playlist(session, web_client, uri, config) + return _browse_playlist(web_client, uri, config) elif uri.startswith("spotify:album:"): return _browse_album(session, uri, config) elif uri.startswith("spotify:artist:"): @@ -88,9 +88,9 @@ def browse(*, config, session, web_client, uri): return [] -def _browse_playlist(session, web_client, uri, config): +def _browse_playlist(web_client, uri, config): return playlists.playlist_lookup( - session, web_client, uri, config["bitrate"], as_items=True + web_client, uri, config["bitrate"], as_items=True ) diff --git a/mopidy_spotify/library.py b/mopidy_spotify/library.py index d62e3da2..b08ff0c3 100644 --- a/mopidy_spotify/library.py +++ b/mopidy_spotify/library.py @@ -17,7 +17,7 @@ def __init__(self, backend): def browse(self, uri): return browse.browse( config=self._config, - session=self._backend._session, + session=None, # TODO web_client=self._backend._web_client, uri=uri, ) diff --git a/mopidy_spotify/lookup.py b/mopidy_spotify/lookup.py index c36f0159..085d2e18 100644 --- a/mopidy_spotify/lookup.py +++ b/mopidy_spotify/lookup.py @@ -12,29 +12,30 @@ def lookup(config, session, web_client, uri): + if web_client is None or not web_client.logged_in: + return [] + try: web_link = WebLink.from_uri(uri) - if web_link.type not in (LinkType.PLAYLIST, LinkType.YOUR): - sp_link = session.get_link(uri) except ValueError as exc: logger.info(f"Failed to lookup {uri!r}: {exc}") return [] try: if web_link.type == LinkType.PLAYLIST: - return _lookup_playlist(config, session, web_client, uri) + return _lookup_playlist(config, web_client, uri) elif web_link.type == LinkType.YOUR: return list(_lookup_your(config, session, web_client, uri)) - elif sp_link.type is spotify.LinkType.TRACK: - return list(_lookup_track(config, sp_link)) - elif sp_link.type is spotify.LinkType.ALBUM: + elif web_link.type == LinkType.TRACK: + return list(_lookup_track(config, web_client, uri)) + elif web_link.type == LinkType.ALBUM: return list(_lookup_album(config, sp_link)) - elif sp_link.type is spotify.LinkType.ARTIST: + elif web_link.type == LinkType.ARTIST: with utils.time_logger("Artist lookup"): return list(_lookup_artist(config, sp_link)) else: logger.info( - f"Failed to lookup {uri!r}: Cannot handle {sp_link.type!r}" + f"Failed to lookup {uri!r}: Cannot handle {web_link.type!r}" ) return [] except spotify.Error as exc: @@ -42,10 +43,14 @@ def lookup(config, session, web_client, uri): return [] -def _lookup_track(config, sp_link): - sp_track = sp_link.as_track() - sp_track.load(config["timeout"]) - track = translator.to_track(sp_track, bitrate=config["bitrate"]) +def _lookup_track(config, web_client, uri): + web_track = web_client.get_track(uri) + + if web_track == {}: + logger.error(f"Failed to lookup Spotify track URI {uri!r}") + return + + track = translator.web_to_track(web_track, bitrate=config["bitrate"]) if track is not None: yield track @@ -88,10 +93,8 @@ def _lookup_artist(config, sp_link): yield track -def _lookup_playlist(config, session, web_client, uri): - playlist = playlists.playlist_lookup( - session, web_client, uri, config["bitrate"] - ) +def _lookup_playlist(config, web_client, uri): + playlist = playlists.playlist_lookup(web_client, uri, config["bitrate"]) if playlist is None: raise spotify.Error("Playlist Web API lookup failed") return playlist.tracks diff --git a/mopidy_spotify/playback.py b/mopidy_spotify/playback.py deleted file mode 100644 index 3161d0f4..00000000 --- a/mopidy_spotify/playback.py +++ /dev/null @@ -1,266 +0,0 @@ -import functools -import collections -import logging -import threading - -from mopidy import audio, backend - -import spotify - -logger = logging.getLogger(__name__) - - -# These GStreamer caps matches the audio data provided by libspotify -GST_CAPS = "audio/x-raw,format=S16LE,rate=44100,channels=2,layout=interleaved" - -# Extra log level with lower importance than DEBUG=10 for noisy debug logging -TRACE_LOG_LEVEL = 5 - - -class SpotifyPlaybackProvider(backend.PlaybackProvider): - def __init__(self, *args, **kwargs): - super().__init__(*args, **kwargs) - self._timeout = self.backend._config["spotify"]["timeout"] - - self._buffer_timestamp = BufferTimestamp(0) - self._seeking_event = threading.Event() - self._first_seek = False - self._push_audio_data_event = threading.Event() - self._push_audio_data_event.set() - self._end_of_track_event = threading.Event() - self._events_connected = False - # libspotify sends a single empty buffer at the end of each track which - # must be discarded to ensure a gapless track transition. We delay using - # each buffer until we receieve the next one, up until we change track - # and clear everything, therefore dropping the unwanted last buffer. - self._held_buffers = collections.deque() - - def _connect_events(self): - if not self._events_connected: - self._events_connected = True - self.backend._session.on( - spotify.SessionEvent.MUSIC_DELIVERY, - music_delivery_callback, - self.audio, - self._seeking_event, - self._push_audio_data_event, - self._buffer_timestamp, - self._held_buffers, - ) - self.backend._session.on( - spotify.SessionEvent.END_OF_TRACK, - end_of_track_callback, - self._end_of_track_event, - self.audio, - ) - - def change_track(self, track): - self._connect_events() - - if track.uri is None: - return False - - logger.debug( - "Audio requested change of track; " - "loading and starting Spotify player" - ) - - need_data_callback_bound = functools.partial( - need_data_callback, self._push_audio_data_event - ) - enough_data_callback_bound = functools.partial( - enough_data_callback, self._push_audio_data_event - ) - - seek_data_callback_bound = functools.partial( - seek_data_callback, self._seeking_event, self.backend._actor_proxy - ) - - self._buffer_timestamp.set(0) - self._first_seek = True - self._end_of_track_event.clear() - - # Discard held buffers - self._held_buffers.clear() - - try: - sp_track = self.backend._session.get_track(track.uri) - sp_track.load(self._timeout) - self.backend._session.player.load(sp_track) - self.backend._session.player.play() - - future = self.audio.set_appsrc( - GST_CAPS, - need_data=need_data_callback_bound, - enough_data=enough_data_callback_bound, - seek_data=seek_data_callback_bound, - ) - self.audio.set_metadata(track) - - # Gapless playback requires that we block until URI change in - # mopidy.audio has completed before we return from change_track(). - future.get() - - return True - except spotify.Error as exc: - logger.info(f"Playback of {track.uri} failed: {exc}") - return False - - def resume(self): - logger.debug("Audio requested resume; starting Spotify player") - self.backend._session.player.play() - return super().resume() - - def stop(self): - logger.debug("Audio requested stop; pausing Spotify player") - self.backend._session.player.pause() - return super().stop() - - def pause(self): - logger.debug("Audio requested pause; pausing Spotify player") - self.backend._session.player.pause() - return super().pause() - - def on_seek_data(self, time_position): - logger.debug(f"Audio requested seek to {time_position}") - - if time_position == 0 and self._first_seek: - self._seeking_event.clear() - self._first_seek = False - logger.debug("Skipping seek due to issue mopidy/mopidy#300") - return - - # After seeking any data buffered so far will be stale, so clear it. - # - # This also seems to fix intermittent soft failures of the player after - # seeking (especially backwards), i.e. it pretends to be playing music, - # but doesn't. - self._held_buffers.clear() - - self._buffer_timestamp.set( - audio.millisecond_to_clocktime(time_position) - ) - self.backend._session.player.seek(time_position) - - -def need_data_callback(push_audio_data_event, length_hint): - # This callback is called from GStreamer/the GObject event loop. - logger.log( - TRACE_LOG_LEVEL, - f"Audio requested more data (hint={length_hint}); " - "accepting deliveries", - ) - push_audio_data_event.set() - - -def enough_data_callback(push_audio_data_event): - # This callback is called from GStreamer/the GObject event loop. - logger.log(TRACE_LOG_LEVEL, "Audio has enough data; rejecting deliveries") - push_audio_data_event.clear() - - -def seek_data_callback(seeking_event, spotify_backend, time_position): - # This callback is called from GStreamer/the GObject event loop. - # It forwards the call to the backend actor. - seeking_event.set() - spotify_backend.playback.on_seek_data(time_position) - - -def music_delivery_callback( - session, - audio_format, - frames, - num_frames, - audio_actor, - seeking_event, - push_audio_data_event, - buffer_timestamp, - held_buffers, -): - # This is called from an internal libspotify thread. - # Ideally, nothing here should block. - - if seeking_event.is_set(): - # A seek has happened, but libspotify hasn't confirmed yet, so - # we're dropping all audio data from libspotify. - if num_frames == 0: - # libspotify signals that it has completed the seek. We'll accept - # the next audio data delivery. - seeking_event.clear() - return num_frames - - if not push_audio_data_event.is_set(): - return 0 # Reject the audio data. It will be redelivered later. - - if not frames: - return 0 # No audio data; return immediately. - - known_format = ( - audio_format.sample_type == spotify.SampleType.INT16_NATIVE_ENDIAN - ) - assert known_format, "Expects 16-bit signed integer samples" - - duration = audio.calculate_duration(num_frames, audio_format.sample_rate) - buffer_ = audio.create_buffer( - bytes(frames), timestamp=buffer_timestamp.get(), duration=duration - ) - - # Try to consume any held buffers. - if held_buffers: - while held_buffers: - buf = held_buffers.popleft() - consumed = audio_actor.emit_data(buf).get() - if not consumed: - held_buffers.appendleft(buf) - break - else: - # No held buffer, don't apply back-pressure - consumed = True - - if consumed: - # Consumed all held buffers so take the new one libspotify delivered us. - held_buffers.append(buffer_) - buffer_timestamp.increase(duration) - return num_frames - else: - # Pass back-pressure on to libspotify, next buffer will be redelivered. - return 0 - - -def end_of_track_callback(session, end_of_track_event, audio_actor): - # This callback is called from the pyspotify event loop. - - if end_of_track_event.is_set(): - logger.debug("End of track already received; ignoring callback") - return - - logger.debug("End of track reached") - end_of_track_event.set() - audio_actor.emit_data(None) - - # Stop the track to prevent receiving empty audio data - session.player.unload() - - -class BufferTimestamp: - """Wrapper around an int to serialize access by multiple threads. - - The value is used both from the backend actor and callbacks called by - internal libspotify threads. - """ - - def __init__(self, value): - self._value = value - self._lock = threading.RLock() - - def get(self): - with self._lock: - return self._value - - def set(self, value): - with self._lock: - self._value = value - - def increase(self, value): - with self._lock: - self._value += value diff --git a/mopidy_spotify/playlists.py b/mopidy_spotify/playlists.py index 5811d9e1..09b83c1e 100644 --- a/mopidy_spotify/playlists.py +++ b/mopidy_spotify/playlists.py @@ -5,7 +5,6 @@ import spotify from mopidy_spotify import translator, utils -_sp_links = {} logger = logging.getLogger(__name__) @@ -42,7 +41,6 @@ def lookup(self, uri): def _get_playlist(self, uri, as_items=False): return playlist_lookup( - self._backend._session, self._backend._web_client, uri, self._backend._bitrate, @@ -56,7 +54,6 @@ def refresh(self): logger.info("Refreshing Spotify playlists") with utils.time_logger("playlists.refresh()", logging.DEBUG): - _sp_links.clear() self._backend._web_client.clear_cache() count = 0 for playlist_ref in self._get_flattened_playlist_refs(): @@ -76,7 +73,7 @@ def save(self, playlist): pass # TODO -def playlist_lookup(session, web_client, uri, bitrate, as_items=False): +def playlist_lookup(web_client, uri, bitrate, as_items=False): if web_client is None or not web_client.logged_in: return @@ -93,22 +90,8 @@ def playlist_lookup(session, web_client, uri, bitrate, as_items=False): bitrate=bitrate, as_items=as_items, ) + #TODO: cache the Mopidy tracks? And handle as_items here instead of translator if playlist is None: return - # Store the libspotify Link for each track so they will be loaded in the - # background ready for using later. - if session.connection.state is spotify.ConnectionState.LOGGED_IN: - if as_items: - tracks = playlist - else: - tracks = playlist.tracks - - for track in tracks: - if track.uri in _sp_links: - continue - try: - _sp_links[track.uri] = session.get_link(track.uri) - except ValueError as exc: - logger.info(f"Failed to get link {track.uri!r}: {exc}") return playlist diff --git a/mopidy_spotify/web.py b/mopidy_spotify/web.py index b85eaaab..d67a6382 100644 --- a/mopidy_spotify/web.py +++ b/mopidy_spotify/web.py @@ -476,6 +476,22 @@ def get_playlist(self, uri): return playlist + def get_track(self, uri): + try: + parsed = WebLink.from_uri(uri) + if parsed.type != LinkType.TRACK: + raise ValueError( + f"Could not parse {uri!r} as a Spotify track URI" + ) + except ValueError as exc: + logger.error(exc) + return {} + + return self.get_one( + f"tracks/{parsed.id}", + params={"market": "from_token"} + ) + def clear_cache(self, extra_expiry=None): self._cache.clear() diff --git a/tests/conftest.py b/tests/conftest.py index f49d576f..a37fef2b 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -423,5 +423,4 @@ def backend_listener_mock(): @pytest.fixture def provider(backend_mock): - playlists._sp_links.clear() return library.SpotifyLibraryProvider(backend_mock) diff --git a/tests/test_backend.py b/tests/test_backend.py index bcf04bb2..68ecc48b 100644 --- a/tests/test_backend.py +++ b/tests/test_backend.py @@ -1,43 +1,39 @@ import threading from unittest import mock +from unittest import skip from mopidy import backend as backend_api -import spotify -from mopidy_spotify import backend, library, playback, playlists +from mopidy_spotify import backend, library, playlists +from mopidy_spotify.backend import SpotifyPlaybackProvider -def get_backend(config, session_mock=None): +def get_backend(config): obj = backend.SpotifyBackend(config=config, audio=None) - if session_mock: - obj._session = session_mock - else: - obj._session = mock.Mock() - obj._web_client = mock.Mock() - obj._event_loop = mock.Mock() + obj._web_client = mock.Mock() return obj -def test_uri_schemes(spotify_mock, config): +def test_uri_schemes(config): backend = get_backend(config) assert "spotify" in backend.uri_schemes -def test_init_sets_up_the_providers(spotify_mock, config): +def test_init_sets_up_the_providers(config): backend = get_backend(config) assert isinstance(backend.library, library.SpotifyLibraryProvider) assert isinstance(backend.library, backend_api.LibraryProvider) - assert isinstance(backend.playback, playback.SpotifyPlaybackProvider) + assert isinstance(backend.playback, SpotifyPlaybackProvider) assert isinstance(backend.playback, backend_api.PlaybackProvider) assert isinstance(backend.playlists, playlists.SpotifyPlaylistsProvider) assert isinstance(backend.playlists, backend_api.PlaylistsProvider) -def test_init_disables_playlists_provider_if_not_allowed(spotify_mock, config): +def test_init_disables_playlists_provider_if_not_allowed(config): config["spotify"]["allow_playlists"] = False backend = get_backend(config) @@ -45,60 +41,24 @@ def test_init_disables_playlists_provider_if_not_allowed(spotify_mock, config): assert backend.playlists is None -def test_on_start_creates_configured_session(tmp_path, spotify_mock, config): +@skip("support this with spotifyaudiosrc") +def test_on_start_creates_configured_session(tmp_path, config): cache_location_mock = mock.PropertyMock() - settings_location_mock = mock.PropertyMock() - config_mock = spotify_mock.Config.return_value - type(config_mock).cache_location = cache_location_mock - type(config_mock).settings_location = settings_location_mock + pass - get_backend(config).on_start() - spotify_mock.Config.assert_called_once_with() - config_mock.load_application_key_file.assert_called_once_with(mock.ANY) - cache_location_mock.assert_called_once_with( - bytes(tmp_path / "cache" / "spotify") - ) - settings_location_mock.assert_called_once_with( - bytes(tmp_path / "data" / "spotify") - ) - spotify_mock.Session.assert_called_once_with(config_mock) - - -def test_on_start_disallows_network_if_config_is_set(spotify_mock, config): - session = spotify_mock.Session.return_value - allow_network_mock = mock.PropertyMock() - type(session.connection).allow_network = allow_network_mock - config["spotify"]["allow_network"] = False - - get_backend(config).on_start() - - allow_network_mock.assert_called_once_with(False) - - -def test_on_start_configures_preferred_bitrate(spotify_mock, config): - session = spotify_mock.Session.return_value - preferred_bitrate_mock = mock.PropertyMock() - type(session).preferred_bitrate = preferred_bitrate_mock - config["spotify"]["bitrate"] = 320 - - get_backend(config).on_start() - - preferred_bitrate_mock.assert_called_once_with(spotify.Bitrate.BITRATE_320k) +@skip("currently can't configure this") +def test_on_start_configures_preferred_bitrate(config): + pass -def test_on_start_configures_volume_normalization(spotify_mock, config): - session = spotify_mock.Session.return_value - volume_normalization_mock = mock.PropertyMock() - type(session).volume_normalization = volume_normalization_mock - config["spotify"]["volume_normalization"] = False +@skip("support this with spotifyaudiosrc?") +def test_on_start_configures_volume_normalization(config): + pass - get_backend(config).on_start() - volume_normalization_mock.assert_called_once_with(False) - - -def test_on_start_configures_proxy(spotify_mock, web_mock, config): +@skip("support this with spotifyaudiosrc?") +def test_on_start_configures_proxy(web_mock, config): config["proxy"] = { "scheme": "https", "hostname": "my-proxy.example.com", @@ -106,14 +66,10 @@ def test_on_start_configures_proxy(spotify_mock, web_mock, config): "username": "alice", "password": "s3cret", } - spotify_config = spotify_mock.Config.return_value - backend = get_backend(config) backend.on_start() - assert spotify_config.proxy == "https://my-proxy.example.com:8080" - assert spotify_config.proxy_username == "alice" - assert spotify_config.proxy_password == "s3cret" + assert True web_mock.SpotifyOAuthClient.assert_called_once_with( client_id=mock.ANY, @@ -122,7 +78,7 @@ def test_on_start_configures_proxy(spotify_mock, web_mock, config): ) -def test_on_start_configures_web_client(spotify_mock, web_mock, config): +def test_on_start_configures_web_client(web_mock, config): config["spotify"]["client_id"] = "1234567" config["spotify"]["client_secret"] = "AbCdEfG" @@ -136,54 +92,14 @@ def test_on_start_configures_web_client(spotify_mock, web_mock, config): ) -def test_on_start_adds_connection_state_changed_handler_to_session( - spotify_mock, config -): - session = spotify_mock.Session.return_value - - get_backend(config).on_start() - - session.on.assert_any_call( - spotify_mock.SessionEvent.CONNECTION_STATE_UPDATED, - backend.on_connection_state_changed, - backend.SpotifyBackend._logged_in, - backend.SpotifyBackend._logged_out, - mock.ANY, - ) - - -def test_on_start_adds_play_token_lost_handler_to_session(spotify_mock, config): - session = spotify_mock.Session.return_value - - obj = get_backend(config) - obj.on_start() - - session.on.assert_any_call( - spotify_mock.SessionEvent.PLAY_TOKEN_LOST, - backend.on_play_token_lost, - mock.ANY, - ) - - -def test_on_start_starts_the_pyspotify_event_loop(spotify_mock, config): - backend = get_backend(config) - backend.on_start() - - spotify_mock.EventLoop.assert_called_once_with(backend._session) - spotify_mock.EventLoop.return_value.start.assert_called_once_with() - - -def test_on_start_logs_in(spotify_mock, web_mock, config): +def test_on_start_logs_in(web_mock, config): backend = get_backend(config) backend.on_start() - spotify_mock.Session.return_value.login.assert_called_once_with( - "alice", "password" - ) web_mock.SpotifyOAuthClient.return_value.login.assert_called_once() -def test_on_start_refreshes_playlists(spotify_mock, web_mock, config, caplog): +def test_on_start_refreshes_playlists(web_mock, config, caplog): backend = get_backend(config) backend.on_start() @@ -194,7 +110,7 @@ def test_on_start_refreshes_playlists(spotify_mock, web_mock, config, caplog): def test_on_start_doesnt_refresh_playlists_if_not_allowed( - spotify_mock, web_mock, config, caplog + web_mock, config, caplog ): config["spotify"]["allow_playlists"] = False @@ -204,134 +120,3 @@ def test_on_start_doesnt_refresh_playlists_if_not_allowed( client_mock = web_mock.SpotifyOAuthClient.return_value client_mock.get_user_playlists.assert_not_called() assert "Refreshed 0 playlists" not in caplog.text - - -def test_on_stop_logs_out_and_waits_for_logout_to_complete( - spotify_mock, config, caplog -): - backend = get_backend(config) - backend._logged_out = mock.Mock() - - backend.on_stop() - - assert "Logging out of Spotify" in caplog.text - backend._session.logout.assert_called_once_with() - backend._logged_out.wait.assert_called_once_with() - backend._event_loop.stop.assert_called_once_with() - - -def test_on_connection_state_changed_when_logged_out(spotify_mock, caplog): - session_mock = spotify_mock.Session.return_value - session_mock.connection.state = spotify_mock.ConnectionState.LOGGED_OUT - logged_in_event = threading.Event() - logged_out_event = threading.Event() - actor_mock = mock.Mock(spec=backend.SpotifyBackend) - - backend.on_connection_state_changed( - session_mock, logged_in_event, logged_out_event, actor_mock - ) - - assert "Logged out of Spotify" in caplog.text - assert not logged_in_event.is_set() - assert logged_out_event.is_set() - - -def test_on_connection_state_changed_when_logged_in(spotify_mock, caplog): - session_mock = spotify_mock.Session.return_value - session_mock.connection.state = spotify_mock.ConnectionState.LOGGED_IN - logged_in_event = threading.Event() - logged_out_event = threading.Event() - actor_mock = mock.Mock(spec=backend.SpotifyBackend) - - backend.on_connection_state_changed( - session_mock, logged_in_event, logged_out_event, actor_mock - ) - - assert "Logged in to Spotify in online mode" in caplog.text - assert logged_in_event.is_set() - assert not logged_out_event.is_set() - actor_mock.on_logged_in.assert_called_once_with() - - -def test_on_connection_state_changed_when_disconnected(spotify_mock, caplog): - session_mock = spotify_mock.Session.return_value - session_mock.connection.state = spotify_mock.ConnectionState.DISCONNECTED - logged_in_event = threading.Event() - logged_out_event = threading.Event() - actor_mock = mock.Mock(spec=backend.SpotifyBackend) - - backend.on_connection_state_changed( - session_mock, logged_in_event, logged_out_event, actor_mock - ) - - assert "Disconnected from Spotify" in caplog.text - - -def test_on_connection_state_changed_when_offline(spotify_mock, caplog): - session_mock = spotify_mock.Session.return_value - session_mock.connection.state = spotify_mock.ConnectionState.OFFLINE - logged_in_event = threading.Event() - logged_out_event = threading.Event() - actor_mock = mock.Mock(spec=backend.SpotifyBackend) - - backend.on_connection_state_changed( - session_mock, logged_in_event, logged_out_event, actor_mock - ) - - assert "Logged in to Spotify in offline mode" in caplog.text - assert logged_in_event.is_set() - assert not logged_out_event.is_set() - - -def test_on_logged_in_event_activates_private_session( - spotify_mock, config, caplog -): - session_mock = spotify_mock.Session.return_value - private_session_mock = mock.PropertyMock() - type(session_mock.social).private_session = private_session_mock - config["spotify"]["private_session"] = True - backend = get_backend(config, session_mock) - - backend.on_logged_in() - - assert "Spotify private session activated" in caplog.text - private_session_mock.assert_called_once_with(True) - - -def test_on_play_token_lost_messages_the_actor(spotify_mock, caplog): - session_mock = spotify_mock.Session.return_value - actor_mock = mock.Mock(spec=backend.SpotifyBackend) - - backend.on_play_token_lost(session_mock, actor_mock) - - assert "Spotify play token lost" in caplog.text - actor_mock.on_play_token_lost.assert_called_once_with() - - -def test_on_play_token_lost_event_when_playing(spotify_mock, config, caplog): - session_mock = spotify_mock.Session.return_value - session_mock.player.state = spotify_mock.PlayerState.PLAYING - backend = get_backend(config, session_mock) - backend.playback = mock.Mock(spec=playback.SpotifyPlaybackProvider) - - backend.on_play_token_lost() - - assert ( - "Spotify has been paused because your account is " - "being used somewhere else." in caplog.text - ) - backend.playback.pause.assert_called_once_with() - - -def test_on_play_token_lost_event_when_not_playing( - spotify_mock, config, caplog -): - session_mock = spotify_mock.Session.return_value - session_mock.player.state = spotify_mock.PlayerState.UNLOADED - backend = get_backend(config, session_mock) - backend.playback = mock.Mock(spec=playback.SpotifyPlaybackProvider) - - backend.on_play_token_lost() - - assert "Spotify has been paused" not in caplog.text - assert backend.playback.pause.call_count == 0 diff --git a/tests/test_playback.py b/tests/test_playback.py index d732c62f..0a0aa439 100644 --- a/tests/test_playback.py +++ b/tests/test_playback.py @@ -1,14 +1,10 @@ -import collections -import threading from unittest import mock import pytest from mopidy import audio from mopidy import backend as backend_api -from mopidy import models -import spotify -from mopidy_spotify import backend, playback +from mopidy_spotify import backend @pytest.fixture @@ -18,30 +14,16 @@ def audio_mock(): @pytest.fixture -def audio_lib_mock(): - patcher = mock.patch.object(playback, "audio", spec=audio) - yield patcher.start() - patcher.stop() - - -@pytest.fixture -def session_mock(): - sp_session_mock = mock.Mock(spec=spotify.Session) - return sp_session_mock - - -@pytest.fixture -def backend_mock(config, session_mock): +def backend_mock(config): backend_mock = mock.Mock(spec=backend.SpotifyBackend) backend_mock._config = config backend_mock._actor_proxy = None - backend_mock._session = session_mock return backend_mock @pytest.fixture def provider(audio_mock, backend_mock): - return playback.SpotifyPlaybackProvider( + return backend.SpotifyPlaybackProvider( audio=audio_mock, backend=backend_mock ) @@ -50,450 +32,8 @@ def test_is_a_playback_provider(provider): assert isinstance(provider, backend_api.PlaybackProvider) -def test_connect_events_adds_music_delivery_handler_to_session( - session_mock, provider, audio_mock -): - - playback_provider = provider - playback_provider._connect_events() - - assert ( - mock.call( - spotify.SessionEvent.MUSIC_DELIVERY, - playback.music_delivery_callback, - audio_mock, - playback_provider._seeking_event, - playback_provider._push_audio_data_event, - playback_provider._buffer_timestamp, - playback_provider._held_buffers, - ) - in session_mock.on.call_args_list - ) - - -def test_connect_events_adds_end_of_track_handler_to_session( - session_mock, provider, audio_mock -): - - playback_provider = provider - playback_provider._connect_events() - - assert ( - mock.call( - spotify.SessionEvent.END_OF_TRACK, - playback.end_of_track_callback, - playback_provider._end_of_track_event, - audio_mock, - ) - in session_mock.on.call_args_list - ) - - -def test_change_track_aborts_if_no_track_uri(provider): - track = models.Track() - - assert provider.change_track(track) is False - - -def test_change_track_loads_and_plays_spotify_track(session_mock, provider): - uri = "spotify:track:test" - track = models.Track(uri=uri) - - assert provider.change_track(track) is True - - session_mock.get_track.assert_called_once_with(uri) - sp_track_mock = session_mock.get_track.return_value - sp_track_mock.load.assert_called_once_with(10) - session_mock.player.load.assert_called_once_with(sp_track_mock) - session_mock.player.play.assert_called_once_with() - - -def test_change_track_aborts_on_spotify_error(session_mock, provider): - track = models.Track(uri="spotfy:track:test") - session_mock.get_track.side_effect = spotify.Error - - assert provider.change_track(track) is False - - -def test_change_track_sets_up_appsrc(audio_mock, provider): - track = models.Track(uri="spotfy:track:test") - - assert provider.change_track(track) is True - - assert provider._buffer_timestamp.get() == 0 - assert audio_mock.prepare_change.call_count == 0 - audio_mock.set_appsrc.assert_called_once_with( - playback.GST_CAPS, - need_data=mock.ANY, - enough_data=mock.ANY, - seek_data=mock.ANY, - ) - assert audio_mock.start_playback.call_count == 0 - audio_mock.set_metadata.assert_called_once_with(track) - - -def test_change_track_clears_state(audio_mock, provider): - track = models.Track(uri="spotfy:track:test") - - provider._first_seek = False - provider._buffer_timestamp.set(99) - provider._end_of_track_event.set() - provider._held_buffers = collections.deque([mock.sentinel.gst_buffer]) - - assert provider.change_track(track) is True - - assert provider._first_seek - assert provider._buffer_timestamp.get() == 0 - assert not provider._end_of_track_event.is_set() - assert len(provider._held_buffers) == 0 - - -def test_resume_starts_spotify_playback(session_mock, provider): - provider.resume() - - session_mock.player.play.assert_called_once_with() - - -def test_stop_pauses_spotify_playback(session_mock, provider): - provider.stop() - - session_mock.player.pause.assert_called_once_with() - - -def test_pause_pauses_spotify_playback(session_mock, provider): - provider.pause() - - session_mock.player.pause.assert_called_once_with() - - -def test_on_seek_data_updates_timestamp_and_seeks_in_spotify( - session_mock, provider -): - provider.on_seek_data(1780) - - assert provider._buffer_timestamp.get() == 1780000000 - session_mock.player.seek.assert_called_once_with(1780) - - -def test_on_seek_data_ignores_first_seek_to_zero_on_every_play( - session_mock, provider -): - provider._seeking_event.set() - track = models.Track(uri="spotfy:track:test") - - provider.change_track(track) - provider.on_seek_data(0) - - assert not provider._seeking_event.is_set() - assert session_mock.player.seek.call_count == 0 - - -def test_need_data_callback(): - event = threading.Event() - assert not event.is_set() - - playback.need_data_callback(event, 100) - - assert event.is_set() - - -def test_enough_data_callback(): - event = threading.Event() - event.set() - assert event.is_set() - - playback.enough_data_callback(event) - - assert not event.is_set() - - -def test_seek_data_callback(): - seeking_event = threading.Event() - backend_mock = mock.Mock() - - playback.seek_data_callback(seeking_event, backend_mock, 1340) - - assert seeking_event.is_set() - backend_mock.playback.on_seek_data.assert_called_once_with(1340) - - -def test_music_delivery_rejects_data_when_seeking(session_mock, audio_mock): - audio_format = mock.Mock() - frames = b"123" - num_frames = 1 - seeking_event = threading.Event() - seeking_event.set() - push_audio_data_event = threading.Event() - push_audio_data_event.set() - buffer_timestamp = mock.Mock() - held_buffer = collections.deque([mock.sentinel.gst_buffer]) - assert seeking_event.is_set() - - result = playback.music_delivery_callback( - session_mock, - audio_format, - frames, - num_frames, - audio_mock, - seeking_event, - push_audio_data_event, - buffer_timestamp, - held_buffer, - ) - - assert seeking_event.is_set() - assert audio_mock.emit_data.call_count == 0 - assert result == num_frames - - -def test_music_delivery_when_seeking_accepts_data_after_empty_delivery( - session_mock, audio_mock -): - - audio_format = mock.Mock() - frames = b"" - num_frames = 0 - seeking_event = threading.Event() - seeking_event.set() - push_audio_data_event = threading.Event() - push_audio_data_event.set() - buffer_timestamp = mock.Mock() - held_buffer = collections.deque([mock.sentinel.gst_buffer]) - assert seeking_event.is_set() - - result = playback.music_delivery_callback( - session_mock, - audio_format, - frames, - num_frames, - audio_mock, - seeking_event, - push_audio_data_event, - buffer_timestamp, - held_buffer, - ) - - assert not seeking_event.is_set() - assert audio_mock.emit_data.call_count == 0 - assert result == num_frames - - -def test_music_delivery_rejects_data_depending_on_push_audio_data_event( - session_mock, audio_mock -): - - audio_format = mock.Mock() - frames = b"123" - num_frames = 1 - seeking_event = threading.Event() - push_audio_data_event = threading.Event() - buffer_timestamp = mock.Mock() - held_buffer = collections.deque([mock.sentinel.gst_buffer]) - assert not push_audio_data_event.is_set() - - result = playback.music_delivery_callback( - session_mock, - audio_format, - frames, - num_frames, - audio_mock, - seeking_event, - push_audio_data_event, - buffer_timestamp, - held_buffer, - ) - - assert audio_mock.emit_data.call_count == 0 - assert result == 0 - - -def test_music_delivery_shortcuts_if_no_data_in_frames( - session_mock, audio_lib_mock, audio_mock -): - - audio_format = mock.Mock(channels=2, sample_rate=44100, sample_type=0) - frames = b"" - num_frames = 1 - seeking_event = threading.Event() - push_audio_data_event = threading.Event() - push_audio_data_event.set() - buffer_timestamp = mock.Mock() - held_buffer = collections.deque([mock.sentinel.gst_buffer]) - - result = playback.music_delivery_callback( - session_mock, - audio_format, - frames, - num_frames, - audio_mock, - seeking_event, - push_audio_data_event, - buffer_timestamp, - held_buffer, - ) - - assert result == 0 - assert audio_lib_mock.create_buffer.call_count == 0 - assert audio_mock.emit_data.call_count == 0 - - -def test_music_delivery_rejects_unknown_audio_formats(session_mock, audio_mock): - - audio_format = mock.Mock(sample_type=17) - frames = b"123" - num_frames = 1 - seeking_event = threading.Event() - push_audio_data_event = threading.Event() - push_audio_data_event.set() - buffer_timestamp = mock.Mock() - held_buffer = collections.deque([mock.sentinel.gst_buffer]) - - with pytest.raises(AssertionError) as excinfo: - playback.music_delivery_callback( - session_mock, - audio_format, - frames, - num_frames, - audio_mock, - seeking_event, - push_audio_data_event, - buffer_timestamp, - held_buffer, - ) - - assert "Expects 16-bit signed integer samples" in str(excinfo.value) - - -def test_music_delivery_creates_gstreamer_buffer_and_holds_it( - session_mock, audio_mock, audio_lib_mock -): - - audio_lib_mock.calculate_duration.return_value = mock.sentinel.duration - audio_lib_mock.create_buffer.return_value = mock.sentinel.gst_buffer - - audio_format = mock.Mock(channels=2, sample_rate=44100, sample_type=0) - frames = b"\x00\x00" - num_frames = 1 - seeking_event = threading.Event() - push_audio_data_event = threading.Event() - push_audio_data_event.set() - buffer_timestamp = mock.Mock() - buffer_timestamp.get.return_value = mock.sentinel.timestamp - held_buffer = collections.deque() - - result = playback.music_delivery_callback( - session_mock, - audio_format, - frames, - num_frames, - audio_mock, - seeking_event, - push_audio_data_event, - buffer_timestamp, - held_buffer, - ) - - audio_lib_mock.calculate_duration.assert_called_once_with(1, 44100) - audio_lib_mock.create_buffer.assert_called_once_with( - frames, - timestamp=mock.sentinel.timestamp, - duration=mock.sentinel.duration, - ) - buffer_timestamp.increase.assert_called_once_with(mock.sentinel.duration) - audio_mock.emit_data.assert_not_called() - assert result == num_frames - assert list(held_buffer) == [mock.sentinel.gst_buffer] - - -def test_music_delivery_gives_held_buffer_to_audio_and_holds_created( - session_mock, audio_mock, audio_lib_mock -): - audio_lib_mock.create_buffer.return_value = mock.sentinel.gst_buffer2 - - audio_format = mock.Mock(channels=2, sample_rate=44100, sample_type=0) - frames = b"\x00\x00" - num_frames = 1 - seeking_event = threading.Event() - push_audio_data_event = threading.Event() - push_audio_data_event.set() - buffer_timestamp = mock.Mock() - held_buffer = collections.deque([mock.sentinel.gst_buffer1]) - - result = playback.music_delivery_callback( - session_mock, - audio_format, - frames, - num_frames, - audio_mock, - seeking_event, - push_audio_data_event, - buffer_timestamp, - held_buffer, - ) - assert result == num_frames - audio_mock.emit_data.assert_called_once_with(mock.sentinel.gst_buffer1) - assert list(held_buffer) == [mock.sentinel.gst_buffer2] - - -def test_music_delivery_consumes_zero_frames_if_audio_fails( - session_mock, audio_mock, audio_lib_mock -): - - audio_lib_mock.create_buffer.return_value = mock.sentinel.gst_buffer2 - audio_mock.emit_data.return_value.get.return_value = False - - audio_format = mock.Mock(channels=2, sample_rate=44100, sample_type=0) - frames = b"\x00\x00" - num_frames = 1 - seeking_event = threading.Event() - push_audio_data_event = threading.Event() - push_audio_data_event.set() - buffer_timestamp = mock.Mock() - buffer_timestamp.get.return_value = mock.sentinel.timestamp - held_buffer = collections.deque([mock.sentinel.gst_buffer1]) - - result = playback.music_delivery_callback( - session_mock, - audio_format, - frames, - num_frames, - audio_mock, - seeking_event, - push_audio_data_event, - buffer_timestamp, - held_buffer, - ) - - assert buffer_timestamp.increase.call_count == 0 - assert result == 0 - assert list(held_buffer) == [mock.sentinel.gst_buffer1] - - -def test_end_of_track_callback(session_mock, audio_mock): - end_of_track_event = threading.Event() - - playback.end_of_track_callback(session_mock, end_of_track_event, audio_mock) - - assert end_of_track_event.is_set() - audio_mock.emit_data.assert_called_once_with(None) - - -def test_duplicate_end_of_track_callback_is_ignored(session_mock, audio_mock): - end_of_track_event = threading.Event() - end_of_track_event.set() - - playback.end_of_track_callback(session_mock, end_of_track_event, audio_mock) - - assert end_of_track_event.is_set() - assert audio_mock.emit_data.call_count == 0 - - -def test_buffer_timestamp_wrapper(): - wrapper = playback.BufferTimestamp(0) - assert wrapper.get() == 0 - - wrapper.set(17) - assert wrapper.get() == 17 +def test_translate_uri_sets_credentials(config, provider): + config["spotify"]["username"] = "foo" + config["spotify"]["password"] = "bar" - wrapper.increase(3) - assert wrapper.get() == 20 + assert provider.translate_uri("baz") == "baz?username=foo&password=bar" diff --git a/tests/test_playlists.py b/tests/test_playlists.py index 899709a3..fc415b49 100644 --- a/tests/test_playlists.py +++ b/tests/test_playlists.py @@ -44,7 +44,6 @@ def get_playlist(*args, **kwargs): @pytest.fixture def provider(backend_mock, web_client_mock): backend_mock._web_client = web_client_mock - playlists._sp_links.clear() provider = playlists.SpotifyPlaylistsProvider(backend_mock) provider._loaded = True return provider @@ -183,11 +182,8 @@ def test_refresh_counts_playlists(provider, caplog): def test_refresh_clears_caches(provider, web_client_mock): - playlists._sp_links = {"bar": "foobar"} - provider.refresh() - assert "bar" not in playlists._sp_links web_client_mock.clear_cache.assert_called_once() @@ -228,81 +224,14 @@ def test_lookup_of_playlist_with_other_owner(provider): assert playlist.name == "Baz (by bob)" -@pytest.mark.parametrize("as_items", [(False), (True)]) -def test_playlist_lookup_stores_track_link( - session_mock, - web_client_mock, - sp_track_mock, - web_playlist_mock, - web_track_mock, - as_items, -): - session_mock.get_link.return_value = sp_track_mock.link - web_playlist_mock["tracks"]["items"] = [{"track": web_track_mock}] * 5 - web_client_mock.get_playlist.return_value = web_playlist_mock - playlists._sp_links.clear() - - playlists.playlist_lookup( - session_mock, - web_client_mock, - "spotify:user:alice:playlist:foo", - None, - as_items, - ) - - session_mock.get_link.assert_called_once_with("spotify:track:abc") - assert {"spotify:track:abc": sp_track_mock.link} == playlists._sp_links - - -@pytest.mark.parametrize( - "connection_state", - [ - (spotify.ConnectionState.OFFLINE), - (spotify.ConnectionState.DISCONNECTED), - (spotify.ConnectionState.LOGGED_OUT), - ], -) -def test_playlist_lookup_when_not_logged_in( - session_mock, web_client_mock, web_playlist_mock, connection_state -): - web_client_mock.get_playlist.return_value = web_playlist_mock - session_mock.connection.state = connection_state - playlists._sp_links.clear() - - playlist = playlists.playlist_lookup( - session_mock, web_client_mock, "spotify:user:alice:playlist:foo", None - ) - - assert playlist.uri == "spotify:user:alice:playlist:foo" - assert playlist.name == "Foo" - assert len(playlists._sp_links) == 0 - - -def test_playlist_lookup_when_playlist_is_empty( - session_mock, web_client_mock, caplog -): - web_client_mock.get_playlist.return_value = {} - playlists._sp_links.clear() - - playlist = playlists.playlist_lookup( - session_mock, web_client_mock, "nothing", None - ) - - assert playlist is None - assert "Failed to lookup Spotify playlist URI 'nothing'" in caplog.text - assert len(playlists._sp_links) == 0 - - def test_playlist_lookup_when_link_invalid( - session_mock, web_client_mock, web_playlist_mock, caplog + web_client_mock, web_playlist_mock, caplog ): - session_mock.get_link.side_effect = ValueError("an error message") web_client_mock.get_playlist.return_value = web_playlist_mock - playlists._sp_links.clear() playlist = playlists.playlist_lookup( - session_mock, web_client_mock, "spotify:user:alice:playlist:foo", None + web_client_mock, "spotify:in:valid", None ) - assert len(playlist.tracks) == 1 - assert "Failed to get link 'spotify:track:abc'" in caplog.text + assert playlist is None + assert "Failed to lookup Spotify playlist URI 'spotify:in:valid'" in caplog.text From 19c32d14b88815a658b0ff17b54efd7977aab925 Mon Sep 17 00:00:00 2001 From: Nick Steel Date: Thu, 9 Jun 2022 01:00:07 +0100 Subject: [PATCH 02/21] Remove pyspotify: lookup --- mopidy_spotify/library.py | 4 +- mopidy_spotify/lookup.py | 108 ++++++-------- mopidy_spotify/playlists.py | 1 - mopidy_spotify/translator.py | 21 ++- mopidy_spotify/web.py | 90 ++++++++---- tests/conftest.py | 59 ++++++-- tests/test_lookup.py | 152 ++++++-------------- tests/test_translator.py | 36 +++++ tests/test_web.py | 270 ++++++++++++++++++++++++++--------- 9 files changed, 460 insertions(+), 281 deletions(-) diff --git a/mopidy_spotify/library.py b/mopidy_spotify/library.py index b08ff0c3..06ae7757 100644 --- a/mopidy_spotify/library.py +++ b/mopidy_spotify/library.py @@ -35,9 +35,7 @@ def get_images(self, uris): return images.get_images(self._backend._web_client, uris) def lookup(self, uri): - return lookup.lookup( - self._config, self._backend._session, self._backend._web_client, uri - ) + return lookup.lookup(self._config, self._backend._web_client, uri) def search(self, query=None, uris=None, exact=False): return search.search( diff --git a/mopidy_spotify/lookup.py b/mopidy_spotify/lookup.py index 085d2e18..a34f05b2 100644 --- a/mopidy_spotify/lookup.py +++ b/mopidy_spotify/lookup.py @@ -1,107 +1,81 @@ import logging -import spotify from mopidy_spotify import browse, playlists, translator, utils -from mopidy_spotify.web import LinkType, WebLink +from mopidy_spotify.web import LinkType, WebLink, WebError logger = logging.getLogger(__name__) -_VARIOUS_ARTISTS_URIS = [ - "spotify:artist:0LyfQWJT6nXafLPZqxe9Of", -] - -def lookup(config, session, web_client, uri): +def lookup(config, web_client, uri): if web_client is None or not web_client.logged_in: return [] try: - web_link = WebLink.from_uri(uri) + link = WebLink.from_uri(uri) except ValueError as exc: logger.info(f"Failed to lookup {uri!r}: {exc}") return [] try: - if web_link.type == LinkType.PLAYLIST: - return _lookup_playlist(config, web_client, uri) - elif web_link.type == LinkType.YOUR: - return list(_lookup_your(config, session, web_client, uri)) - elif web_link.type == LinkType.TRACK: - return list(_lookup_track(config, web_client, uri)) - elif web_link.type == LinkType.ALBUM: - return list(_lookup_album(config, sp_link)) - elif web_link.type == LinkType.ARTIST: + if link.type == LinkType.PLAYLIST: + return _lookup_playlist(config, web_client, link) + elif link.type == LinkType.YOUR: + return list(_lookup_your(config, web_client, link)) + elif link.type == LinkType.TRACK: + return list(_lookup_track(config, web_client, link)) + elif link.type == LinkType.ALBUM: + return list(_lookup_album(config, web_client, link)) + elif link.type == LinkType.ARTIST: with utils.time_logger("Artist lookup"): - return list(_lookup_artist(config, sp_link)) + return list(_lookup_artist(config, web_client, link)) else: logger.info( - f"Failed to lookup {uri!r}: Cannot handle {web_link.type!r}" + f"Failed to lookup {uri!r}: Cannot handle {link.type!r}" ) return [] - except spotify.Error as exc: - logger.info(f"Failed to lookup {uri!r}: {exc}") + except WebError as exc: + logger.info(f"Failed to lookup Spotify {link.type.value} {link.uri!r}: {exc}") return [] -def _lookup_track(config, web_client, uri): - web_track = web_client.get_track(uri) +def _lookup_track(config, web_client, link): + web_track = web_client.get_track(link) if web_track == {}: - logger.error(f"Failed to lookup Spotify track URI {uri!r}") - return + raise WebError("Invalid track response") track = translator.web_to_track(web_track, bitrate=config["bitrate"]) if track is not None: yield track -def _lookup_album(config, sp_link): - sp_album = sp_link.as_album() - sp_album_browser = sp_album.browse() - sp_album_browser.load(config["timeout"]) - for sp_track in sp_album_browser.tracks: - track = translator.to_track(sp_track, bitrate=config["bitrate"]) - if track is not None: - yield track +def _lookup_album(config, web_client, link): + web_album = web_client.get_album(link) + if web_album == {}: + raise WebError("Invalid album response") + tracks = translator.web_to_album_tracks(web_album, bitrate=config["bitrate"]) + for track in tracks: + yield track -def _lookup_artist(config, sp_link): - sp_artist = sp_link.as_artist() - sp_artist_browser = sp_artist.browse( - type=spotify.ArtistBrowserType.NO_TRACKS - ) - sp_artist_browser.load(config["timeout"]) - - # Get all album browsers we need first, so they can start retrieving - # data in the background. - sp_album_browsers = [] - for sp_album in sp_artist_browser.albums: - sp_album.load(config["timeout"]) - if not sp_album.is_available: - continue - if sp_album.type is spotify.AlbumType.COMPILATION: - continue - if sp_album.artist.link.uri in _VARIOUS_ARTISTS_URIS: - continue - sp_album_browsers.append(sp_album.browse()) - - for sp_album_browser in sp_album_browsers: - sp_album_browser.load(config["timeout"]) - for sp_track in sp_album_browser.tracks: - track = translator.to_track(sp_track, bitrate=config["bitrate"]) - if track is not None: - yield track + +def _lookup_artist(config, web_client, link): + web_albums = web_client.get_artist_albums(link) + for web_album in web_albums: + tracks = translator.web_to_album_tracks(web_album, bitrate=config["bitrate"]) + for track in tracks: + yield track -def _lookup_playlist(config, web_client, uri): - playlist = playlists.playlist_lookup(web_client, uri, config["bitrate"]) +def _lookup_playlist(config, web_client, link): + playlist = playlists.playlist_lookup(web_client, link.uri, config["bitrate"]) if playlist is None: - raise spotify.Error("Playlist Web API lookup failed") + raise WebError("Invalid playlist response") return playlist.tracks -def _lookup_your(config, session, web_client, uri): - parts = uri.replace("spotify:your:", "").split(":") +def _lookup_your(config, web_client, link): + parts = link.uri.replace("spotify:your:", "").split(":") if len(parts) != 1: return variant = parts[0] @@ -123,6 +97,6 @@ def _lookup_your(config, session, web_client, uri): album_ref = translator.web_to_album_ref(web_album) if album_ref is None: continue - sp_link = session.get_link(album_ref.uri) - if sp_link.type is spotify.LinkType.ALBUM: - yield from _lookup_album(config, sp_link) + web_link = WebLink.from_uri(album_ref.uri) + if web_link.type == LinkType.ALBUM: + yield from _lookup_album(config, web_client, web_link) diff --git a/mopidy_spotify/playlists.py b/mopidy_spotify/playlists.py index 09b83c1e..def97c6c 100644 --- a/mopidy_spotify/playlists.py +++ b/mopidy_spotify/playlists.py @@ -2,7 +2,6 @@ from mopidy import backend -import spotify from mopidy_spotify import translator, utils diff --git a/mopidy_spotify/translator.py b/mopidy_spotify/translator.py index afd8ce8c..c12062c7 100644 --- a/mopidy_spotify/translator.py +++ b/mopidy_spotify/translator.py @@ -313,6 +313,22 @@ def web_to_artist(web_artist): return models.Artist(uri=ref.uri, name=ref.name) +def web_to_album_tracks(web_album, bitrate=None): + album = web_to_album(web_album) + if album is None: + return [] + + web_tracks = web_album.get("tracks", {}).get("items", []) + if not isinstance(web_tracks, list): + return [] + + tracks = [ + web_to_track(web_track, bitrate, album) + for web_track in web_tracks + ] + return [t for t in tracks if t] + + def web_to_album(web_album): ref = web_to_album_ref(web_album) if ref is None: @@ -326,7 +342,7 @@ def web_to_album(web_album): return models.Album(uri=ref.uri, name=ref.name, artists=artists) -def web_to_track(web_track, bitrate=None): +def web_to_track(web_track, bitrate=None, album=None): ref = web_to_track_ref(web_track) if ref is None: return @@ -336,7 +352,8 @@ def web_to_track(web_track, bitrate=None): ] artists = [a for a in artists if a] - album = web_to_album(web_track.get("album", {})) + if album is None: + album = web_to_album(web_track.get("album", {})) return models.Track( uri=ref.uri, diff --git a/mopidy_spotify/web.py b/mopidy_spotify/web.py index d67a6382..68b8f77c 100644 --- a/mopidy_spotify/web.py +++ b/mopidy_spotify/web.py @@ -440,6 +440,30 @@ def get_user_playlists(self): for page in pages: yield from page.get("items", []) + def _with_all_tracks(self, obj, params=None): + if params is None: + params = {} + tracks_path = obj.get("tracks", {}).get("next") + track_pages = self.get_all( + tracks_path, + params=params, + ignore_expiry=obj.status_unchanged, + ) + + more_tracks = [] + for page in track_pages: + if "items" not in page: + return {} # Return nothing on error, or what we have so far? + more_tracks += page["items"] + + if more_tracks: + # Take a copy to avoid changing the cached response. + obj = copy.deepcopy(obj) + obj.setdefault("tracks", {}).setdefault("items", []) + obj["tracks"]["items"] += more_tracks + + return obj + def get_playlist(self, uri): try: parsed = WebLink.from_uri(uri) @@ -455,40 +479,47 @@ def get_playlist(self, uri): f"playlists/{parsed.id}", params={"fields": self.PLAYLIST_FIELDS, "market": "from_token"}, ) + return self._with_all_tracks(playlist, {"fields": self.TRACK_FIELDS}) + - tracks_path = playlist.get("tracks", {}).get("next") - track_pages = self.get_all( - tracks_path, - params={"fields": self.TRACK_FIELDS, "market": "from_token"}, - ignore_expiry=playlist.status_unchanged, - ) + def get_album(self, web_link): + if web_link.type != LinkType.ALBUM: + logger.error(f"Expecting Spotify album URI") + return {} - more_tracks = [] - for page in track_pages: - if "items" not in page: - return {} - more_tracks += page.get("items", []) - if more_tracks: - # Take a copy to avoid changing the cached response. - playlist = copy.deepcopy(playlist) - playlist.setdefault("tracks", {}).setdefault("items", []) - playlist["tracks"]["items"] += more_tracks + album = self.get_one( + f"albums/{web_link.id}", + params={"market": "from_token"}, + ) + return self._with_all_tracks(album) - return playlist + def get_artist_albums(self, web_link): + if web_link.type != LinkType.ARTIST: + logger.error(f"Expecting Spotify artist URI") + return [] - def get_track(self, uri): - try: - parsed = WebLink.from_uri(uri) - if parsed.type != LinkType.TRACK: - raise ValueError( - f"Could not parse {uri!r} as a Spotify track URI" - ) - except ValueError as exc: - logger.error(exc) + pages = self.get_all( + f"artists/{web_link.id}/albums", + params={"market": "from_token", "include_groups": "single,album"}, + ) + for page in pages: + for album in page["items"]: + try: + web_link = WebLink.from_uri(album.get("uri")) + except ValueError as exc: + logger.error(exc) + continue + full_album = self.get_album(web_link) + if full_album.get("is_playable", False): + yield full_album + + def get_track(self, web_link): + if web_link.type != LinkType.TRACK: + logger.error(f"Expecting Spotify track URI") return {} return self.get_one( - f"tracks/{parsed.id}", + f"tracks/{web_link.id}", params={"market": "from_token"} ) @@ -547,3 +578,8 @@ def from_uri(cls, uri): return cls(uri, LinkType.PLAYLIST, parts[3], parts[1]) raise ValueError(f"Could not parse {uri!r} as a Spotify URI") + + +class WebError(Exception): + def __init__(self, message): + super().__init__(message) diff --git a/tests/conftest.py b/tests/conftest.py index a37fef2b..5ba4def4 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -287,9 +287,9 @@ def sp_playlist_container_mock(): @pytest.fixture -def web_search_mock(web_album_mock, web_artist_mock, web_track_mock): +def web_search_mock(web_album_mock_base, web_artist_mock, web_track_mock): return { - "albums": {"items": [web_album_mock]}, + "albums": {"items": [web_album_mock_base]}, "artists": {"items": [web_artist_mock]}, "tracks": {"items": [web_track_mock, web_track_mock]}, } @@ -310,30 +310,67 @@ def web_artist_mock(): @pytest.fixture -def web_album_mock(web_artist_mock): +def web_track_mock_base(web_artist_mock): + return { + "artists": [web_artist_mock], + "disc_number": 1, + "duration_ms": 174300, + "name": "ABC 123", + "track_number": 7, + "uri": "spotify:track:abc", + "type": "track", + "is_playable": True, + } + + +@pytest.fixture +def web_album_mock_base(web_artist_mock): return { "name": "DEF 456", "uri": "spotify:album:def", "type": "album", + "album_type": "album", "artists": [web_artist_mock], } @pytest.fixture -def web_track_mock(web_artist_mock, web_album_mock): +def web_album_mock(web_album_mock_base, web_track_mock_base): + return { + **web_album_mock_base, + **{"tracks": {"items": [web_track_mock_base] * 10}}, + "is_playable": True, + } + + +@pytest.fixture +def web_album_mock_base2(web_artist_mock): return { - "album": web_album_mock, + "name": "XYZ 789", + "uri": "spotify:album:xyz", + "type": "album", + "album_type": "album", "artists": [web_artist_mock], - "disc_number": 1, - "duration_ms": 174300, - "name": "ABC 123", - "track_number": 7, - "uri": "spotify:track:abc", - "type": "track", + } + + +@pytest.fixture +def web_album_mock2(web_album_mock_base2, web_track_mock_base): + return { + **web_album_mock_base2, + **{"tracks": {"items": [web_track_mock_base] * 2}}, "is_playable": True, } +@pytest.fixture +def web_track_mock(web_track_mock_base, web_album_mock_base): + return { + **web_track_mock_base, + **{"album": web_album_mock_base}, + } + + @pytest.fixture def web_response_mock(web_track_mock): return web.WebResponse( diff --git a/tests/test_lookup.py b/tests/test_lookup.py index 2f1cea16..3d1e45a8 100644 --- a/tests/test_lookup.py +++ b/tests/test_lookup.py @@ -1,3 +1,4 @@ +import copy from unittest import mock import spotify @@ -17,106 +18,87 @@ def test_lookup_of_invalid_playlist_uri(provider, caplog): assert "Failed to lookup 'spotify:playlist': Could not parse" in caplog.text -def test_lookup_of_invalid_track_uri(session_mock, provider, caplog): - session_mock.get_link.side_effect = ValueError("an error message") +def test_lookup_of_invalid_track_uri(web_client_mock, provider, caplog): + web_client_mock.get_track.return_value = {} results = provider.lookup("spotify:track:invalid") assert len(results) == 0 assert ( - "Failed to lookup 'spotify:track:invalid': an error message" + "Failed to lookup Spotify track 'spotify:track:invalid': Invalid track response" in caplog.text ) -def test_lookup_of_unhandled_uri(session_mock, provider, caplog): - sp_link_mock = mock.Mock(spec=spotify.Link) - sp_link_mock.type = spotify.LinkType.INVALID - session_mock.get_link.return_value = sp_link_mock - - results = provider.lookup("spotify:artist:something") +def test_lookup_of_unhandled_uri(provider, caplog): + results = provider.lookup("spotify:invalid:something") assert len(results) == 0 assert ( - "Failed to lookup 'spotify:artist:something': " - "Cannot handle " in caplog.text + "Failed to lookup 'spotify:invalid:something': " + "Could not parse 'spotify:invalid:something' as a Spotify URI" in caplog.text ) -def test_lookup_when_offline(session_mock, sp_track_mock, provider, caplog): - session_mock.get_link.return_value = sp_track_mock.link - sp_track_mock.link.as_track.return_value.load.side_effect = spotify.Error( - "Must be online to load objects" - ) +def test_lookup_when_offline(web_client_mock, provider, caplog): + web_client_mock.logged_in = False - results = provider.lookup("spotify:track:abc") + results = provider.lookup("spotify:invalid:something") assert len(results) == 0 - assert ( - "Failed to lookup 'spotify:track:abc': Must be online to load objects" - in caplog.text - ) + assert "Failed to lookup" not in caplog.text -def test_lookup_of_track_uri(session_mock, sp_track_mock, provider): - session_mock.get_link.return_value = sp_track_mock.link +def test_lookup_of_track_uri(web_client_mock, web_track_mock, provider): + web_client_mock.get_track.return_value = web_track_mock results = provider.lookup("spotify:track:abc") - session_mock.get_link.assert_called_once_with("spotify:track:abc") - sp_track_mock.link.as_track.assert_called_once_with() - sp_track_mock.load.assert_called_once_with(10) - assert len(results) == 1 track = results[0] assert track.uri == "spotify:track:abc" assert track.name == "ABC 123" assert track.bitrate == 160 + assert track.album.name == "DEF 456" -def test_lookup_of_album_uri(session_mock, sp_album_browser_mock, provider): - sp_album_mock = sp_album_browser_mock.album - session_mock.get_link.return_value = sp_album_mock.link - +def test_lookup_of_album_uri(web_client_mock, web_album_mock, provider): + web_client_mock.get_album.return_value = web_album_mock + results = provider.lookup("spotify:album:def") - session_mock.get_link.assert_called_once_with("spotify:album:def") - sp_album_mock.link.as_album.assert_called_once_with() - - sp_album_mock.browse.assert_called_once_with() - sp_album_browser_mock.load.assert_called_once_with(10) - - assert len(results) == 2 + assert len(results) == 10 track = results[0] assert track.uri == "spotify:track:abc" assert track.name == "ABC 123" assert track.bitrate == 160 - - -def test_lookup_of_artist_uri( - session_mock, sp_artist_browser_mock, sp_album_browser_mock, provider -): - sp_artist_mock = sp_artist_browser_mock.artist - sp_album_mock = sp_album_browser_mock.album - session_mock.get_link.return_value = sp_artist_mock.link - + assert track.album.name == "DEF 456" + + +def test_lookup_of_artist_uri(web_track_mock, web_album_mock, web_client_mock, provider): + web_track_mock2 = copy.deepcopy(web_track_mock) + web_track_mock2['name'] = "XYZ track" + web_album_mock2 = copy.deepcopy(web_album_mock) + web_album_mock2['name'] = "XYZ album" + web_album_mock2['tracks']['items'] = [web_track_mock2] * 3 + + web_client_mock.get_artist_albums.return_value = [ + web_album_mock, web_album_mock2, + ] results = provider.lookup("spotify:artist:abba") - session_mock.get_link.assert_called_once_with("spotify:artist:abba") - sp_artist_mock.link.as_artist.assert_called_once_with() - - sp_artist_mock.browse.assert_called_once_with( - type=spotify.ArtistBrowserType.NO_TRACKS - ) - sp_artist_browser_mock.load.assert_called_once_with(10) - - assert sp_album_mock.browse.call_count == 2 - assert sp_album_browser_mock.load.call_count == 2 + assert len(results) == 13 - assert len(results) == 4 track = results[0] assert track.uri == "spotify:track:abc" assert track.name == "ABC 123" + assert track.album.name == "DEF 456" + assert track.bitrate == 160 + + track = results[10] + assert track.uri == "spotify:track:abc" + assert track.name == "XYZ track" + assert track.album.name == "XYZ album" assert track.bitrate == 160 @@ -160,15 +142,11 @@ def test_lookup_of_artist_uri_ignores_various_artists_albums( assert len(results) == 0 -def test_lookup_of_playlist_uri( - session_mock, web_client_mock, web_playlist_mock, sp_track_mock, provider -): +def test_lookup_of_playlist_uri(web_client_mock, web_playlist_mock, provider): web_client_mock.get_playlist.return_value = web_playlist_mock - session_mock.get_link.return_value = sp_track_mock.link results = provider.lookup("spotify:playlist:alice:foo") - session_mock.get_link.assert_called_once_with("spotify:track:abc") web_client_mock.get_playlist.assert_called_once_with( "spotify:playlist:alice:foo" ) @@ -180,20 +158,6 @@ def test_lookup_of_playlist_uri( assert track.bitrate == 160 -def test_lookup_of_playlist_uri_when_not_logged_in( - web_client_mock, provider, caplog -): - web_client_mock.user_id = None - - results = provider.lookup("spotify:playlist:alice:foo") - - assert len(results) == 0 - assert ( - "Failed to lookup 'spotify:playlist:alice:foo': " - "Playlist Web API lookup failed" in caplog.text - ) - - def test_lookup_of_yourtracks_uri(web_client_mock, web_track_mock, provider): web_saved_track_mock = {"track": web_track_mock} web_client_mock.get_all.return_value = [ @@ -208,50 +172,26 @@ def test_lookup_of_yourtracks_uri(web_client_mock, web_track_mock, provider): assert track.uri == "spotify:track:abc" assert track.name == "ABC 123" assert track.bitrate == 160 + assert track.album.name == "DEF 456" -def test_lookup_of_youralbums_uri( - session_mock, - web_client_mock, - web_album_mock, - sp_album_browser_mock, - provider, -): +def test_lookup_of_youralbums_uri(web_client_mock, web_album_mock, provider): web_saved_album_mock = {"album": web_album_mock} web_client_mock.get_all.return_value = [ {"items": [web_saved_album_mock, web_saved_album_mock]}, {"items": [web_saved_album_mock, web_saved_album_mock]}, ] - - sp_album_mock = sp_album_browser_mock.album - session_mock.get_link.return_value = sp_album_mock.link + web_client_mock.get_album.return_value = web_album_mock results = provider.lookup("spotify:your:albums") - get_link_call = mock.call("spotify:album:def") - assert session_mock.get_link.call_args_list == [ - get_link_call, - get_link_call, - get_link_call, - get_link_call, - ] - assert sp_album_mock.link.as_album.call_count == 4 - - assert sp_album_mock.browse.call_count == 4 - load_call = mock.call(10) - assert sp_album_browser_mock.load.call_args_list == [ - load_call, - load_call, - load_call, - load_call, - ] - - assert len(results) == 8 + assert len(results) == 4*10 for track in results: assert track.uri == "spotify:track:abc" assert track.name == "ABC 123" assert track.album.uri == "spotify:album:def" assert track.bitrate == 160 + assert track.album.name == "DEF 456" def test_lookup_of_your_uri_when_not_logged_in(web_client_mock, provider): diff --git a/tests/test_translator.py b/tests/test_translator.py index f40cb8bf..a0120315 100644 --- a/tests/test_translator.py +++ b/tests/test_translator.py @@ -788,6 +788,34 @@ def test_ignores_invalid_artists(self, web_album_mock, web_artist_mock): assert album.name == "DEF 456" assert list(album.artists) == artists + def test_returns_empty_artists_list_if_artist_is_empty(self, web_album_mock): + web_album_mock["artists"] = [] + + album = translator.web_to_album(web_album_mock) + + assert list(album.artists) == [] + + def test_caches_results(self, web_album_mock): + album1 = translator.web_to_album(web_album_mock) + album2 = translator.web_to_album(web_album_mock) + + assert album1 is album2 + + def test_web_to_album_tracks(self, web_album_mock): + tracks = translator.web_to_album_tracks(web_album_mock) + + assert len(tracks) == 10 + track = tracks[0] + assert track.album.name == "DEF 456" + assert track.album.artists == track.artists + + def test_web_to_album_tracks_empty(self, web_album_mock): + web_album_mock["tracks"]["items"] = [] + + tracks = translator.web_to_album_tracks(web_album_mock) + + assert len(tracks) == 0 + class TestWebToTrack: def test_calls_web_to_track_ref(self, web_track_mock): @@ -828,6 +856,14 @@ def test_sets_bitrate(self, web_track_mock): assert track.bitrate == 100 + def test_sets_specified_album(self, web_track_mock): + alt_album = models.Album(uri="spotify:album:xyz", name="XYZ 789") + + track = translator.web_to_track(web_track_mock, album=alt_album) + + assert track.album.uri == "spotify:album:xyz" + assert track.album.name == "XYZ 789" + def test_filters_out_none_artists(self, web_track_mock): web_track_mock["artists"].insert(0, {}) web_track_mock["artists"].insert(0, {"foo": "bar"}) diff --git a/tests/test_web.py b/tests/test_web.py index ef4cb4fb..2f522d04 100644 --- a/tests/test_web.py +++ b/tests/test_web.py @@ -4,6 +4,7 @@ import pytest import requests import responses +from responses import matchers import mopidy_spotify from mopidy_spotify import web @@ -556,17 +557,19 @@ def test_cache_normalised_query_string( mock_time, skip_refresh_token, oauth_client ): cache = {} + url = "https://api.spotify.com/v1/tracks/abc" responses.add( responses.GET, - "https://api.spotify.com/v1/tracks/abc?b=bar&f=foo", + url, json={"uri": "foobar"}, - match_querystring=True, + # match_querystring=True, + match=[matchers.query_string_matcher("b=bar&f=foo")], ) responses.add( responses.GET, - "https://api.spotify.com/v1/tracks/abc?b=bar&f=cat", + url, json={"uri": "cat"}, - match_querystring=True, + match=[matchers.query_string_matcher("b=bar&f=cat")], ) mock_time.return_value = 0 @@ -744,6 +747,44 @@ def foo_playlist(playlist_parms, foo_playlist_tracks): } +@pytest.fixture +def foo_album_next_tracks(): + params = urllib.parse.urlencode({"market": "from_token", "offset": 3}) + return { + "href": url(f"albums/foo/tracks?{params}"), + "items": [6, 7, 8], + "next": None, + } + + +@pytest.fixture +def foo_album(foo_album_next_tracks): + params = urllib.parse.urlencode({"market": "from_token"}) + return { + "href": url(f"albums/foo?{params}"), + "tracks": { + "href": url(f"albums/foo/tracks?{params}"), + "items": [3, 4, 5], + "next": foo_album_next_tracks["href"], + }, + } + + +@pytest.fixture +def foo_album_response(foo_album): + return web.WebResponse(foo_album["href"], foo_album) + + +@pytest.fixture +def artist_albums_mock(web_album_mock_base, web_album_mock_base2): + params = urllib.parse.urlencode({"market": "from_token", "include_groups": "single,album"}) + return { + "href": url(f"artists/abba/albums?{params}"), + "items": [web_album_mock_base, web_album_mock_base2], + "next": None, + } + + @pytest.mark.usefixtures("skip_refresh_token") class TestSpotifyOAuthClient: @pytest.mark.parametrize( @@ -915,6 +956,60 @@ def test_get_user_playlists(self, spotify_client): assert len(results) == 6 assert [f"playlist{i}" for i in range(6)] == results + @responses.activate + def test_with_all_tracks_error( + self, spotify_client, foo_album_response, foo_album_next_tracks, caplog + ): + responses.add( + responses.GET, + foo_album_response["tracks"]["next"], + json={"error": "baz"}, + ) + + result = spotify_client._with_all_tracks(foo_album_response) + + assert result == {} + assert "Spotify Web API request failed: baz" in caplog.text + + @responses.activate + def test_with_all_tracks(self, spotify_client, foo_album_response, foo_album_next_tracks): + responses.add( + responses.GET, + foo_album_next_tracks["href"], + json=foo_album_next_tracks, + ) + + result = spotify_client._with_all_tracks(foo_album_response) + + assert len(responses.calls) == 1 + assert result["tracks"]["items"] == [3, 4, 5, 6, 7, 8] + + @responses.activate + def test_with_all_tracks_uses_cached_tracks_when_unchanged( + self, mock_time, foo_album_response, foo_album_next_tracks, spotify_client + ): + responses.add( + responses.GET, + foo_album_next_tracks["href"], + json=foo_album_next_tracks, + ) + mock_time.return_value = -1000 + + result1 = spotify_client._with_all_tracks(foo_album_response) + + assert len(responses.calls) == 1 + cache_keys = list(spotify_client._cache.keys()) + assert len(cache_keys) == 1 + + responses.calls.reset() + mock_time.return_value = 1000 + + foo_album_response._status_code = 304 + result2 = spotify_client._with_all_tracks(foo_album_response) + + assert len(responses.calls) == 0 + assert result1 == result2 + @responses.activate @pytest.mark.parametrize( "uri,success", @@ -963,26 +1058,6 @@ def test_get_playlist_error(self, foo_playlist, spotify_client, caplog): assert result == {} assert "Spotify Web API request failed: bar" in caplog.text - @responses.activate - def test_get_playlist_tracks_error( - self, foo_playlist, foo_playlist_tracks, spotify_client, caplog - ): - responses.add( - responses.GET, - foo_playlist["href"], - json=foo_playlist, - ) - responses.add( - responses.GET, - foo_playlist_tracks["href"], - json={"error": "baz"}, - ) - - result = spotify_client.get_playlist("spotify:playlist:foo") - - assert result == {} - assert "Spotify Web API request failed: baz" in caplog.text - @responses.activate def test_get_playlist_sets_params_for_tracks( self, @@ -1032,46 +1107,6 @@ def test_get_playlist_collates_tracks( assert len(responses.calls) == 2 assert result["tracks"]["items"] == [1, 2, 3, 4, 5] - @responses.activate - def test_get_playlist_uses_cached_tracks_when_unchanged( - self, mock_time, foo_playlist, foo_playlist_tracks, spotify_client - ): - responses.add( - responses.GET, - foo_playlist["href"], - json=foo_playlist, - status=304, - ) - responses.add( - responses.GET, - foo_playlist_tracks["href"], - json=foo_playlist_tracks, - ) - mock_time.return_value = -1000 - - result1 = spotify_client.get_playlist("spotify:playlist:foo") - - assert len(responses.calls) == 2 - assert result1["tracks"]["items"] == [1, 2, 3, 4, 5] - cache_keys = list(spotify_client._cache.keys()) - assert len(cache_keys) == 2 - assert cache_keys[0].startswith("playlists/foo?") - assert cache_keys[1].startswith(url("playlists/foo/tracks?")) - - # The requested and cached URIs differ where we specified a relative URI but - # match where we used the full URI in the response: - assert cache_keys[0] != responses.calls[1].request.url - assert cache_keys[1] == responses.calls[1].request.url - - responses.calls.reset() - mock_time.return_value = 1000 - - result2 = spotify_client.get_playlist("spotify:playlist:foo") - - assert len(responses.calls) == 1 - assert responses.calls[0].request.url == foo_playlist["href"] - assert result1["tracks"]["items"] == result2["tracks"]["items"] - @pytest.mark.parametrize( "uri,msg", [ @@ -1098,6 +1133,113 @@ def test_logged_in(self, spotify_client, user_id, expected): assert spotify_client.logged_in is expected + @responses.activate + def test_get_album(self, foo_album, foo_album_next_tracks, spotify_client): + responses.add( + responses.GET, + url("albums/foo"), + json=foo_album, + match=[matchers.query_string_matcher("market=from_token")], + ) + responses.add( + responses.GET, + url("albums/foo/tracks"), + json=foo_album_next_tracks, + ) + + link = web.WebLink.from_uri("spotify:album:foo") + result = spotify_client.get_album(link) + + assert len(responses.calls) == 2 + assert result["tracks"]["items"] == [3, 4, 5, 6, 7, 8] + + @responses.activate + def test_get_artist_albums(self, artist_albums_mock, web_album_mock, web_album_mock2, spotify_client): + responses.add( + responses.GET, + url("artists/abba/albums"), + json=artist_albums_mock, + match=[matchers.query_string_matcher("market=from_token&include_groups=single%2Calbum")], + ) + responses.add( + responses.GET, + url(f"albums/def"), + json=web_album_mock, + ) + responses.add( + responses.GET, + url(f"albums/xyz"), + json=web_album_mock2, + ) + + link = web.WebLink.from_uri("spotify:artist:abba") + results = list(spotify_client.get_artist_albums(link)) + + assert len(responses.calls) == 3 + + assert len(results) == 2 + assert results[0]["name"] == "DEF 456" + assert results[1]["name"] == "XYZ 789" + + @responses.activate + def test_get_artist_albums_ignores_unplayable(self, artist_albums_mock, web_album_mock, web_album_mock2, spotify_client): + web_album_mock["is_playable"] = False + responses.add( + responses.GET, + url(f"artists/abba/albums"), + json=artist_albums_mock, + ) + responses.add( + responses.GET, + url(f"albums/def"), + json=web_album_mock, + ) + responses.add( + responses.GET, + url(f"albums/xyz"), + json=web_album_mock2, + ) + + link = web.WebLink.from_uri("spotify:artist:abba") + results = list(spotify_client.get_artist_albums(link)) + + assert len(responses.calls) == 3 + assert len(results) == 1 + assert results[0]["name"] == "XYZ 789" + + @responses.activate + @pytest.mark.parametrize( + "uri,success", + [ + ("spotify:track:abc", True), + ("spotify:track:xyz", False), + ("spotify:user:alice:playlist:bar", False), + ("spotify:playlist:bar", False), + ("spotify:artist:baz", False), + ("spotify:album:foo", False), + ], + ) + def test_get_track(self, web_track_mock, spotify_client, uri, success): + responses.add( + responses.GET, + url(f"tracks/abc"), + json=web_track_mock, + ) + responses.add( + responses.GET, + url(f"tracks/xyz"), + json={}, + ) + + link = web.WebLink.from_uri(uri) + result = spotify_client.get_track(link) + + if success: + assert len(responses.calls) == 1 + assert result == web_track_mock + else: + assert result == {} + @pytest.mark.parametrize( "uri,type_,id_", From 493a61fab27a183d96bbe70937d8bbe64bee21e1 Mon Sep 17 00:00:00 2001 From: 3np <3np@example.com> Date: Thu, 16 Jun 2022 00:36:24 +0900 Subject: [PATCH 03/21] Handle empty session --- mopidy_spotify/browse.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/mopidy_spotify/browse.py b/mopidy_spotify/browse.py index d8f31690..37a579d6 100644 --- a/mopidy_spotify/browse.py +++ b/mopidy_spotify/browse.py @@ -95,7 +95,7 @@ def _browse_playlist(web_client, uri, config): def _browse_album(session, uri, config): - if session.connection.state is not spotify.ConnectionState.LOGGED_IN: + if session is None or session.connection.state is not spotify.ConnectionState.LOGGED_IN: return [] sp_album_browser = session.get_album(uri).browse() @@ -108,7 +108,7 @@ def _browse_album(session, uri, config): def _browse_artist(session, uri, config): - if session.connection.state is not spotify.ConnectionState.LOGGED_IN: + if session is None or session.connection.state is not spotify.ConnectionState.LOGGED_IN: return [] sp_artist_browser = session.get_artist(uri).browse( @@ -176,6 +176,8 @@ def _browse_toplist_user(web_client, variant): def _browse_toplist(config, session, variant, region): + if session is None: + return [] if region == "countries": codes = config["toplist_countries"] if not codes: From 94ab1449dea35d0ec0a19d3a2427ffb1d23a285f Mon Sep 17 00:00:00 2001 From: 3np <3np@example.com> Date: Thu, 16 Jun 2022 00:36:51 +0900 Subject: [PATCH 04/21] Fix lookup call in search --- mopidy_spotify/search.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/mopidy_spotify/search.py b/mopidy_spotify/search.py index 43fdc8f2..7e940e6e 100644 --- a/mopidy_spotify/search.py +++ b/mopidy_spotify/search.py @@ -110,7 +110,7 @@ def search( def _search_by_uri(config, session, web_client, query): tracks = [] for uri in query["uri"]: - tracks += lookup.lookup(config, session, web_client, uri) + tracks += lookup.lookup(config, web_client, uri) uri = "spotify:search" if len(query["uri"]) == 1: From 6999ab7e20f4cbe3854ce2678673cb070b9ba75f Mon Sep 17 00:00:00 2001 From: Nick Steel Date: Thu, 16 Jun 2022 01:58:28 +0100 Subject: [PATCH 05/21] Remove pyspotify: search --- mopidy_spotify/library.py | 1 - mopidy_spotify/search.py | 7 +++---- tests/test_search.py | 16 +++++++--------- 3 files changed, 10 insertions(+), 14 deletions(-) diff --git a/mopidy_spotify/library.py b/mopidy_spotify/library.py index 06ae7757..05ca19ac 100644 --- a/mopidy_spotify/library.py +++ b/mopidy_spotify/library.py @@ -40,7 +40,6 @@ def lookup(self, uri): def search(self, query=None, uris=None, exact=False): return search.search( self._config, - self._backend._session, self._backend._web_client, query, uris, diff --git a/mopidy_spotify/search.py b/mopidy_spotify/search.py index 7e940e6e..74413a17 100644 --- a/mopidy_spotify/search.py +++ b/mopidy_spotify/search.py @@ -13,7 +13,6 @@ def search( config, - session, web_client, query=None, uris=None, @@ -27,7 +26,7 @@ def search( return models.SearchResult(uri="spotify:search") if "uri" in query: - return _search_by_uri(config, session, web_client, query) + return _search_by_uri(config, web_client, query) sp_query = translator.sp_search_query(query, exact) if not sp_query: @@ -37,7 +36,7 @@ def search( uri = f"spotify:search:{urllib.parse.quote(sp_query)}" logger.info(f"Searching Spotify for: {sp_query}") - if session.connection.state is not spotify.ConnectionState.LOGGED_IN: + if web_client is None or not web_client.logged_in: logger.info("Spotify search aborted: Spotify is offline") return models.SearchResult(uri=uri) @@ -107,7 +106,7 @@ def search( ) -def _search_by_uri(config, session, web_client, query): +def _search_by_uri(config, web_client, query): tracks = [] for uri in query["uri"]: tracks += lookup.lookup(config, web_client, uri) diff --git a/tests/test_search.py b/tests/test_search.py index 6bca63e9..7328fc16 100644 --- a/tests/test_search.py +++ b/tests/test_search.py @@ -1,6 +1,5 @@ from mopidy import models -import spotify from mopidy_spotify import search @@ -22,8 +21,8 @@ def test_search_with_empty_query_returns_nothing(provider, caplog): assert "Ignored search with empty query" in caplog.text -def test_search_by_single_uri(session_mock, sp_track_mock, provider): - session_mock.get_link.return_value = sp_track_mock.link +def test_search_by_single_uri(web_client_mock, web_track_mock, provider): + web_client_mock.get_track.return_value = web_track_mock result = provider.search({"uri": ["spotify:track:abc"]}) @@ -36,8 +35,8 @@ def test_search_by_single_uri(session_mock, sp_track_mock, provider): assert track.bitrate == 160 -def test_search_by_multiple_uris(session_mock, sp_track_mock, provider): - session_mock.get_link.return_value = sp_track_mock.link +def test_search_by_multiple_uris(web_client_mock, web_track_mock, provider): + web_client_mock.get_track.return_value = web_track_mock result = provider.search( {"uri": ["spotify:track:abc", "spotify:track:abc"]} @@ -52,8 +51,8 @@ def test_search_by_multiple_uris(session_mock, sp_track_mock, provider): assert track.bitrate == 160 -def test_search_when_offline_returns_nothing(session_mock, provider, caplog): - session_mock.connection.state = spotify.ConnectionState.OFFLINE +def test_search_when_offline_returns_nothing(web_client_mock, provider, caplog): + web_client_mock.logged_in = False result = provider.search({"any": ["ABBA"]}) @@ -184,13 +183,12 @@ def test_sets_api_limit_to_track_count_when_max( def test_sets_types_parameter( - web_client_mock, web_search_mock_large, provider, config, session_mock + web_client_mock, web_search_mock_large, provider, config ): web_client_mock.get.return_value = web_search_mock_large search.search( config["spotify"], - session_mock, web_client_mock, {"any": ["ABBA"]}, types=["album", "artist"], From b1827a3a2fc0447e53d36ca3d6124298b6594aa3 Mon Sep 17 00:00:00 2001 From: Nick Steel Date: Thu, 16 Jun 2022 02:59:15 +0100 Subject: [PATCH 06/21] Remove pyspotify: dictinct --- mopidy_spotify/distinct.py | 55 +++++++++++++++--------------------- mopidy_spotify/library.py | 2 +- mopidy_spotify/lookup.py | 8 ++---- mopidy_spotify/translator.py | 3 ++ mopidy_spotify/web.py | 4 +-- tests/conftest.py | 10 +------ tests/test_distinct.py | 24 +++++++--------- tests/test_lookup.py | 12 ++++---- tests/test_translator.py | 7 +++++ tests/test_web.py | 11 ++++---- 10 files changed, 60 insertions(+), 76 deletions(-) diff --git a/mopidy_spotify/distinct.py b/mopidy_spotify/distinct.py index 684036df..61c254f0 100644 --- a/mopidy_spotify/distinct.py +++ b/mopidy_spotify/distinct.py @@ -1,50 +1,49 @@ import logging -import spotify from mopidy_spotify import search logger = logging.getLogger(__name__) -def get_distinct(config, session, web_client, field, query=None): +def get_distinct(config, playlists, web_client, field, query=None): # To make the returned data as interesting as possible, we limit # ourselves to data extracted from the user's playlists when no search # query is included. if field == "artist": - result = _get_distinct_artists(config, session, web_client, query) + result = _get_distinct_artists(config, playlists, web_client, query) elif field == "albumartist": - result = _get_distinct_albumartists(config, session, web_client, query) + result = _get_distinct_albumartists(config, playlists, web_client, query) elif field == "album": - result = _get_distinct_albums(config, session, web_client, query) + result = _get_distinct_albums(config, playlists, web_client, query) elif field == "date": - result = _get_distinct_dates(config, session, web_client, query) + result = _get_distinct_dates(config, playlists, web_client, query) else: result = set() return result - {None} -def _get_distinct_artists(config, session, web_client, query): +def _get_distinct_artists(config, playlists, web_client, query): logger.debug(f"Getting distinct artists: {query}") if query: search_result = _get_search( - config, session, web_client, query, artist=True + config, web_client, query, artist=True ) return {artist.name for artist in search_result.artists} else: return { artist.name - for track in _get_playlist_tracks(config, session) + for track in _get_playlist_tracks(config, playlists, web_client) for artist in track.artists } -def _get_distinct_albumartists(config, session, web_client, query): +def _get_distinct_albumartists(config, playlists, web_client, query): logger.debug(f"Getting distinct albumartists: {query}") if query: search_result = _get_search( - config, session, web_client, query, album=True + config, web_client, query, album=True ) return { artist.name @@ -55,31 +54,31 @@ def _get_distinct_albumartists(config, session, web_client, query): else: return { track.album.artist.name - for track in _get_playlist_tracks(config, session) + for track in _get_playlist_tracks(config, playlists, web_client) if track.album and track.album.artist } -def _get_distinct_albums(config, session, web_client, query): +def _get_distinct_albums(config, playlists, web_client, query): logger.debug(f"Getting distinct albums: {query}") if query: search_result = _get_search( - config, session, web_client, query, album=True + config, web_client, query, album=True ) return {album.name for album in search_result.albums} else: return { track.album.name - for track in _get_playlist_tracks(config, session) + for track in _get_playlist_tracks(config, playlists, web_client) if track.album } -def _get_distinct_dates(config, session, web_client, query): +def _get_distinct_dates(config, playlists, web_client, query): logger.debug(f"Getting distinct album years: {query}") if query: search_result = _get_search( - config, session, web_client, query, album=True + config, web_client, query, album=True ) return { album.date @@ -89,13 +88,13 @@ def _get_distinct_dates(config, session, web_client, query): else: return { f"{track.album.year}" - for track in _get_playlist_tracks(config, session) + for track in _get_playlist_tracks(config, playlists, web_client) if track.album and track.album.year not in (None, 0) } def _get_search( - config, session, web_client, query, album=False, artist=False, track=False + config, web_client, query, album=False, artist=False, track=False ): types = [] @@ -106,20 +105,12 @@ def _get_search( if track: types.append("track") - return search.search(config, session, web_client, query, types=types) + return search.search(config, web_client, query, types=types) -def _get_playlist_tracks(config, session): - if not config["allow_playlists"]: +def _get_playlist_tracks(config, playlists, web_client): + if not playlists or not config["allow_playlists"]: return - for playlist in session.playlist_container: - if not isinstance(playlist, spotify.Playlist): - continue - playlist.load(config["timeout"]) - for track in playlist.tracks: - try: - track.load(config["timeout"]) - yield track - except spotify.Error: # TODO Why did we get "General error"? - continue + for playlist_ref in playlists.as_list(): + yield from playlist.get_items(playlist_ref.uri) diff --git a/mopidy_spotify/library.py b/mopidy_spotify/library.py index 05ca19ac..f2024d4f 100644 --- a/mopidy_spotify/library.py +++ b/mopidy_spotify/library.py @@ -25,7 +25,7 @@ def browse(self, uri): def get_distinct(self, field, query=None): return distinct.get_distinct( self._config, - self._backend._session, + self._backend.playlists, self._backend._web_client, field, query, diff --git a/mopidy_spotify/lookup.py b/mopidy_spotify/lookup.py index a34f05b2..dc9e9598 100644 --- a/mopidy_spotify/lookup.py +++ b/mopidy_spotify/lookup.py @@ -54,17 +54,13 @@ def _lookup_album(config, web_client, link): if web_album == {}: raise WebError("Invalid album response") - tracks = translator.web_to_album_tracks(web_album, bitrate=config["bitrate"]) - for track in tracks: - yield track + yield from translator.web_to_album_tracks(web_album, bitrate=config["bitrate"]) def _lookup_artist(config, web_client, link): web_albums = web_client.get_artist_albums(link) for web_album in web_albums: - tracks = translator.web_to_album_tracks(web_album, bitrate=config["bitrate"]) - for track in tracks: - yield track + yield from translator.web_to_album_tracks(web_album, bitrate=config["bitrate"]) def _lookup_playlist(config, web_client, link): diff --git a/mopidy_spotify/translator.py b/mopidy_spotify/translator.py index c12062c7..d180fa8e 100644 --- a/mopidy_spotify/translator.py +++ b/mopidy_spotify/translator.py @@ -318,6 +318,9 @@ def web_to_album_tracks(web_album, bitrate=None): if album is None: return [] + if not web_album.get("is_playable", False): + return [] + web_tracks = web_album.get("tracks", {}).get("items", []) if not isinstance(web_tracks, list): return [] diff --git a/mopidy_spotify/web.py b/mopidy_spotify/web.py index 68b8f77c..ee01090a 100644 --- a/mopidy_spotify/web.py +++ b/mopidy_spotify/web.py @@ -509,9 +509,7 @@ def get_artist_albums(self, web_link): except ValueError as exc: logger.error(exc) continue - full_album = self.get_album(web_link) - if full_album.get("is_playable", False): - yield full_album + yield self.get_album(web_link) def get_track(self, web_link): if web_link.type != LinkType.TRACK: diff --git a/tests/conftest.py b/tests/conftest.py index 5ba4def4..d34ac1db 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -424,13 +424,6 @@ def mopidy_album_mock(mopidy_artist_mock): ) -@pytest.fixture -def session_mock(): - sp_session_mock = mock.Mock(spec=spotify.Session) - sp_session_mock.connection.state = spotify.ConnectionState.LOGGED_IN - return sp_session_mock - - @pytest.fixture def web_client_mock(): web_client_mock = mock.MagicMock(spec=web.SpotifyOAuthClient) @@ -440,10 +433,9 @@ def web_client_mock(): @pytest.fixture -def backend_mock(session_mock, config, web_client_mock): +def backend_mock(config, web_client_mock): backend_mock = mock.Mock(spec=backend.SpotifyBackend) backend_mock._config = config - backend_mock._session = session_mock backend_mock._bitrate = 160 backend_mock._web_client = web_client_mock return backend_mock diff --git a/tests/test_distinct.py b/tests/test_distinct.py index 88de6846..afc98b4a 100644 --- a/tests/test_distinct.py +++ b/tests/test_distinct.py @@ -7,19 +7,15 @@ @pytest.fixture -def session_mock_with_playlists( - session_mock, - sp_playlist_mock, - sp_unloaded_playlist_mock, - sp_playlist_folder_start_mock, - sp_playlist_folder_end_mock, +def web_client_mock_with_playlists( + web_client_mock, + web_playlist_mock, ): - session_mock.playlist_container = [ - sp_playlist_folder_start_mock, - sp_playlist_mock, - sp_unloaded_playlist_mock, - sp_playlist_folder_end_mock, + web_client_mock.get_user_playlists.return_value = [ + web_playlist_mock, + {}, + web_playlist_mock, ] return session_mock @@ -52,7 +48,7 @@ def test_get_distinct_unsupported_field_types_returns_nothing(provider, field): ], ) def test_get_distinct_without_query_when_playlists_enabled( - session_mock_with_playlists, provider, field, expected + web_client_mock_with_playlists, provider, field, expected ): assert provider.get_distinct(field) == expected @@ -98,10 +94,10 @@ def test_get_distinct_without_query_returns_nothing_when_playlists_disabled( ], ) def test_get_distinct_with_query( - search_mock, provider, config, session_mock, field, query, expected, types + search_mock, provider, config, web_client_mock, field, query, expected, types ): assert provider.get_distinct(field, query) == expected search_mock.search.assert_called_once_with( - mock.ANY, mock.ANY, mock.ANY, query, types=types + mock.ANY, mock.ANY, query, types=types ) diff --git a/tests/test_lookup.py b/tests/test_lookup.py index 3d1e45a8..65222643 100644 --- a/tests/test_lookup.py +++ b/tests/test_lookup.py @@ -103,16 +103,16 @@ def test_lookup_of_artist_uri(web_track_mock, web_album_mock, web_client_mock, p def test_lookup_of_artist_ignores_unavailable_albums( - session_mock, sp_artist_browser_mock, sp_album_browser_mock, provider + web_client_mock, web_album_mock, web_album_mock2, provider ): - sp_artist_mock = sp_artist_browser_mock.artist - session_mock.get_link.return_value = sp_artist_mock.link - sp_album_mock = sp_album_browser_mock.album - sp_album_mock.is_available = False + web_album_mock["is_playable"] = False + web_client_mock.get_artist_albums.return_value = [ + web_album_mock, web_album_mock2, + ] results = provider.lookup("spotify:artist:abba") - assert len(results) == 0 + assert len(results) == 2 def test_lookup_of_artist_uri_ignores_compilations( diff --git a/tests/test_translator.py b/tests/test_translator.py index a0120315..460158f1 100644 --- a/tests/test_translator.py +++ b/tests/test_translator.py @@ -816,6 +816,13 @@ def test_web_to_album_tracks_empty(self, web_album_mock): assert len(tracks) == 0 + def test_web_to_album_tracks_unplayable(self, web_album_mock): + web_album_mock["is_playable"] = False + + tracks = translator.web_to_album_tracks(web_album_mock) + + assert len(tracks) == 0 + class TestWebToTrack: def test_calls_web_to_track_ref(self, web_track_mock): diff --git a/tests/test_web.py b/tests/test_web.py index 2f522d04..1a4a17fe 100644 --- a/tests/test_web.py +++ b/tests/test_web.py @@ -1182,8 +1182,7 @@ def test_get_artist_albums(self, artist_albums_mock, web_album_mock, web_album_m assert results[1]["name"] == "XYZ 789" @responses.activate - def test_get_artist_albums_ignores_unplayable(self, artist_albums_mock, web_album_mock, web_album_mock2, spotify_client): - web_album_mock["is_playable"] = False + def test_get_artist_albums_error(self, artist_albums_mock, web_album_mock, spotify_client, caplog): responses.add( responses.GET, url(f"artists/abba/albums"), @@ -1197,15 +1196,17 @@ def test_get_artist_albums_ignores_unplayable(self, artist_albums_mock, web_albu responses.add( responses.GET, url(f"albums/xyz"), - json=web_album_mock2, + json={"error": "bar"}, ) link = web.WebLink.from_uri("spotify:artist:abba") results = list(spotify_client.get_artist_albums(link)) assert len(responses.calls) == 3 - assert len(results) == 1 - assert results[0]["name"] == "XYZ 789" + assert len(results) == 2 + assert results[0]["name"] == "DEF 456" + assert results[1] == {} + assert "Spotify Web API request failed: bar" in caplog.text @responses.activate @pytest.mark.parametrize( From fd6847a4ec18e5bbe64b699346b55f332d59939d Mon Sep 17 00:00:00 2001 From: Nick Steel Date: Mon, 20 Jun 2022 23:18:20 +0100 Subject: [PATCH 07/21] Remove pyspotify: dictinct part 2 --- mopidy_spotify/distinct.py | 15 ++++++++++----- tests/test_distinct.py | 20 ++++++++++++++++---- 2 files changed, 26 insertions(+), 9 deletions(-) diff --git a/mopidy_spotify/distinct.py b/mopidy_spotify/distinct.py index 61c254f0..abecff0c 100644 --- a/mopidy_spotify/distinct.py +++ b/mopidy_spotify/distinct.py @@ -9,6 +9,7 @@ def get_distinct(config, playlists, web_client, field, query=None): # To make the returned data as interesting as possible, we limit # ourselves to data extracted from the user's playlists when no search # query is included. + # TODO: Perhaps should use tracks from My Music instead? if field == "artist": result = _get_distinct_artists(config, playlists, web_client, query) @@ -53,9 +54,10 @@ def _get_distinct_albumartists(config, playlists, web_client, query): } else: return { - track.album.artist.name + artists.name for track in _get_playlist_tracks(config, playlists, web_client) - if track.album and track.album.artist + for artists in track.album.artists + if track.album and track.album.artists } @@ -87,9 +89,9 @@ def _get_distinct_dates(config, playlists, web_client, query): } else: return { - f"{track.album.year}" + f"{track.album.date}" for track in _get_playlist_tracks(config, playlists, web_client) - if track.album and track.album.year not in (None, 0) + if track.album and track.album.date not in (None, 0) } @@ -113,4 +115,7 @@ def _get_playlist_tracks(config, playlists, web_client): return for playlist_ref in playlists.as_list(): - yield from playlist.get_items(playlist_ref.uri) + playlist = playlists.lookup(playlist_ref.uri) + if playlist: + for track in playlist.tracks: + yield track diff --git a/tests/test_distinct.py b/tests/test_distinct.py index afc98b4a..822b11e6 100644 --- a/tests/test_distinct.py +++ b/tests/test_distinct.py @@ -3,7 +3,7 @@ import pytest from mopidy import models -from mopidy_spotify import distinct, search +from mopidy_spotify import distinct, search, playlists @pytest.fixture @@ -11,13 +11,13 @@ def web_client_mock_with_playlists( web_client_mock, web_playlist_mock, ): - web_client_mock.get_user_playlists.return_value = [ web_playlist_mock, {}, web_playlist_mock, ] - return session_mock + web_client_mock.get_playlist.return_value = web_playlist_mock + return web_client_mock @pytest.fixture @@ -44,16 +44,28 @@ def test_get_distinct_unsupported_field_types_returns_nothing(provider, field): ("artist", {"ABBA"}), ("albumartist", {"ABBA"}), ("album", {"DEF 456"}), - ("date", {"2001"}), ], ) +# ("date", {"2001"}), def test_get_distinct_without_query_when_playlists_enabled( web_client_mock_with_playlists, provider, field, expected ): + provider._backend.playlists = playlists.SpotifyPlaylistsProvider(backend=provider._backend) + provider._backend.playlists._loaded = True assert provider.get_distinct(field) == expected +def test_get_distinct_without_query_returns_nothing_when_playlists_empty( + web_client_mock_with_playlists, provider +): + provider._backend.playlists = playlists.SpotifyPlaylistsProvider(backend=provider._backend) + provider._backend.playlists._loaded = True + web_client_mock_with_playlists.get_playlist.return_value = {} + + assert provider.get_distinct("artist") == set() + + @pytest.mark.parametrize("field", ["artist", "albumartist", "album", "date"]) def test_get_distinct_without_query_returns_nothing_when_playlists_disabled( provider, config, field From d213f1a7a453f548f2dbdb94a1dc4aa684a59e96 Mon Sep 17 00:00:00 2001 From: Nick Steel Date: Mon, 20 Jun 2022 23:42:08 +0100 Subject: [PATCH 08/21] Remove pyspotify: lookup filters compilations and various artists --- mopidy_spotify/lookup.py | 11 +++++++++++ tests/test_lookup.py | 17 ++++++----------- 2 files changed, 17 insertions(+), 11 deletions(-) diff --git a/mopidy_spotify/lookup.py b/mopidy_spotify/lookup.py index dc9e9598..1ec97cf9 100644 --- a/mopidy_spotify/lookup.py +++ b/mopidy_spotify/lookup.py @@ -5,6 +5,8 @@ logger = logging.getLogger(__name__) +_VARIOUS_ARTISTS_URI = "spotify:artist:0LyfQWJT6nXafLPZqxe9Of" + def lookup(config, web_client, uri): if web_client is None or not web_client.logged_in: @@ -60,6 +62,15 @@ def _lookup_album(config, web_client, link): def _lookup_artist(config, web_client, link): web_albums = web_client.get_artist_albums(link) for web_album in web_albums: + is_various_artists = False + if web_album.get("album_type", "") == "compilation": + continue + for artist in web_album.get("artists", []): + if artist.get("uri") == _VARIOUS_ARTISTS_URI: + is_various_artists = True + break + if is_various_artists: + continue yield from translator.web_to_album_tracks(web_album, bitrate=config["bitrate"]) diff --git a/tests/test_lookup.py b/tests/test_lookup.py index 65222643..f90296c7 100644 --- a/tests/test_lookup.py +++ b/tests/test_lookup.py @@ -116,12 +116,10 @@ def test_lookup_of_artist_ignores_unavailable_albums( def test_lookup_of_artist_uri_ignores_compilations( - session_mock, sp_artist_browser_mock, sp_album_browser_mock, provider + web_client_mock, web_album_mock, provider ): - sp_artist_mock = sp_artist_browser_mock.artist - session_mock.get_link.return_value = sp_artist_mock.link - sp_album_mock = sp_album_browser_mock.album - sp_album_mock.type = spotify.AlbumType.COMPILATION + web_album_mock["album_type"] = "compilation" + web_client_mock.get_artist_albums.return_value = [web_album_mock] results = provider.lookup("spotify:artist:abba") @@ -129,13 +127,10 @@ def test_lookup_of_artist_uri_ignores_compilations( def test_lookup_of_artist_uri_ignores_various_artists_albums( - session_mock, sp_artist_browser_mock, sp_album_browser_mock, provider + web_client_mock, web_album_mock, provider ): - sp_artist_mock = sp_artist_browser_mock.artist - session_mock.get_link.return_value = sp_artist_mock.link - sp_album_browser_mock.album.artist.link.uri = ( - "spotify:artist:0LyfQWJT6nXafLPZqxe9Of" - ) + web_album_mock["artists"][0]["uri"] = "spotify:artist:0LyfQWJT6nXafLPZqxe9Of" + web_client_mock.get_artist_albums.return_value = [web_album_mock] results = provider.lookup("spotify:artist:abba") From 7300bd834b22a0a14b3daa6610bb97d57a64577a Mon Sep 17 00:00:00 2001 From: Nick Steel Date: Sat, 25 Jun 2022 23:12:16 +0100 Subject: [PATCH 09/21] Remove pyspotify: dictinct offline check --- mopidy_spotify/distinct.py | 2 ++ tests/test_distinct.py | 8 ++++++++ 2 files changed, 10 insertions(+) diff --git a/mopidy_spotify/distinct.py b/mopidy_spotify/distinct.py index abecff0c..d1224ed7 100644 --- a/mopidy_spotify/distinct.py +++ b/mopidy_spotify/distinct.py @@ -10,6 +10,8 @@ def get_distinct(config, playlists, web_client, field, query=None): # ourselves to data extracted from the user's playlists when no search # query is included. # TODO: Perhaps should use tracks from My Music instead? + if web_client is None or not web_client.logged_in: + return set() if field == "artist": result = _get_distinct_artists(config, playlists, web_client, query) diff --git a/tests/test_distinct.py b/tests/test_distinct.py index 822b11e6..4a3eb20d 100644 --- a/tests/test_distinct.py +++ b/tests/test_distinct.py @@ -31,6 +31,14 @@ def search_mock(mopidy_album_mock, mopidy_artist_mock): patcher.stop() +def test_get_distinct_when_offine(web_client_mock, provider): + web_client_mock.logged_in = False + + results = provider.get_distinct("artist") + + assert results == set() + + @pytest.mark.parametrize( "field", ["composer", "performer", "genre", "unknown-field-type"] ) From fb51baa7ea0f1d7e36b9ae5a56ea10cb9354e222 Mon Sep 17 00:00:00 2001 From: Nick Steel Date: Sun, 26 Jun 2022 00:31:26 +0100 Subject: [PATCH 10/21] Album ref name should include artist name --- mopidy_spotify/translator.py | 15 ++++++++++++--- tests/test_browse.py | 2 +- tests/test_translator.py | 13 ++++++++++--- 3 files changed, 23 insertions(+), 7 deletions(-) diff --git a/mopidy_spotify/translator.py b/mopidy_spotify/translator.py index d180fa8e..fb9b640c 100644 --- a/mopidy_spotify/translator.py +++ b/mopidy_spotify/translator.py @@ -110,8 +110,16 @@ def web_to_album_ref(web_album): if not valid_web_data(web_album, "album"): return - uri = web_album["uri"] - return models.Ref.album(uri=uri, name=web_album.get("name", uri)) + if "name" in web_album: + artists = web_album.get("artists", []) + if artists and artists[0].get("name"): + name = f"{artists[0].get('name')} - {web_album['name']}" + else: + name = web_album["name"] + else: + name = web_album["uri"] + + return models.Ref.album(uri=web_album["uri"], name=name) def web_to_album_refs(web_albums): @@ -342,7 +350,8 @@ def web_to_album(web_album): ] artists = [a for a in artists if a] - return models.Album(uri=ref.uri, name=ref.name, artists=artists) + name = web_album.get("name", "Unknown album") + return models.Album(uri=ref.uri, name=name, artists=artists) def web_to_track(web_track, bitrate=None, album=None): diff --git a/tests/test_browse.py b/tests/test_browse.py index 9b2bc277..e66f18f2 100644 --- a/tests/test_browse.py +++ b/tests/test_browse.py @@ -511,7 +511,7 @@ def test_browse_your_music_albums(web_client_mock, web_album_mock, provider): ) assert results == [results[0]] * 4 assert results[0] == models.Ref.album( - uri="spotify:album:def", name="DEF 456" + uri="spotify:album:def", name="ABBA - DEF 456" ) diff --git a/tests/test_translator.py b/tests/test_translator.py index 460158f1..4686d9ab 100644 --- a/tests/test_translator.py +++ b/tests/test_translator.py @@ -323,6 +323,13 @@ def test_successful_translation(self, web_album_mock): assert ref.type == models.Ref.ALBUM assert ref.uri == "spotify:album:def" + assert ref.name == "ABBA - DEF 456" + + def test_without_artists_uses_name(self, web_album_mock): + del web_album_mock["artists"] + + ref = translator.web_to_album_ref(web_album_mock) + assert ref.name == "DEF 456" def test_without_name_uses_uri(self, web_album_mock): @@ -342,7 +349,7 @@ def test_returns_albums(self, web_album_mock): assert refs[0].type == models.Ref.ALBUM assert refs[0].uri == "spotify:album:def" - assert refs[0].name == "DEF 456" + assert refs[0].name == "ABBA - DEF 456" def test_returns_bare_albums(self, web_album_mock): web_albums = [web_album_mock] * 3 @@ -352,7 +359,7 @@ def test_returns_bare_albums(self, web_album_mock): assert refs[0].type == models.Ref.ALBUM assert refs[0].uri == "spotify:album:def" - assert refs[0].name == "DEF 456" + assert refs[0].name == "ABBA - DEF 456" def test_bad_albums_filtered(self, web_album_mock, web_artist_mock): refs = list( @@ -757,7 +764,7 @@ def test_calls_web_to_album_ref(self, web_album_mock): ref_func_mock.assert_called_once_with(web_album_mock) assert album.uri == str(sentinel.uri) - assert album.name == str(sentinel.name) + assert album.name == "DEF 456" def test_returns_none_if_invalid_ref(self, web_album_mock): with patch.object(translator, "web_to_album_ref", return_value=None): From 2fd5cee0159dc1b2cdbe02926c58178f91927d6f Mon Sep 17 00:00:00 2001 From: Nick Steel Date: Sun, 26 Jun 2022 01:30:48 +0100 Subject: [PATCH 11/21] Remove pyspotify: browse Removed generic toplists, unavailable through Web API? --- mopidy_spotify/browse.py | 155 ++++--------------- mopidy_spotify/web.py | 27 +++- tests/test_browse.py | 323 +++++---------------------------------- tests/test_web.py | 37 ++++- 4 files changed, 122 insertions(+), 420 deletions(-) diff --git a/mopidy_spotify/browse.py b/mopidy_spotify/browse.py index 37a579d6..425c92e9 100644 --- a/mopidy_spotify/browse.py +++ b/mopidy_spotify/browse.py @@ -4,6 +4,7 @@ import spotify from mopidy_spotify import countries, playlists, translator +from mopidy_spotify.web import WebLink from mopidy_spotify.utils import flatten logger = logging.getLogger(__name__) @@ -22,7 +23,6 @@ _TOP_LIST_DIR_CONTENTS = [ models.Ref.directory(uri="spotify:top:tracks", name="Top tracks"), - models.Ref.directory(uri="spotify:top:albums", name="Top albums"), models.Ref.directory(uri="spotify:top:artists", name="Top artists"), ] @@ -35,17 +35,6 @@ models.Ref.directory(uri="spotify:playlists:featured", name="Featured"), ] -_TOPLIST_TYPES = { - "albums": spotify.ToplistType.ALBUMS, - "artists": spotify.ToplistType.ARTISTS, - "tracks": spotify.ToplistType.TRACKS, -} - -_TOPLIST_REGIONS = { - "country": lambda session: session.user_country, - "everywhere": lambda session: spotify.ToplistRegion.EVERYWHERE, -} - def browse(*, config, session, web_client, uri): if uri == ROOT_DIR.uri: @@ -56,22 +45,21 @@ def browse(*, config, session, web_client, uri): return _YOUR_MUSIC_DIR_CONTENTS elif uri == _PLAYLISTS_DIR.uri: return _PLAYLISTS_DIR_CONTENTS - elif uri.startswith("spotify:user:") or uri.startswith("spotify:playlist:"): - return _browse_playlist(web_client, uri, config) + + if web_client is None or not web_client.logged_in: + return [] + + #TODO: Support for category browsing. + if uri.startswith("spotify:user:") or uri.startswith("spotify:playlist:"): + return _browse_playlist(web_client, uri) elif uri.startswith("spotify:album:"): - return _browse_album(session, uri, config) + return _browse_album(web_client, uri) elif uri.startswith("spotify:artist:"): - return _browse_artist(session, uri, config) + return _browse_artist(web_client, uri) elif uri.startswith("spotify:top:"): parts = uri.replace("spotify:top:", "").split(":") if len(parts) == 1: - return _browse_toplist_regions(variant=parts[0]) - elif len(parts) == 2: - if parts[1] == "user": - return _browse_toplist_user(web_client, variant=parts[0]) - return _browse_toplist( - config, session, variant=parts[0], region=parts[1] - ) + return _browse_toplist_user(web_client, variant=parts[0]) else: logger.info(f"Failed to browse {uri!r}: Toplist URI parsing failed") return [] @@ -88,66 +76,36 @@ def browse(*, config, session, web_client, uri): return [] -def _browse_playlist(web_client, uri, config): - return playlists.playlist_lookup( - web_client, uri, config["bitrate"], as_items=True - ) +def _browse_playlist(web_client, uri): + return playlists.playlist_lookup(web_client, uri, None, as_items=True) -def _browse_album(session, uri, config): - if session is None or session.connection.state is not spotify.ConnectionState.LOGGED_IN: +def _browse_album(web_client, uri): + try: + link = WebLink.from_uri(uri) + except ValueError as exc: + logger.info(f"Failed to browse {uri!r}: {exc}") return [] - sp_album_browser = session.get_album(uri).browse() - sp_album_browser.load(config["timeout"]) - return list( - translator.to_track_refs( - sp_album_browser.tracks, timeout=config["timeout"] - ) - ) + web_album = web_client.get_album(link) + web_tracks = web_album.get("tracks", {}).get("items", []) + return list(translator.web_to_track_refs(web_tracks)) -def _browse_artist(session, uri, config): - if session is None or session.connection.state is not spotify.ConnectionState.LOGGED_IN: +def _browse_artist(web_client, uri): + try: + link = WebLink.from_uri(uri) + except ValueError as exc: + logger.info(f"Failed to browse {uri!r}: {exc}") return [] - sp_artist_browser = session.get_artist(uri).browse( - type=spotify.ArtistBrowserType.NO_TRACKS - ) - sp_artist_browser.load(config["timeout"]) - top_tracks = list( - translator.to_track_refs( - sp_artist_browser.tophit_tracks, timeout=config["timeout"] - ) - ) - albums = list( - translator.to_album_refs( - sp_artist_browser.albums, timeout=config["timeout"] - ) - ) - return top_tracks + albums + web_top_tracks = web_client.get_artist_top_tracks(link) + top_tracks = list(translator.web_to_track_refs(web_top_tracks)) + web_albums = web_client.get_artist_albums(link, all_tracks=False) + albums = list(translator.web_to_album_refs(web_albums)) -def _browse_toplist_regions(variant): - dir_contents = [ - models.Ref.directory( - uri=f"spotify:top:{variant}:country", name="Country" - ), - models.Ref.directory( - uri=f"spotify:top:{variant}:countries", name="Other countries" - ), - models.Ref.directory( - uri=f"spotify:top:{variant}:everywhere", name="Global" - ), - ] - if variant in ("tracks", "artists"): - dir_contents.insert( - 0, - models.Ref.directory( - uri=f"spotify:top:{variant}:user", name="Personal" - ), - ) - return dir_contents + return top_tracks + albums def _browse_toplist_user(web_client, variant): @@ -175,57 +133,6 @@ def _browse_toplist_user(web_client, variant): return [] -def _browse_toplist(config, session, variant, region): - if session is None: - return [] - if region == "countries": - codes = config["toplist_countries"] - if not codes: - codes = countries.COUNTRIES.keys() - return [ - models.Ref.directory( - uri=f"spotify:top:{variant}:{code.lower()}", - name=countries.COUNTRIES.get(code.upper(), code.upper()), - ) - for code in codes - ] - - if region in ("country", "everywhere"): - sp_toplist = session.get_toplist( - type=_TOPLIST_TYPES[variant], - region=_TOPLIST_REGIONS[region](session), - ) - elif len(region) == 2: - sp_toplist = session.get_toplist( - type=_TOPLIST_TYPES[variant], region=region.upper() - ) - else: - return [] - - if session.connection.state is spotify.ConnectionState.LOGGED_IN: - sp_toplist.load(config["timeout"]) - - if not sp_toplist.is_loaded: - return [] - - if variant == "tracks": - return list(translator.to_track_refs(sp_toplist.tracks)) - elif variant == "albums": - return list( - translator.to_album_refs( - sp_toplist.albums, timeout=config["timeout"] - ) - ) - elif variant == "artists": - return list( - translator.to_artist_refs( - sp_toplist.artists, timeout=config["timeout"] - ) - ) - else: - return [] - - def _load_your_music(web_client, variant): if web_client is None or not web_client.logged_in: return diff --git a/mopidy_spotify/web.py b/mopidy_spotify/web.py index ee01090a..0a7a1043 100644 --- a/mopidy_spotify/web.py +++ b/mopidy_spotify/web.py @@ -493,7 +493,7 @@ def get_album(self, web_link): ) return self._with_all_tracks(album) - def get_artist_albums(self, web_link): + def get_artist_albums(self, web_link, all_tracks=True): if web_link.type != LinkType.ARTIST: logger.error(f"Expecting Spotify artist URI") return [] @@ -504,12 +504,25 @@ def get_artist_albums(self, web_link): ) for page in pages: for album in page["items"]: - try: - web_link = WebLink.from_uri(album.get("uri")) - except ValueError as exc: - logger.error(exc) - continue - yield self.get_album(web_link) + if all_tracks: + try: + web_link = WebLink.from_uri(album.get("uri")) + except ValueError as exc: + logger.error(exc) + continue + yield self.get_album(web_link) + else: + yield album + + def get_artist_top_tracks(self, web_link): + if web_link.type != LinkType.ARTIST: + logger.error(f"Expecting Spotify artist URI") + return [] + + return self.get_one( + f"artists/{web_link.id}/top-tracks", + params={"market": "from_token"}, + ).get("tracks") def get_track(self, web_link): if web_link.type != LinkType.TRACK: diff --git a/tests/test_browse.py b/tests/test_browse.py index e66f18f2..5bfcfaf7 100644 --- a/tests/test_browse.py +++ b/tests/test_browse.py @@ -1,5 +1,6 @@ from unittest import mock +import pytest from mopidy import models import spotify @@ -25,18 +26,22 @@ def test_browse_root_directory(provider): ) +def test_browse_root_when_offline(web_client_mock, provider): + web_client_mock.logged_in = False + + results = provider.browse("spotify:directory") + + assert len(results) == 3 + + def test_browse_top_lists_directory(provider): results = provider.browse("spotify:top") - assert len(results) == 3 + assert len(results) == 2 assert ( models.Ref.directory(uri="spotify:top:tracks", name="Top tracks") in results ) - assert ( - models.Ref.directory(uri="spotify:top:albums", name="Top albums") - in results - ) assert ( models.Ref.directory(uri="spotify:top:artists", name="Top artists") in results @@ -80,172 +85,59 @@ def test_browse_playlist(web_client_mock, web_playlist_mock, provider): uri="spotify:track:abc", name="ABC 123" ) +@pytest.mark.parametrize( + "uri", ["album:def", "artist:abba", "user:alice:playlist:foo", "unknown", "top:tracks", "your:tracks"] +) +def test_browse_item_when_offline(web_client_mock, uri, provider, caplog): + web_client_mock.logged_in = False -def test_browse_album_when_offline(session_mock, sp_album_mock, provider): - session_mock.connection.state = spotify.ConnectionState.OFFLINE - session_mock.get_album.return_value = sp_album_mock - sp_album_browser_mock = sp_album_mock.browse.return_value - sp_album_browser_mock.is_loaded = False - type(sp_album_browser_mock).tracks = mock.PropertyMock( - side_effect=Exception - ) + results = provider.browse(f"spotify:{uri}") - provider.browse("spotify:album:def") - - assert sp_album_browser_mock.load.call_count == 0 + assert len(results) == 0 + assert "Failed to browse" not in caplog.text -def test_browse_album( - session_mock, sp_album_mock, sp_album_browser_mock, sp_track_mock, provider -): - session_mock.get_album.return_value = sp_album_mock - sp_album_mock.browse.return_value = sp_album_browser_mock - sp_album_browser_mock.tracks = [sp_track_mock, sp_track_mock] +def test_browse_album(web_client_mock, web_album_mock, provider): + web_client_mock.get_album.return_value = web_album_mock results = provider.browse("spotify:album:def") - session_mock.get_album.assert_called_once_with("spotify:album:def") - sp_album_mock.browse.assert_called_once_with() - assert len(results) == 2 + assert len(results) == 10 assert results[0] == models.Ref.track( uri="spotify:track:abc", name="ABC 123" ) -def test_browse_artist_when_offline(session_mock, sp_artist_mock, provider): - session_mock.connection.state = spotify.ConnectionState.OFFLINE - session_mock.get_artist.return_value = sp_artist_mock - sp_artist_browser_mock = sp_artist_mock.browse.return_value - sp_artist_browser_mock.is_loaded = False - type(sp_artist_browser_mock).tophit_tracks = mock.PropertyMock( - side_effect=Exception - ) - type(sp_artist_browser_mock).albums = mock.PropertyMock( - side_effect=Exception - ) +def test_browse_album_bad_uri(web_client_mock, web_album_mock, provider, caplog): + web_client_mock.get_album.return_value = web_album_mock - provider.browse("spotify:artist:abba") + results = provider.browse("spotify:album:def:xyz") - assert sp_artist_browser_mock.load.call_count == 0 + assert len(results) == 0 + assert "Failed to browse" in caplog.text def test_browse_artist( - session_mock, - sp_artist_mock, - sp_artist_browser_mock, - sp_album_mock, - sp_track_mock, + web_client_mock, + web_album_mock_base, + web_track_mock, provider, ): - session_mock.get_artist.return_value = sp_artist_mock - sp_artist_mock.browse.return_value = sp_artist_browser_mock - sp_artist_browser_mock.albums = [sp_album_mock, sp_album_mock] - sp_artist_browser_mock.tophit_tracks = [sp_track_mock, sp_track_mock] - + web_client_mock.get_artist_albums.return_value = [web_album_mock_base] + web_client_mock.get_artist_top_tracks.return_value = [web_track_mock, web_track_mock, web_track_mock] + results = provider.browse("spotify:artist:abba") - session_mock.get_artist.assert_called_once_with("spotify:artist:abba") - sp_artist_mock.browse.assert_called_once_with( - type=spotify.ArtistBrowserType.NO_TRACKS - ) + web_client_mock.get_artist_albums.assert_called_once_with(mock.ANY, all_tracks=False) assert len(results) == 4 assert results[0] == models.Ref.track( uri="spotify:track:abc", name="ABC 123" ) - assert results[2] == models.Ref.album( + assert results[3] == models.Ref.album( uri="spotify:album:def", name="ABBA - DEF 456" ) -def test_browse_toplist_when_offline(session_mock, provider): - session_mock.connection.state = spotify.ConnectionState.OFFLINE - toplist_mock = session_mock.get_toplist.return_value - toplist_mock.is_loaded = False - type(toplist_mock).tracks = mock.PropertyMock(side_effect=Exception) - - provider.browse("spotify:top:tracks:country") - - assert toplist_mock.load.call_count == 0 - - -def test_browse_toplist_when_offline_web(web_client_mock, provider): - web_client_mock.user_id = None - - results = provider.browse("spotify:top:tracks:user") - - assert len(results) == 0 - - -def test_browse_top_tracks(provider): - results = provider.browse("spotify:top:tracks") - - assert len(results) == 4 - assert ( - models.Ref.directory(uri="spotify:top:tracks:user", name="Personal") - in results - ) - assert ( - models.Ref.directory(uri="spotify:top:tracks:country", name="Country") - in results - ) - assert ( - models.Ref.directory(uri="spotify:top:tracks:everywhere", name="Global") - in results - ) - assert ( - models.Ref.directory( - uri="spotify:top:tracks:countries", name="Other countries" - ) - in results - ) - - -def test_browse_top_albums(provider): - results = provider.browse("spotify:top:albums") - - assert len(results) == 3 - assert ( - models.Ref.directory(uri="spotify:top:albums:country", name="Country") - in results - ) - assert ( - models.Ref.directory(uri="spotify:top:albums:everywhere", name="Global") - in results - ) - assert ( - models.Ref.directory( - uri="spotify:top:albums:countries", name="Other countries" - ) - in results - ) - - -def test_browse_top_artists(provider): - results = provider.browse("spotify:top:artists") - - assert len(results) == 4 - assert ( - models.Ref.directory(uri="spotify:top:artists:user", name="Personal") - in results - ) - assert ( - models.Ref.directory(uri="spotify:top:artists:country", name="Country") - in results - ) - assert ( - models.Ref.directory( - uri="spotify:top:artists:everywhere", name="Global" - ) - in results - ) - assert ( - models.Ref.directory( - uri="spotify:top:artists:countries", name="Other countries" - ) - in results - ) - - def test_browse_top_tracks_with_too_many_uri_parts(provider): results = provider.browse("spotify:top:tracks:foo:bar") @@ -253,7 +145,7 @@ def test_browse_top_tracks_with_too_many_uri_parts(provider): def test_browse_unsupported_top_tracks(web_client_mock, provider): - results = provider.browse("spotify:top:albums:user") + results = provider.browse("spotify:top:albums") web_client_mock.get_one.assert_not_called() assert len(results) == 0 @@ -262,7 +154,7 @@ def test_browse_unsupported_top_tracks(web_client_mock, provider): def test_browse_personal_top_tracks_empty(web_client_mock, provider): web_client_mock.get_all.return_value = [{}] - results = provider.browse("spotify:top:tracks:user") + results = provider.browse("spotify:top:tracks") web_client_mock.get_all.assert_called_once_with( "me/top/tracks", params={"limit": 50} @@ -278,7 +170,7 @@ def test_browse_personal_top_tracks(web_client_mock, web_track_mock, provider): {"items": [web_track_mock, web_track_mock]}, ] - results = provider.browse("spotify:top:tracks:user") + results = provider.browse("spotify:top:tracks") web_client_mock.get_all.assert_called_once_with( "me/top/tracks", params={"limit": 50} @@ -289,145 +181,6 @@ def test_browse_personal_top_tracks(web_client_mock, web_track_mock, provider): ) -def test_browse_country_top_tracks(session_mock, sp_track_mock, provider): - session_mock.user_country = "NO" - session_mock.get_toplist.return_value.tracks = [ - sp_track_mock, - sp_track_mock, - ] - - results = provider.browse("spotify:top:tracks:country") - - session_mock.get_toplist.assert_called_once_with( - type=spotify.ToplistType.TRACKS, region="NO" - ) - assert len(results) == 2 - assert results[0] == models.Ref.track( - uri="spotify:track:abc", name="ABC 123" - ) - - -def test_browse_global_top_tracks(session_mock, sp_track_mock, provider): - session_mock.get_toplist.return_value.tracks = [ - sp_track_mock, - sp_track_mock, - ] - - results = provider.browse("spotify:top:tracks:everywhere") - - session_mock.get_toplist.assert_called_once_with( - type=spotify.ToplistType.TRACKS, region=spotify.ToplistRegion.EVERYWHERE - ) - assert len(results) == 2 - assert results[0] == models.Ref.track( - uri="spotify:track:abc", name="ABC 123" - ) - - -def test_browse_top_track_countries_list_limited_by_config( - session_mock, sp_track_mock, provider -): - session_mock.get_toplist.return_value.tracks = [ - sp_track_mock, - sp_track_mock, - ] - - results = provider.browse("spotify:top:tracks:countries") - - assert len(results) == 2 - assert ( - models.Ref.directory(uri="spotify:top:tracks:gb", name="United Kingdom") - in results - ) - assert ( - models.Ref.directory(uri="spotify:top:tracks:us", name="United States") - in results - ) - - -def test_browse_top_tracks_countries_unlimited_by_config( - provider, backend_mock -): - backend_mock._config["spotify"]["toplist_countries"] = [] - - results = provider.browse("spotify:top:tracks:countries") - - assert len(results) > 50 - assert ( - models.Ref.directory(uri="spotify:top:tracks:jp", name="Japan") - in results - ) - assert ( - models.Ref.directory(uri="spotify:top:tracks:no", name="Norway") - in results - ) - assert ( - models.Ref.directory(uri="spotify:top:tracks:gb", name="United Kingdom") - in results - ) - assert ( - models.Ref.directory(uri="spotify:top:tracks:us", name="United States") - in results - ) - - -def test_browse_other_country_top_tracks(session_mock, sp_track_mock, provider): - session_mock.get_toplist.return_value.tracks = [ - sp_track_mock, - sp_track_mock, - ] - - results = provider.browse("spotify:top:tracks:us") - - session_mock.get_toplist.assert_called_once_with( - type=spotify.ToplistType.TRACKS, region="US" - ) - assert len(results) == 2 - assert results[0] == models.Ref.track( - uri="spotify:track:abc", name="ABC 123" - ) - - -def test_browse_unknown_country_top_tracks( - session_mock, sp_track_mock, provider -): - session_mock.get_toplist.return_value.tracks = [ - sp_track_mock, - sp_track_mock, - ] - - results = provider.browse("spotify:top:tracks:aa") - - session_mock.get_toplist.assert_called_once_with( - type=spotify.ToplistType.TRACKS, region="AA" - ) - assert len(results) == 2 - assert results[0] == models.Ref.track( - uri="spotify:track:abc", name="ABC 123" - ) - - -def test_browse_top_albums_countries_list( - session_mock, sp_track_mock, provider -): - session_mock.get_toplist.return_value.tracks = [ - sp_track_mock, - sp_track_mock, - ] - - results = provider.browse("spotify:top:albums:countries") - - assert len(results) == 2 - assert ( - models.Ref.directory(uri="spotify:top:albums:gb", name="United Kingdom") - in results - ) - assert ( - models.Ref.directory(uri="spotify:top:albums:us", name="United States") - in results - ) - - def test_browse_personal_top_artists( web_client_mock, web_artist_mock, provider ): @@ -436,7 +189,7 @@ def test_browse_personal_top_artists( {"items": [web_artist_mock, web_artist_mock]}, ] - results = provider.browse("spotify:top:artists:user") + results = provider.browse("spotify:top:artists") web_client_mock.get_all.assert_called_once_with( "me/top/artists", params={"limit": 50} diff --git a/tests/test_web.py b/tests/test_web.py index 1a4a17fe..621439d6 100644 --- a/tests/test_web.py +++ b/tests/test_web.py @@ -1154,7 +1154,11 @@ def test_get_album(self, foo_album, foo_album_next_tracks, spotify_client): assert result["tracks"]["items"] == [3, 4, 5, 6, 7, 8] @responses.activate - def test_get_artist_albums(self, artist_albums_mock, web_album_mock, web_album_mock2, spotify_client): + @pytest.mark.parametrize( + "all_tracks,", + [(True), (False)], + ) + def test_get_artist_albums(self, artist_albums_mock, web_album_mock, web_album_mock2, spotify_client, all_tracks): responses.add( responses.GET, url("artists/abba/albums"), @@ -1173,14 +1177,25 @@ def test_get_artist_albums(self, artist_albums_mock, web_album_mock, web_album_m ) link = web.WebLink.from_uri("spotify:artist:abba") - results = list(spotify_client.get_artist_albums(link)) + results = list(spotify_client.get_artist_albums(link, all_tracks=all_tracks)) - assert len(responses.calls) == 3 - + if all_tracks: + assert len(responses.calls) == 3 + else: + assert len(responses.calls) == 1 + assert len(results) == 2 assert results[0]["name"] == "DEF 456" assert results[1]["name"] == "XYZ 789" + if all_tracks: + assert results[0]["tracks"] + assert results[1]["tracks"] + else: + assert "tracks" not in results[0] + assert "tracks" not in results[1] + + @responses.activate def test_get_artist_albums_error(self, artist_albums_mock, web_album_mock, spotify_client, caplog): responses.add( @@ -1241,6 +1256,20 @@ def test_get_track(self, web_track_mock, spotify_client, uri, success): else: assert result == {} + @responses.activate + def test_get_artist_top_tracks(self, web_track_mock, spotify_client): + responses.add( + responses.GET, + url(f"artists/baz/top-tracks"), + json={"tracks": [web_track_mock, web_track_mock]}, + ) + link = web.WebLink.from_uri("spotify:artist:baz") + results = spotify_client.get_artist_top_tracks(link) + + assert len(responses.calls) == 1 + assert len(results) == 2 + assert results[0]["name"] == "ABC 123" + @pytest.mark.parametrize( "uri,type_,id_", From c1dc92b61d2b31d8b00a75a03b62f47874366f7e Mon Sep 17 00:00:00 2001 From: Nick Steel Date: Sun, 24 Jul 2022 00:38:55 +0100 Subject: [PATCH 12/21] Remove pyspotify: removed all remaining pyspotify usage. --- MANIFEST.in | 1 - mopidy_spotify/backend.py | 1 - mopidy_spotify/browse.py | 1 - mopidy_spotify/search.py | 1 - mopidy_spotify/translator.py | 120 +-------------- setup.cfg | 1 - tests/conftest.py | 238 ------------------------------ tests/test_browse.py | 2 - tests/test_lookup.py | 2 - tests/test_playlists.py | 1 - tests/test_translator.py | 236 ----------------------------- 12 files changed, 1 insertion(+), 603 deletions(-) diff --git a/MANIFEST.in b/MANIFEST.in index 57401cdb..9d9fd9b3 100644 --- a/MANIFEST.in +++ b/MANIFEST.in @@ -9,7 +9,6 @@ include tox.ini recursive-include .github * include mopidy_*/ext.conf -include mopidy_spotify/spotify_appkey.key recursive-include tests *.py recursive-include tests/data * diff --git a/mopidy_spotify/backend.py b/mopidy_spotify/backend.py index 59a6a467..f1deb961 100644 --- a/mopidy_spotify/backend.py +++ b/mopidy_spotify/backend.py @@ -5,7 +5,6 @@ import pykka from mopidy import backend, httpclient -import spotify from mopidy_spotify import Extension, library, playlists, web logger = logging.getLogger(__name__) diff --git a/mopidy_spotify/browse.py b/mopidy_spotify/browse.py index 425c92e9..2e8ab8f3 100644 --- a/mopidy_spotify/browse.py +++ b/mopidy_spotify/browse.py @@ -2,7 +2,6 @@ from mopidy import models -import spotify from mopidy_spotify import countries, playlists, translator from mopidy_spotify.web import WebLink from mopidy_spotify.utils import flatten diff --git a/mopidy_spotify/search.py b/mopidy_spotify/search.py index 74413a17..e3d87510 100644 --- a/mopidy_spotify/search.py +++ b/mopidy_spotify/search.py @@ -3,7 +3,6 @@ from mopidy import models -import spotify from mopidy_spotify import lookup, translator _SEARCH_TYPES = ["album", "artist", "track"] diff --git a/mopidy_spotify/translator.py b/mopidy_spotify/translator.py index fb9b640c..e7db3f2f 100644 --- a/mopidy_spotify/translator.py +++ b/mopidy_spotify/translator.py @@ -3,8 +3,6 @@ from mopidy import models -import spotify - logger = logging.getLogger(__name__) @@ -26,29 +24,7 @@ def __call__(self, *args, **kwargs): return value -@memoized -def to_artist(sp_artist): - if not sp_artist.is_loaded: - return - - return models.Artist(uri=sp_artist.link.uri, name=sp_artist.name) - - -@memoized -def to_artist_ref(sp_artist): - if not sp_artist.is_loaded: - return - - return models.Ref.artist(uri=sp_artist.link.uri, name=sp_artist.name) - - -def to_artist_refs(sp_artists, timeout=None): - for sp_artist in sp_artists: - sp_artist.load(timeout) - ref = to_artist_ref(sp_artist) - if ref is not None: - yield ref - +#TODO: memoize web functions? def web_to_artist_ref(web_artist): if not valid_web_data(web_artist, "artist"): @@ -65,47 +41,6 @@ def web_to_artist_refs(web_artists): yield ref -@memoized -def to_album(sp_album): - if not sp_album.is_loaded: - return - - if sp_album.artist is not None and sp_album.artist.is_loaded: - artists = [to_artist(sp_album.artist)] - else: - artists = [] - - if sp_album.year is not None and sp_album.year != 0: - date = f"{sp_album.year}" - else: - date = None - - return models.Album( - uri=sp_album.link.uri, name=sp_album.name, artists=artists, date=date - ) - - -@memoized -def to_album_ref(sp_album): - if not sp_album.is_loaded: - return - - if sp_album.artist is None or not sp_album.artist.is_loaded: - name = sp_album.name - else: - name = f"{sp_album.artist.name} - {sp_album.name}" - - return models.Ref.album(uri=sp_album.link.uri, name=name) - - -def to_album_refs(sp_albums, timeout=None): - for sp_album in sp_albums: - sp_album.load(timeout) - ref = to_album_ref(sp_album) - if ref is not None: - yield ref - - def web_to_album_ref(web_album): if not valid_web_data(web_album, "album"): return @@ -131,51 +66,6 @@ def web_to_album_refs(web_albums): yield ref -@memoized -def to_track(sp_track, bitrate=None): - if not sp_track.is_loaded: - return - - if sp_track.error != spotify.ErrorType.OK: - logger.debug(f"Error loading {sp_track.link.uri!r}: {sp_track.error!r}") - return - - if sp_track.availability != spotify.TrackAvailability.AVAILABLE: - return - - artists = [to_artist(sp_artist) for sp_artist in sp_track.artists] - artists = [a for a in artists if a] - - album = to_album(sp_track.album) - - return models.Track( - uri=sp_track.link.uri, - name=sp_track.name, - artists=artists, - album=album, - date=album.date, - length=sp_track.duration, - disc_no=sp_track.disc, - track_no=sp_track.index, - bitrate=bitrate, - ) - - -@memoized -def to_track_ref(sp_track): - if not sp_track.is_loaded: - return - - if sp_track.error != spotify.ErrorType.OK: - logger.debug(f"Error loading {sp_track.link.uri!r}: {sp_track.error!r}") - return - - if sp_track.availability != spotify.TrackAvailability.AVAILABLE: - return - - return models.Ref.track(uri=sp_track.link.uri, name=sp_track.name) - - def valid_web_data(data, object_type): return ( isinstance(data, dict) @@ -184,14 +74,6 @@ def valid_web_data(data, object_type): ) -def to_track_refs(sp_tracks, timeout=None): - for sp_track in sp_tracks: - sp_track.load(timeout) - ref = to_track_ref(sp_track) - if ref is not None: - yield ref - - def web_to_track_ref(web_track, *, check_playable=True): if not valid_web_data(web_track, "track"): return diff --git a/setup.cfg b/setup.cfg index 4d45fe57..a57b2bb5 100644 --- a/setup.cfg +++ b/setup.cfg @@ -29,7 +29,6 @@ python_requires = >= 3.7 install_requires = Mopidy >= 3.0.0 Pykka >= 2.0.1 - pyspotify >= 2.0.5 requests >= 2.20.0 setuptools diff --git a/tests/conftest.py b/tests/conftest.py index d34ac1db..f536b564 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -4,7 +4,6 @@ from mopidy import backend as backend_api from mopidy import models -import spotify from mopidy_spotify import backend, library, utils, playlists, web @@ -49,243 +48,6 @@ def web_mock(): patcher.stop() -@pytest.fixture -def spotify_mock(web_mock): - patcher = mock.patch.object(backend, "spotify", spec=spotify) - yield patcher.start() - patcher.stop() - - -@pytest.fixture -def sp_user_mock(): - sp_user = mock.Mock(spec=spotify.User) - sp_user.is_loaded = True - sp_user.canonical_name = "alice" - return sp_user - - -@pytest.fixture -def sp_artist_mock(): - sp_artist = mock.Mock(spec=spotify.Artist) - sp_artist.is_loaded = True - sp_artist.name = "ABBA" - - sp_link = mock.Mock(spec=spotify.Link) - sp_link.uri = "spotify:artist:abba" - sp_link.type = spotify.LinkType.ARTIST - sp_link.as_artist.return_value = sp_artist - sp_artist.link = sp_link - - return sp_artist - - -@pytest.fixture -def sp_unloaded_artist_mock(): - sp_artist = mock.Mock(spec=spotify.Artist) - sp_artist.is_loaded = False - sp_artist.name = None - - sp_link = mock.Mock(spec=spotify.Link) - sp_link.uri = "spotify:artist:abba" - sp_link.type = spotify.LinkType.ARTIST - sp_link.as_artist.return_value = sp_artist - sp_artist.link = sp_link - - return sp_artist - - -@pytest.fixture -def sp_artist_browser_mock(sp_artist_mock, sp_album_mock): - sp_artist_browser = mock.Mock(spec=spotify.ArtistBrowser) - sp_artist_browser.artist = sp_artist_mock - sp_artist_browser.albums = [sp_album_mock, sp_album_mock] - - sp_artist_mock.browse.return_value = sp_artist_browser - - return sp_artist_browser - - -@pytest.fixture -def sp_album_mock(sp_artist_mock): - sp_album = mock.Mock(spec=spotify.Album) - sp_album.is_loaded = True - sp_album.name = "DEF 456" - sp_album.artist = sp_artist_mock - sp_album.year = 2001 - - sp_link = mock.Mock(spec=spotify.Link) - sp_link.uri = "spotify:album:def" - sp_link.type = spotify.LinkType.ALBUM - sp_link.as_album.return_value = sp_album - sp_album.link = sp_link - - return sp_album - - -@pytest.fixture -def sp_unloaded_album_mock(sp_unloaded_artist_mock): - sp_album = mock.Mock(spec=spotify.Album) - sp_album.is_loaded = True - sp_album.is_loaded = False - sp_album.name = None - # Optimally, we should test with both None and sp_unloaded_artist_mock - sp_album.artist = sp_unloaded_artist_mock - sp_album.year = None - - sp_link = mock.Mock(spec=spotify.Link) - sp_link.uri = "spotify:album:def" - sp_link.type = spotify.LinkType.ALBUM - sp_link.as_album.return_value = sp_album - sp_album.link = sp_link - - return sp_album - - -@pytest.fixture -def sp_album_browser_mock(sp_album_mock, sp_track_mock): - sp_album_browser = mock.Mock(spec=spotify.AlbumBrowser) - sp_album_browser.album = sp_album_mock - sp_album_browser.tracks = [sp_track_mock, sp_track_mock] - sp_album_browser.load.return_value = sp_album_browser - - sp_album_mock.browse.return_value = sp_album_browser - - return sp_album_browser - - -def sp_track_mock(sp_artist_mock, sp_album_mock): - sp_track = mock.Mock(spec=spotify.Track) - sp_track.is_loaded = True - sp_track.error = spotify.ErrorType.OK - sp_track.availability = spotify.TrackAvailability.AVAILABLE - sp_track.name = "ABC 123" - sp_track.artists = [sp_artist_mock] - sp_track.album = sp_album_mock - sp_track.duration = 174300 - sp_track.disc = 1 - sp_track.index = 7 - - sp_link = mock.Mock(spec=spotify.Link) - sp_link.uri = "spotify:track:abc" - sp_link.type = spotify.LinkType.TRACK - sp_link.as_track.return_value = sp_track - sp_track.link = sp_link - - return sp_track - - -@pytest.fixture(name="sp_track_mock") -def sp_track_mock_fixture(sp_artist_mock, sp_album_mock): - return sp_track_mock(sp_artist_mock, sp_album_mock) - - -@pytest.fixture -def sp_unloaded_track_mock(sp_unloaded_artist_mock, sp_unloaded_album_mock): - sp_track = mock.Mock(spec=spotify.Track) - sp_track.is_loaded = False - sp_track.error = spotify.ErrorType.OK - sp_track.availability = None - sp_track.name = None - # Optimally, we should test with both None and [sp_unloaded_artist_mock] - sp_track.artists = [sp_unloaded_artist_mock] - # Optimally, we should test with both None and sp_unloaded_album_mock - sp_track.album = sp_unloaded_album_mock - sp_track.duration = None - sp_track.disc = None - sp_track.index = None - - sp_link = mock.Mock(spec=spotify.Link) - sp_link.uri = "spotify:track:abc" - sp_link.type = spotify.LinkType.TRACK - sp_link.as_track.return_value = sp_track - sp_track.link = sp_link - - return sp_track - - -@pytest.fixture -def sp_starred_mock(sp_user_mock, sp_artist_mock, sp_album_mock): - sp_track1 = sp_track_mock(sp_artist_mock, sp_album_mock) - sp_track1.link.uri = "spotify:track:oldest" - sp_track1.name = "Oldest" - - sp_track2 = sp_track_mock(sp_artist_mock, sp_album_mock) - sp_track2.link.uri = "spotify:track:newest" - sp_track2.name = "Newest" - - sp_starred = mock.Mock(spec=spotify.Playlist) - sp_starred.is_loaded = True - sp_starred.owner = sp_user_mock - sp_starred.name = None - sp_starred.tracks = [sp_track1, sp_track2] - - sp_link = mock.Mock(spec=spotify.Link) - sp_link.uri = "spotify:user:alice:starred" - sp_link.type = spotify.LinkType.STARRED - sp_link.as_playlist.return_value = sp_starred - sp_starred.link = sp_link - - return sp_starred - - -@pytest.fixture -def sp_playlist_mock(sp_user_mock, sp_track_mock): - sp_playlist = mock.Mock(spec=spotify.Playlist) - sp_playlist.is_loaded = True - sp_playlist.owner = sp_user_mock - sp_playlist.name = "Foo" - sp_playlist.tracks = [sp_track_mock] - - sp_link = mock.Mock(spec=spotify.Link) - sp_link.uri = "spotify:user:alice:playlist:foo" - sp_link.type = spotify.LinkType.PLAYLIST - sp_link.as_playlist.return_value = sp_playlist - sp_playlist.link = sp_link - - return sp_playlist - - -@pytest.fixture -def sp_unloaded_playlist_mock(sp_unloaded_track_mock): - sp_playlist = mock.Mock(spec=spotify.Playlist) - sp_playlist.is_loaded = False - sp_playlist.owner = None - sp_playlist.name = None - # Optimally, we should test with both None and [sp_unloaded_track_mock] - sp_playlist.tracks = [sp_unloaded_track_mock] - - sp_link = mock.Mock(spec=spotify.Link) - sp_link.uri = "spotify:user:alice:playlist:foo" - sp_link.type = spotify.LinkType.PLAYLIST - sp_link.as_playlist.return_value = sp_playlist - sp_playlist.link = sp_link - - return sp_playlist - - -@pytest.fixture -def sp_playlist_folder_start_mock(): - sp_playlist_folder_start = mock.Mock(spec=spotify.PlaylistFolder) - sp_playlist_folder_start.type = spotify.PlaylistType.START_FOLDER - sp_playlist_folder_start.name = "Bar" - sp_playlist_folder_start.id = 17 - return sp_playlist_folder_start - - -@pytest.fixture -def sp_playlist_folder_end_mock(): - sp_playlist_folder_end = mock.Mock(spec=spotify.PlaylistFolder) - sp_playlist_folder_end.type = spotify.PlaylistType.END_FOLDER - sp_playlist_folder_end.id = 17 - return sp_playlist_folder_end - - -@pytest.fixture -def sp_playlist_container_mock(): - sp_playlist_container = mock.Mock(spec=spotify.PlaylistContainer) - return sp_playlist_container - - @pytest.fixture def web_search_mock(web_album_mock_base, web_artist_mock, web_track_mock): return { diff --git a/tests/test_browse.py b/tests/test_browse.py index 5bfcfaf7..88f8f8dd 100644 --- a/tests/test_browse.py +++ b/tests/test_browse.py @@ -3,8 +3,6 @@ import pytest from mopidy import models -import spotify - def test_has_a_root_directory(provider): assert provider.root_directory == models.Ref.directory( diff --git a/tests/test_lookup.py b/tests/test_lookup.py index f90296c7..a24e3fbf 100644 --- a/tests/test_lookup.py +++ b/tests/test_lookup.py @@ -1,8 +1,6 @@ import copy from unittest import mock -import spotify - def test_lookup_of_invalid_uri(provider, caplog): results = provider.lookup("invalid") diff --git a/tests/test_playlists.py b/tests/test_playlists.py index fc415b49..16905fd1 100644 --- a/tests/test_playlists.py +++ b/tests/test_playlists.py @@ -4,7 +4,6 @@ from mopidy import backend as backend_api from mopidy.models import Ref -import spotify from mopidy_spotify import playlists diff --git a/tests/test_translator.py b/tests/test_translator.py index 4686d9ab..ac83a0db 100644 --- a/tests/test_translator.py +++ b/tests/test_translator.py @@ -3,63 +3,9 @@ from unittest import mock from unittest.mock import patch, sentinel -import spotify from mopidy_spotify import translator -class TestToArtist: - def test_returns_none_if_unloaded(self, sp_artist_mock): - sp_artist_mock.is_loaded = False - - artist = translator.to_artist(sp_artist_mock) - - assert artist is None - - def test_successful_translation(self, sp_artist_mock): - artist = translator.to_artist(sp_artist_mock) - - assert artist.uri == "spotify:artist:abba" - assert artist.name == "ABBA" - - def test_caches_results(self, sp_artist_mock): - artist1 = translator.to_artist(sp_artist_mock) - artist2 = translator.to_artist(sp_artist_mock) - - assert artist1 is artist2 - - def test_does_not_cache_none_results(self, sp_artist_mock): - sp_artist_mock.is_loaded = False - artist1 = translator.to_artist(sp_artist_mock) - - sp_artist_mock.is_loaded = True - artist2 = translator.to_artist(sp_artist_mock) - - assert artist1 is None - assert artist2 is not None - - -class TestToArtistRef: - def test_returns_none_if_unloaded(self, sp_artist_mock): - sp_artist_mock.is_loaded = False - - ref = translator.to_artist_ref(sp_artist_mock) - - assert ref is None - - def test_successful_translation(self, sp_artist_mock): - ref = translator.to_artist_ref(sp_artist_mock) - - assert ref.type == "artist" - assert ref.uri == "spotify:artist:abba" - assert ref.name == "ABBA" - - def test_caches_results(self, sp_artist_mock): - ref1 = translator.to_artist_ref(sp_artist_mock) - ref2 = translator.to_artist_ref(sp_artist_mock) - - assert ref1 is ref2 - - class TestWebToArtistRef: @pytest.mark.parametrize( "web_data", @@ -100,188 +46,6 @@ def test_bad_artists_filtered(self, web_artist_mock, web_track_mock): assert refs[0].name == "ABBA" -class TestToAlbum: - def test_returns_none_if_unloaded(self, sp_album_mock): - sp_album_mock.is_loaded = False - - album = translator.to_album(sp_album_mock) - - assert album is None - - def test_successful_translation(self, sp_album_mock): - album = translator.to_album(sp_album_mock) - - assert album.uri == "spotify:album:def" - assert album.name == "DEF 456" - assert list(album.artists) == [ - models.Artist(uri="spotify:artist:abba", name="ABBA") - ] - assert album.date == "2001" - - def test_returns_empty_artists_list_if_artist_is_none(self, sp_album_mock): - sp_album_mock.artist = None - - album = translator.to_album(sp_album_mock) - - assert list(album.artists) == [] - - def test_returns_unknown_date_if_year_is_none(self, sp_album_mock): - sp_album_mock.year = None - - album = translator.to_album(sp_album_mock) - - assert album.date is None - - def test_returns_unknown_date_if_year_is_zero(self, sp_album_mock): - sp_album_mock.year = 0 - - album = translator.to_album(sp_album_mock) - - assert album.date is None - - def test_caches_results(self, sp_album_mock): - album1 = translator.to_album(sp_album_mock) - album2 = translator.to_album(sp_album_mock) - - assert album1 is album2 - - -class TestToAlbumRef: - def test_returns_none_if_unloaded(self, sp_album_mock): - sp_album_mock.is_loaded = False - - ref = translator.to_album_ref(sp_album_mock) - - assert ref is None - - def test_successful_translation(self, sp_album_mock): - ref = translator.to_album_ref(sp_album_mock) - - assert ref.type == "album" - assert ref.uri == "spotify:album:def" - assert ref.name == "ABBA - DEF 456" - - def test_if_artist_is_none(self, sp_album_mock): - sp_album_mock.artist = None - - ref = translator.to_album_ref(sp_album_mock) - - assert ref.name == "DEF 456" - - def test_if_artist_is_not_loaded(self, sp_album_mock): - sp_album_mock.artist.is_loaded = False - - ref = translator.to_album_ref(sp_album_mock) - - assert ref.name == "DEF 456" - - def test_caches_results(self, sp_album_mock): - ref1 = translator.to_album_ref(sp_album_mock) - ref2 = translator.to_album_ref(sp_album_mock) - - assert ref1 is ref2 - - -class TestToTrack: - def test_returns_none_if_unloaded(self, sp_track_mock): - sp_track_mock.is_loaded = False - - track = translator.to_track(sp_track_mock) - - assert track is None - - def test_returns_none_if_error(self, sp_track_mock, caplog): - sp_track_mock.error = spotify.ErrorType.OTHER_PERMANENT - - track = translator.to_track(sp_track_mock) - - assert track is None - assert ( - "Error loading 'spotify:track:abc': " - in caplog.text - ) - - def test_returns_none_if_not_available(self, sp_track_mock): - sp_track_mock.availability = spotify.TrackAvailability.UNAVAILABLE - - track = translator.to_track(sp_track_mock) - - assert track is None - - def test_successful_translation(self, sp_track_mock): - track = translator.to_track(sp_track_mock, bitrate=320) - - assert track.uri == "spotify:track:abc" - assert track.name == "ABC 123" - assert list(track.artists) == [ - models.Artist(uri="spotify:artist:abba", name="ABBA") - ] - assert track.album == models.Album( - uri="spotify:album:def", - name="DEF 456", - artists=[models.Artist(uri="spotify:artist:abba", name="ABBA")], - date="2001", - ) - assert track.track_no == 7 - assert track.disc_no == 1 - assert track.date == "2001" - assert track.length == 174300 - assert track.bitrate == 320 - - def test_filters_out_none_artists(self, sp_artist_mock, sp_track_mock): - sp_artist_mock.is_loaded = False - - track = translator.to_track(sp_track_mock) - - assert list(track.artists) == [] - - def test_caches_results(self, sp_track_mock): - track1 = translator.to_track(sp_track_mock) - track2 = translator.to_track(sp_track_mock) - - assert track1 is track2 - - -class TestToTrackRef: - def test_returns_none_if_unloaded(self, sp_track_mock): - sp_track_mock.is_loaded = False - - ref = translator.to_track_ref(sp_track_mock) - - assert ref is None - - def test_returns_none_if_error(self, sp_track_mock, caplog): - sp_track_mock.error = spotify.ErrorType.OTHER_PERMANENT - - ref = translator.to_track_ref(sp_track_mock) - - assert ref is None - assert ( - "Error loading 'spotify:track:abc': " - in caplog.text - ) - - def test_returns_none_if_not_available(self, sp_track_mock): - sp_track_mock.availability = spotify.TrackAvailability.UNAVAILABLE - - ref = translator.to_track_ref(sp_track_mock) - - assert ref is None - - def test_successful_translation(self, sp_track_mock): - ref = translator.to_track_ref(sp_track_mock) - - assert ref.type == models.Ref.TRACK - assert ref.uri == "spotify:track:abc" - assert ref.name == "ABC 123" - - def test_caches_results(self, sp_track_mock): - ref1 = translator.to_track_ref(sp_track_mock) - ref2 = translator.to_track_ref(sp_track_mock) - - assert ref1 is ref2 - - class TestValidWebData(object): def test_returns_false_if_none(self): assert translator.valid_web_data(None, "track") is False From ed98d03e47d3b9c008d7463612124a90af5cf017 Mon Sep 17 00:00:00 2001 From: Nick Steel Date: Mon, 25 Jul 2022 22:34:14 +0100 Subject: [PATCH 13/21] Fixups for flake8 and black --- mopidy_spotify/backend.py | 20 +++++++------ mopidy_spotify/browse.py | 4 +-- mopidy_spotify/distinct.py | 20 +++++-------- mopidy_spotify/library.py | 2 +- mopidy_spotify/lookup.py | 16 +++++++--- mopidy_spotify/playlists.py | 2 +- mopidy_spotify/translator.py | 6 ++-- mopidy_spotify/web.py | 16 +++++----- tests/conftest.py | 2 +- tests/test_backend.py | 7 ----- tests/test_browse.py | 29 ++++++++++++++---- tests/test_distinct.py | 17 +++++++++-- tests/test_lookup.py | 30 +++++++++++-------- tests/test_playback.py | 8 ++--- tests/test_playlists.py | 5 +++- tests/test_translator.py | 4 ++- tests/test_web.py | 58 +++++++++++++++++++++++++----------- 17 files changed, 150 insertions(+), 96 deletions(-) diff --git a/mopidy_spotify/backend.py b/mopidy_spotify/backend.py index f1deb961..6ba6aa97 100644 --- a/mopidy_spotify/backend.py +++ b/mopidy_spotify/backend.py @@ -1,9 +1,7 @@ import logging -import pathlib -import threading import pykka -from mopidy import backend, httpclient +from mopidy import backend from mopidy_spotify import Extension, library, playlists, web @@ -11,16 +9,17 @@ class SpotifyBackend(pykka.ThreadingActor, backend.Backend): - def __init__(self, config, audio): super().__init__() self._config = config self._audio = audio - self._actor_proxy = None self._bitrate = config["spotify"]["bitrate"] self._web_client = None + if config["spotify"]["allow_cache"]: + self._cache_location = Extension().get_cache_dir(config) + self.library = library.SpotifyLibraryProvider(backend=self) self.playback = SpotifyPlaybackProvider(audio=audio, backend=self) if config["spotify"]["allow_playlists"]: @@ -30,8 +29,6 @@ def __init__(self, config, audio): self.uri_schemes = ["spotify"] def on_start(self): - self._actor_proxy = self.actor_ref.proxy() - self._web_client = web.SpotifyOAuthClient( client_id=self._config["spotify"]["client_id"], client_secret=self._config["spotify"]["client_secret"], @@ -42,8 +39,13 @@ def on_start(self): if self.playlists is not None: self.playlists.refresh() + class SpotifyPlaybackProvider(backend.PlaybackProvider): - def translate_uri(self, uri): + def __init__(self, *args, **kwargs): + super().__init__(*args, **kwargs) username = self.backend._config["spotify"]["username"] password = self.backend._config["spotify"]["password"] - return f"{uri}?username={username}&password={password}" + self._auth_string = f"username={username}&password={password}" + + def translate_uri(self, uri): + return f"{uri}?{self._auth_string}" diff --git a/mopidy_spotify/browse.py b/mopidy_spotify/browse.py index 2e8ab8f3..3b4dfed2 100644 --- a/mopidy_spotify/browse.py +++ b/mopidy_spotify/browse.py @@ -2,7 +2,7 @@ from mopidy import models -from mopidy_spotify import countries, playlists, translator +from mopidy_spotify import playlists, translator from mopidy_spotify.web import WebLink from mopidy_spotify.utils import flatten @@ -48,7 +48,7 @@ def browse(*, config, session, web_client, uri): if web_client is None or not web_client.logged_in: return [] - #TODO: Support for category browsing. + # TODO: Support for category browsing. if uri.startswith("spotify:user:") or uri.startswith("spotify:playlist:"): return _browse_playlist(web_client, uri) elif uri.startswith("spotify:album:"): diff --git a/mopidy_spotify/distinct.py b/mopidy_spotify/distinct.py index d1224ed7..a3bb2bdc 100644 --- a/mopidy_spotify/distinct.py +++ b/mopidy_spotify/distinct.py @@ -16,7 +16,9 @@ def get_distinct(config, playlists, web_client, field, query=None): if field == "artist": result = _get_distinct_artists(config, playlists, web_client, query) elif field == "albumartist": - result = _get_distinct_albumartists(config, playlists, web_client, query) + result = _get_distinct_albumartists( + config, playlists, web_client, query + ) elif field == "album": result = _get_distinct_albums(config, playlists, web_client, query) elif field == "date": @@ -30,9 +32,7 @@ def get_distinct(config, playlists, web_client, field, query=None): def _get_distinct_artists(config, playlists, web_client, query): logger.debug(f"Getting distinct artists: {query}") if query: - search_result = _get_search( - config, web_client, query, artist=True - ) + search_result = _get_search(config, web_client, query, artist=True) return {artist.name for artist in search_result.artists} else: return { @@ -45,9 +45,7 @@ def _get_distinct_artists(config, playlists, web_client, query): def _get_distinct_albumartists(config, playlists, web_client, query): logger.debug(f"Getting distinct albumartists: {query}") if query: - search_result = _get_search( - config, web_client, query, album=True - ) + search_result = _get_search(config, web_client, query, album=True) return { artist.name for album in search_result.albums @@ -66,9 +64,7 @@ def _get_distinct_albumartists(config, playlists, web_client, query): def _get_distinct_albums(config, playlists, web_client, query): logger.debug(f"Getting distinct albums: {query}") if query: - search_result = _get_search( - config, web_client, query, album=True - ) + search_result = _get_search(config, web_client, query, album=True) return {album.name for album in search_result.albums} else: return { @@ -81,9 +77,7 @@ def _get_distinct_albums(config, playlists, web_client, query): def _get_distinct_dates(config, playlists, web_client, query): logger.debug(f"Getting distinct album years: {query}") if query: - search_result = _get_search( - config, web_client, query, album=True - ) + search_result = _get_search(config, web_client, query, album=True) return { album.date for album in search_result.albums diff --git a/mopidy_spotify/library.py b/mopidy_spotify/library.py index f2024d4f..4c1f0d73 100644 --- a/mopidy_spotify/library.py +++ b/mopidy_spotify/library.py @@ -17,7 +17,7 @@ def __init__(self, backend): def browse(self, uri): return browse.browse( config=self._config, - session=None, # TODO + session=None, # TODO web_client=self._backend._web_client, uri=uri, ) diff --git a/mopidy_spotify/lookup.py b/mopidy_spotify/lookup.py index 1ec97cf9..f92c6e7e 100644 --- a/mopidy_spotify/lookup.py +++ b/mopidy_spotify/lookup.py @@ -36,7 +36,9 @@ def lookup(config, web_client, uri): ) return [] except WebError as exc: - logger.info(f"Failed to lookup Spotify {link.type.value} {link.uri!r}: {exc}") + logger.info( + f"Failed to lookup Spotify {link.type.value} {link.uri!r}: {exc}" + ) return [] @@ -56,7 +58,9 @@ def _lookup_album(config, web_client, link): if web_album == {}: raise WebError("Invalid album response") - yield from translator.web_to_album_tracks(web_album, bitrate=config["bitrate"]) + yield from translator.web_to_album_tracks( + web_album, bitrate=config["bitrate"] + ) def _lookup_artist(config, web_client, link): @@ -71,11 +75,15 @@ def _lookup_artist(config, web_client, link): break if is_various_artists: continue - yield from translator.web_to_album_tracks(web_album, bitrate=config["bitrate"]) + yield from translator.web_to_album_tracks( + web_album, bitrate=config["bitrate"] + ) def _lookup_playlist(config, web_client, link): - playlist = playlists.playlist_lookup(web_client, link.uri, config["bitrate"]) + playlist = playlists.playlist_lookup( + web_client, link.uri, config["bitrate"] + ) if playlist is None: raise WebError("Invalid playlist response") return playlist.tracks diff --git a/mopidy_spotify/playlists.py b/mopidy_spotify/playlists.py index def97c6c..493d602c 100644 --- a/mopidy_spotify/playlists.py +++ b/mopidy_spotify/playlists.py @@ -89,7 +89,7 @@ def playlist_lookup(web_client, uri, bitrate, as_items=False): bitrate=bitrate, as_items=as_items, ) - #TODO: cache the Mopidy tracks? And handle as_items here instead of translator + # TODO: cache the Mopidy tracks? And handle as_items here instead of translator if playlist is None: return diff --git a/mopidy_spotify/translator.py b/mopidy_spotify/translator.py index e7db3f2f..adcda059 100644 --- a/mopidy_spotify/translator.py +++ b/mopidy_spotify/translator.py @@ -24,7 +24,8 @@ def __call__(self, *args, **kwargs): return value -#TODO: memoize web functions? +# TODO: memoize web functions? + def web_to_artist_ref(web_artist): if not valid_web_data(web_artist, "artist"): @@ -216,8 +217,7 @@ def web_to_album_tracks(web_album, bitrate=None): return [] tracks = [ - web_to_track(web_track, bitrate, album) - for web_track in web_tracks + web_to_track(web_track, bitrate, album) for web_track in web_tracks ] return [t for t in tracks if t] diff --git a/mopidy_spotify/web.py b/mopidy_spotify/web.py index 0a7a1043..5f3bb4b4 100644 --- a/mopidy_spotify/web.py +++ b/mopidy_spotify/web.py @@ -453,7 +453,7 @@ def _with_all_tracks(self, obj, params=None): more_tracks = [] for page in track_pages: if "items" not in page: - return {} # Return nothing on error, or what we have so far? + return {} # Return nothing on error, or what we have so far? more_tracks += page["items"] if more_tracks: @@ -461,7 +461,7 @@ def _with_all_tracks(self, obj, params=None): obj = copy.deepcopy(obj) obj.setdefault("tracks", {}).setdefault("items", []) obj["tracks"]["items"] += more_tracks - + return obj def get_playlist(self, uri): @@ -480,11 +480,10 @@ def get_playlist(self, uri): params={"fields": self.PLAYLIST_FIELDS, "market": "from_token"}, ) return self._with_all_tracks(playlist, {"fields": self.TRACK_FIELDS}) - def get_album(self, web_link): if web_link.type != LinkType.ALBUM: - logger.error(f"Expecting Spotify album URI") + logger.error("Expecting Spotify album URI") return {} album = self.get_one( @@ -495,7 +494,7 @@ def get_album(self, web_link): def get_artist_albums(self, web_link, all_tracks=True): if web_link.type != LinkType.ARTIST: - logger.error(f"Expecting Spotify artist URI") + logger.error("Expecting Spotify artist URI") return [] pages = self.get_all( @@ -516,7 +515,7 @@ def get_artist_albums(self, web_link, all_tracks=True): def get_artist_top_tracks(self, web_link): if web_link.type != LinkType.ARTIST: - logger.error(f"Expecting Spotify artist URI") + logger.error("Expecting Spotify artist URI") return [] return self.get_one( @@ -526,12 +525,11 @@ def get_artist_top_tracks(self, web_link): def get_track(self, web_link): if web_link.type != LinkType.TRACK: - logger.error(f"Expecting Spotify track URI") + logger.error("Expecting Spotify track URI") return {} return self.get_one( - f"tracks/{web_link.id}", - params={"market": "from_token"} + f"tracks/{web_link.id}", params={"market": "from_token"} ) def clear_cache(self, extra_expiry=None): diff --git a/tests/conftest.py b/tests/conftest.py index f536b564..b7d3f2f3 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -4,7 +4,7 @@ from mopidy import backend as backend_api from mopidy import models -from mopidy_spotify import backend, library, utils, playlists, web +from mopidy_spotify import backend, library, utils, web @pytest.fixture diff --git a/tests/test_backend.py b/tests/test_backend.py index 68ecc48b..7a1dd38e 100644 --- a/tests/test_backend.py +++ b/tests/test_backend.py @@ -1,4 +1,3 @@ -import threading from unittest import mock from unittest import skip @@ -41,12 +40,6 @@ def test_init_disables_playlists_provider_if_not_allowed(config): assert backend.playlists is None -@skip("support this with spotifyaudiosrc") -def test_on_start_creates_configured_session(tmp_path, config): - cache_location_mock = mock.PropertyMock() - pass - - @skip("currently can't configure this") def test_on_start_configures_preferred_bitrate(config): pass diff --git a/tests/test_browse.py b/tests/test_browse.py index 88f8f8dd..665adc44 100644 --- a/tests/test_browse.py +++ b/tests/test_browse.py @@ -26,7 +26,7 @@ def test_browse_root_directory(provider): def test_browse_root_when_offline(web_client_mock, provider): web_client_mock.logged_in = False - + results = provider.browse("spotify:directory") assert len(results) == 3 @@ -83,8 +83,17 @@ def test_browse_playlist(web_client_mock, web_playlist_mock, provider): uri="spotify:track:abc", name="ABC 123" ) + @pytest.mark.parametrize( - "uri", ["album:def", "artist:abba", "user:alice:playlist:foo", "unknown", "top:tracks", "your:tracks"] + "uri", + [ + "album:def", + "artist:abba", + "user:alice:playlist:foo", + "unknown", + "top:tracks", + "your:tracks", + ], ) def test_browse_item_when_offline(web_client_mock, uri, provider, caplog): web_client_mock.logged_in = False @@ -106,7 +115,9 @@ def test_browse_album(web_client_mock, web_album_mock, provider): ) -def test_browse_album_bad_uri(web_client_mock, web_album_mock, provider, caplog): +def test_browse_album_bad_uri( + web_client_mock, web_album_mock, provider, caplog +): web_client_mock.get_album.return_value = web_album_mock results = provider.browse("spotify:album:def:xyz") @@ -122,11 +133,17 @@ def test_browse_artist( provider, ): web_client_mock.get_artist_albums.return_value = [web_album_mock_base] - web_client_mock.get_artist_top_tracks.return_value = [web_track_mock, web_track_mock, web_track_mock] - + web_client_mock.get_artist_top_tracks.return_value = [ + web_track_mock, + web_track_mock, + web_track_mock, + ] + results = provider.browse("spotify:artist:abba") - web_client_mock.get_artist_albums.assert_called_once_with(mock.ANY, all_tracks=False) + web_client_mock.get_artist_albums.assert_called_once_with( + mock.ANY, all_tracks=False + ) assert len(results) == 4 assert results[0] == models.Ref.track( uri="spotify:track:abc", name="ABC 123" diff --git a/tests/test_distinct.py b/tests/test_distinct.py index 4a3eb20d..ce274076 100644 --- a/tests/test_distinct.py +++ b/tests/test_distinct.py @@ -58,7 +58,9 @@ def test_get_distinct_unsupported_field_types_returns_nothing(provider, field): def test_get_distinct_without_query_when_playlists_enabled( web_client_mock_with_playlists, provider, field, expected ): - provider._backend.playlists = playlists.SpotifyPlaylistsProvider(backend=provider._backend) + provider._backend.playlists = playlists.SpotifyPlaylistsProvider( + backend=provider._backend + ) provider._backend.playlists._loaded = True assert provider.get_distinct(field) == expected @@ -67,7 +69,9 @@ def test_get_distinct_without_query_when_playlists_enabled( def test_get_distinct_without_query_returns_nothing_when_playlists_empty( web_client_mock_with_playlists, provider ): - provider._backend.playlists = playlists.SpotifyPlaylistsProvider(backend=provider._backend) + provider._backend.playlists = playlists.SpotifyPlaylistsProvider( + backend=provider._backend + ) provider._backend.playlists._loaded = True web_client_mock_with_playlists.get_playlist.return_value = {} @@ -114,7 +118,14 @@ def test_get_distinct_without_query_returns_nothing_when_playlists_disabled( ], ) def test_get_distinct_with_query( - search_mock, provider, config, web_client_mock, field, query, expected, types + search_mock, + provider, + config, + web_client_mock, + field, + query, + expected, + types, ): assert provider.get_distinct(field, query) == expected diff --git a/tests/test_lookup.py b/tests/test_lookup.py index a24e3fbf..1688f045 100644 --- a/tests/test_lookup.py +++ b/tests/test_lookup.py @@ -1,5 +1,4 @@ import copy -from unittest import mock def test_lookup_of_invalid_uri(provider, caplog): @@ -34,7 +33,8 @@ def test_lookup_of_unhandled_uri(provider, caplog): assert len(results) == 0 assert ( "Failed to lookup 'spotify:invalid:something': " - "Could not parse 'spotify:invalid:something' as a Spotify URI" in caplog.text + "Could not parse 'spotify:invalid:something' as a Spotify URI" + in caplog.text ) @@ -62,7 +62,7 @@ def test_lookup_of_track_uri(web_client_mock, web_track_mock, provider): def test_lookup_of_album_uri(web_client_mock, web_album_mock, provider): web_client_mock.get_album.return_value = web_album_mock - + results = provider.lookup("spotify:album:def") assert len(results) == 10 @@ -73,15 +73,18 @@ def test_lookup_of_album_uri(web_client_mock, web_album_mock, provider): assert track.album.name == "DEF 456" -def test_lookup_of_artist_uri(web_track_mock, web_album_mock, web_client_mock, provider): +def test_lookup_of_artist_uri( + web_track_mock, web_album_mock, web_client_mock, provider +): web_track_mock2 = copy.deepcopy(web_track_mock) - web_track_mock2['name'] = "XYZ track" + web_track_mock2["name"] = "XYZ track" web_album_mock2 = copy.deepcopy(web_album_mock) - web_album_mock2['name'] = "XYZ album" - web_album_mock2['tracks']['items'] = [web_track_mock2] * 3 - + web_album_mock2["name"] = "XYZ album" + web_album_mock2["tracks"]["items"] = [web_track_mock2] * 3 + web_client_mock.get_artist_albums.return_value = [ - web_album_mock, web_album_mock2, + web_album_mock, + web_album_mock2, ] results = provider.lookup("spotify:artist:abba") @@ -105,7 +108,8 @@ def test_lookup_of_artist_ignores_unavailable_albums( ): web_album_mock["is_playable"] = False web_client_mock.get_artist_albums.return_value = [ - web_album_mock, web_album_mock2, + web_album_mock, + web_album_mock2, ] results = provider.lookup("spotify:artist:abba") @@ -127,7 +131,9 @@ def test_lookup_of_artist_uri_ignores_compilations( def test_lookup_of_artist_uri_ignores_various_artists_albums( web_client_mock, web_album_mock, provider ): - web_album_mock["artists"][0]["uri"] = "spotify:artist:0LyfQWJT6nXafLPZqxe9Of" + web_album_mock["artists"][0][ + "uri" + ] = "spotify:artist:0LyfQWJT6nXafLPZqxe9Of" web_client_mock.get_artist_albums.return_value = [web_album_mock] results = provider.lookup("spotify:artist:abba") @@ -178,7 +184,7 @@ def test_lookup_of_youralbums_uri(web_client_mock, web_album_mock, provider): results = provider.lookup("spotify:your:albums") - assert len(results) == 4*10 + assert len(results) == 4 * 10 for track in results: assert track.uri == "spotify:track:abc" assert track.name == "ABC 123" diff --git a/tests/test_playback.py b/tests/test_playback.py index 0a0aa439..35692e24 100644 --- a/tests/test_playback.py +++ b/tests/test_playback.py @@ -17,7 +17,6 @@ def audio_mock(): def backend_mock(config): backend_mock = mock.Mock(spec=backend.SpotifyBackend) backend_mock._config = config - backend_mock._actor_proxy = None return backend_mock @@ -33,7 +32,6 @@ def test_is_a_playback_provider(provider): def test_translate_uri_sets_credentials(config, provider): - config["spotify"]["username"] = "foo" - config["spotify"]["password"] = "bar" - - assert provider.translate_uri("baz") == "baz?username=foo&password=bar" + assert ( + provider.translate_uri("baz") == "baz?username=alice&password=password" + ) diff --git a/tests/test_playlists.py b/tests/test_playlists.py index 16905fd1..8021335c 100644 --- a/tests/test_playlists.py +++ b/tests/test_playlists.py @@ -233,4 +233,7 @@ def test_playlist_lookup_when_link_invalid( ) assert playlist is None - assert "Failed to lookup Spotify playlist URI 'spotify:in:valid'" in caplog.text + assert ( + "Failed to lookup Spotify playlist URI 'spotify:in:valid'" + in caplog.text + ) diff --git a/tests/test_translator.py b/tests/test_translator.py index ac83a0db..57203cd2 100644 --- a/tests/test_translator.py +++ b/tests/test_translator.py @@ -559,7 +559,9 @@ def test_ignores_invalid_artists(self, web_album_mock, web_artist_mock): assert album.name == "DEF 456" assert list(album.artists) == artists - def test_returns_empty_artists_list_if_artist_is_empty(self, web_album_mock): + def test_returns_empty_artists_list_if_artist_is_empty( + self, web_album_mock + ): web_album_mock["artists"] = [] album = translator.web_to_album(web_album_mock) diff --git a/tests/test_web.py b/tests/test_web.py index 621439d6..98c4fd54 100644 --- a/tests/test_web.py +++ b/tests/test_web.py @@ -777,7 +777,9 @@ def foo_album_response(foo_album): @pytest.fixture def artist_albums_mock(web_album_mock_base, web_album_mock_base2): - params = urllib.parse.urlencode({"market": "from_token", "include_groups": "single,album"}) + params = urllib.parse.urlencode( + {"market": "from_token", "include_groups": "single,album"} + ) return { "href": url(f"artists/abba/albums?{params}"), "items": [web_album_mock_base, web_album_mock_base2], @@ -972,7 +974,9 @@ def test_with_all_tracks_error( assert "Spotify Web API request failed: baz" in caplog.text @responses.activate - def test_with_all_tracks(self, spotify_client, foo_album_response, foo_album_next_tracks): + def test_with_all_tracks( + self, spotify_client, foo_album_response, foo_album_next_tracks + ): responses.add( responses.GET, foo_album_next_tracks["href"], @@ -986,7 +990,11 @@ def test_with_all_tracks(self, spotify_client, foo_album_response, foo_album_nex @responses.activate def test_with_all_tracks_uses_cached_tracks_when_unchanged( - self, mock_time, foo_album_response, foo_album_next_tracks, spotify_client + self, + mock_time, + foo_album_response, + foo_album_next_tracks, + spotify_client, ): responses.add( responses.GET, @@ -1158,32 +1166,45 @@ def test_get_album(self, foo_album, foo_album_next_tracks, spotify_client): "all_tracks,", [(True), (False)], ) - def test_get_artist_albums(self, artist_albums_mock, web_album_mock, web_album_mock2, spotify_client, all_tracks): + def test_get_artist_albums( + self, + artist_albums_mock, + web_album_mock, + web_album_mock2, + spotify_client, + all_tracks, + ): responses.add( responses.GET, url("artists/abba/albums"), json=artist_albums_mock, - match=[matchers.query_string_matcher("market=from_token&include_groups=single%2Calbum")], + match=[ + matchers.query_string_matcher( + "market=from_token&include_groups=single%2Calbum" + ) + ], ) responses.add( responses.GET, - url(f"albums/def"), + url("albums/def"), json=web_album_mock, ) responses.add( responses.GET, - url(f"albums/xyz"), + url("albums/xyz"), json=web_album_mock2, ) link = web.WebLink.from_uri("spotify:artist:abba") - results = list(spotify_client.get_artist_albums(link, all_tracks=all_tracks)) - + results = list( + spotify_client.get_artist_albums(link, all_tracks=all_tracks) + ) + if all_tracks: assert len(responses.calls) == 3 else: assert len(responses.calls) == 1 - + assert len(results) == 2 assert results[0]["name"] == "DEF 456" assert results[1]["name"] == "XYZ 789" @@ -1195,22 +1216,23 @@ def test_get_artist_albums(self, artist_albums_mock, web_album_mock, web_album_m assert "tracks" not in results[0] assert "tracks" not in results[1] - @responses.activate - def test_get_artist_albums_error(self, artist_albums_mock, web_album_mock, spotify_client, caplog): + def test_get_artist_albums_error( + self, artist_albums_mock, web_album_mock, spotify_client, caplog + ): responses.add( responses.GET, - url(f"artists/abba/albums"), + url("artists/abba/albums"), json=artist_albums_mock, ) responses.add( responses.GET, - url(f"albums/def"), + url("albums/def"), json=web_album_mock, ) responses.add( responses.GET, - url(f"albums/xyz"), + url("albums/xyz"), json={"error": "bar"}, ) @@ -1238,12 +1260,12 @@ def test_get_artist_albums_error(self, artist_albums_mock, web_album_mock, spoti def test_get_track(self, web_track_mock, spotify_client, uri, success): responses.add( responses.GET, - url(f"tracks/abc"), + url("tracks/abc"), json=web_track_mock, ) responses.add( responses.GET, - url(f"tracks/xyz"), + url("tracks/xyz"), json={}, ) @@ -1260,7 +1282,7 @@ def test_get_track(self, web_track_mock, spotify_client, uri, success): def test_get_artist_top_tracks(self, web_track_mock, spotify_client): responses.add( responses.GET, - url(f"artists/baz/top-tracks"), + url("artists/baz/top-tracks"), json={"tracks": [web_track_mock, web_track_mock]}, ) link = web.WebLink.from_uri("spotify:artist:baz") From 65fda08eb943e050d24db3a804082ff36202d57e Mon Sep 17 00:00:00 2001 From: 3np <3np@example.com> Date: Sun, 31 Jul 2022 16:57:18 +0900 Subject: [PATCH 14/21] Add tests --- tests/test_browse.py | 20 ++++++++++++ tests/test_lookup.py | 20 ++++++++++++ tests/test_translator.py | 14 +++++++++ tests/test_web.py | 66 +++++++++++++++++++++++++++++++++++++++- 4 files changed, 119 insertions(+), 1 deletion(-) diff --git a/tests/test_browse.py b/tests/test_browse.py index 665adc44..4f6845cd 100644 --- a/tests/test_browse.py +++ b/tests/test_browse.py @@ -153,6 +153,26 @@ def test_browse_artist( ) +def test_browse_artist_bad_uri( + web_client_mock, + web_album_mock_base, + web_track_mock, + provider, + caplog, +): + web_client_mock.get_artist_albums.return_value = [web_album_mock_base] + web_client_mock.get_artist_top_tracks.return_value = [ + web_track_mock, + web_track_mock, + web_track_mock, + ] + + results = provider.browse("spotify:artist:def:xyz") + + assert len(results) == 0 + assert "Failed to browse" in caplog.text + + def test_browse_top_tracks_with_too_many_uri_parts(provider): results = provider.browse("spotify:top:tracks:foo:bar") diff --git a/tests/test_lookup.py b/tests/test_lookup.py index 1688f045..da76edfb 100644 --- a/tests/test_lookup.py +++ b/tests/test_lookup.py @@ -73,6 +73,15 @@ def test_lookup_of_album_uri(web_client_mock, web_album_mock, provider): assert track.album.name == "DEF 456" +def test_lookup_of_album_uri_empty_response(web_client_mock, provider, caplog): + web_client_mock.get_album.return_value = {} + + results = provider.lookup("spotify:album:def") + + assert len(results) == 0 + assert "Invalid album response" in caplog.text + + def test_lookup_of_artist_uri( web_track_mock, web_album_mock, web_client_mock, provider ): @@ -157,6 +166,17 @@ def test_lookup_of_playlist_uri(web_client_mock, web_playlist_mock, provider): assert track.bitrate == 160 +def test_lookup_of_playlist_uri_empty_response( + web_client_mock, provider, caplog +): + web_client_mock.get_playlist.return_value = None + + results = provider.lookup("spotify:playlist:alice:foo") + + assert len(results) == 0 + assert "Invalid playlist response" in caplog.text + + def test_lookup_of_yourtracks_uri(web_client_mock, web_track_mock, provider): web_saved_track_mock = {"track": web_track_mock} web_client_mock.get_all.return_value = [ diff --git a/tests/test_translator.py b/tests/test_translator.py index 57203cd2..ec9e8d66 100644 --- a/tests/test_translator.py +++ b/tests/test_translator.py @@ -596,6 +596,20 @@ def test_web_to_album_tracks_unplayable(self, web_album_mock): assert len(tracks) == 0 + def test_web_to_album_tracks_nolist(self, web_album_mock): + web_album_mock["tracks"] = {"items": {}} + + tracks = translator.web_to_album_tracks(web_album_mock) + + assert isinstance(tracks, list) + assert len(tracks) == 0 + + def test_web_to_album_tracks_none(self): + tracks = translator.web_to_album_tracks(None) + + assert isinstance(tracks, list) + assert len(tracks) == 0 + class TestWebToTrack: def test_calls_web_to_track_ref(self, web_track_mock): diff --git a/tests/test_web.py b/tests/test_web.py index 98c4fd54..90f29204 100644 --- a/tests/test_web.py +++ b/tests/test_web.py @@ -960,7 +960,7 @@ def test_get_user_playlists(self, spotify_client): @responses.activate def test_with_all_tracks_error( - self, spotify_client, foo_album_response, foo_album_next_tracks, caplog + self, spotify_client, foo_album_response, caplog ): responses.add( responses.GET, @@ -1161,6 +1161,16 @@ def test_get_album(self, foo_album, foo_album_next_tracks, spotify_client): assert len(responses.calls) == 2 assert result["tracks"]["items"] == [3, 4, 5, 6, 7, 8] + @responses.activate + def test_get_album_wrong_linktype(self, spotify_client, caplog): + link = web.WebLink.from_uri("spotify:album:abba") + link.type = "your" + results = list(spotify_client.get_album(link)) + + assert len(responses.calls) == 0 + assert len(results) == 0 + assert "Expecting Spotify album URI" in caplog.text + @responses.activate @pytest.mark.parametrize( "all_tracks,", @@ -1245,6 +1255,43 @@ def test_get_artist_albums_error( assert results[1] == {} assert "Spotify Web API request failed: bar" in caplog.text + @responses.activate + def test_get_artist_albums_wrong_linktype(self, spotify_client, caplog): + link = web.WebLink.from_uri("spotify:artist:abba") + link.type = "your" + results = list(spotify_client.get_artist_albums(link)) + + assert len(responses.calls) == 0 + assert len(results) == 0 + assert "Expecting Spotify artist URI" in caplog.text + + @responses.activate + def test_get_artist_albums_value_error( + self, web_album_mock, spotify_client, caplog + ): + responses.add( + responses.GET, + url("artists/abba/albums"), + json={ + "href": url("artists/abba/albums"), + "items": [{"uri": "BLOPP"}, web_album_mock], + "next": None, + }, + ) + responses.add( + responses.GET, + url("albums/def"), + json=web_album_mock, + ) + + link = web.WebLink.from_uri("spotify:artist:abba") + results = list(spotify_client.get_artist_albums(link)) + + assert len(responses.calls) == 2 + assert len(results) == 1 + assert results[0]["name"] == "DEF 456" + assert "Could not parse 'BLOPP' as a Spotify URI" in caplog.text + @responses.activate @pytest.mark.parametrize( "uri,success", @@ -1292,6 +1339,23 @@ def test_get_artist_top_tracks(self, web_track_mock, spotify_client): assert len(results) == 2 assert results[0]["name"] == "ABC 123" + @responses.activate + def test_get_artist_top_tracks_invalid_uri( + self, web_track_mock, spotify_client, caplog + ): + responses.add( + responses.GET, + url("artists/baz/top-tracks"), + json={"tracks": [web_track_mock, web_track_mock]}, + ) + link = web.WebLink.from_uri("spotify:artist:baz") + link.type = "your" + results = spotify_client.get_artist_top_tracks(link) + + assert len(responses.calls) == 0 + assert len(results) == 0 + assert "Expecting Spotify artist URI" in caplog.text + @pytest.mark.parametrize( "uri,type_,id_", From 5a65435dc33dcba889237c9381b519c8831fe825 Mon Sep 17 00:00:00 2001 From: 3np <3np@example.com> Date: Mon, 1 Aug 2022 13:31:57 +0900 Subject: [PATCH 15/21] web: fix timezone mishandling in Retry-Header parsing --- mopidy_spotify/web.py | 11 ++++---- tests/test_web.py | 66 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 72 insertions(+), 5 deletions(-) diff --git a/mopidy_spotify/web.py b/mopidy_spotify/web.py index 5f3bb4b4..4499e19b 100644 --- a/mopidy_spotify/web.py +++ b/mopidy_spotify/web.py @@ -1,5 +1,4 @@ import copy -import email import logging import os import re @@ -9,6 +8,7 @@ from datetime import datetime from enum import Enum, unique from typing import Optional +from email.utils import parsedate_to_datetime import requests @@ -258,11 +258,12 @@ def _parse_retry_after(self, response): elif re.match(r"^\s*[0-9]+\s*$", value): seconds = int(value) else: - date_tuple = email.utils.parsedate(value) - if date_tuple is None: + now = datetime.utcnow() + try: + date_tuple = parsedate_to_datetime(value) + seconds = (date_tuple - now).seconds + except ValueError: seconds = 0 - else: - seconds = time.mktime(date_tuple) - time.time() return max(0, seconds) diff --git a/tests/test_web.py b/tests/test_web.py index 90f29204..5ec6a79d 100644 --- a/tests/test_web.py +++ b/tests/test_web.py @@ -1,10 +1,12 @@ import urllib from unittest import mock +from time import time import pytest import requests import responses from responses import matchers +from email.utils import formatdate import mopidy_spotify from mopidy_spotify import web @@ -892,6 +894,70 @@ def test_get_one_increased_expiry(self, mock_time, spotify_client): assert 1000 + spotify_client.DEFAULT_EXTRA_EXPIRY == result._expires + @responses.activate + def test_get_one_retry_header_invalid(self, spotify_client, caplog): + responses.add( + responses.GET, + url("foo"), + status=429, + adding_headers={"Retry-After": " herpderp "}, + ) + responses.add( + responses.GET, url("foo"), json={"error": "bar"}, status=403 + ) + + result = spotify_client.get_one("foo") + + assert result == {} + assert ( + "Retrying https://api.spotify.com/v1/foo in 0.500 seconds." + in caplog.text + ) + assert "Spotify Web API request failed: bar" in caplog.text + + @responses.activate + def test_get_one_retry_header_date(self, spotify_client, caplog): + t = time() + 2 + responses.add( + responses.GET, + url("foo"), + status=429, + adding_headers={"Retry-After": formatdate(t)}, + ) + responses.add( + responses.GET, url("foo"), json={"error": "bar"}, status=403 + ) + + result = spotify_client.get_one("foo") + + assert result == {} + assert ( + "Retrying https://api.spotify.com/v1/foo in 1.000 seconds." + in caplog.text + ) + assert "Spotify Web API request failed: bar" in caplog.text + + @responses.activate + def test_get_one_retry_header_seconds(self, spotify_client, caplog): + responses.add( + responses.GET, + url("foo"), + status=429, + adding_headers={"Retry-After": " 1 "}, + ) + responses.add( + responses.GET, url("foo"), json={"error": "bar"}, status=403 + ) + + result = spotify_client.get_one("foo") + + assert result == {} + assert ( + "Retrying https://api.spotify.com/v1/foo in 1.000 seconds." + in caplog.text + ) + assert "Spotify Web API request failed: bar" in caplog.text + @responses.activate def test_get_all(self, spotify_client): responses.add( From e2ae214e3eed0ea64cd7a0979ea6a30c57136a4c Mon Sep 17 00:00:00 2001 From: 3np <3np@example.com> Date: Mon, 1 Aug 2022 14:06:02 +0900 Subject: [PATCH 16/21] fix: don't error on non-responses --- mopidy_spotify/web.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/mopidy_spotify/web.py b/mopidy_spotify/web.py index 4499e19b..c3ed571e 100644 --- a/mopidy_spotify/web.py +++ b/mopidy_spotify/web.py @@ -98,7 +98,7 @@ def get(self, path, cache=None, *args, **kwargs): if result is None or "error" in result: logger.error( "Spotify Web API request failed: " - f"{result.get('error','Unknown')}" + f"{result.get('error','Unknown') if result else 'Unknown'}" ) return WebResponse(None, None) From 141b72c96c7b34c116a032e848c557c3356e114c Mon Sep 17 00:00:00 2001 From: 3np <3np@example.com> Date: Mon, 1 Aug 2022 14:06:22 +0900 Subject: [PATCH 17/21] test: add test for starred playlist link --- tests/test_web.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/test_web.py b/tests/test_web.py index 5ec6a79d..6c8dd8df 100644 --- a/tests/test_web.py +++ b/tests/test_web.py @@ -1446,6 +1446,7 @@ def test_weblink_from_uri_spotify_uri(uri, type_, id_): "uri,id_,owner", [ ("spotify:user:alice:playlist:foo", "foo", "alice"), + ("spotify:user:alice:starred", None, "alice"), ("spotify:playlist:foo", "foo", None), ("http://open.spotify.com/playlist/foo", "foo", None), ("https://open.spotify.com/playlist/foo", "foo", None), From d3797b9ed912d71158ebfa682233af91d1bf4bdb Mon Sep 17 00:00:00 2001 From: Nick Steel Date: Thu, 28 Jul 2022 01:12:09 +0100 Subject: [PATCH 18/21] Remove pyspotify: set user/pass source properties using on_source_setup --- mopidy_spotify/backend.py | 18 ++++++++---------- tests/test_playback.py | 24 ++++++++++++++++++++---- 2 files changed, 28 insertions(+), 14 deletions(-) diff --git a/mopidy_spotify/backend.py b/mopidy_spotify/backend.py index 6ba6aa97..98869839 100644 --- a/mopidy_spotify/backend.py +++ b/mopidy_spotify/backend.py @@ -1,12 +1,8 @@ -import logging - import pykka from mopidy import backend from mopidy_spotify import Extension, library, playlists, web -logger = logging.getLogger(__name__) - class SpotifyBackend(pykka.ThreadingActor, backend.Backend): def __init__(self, config, audio): @@ -43,9 +39,11 @@ def on_start(self): class SpotifyPlaybackProvider(backend.PlaybackProvider): def __init__(self, *args, **kwargs): super().__init__(*args, **kwargs) - username = self.backend._config["spotify"]["username"] - password = self.backend._config["spotify"]["password"] - self._auth_string = f"username={username}&password={password}" - - def translate_uri(self, uri): - return f"{uri}?{self._auth_string}" + self._cache_location = Extension().get_cache_dir(self.backend._config) + + def on_source_setup(self, source): + config = self.backend._config["spotify"] + for prop in ["username", "password"]: + source.set_property(prop, config[prop]) + if config["allow_cache"]: + source.set_property("cache-credentials", self._cache_location) diff --git a/tests/test_playback.py b/tests/test_playback.py index 35692e24..50a10f33 100644 --- a/tests/test_playback.py +++ b/tests/test_playback.py @@ -31,7 +31,23 @@ def test_is_a_playback_provider(provider): assert isinstance(provider, backend_api.PlaybackProvider) -def test_translate_uri_sets_credentials(config, provider): - assert ( - provider.translate_uri("baz") == "baz?username=alice&password=password" - ) +def test_on_source_setup_sets_properties(config, provider): + mock_source = mock.MagicMock() + provider.on_source_setup(mock_source) + + assert mock_source.set_property.mock_calls == [ + mock.call("username", "alice"), + mock.call("password", "password"), + mock.call("cache-credentials", mock.ANY), + ] + + +def test_on_source_setup_without_caching(config, provider): + config["spotify"]["allow_cache"] = False + mock_source = mock.MagicMock() + provider.on_source_setup(mock_source) + + assert mock_source.set_property.mock_calls == [ + mock.call("username", "alice"), + mock.call("password", "password"), + ] From 270e0c71a18b38961d7755a1363402bafde73927 Mon Sep 17 00:00:00 2001 From: Nick Steel Date: Mon, 28 Nov 2022 17:52:56 +0000 Subject: [PATCH 19/21] Update tests/test_distinct.py Co-authored-by: 3nprob <74199244+3nprob@users.noreply.github.com> --- tests/test_distinct.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_distinct.py b/tests/test_distinct.py index ce274076..89c629d6 100644 --- a/tests/test_distinct.py +++ b/tests/test_distinct.py @@ -31,7 +31,7 @@ def search_mock(mopidy_album_mock, mopidy_artist_mock): patcher.stop() -def test_get_distinct_when_offine(web_client_mock, provider): +def test_get_distinct_when_offline(web_client_mock, provider): web_client_mock.logged_in = False results = provider.get_distinct("artist") From 6d6a87f248989de66aa195acc226942bcd37e8fc Mon Sep 17 00:00:00 2001 From: Nick Steel Date: Mon, 28 Nov 2022 23:03:10 +0000 Subject: [PATCH 20/21] Requires mopidy.backend.PlaybackProvider.on_source_setup in Mopidy v3.4.x --- setup.cfg | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/setup.cfg b/setup.cfg index a57b2bb5..c811dbe2 100644 --- a/setup.cfg +++ b/setup.cfg @@ -27,7 +27,7 @@ include_package_data = True packages = find: python_requires = >= 3.7 install_requires = - Mopidy >= 3.0.0 + Mopidy >= 3.4.0 Pykka >= 2.0.1 requests >= 2.20.0 setuptools From 3c766ba1c0354164c8515d342112ca1562de1271 Mon Sep 17 00:00:00 2001 From: Nick Steel Date: Wed, 7 Dec 2022 12:50:01 +0000 Subject: [PATCH 21/21] fixed retry_after when date in the past --- mopidy_spotify/web.py | 4 +- tests/test_web.py | 92 +++++++++++++++++++------------------------ 2 files changed, 42 insertions(+), 54 deletions(-) diff --git a/mopidy_spotify/web.py b/mopidy_spotify/web.py index c3ed571e..da9d2efe 100644 --- a/mopidy_spotify/web.py +++ b/mopidy_spotify/web.py @@ -261,8 +261,8 @@ def _parse_retry_after(self, response): now = datetime.utcnow() try: date_tuple = parsedate_to_datetime(value) - seconds = (date_tuple - now).seconds - except ValueError: + seconds = (date_tuple - now).total_seconds() + except Exception: seconds = 0 return max(0, seconds) diff --git a/tests/test_web.py b/tests/test_web.py index 6c8dd8df..90358aa9 100644 --- a/tests/test_web.py +++ b/tests/test_web.py @@ -1,12 +1,11 @@ import urllib from unittest import mock -from time import time +from datetime import datetime import pytest import requests import responses from responses import matchers -from email.utils import formatdate import mopidy_spotify from mopidy_spotify import web @@ -32,6 +31,16 @@ def mock_time(): patcher.stop() +@pytest.fixture +def mock_utcnow(): + patcher = mock.patch("mopidy_spotify.web.datetime") + mock_datetime = patcher.start() + mock_datetime.utcnow = mock.Mock() + mock_datetime.side_effect = lambda *args, **kw: datetime(*args, **kw) + yield mock_datetime.utcnow + patcher.stop() + + @pytest.fixture def skip_refresh_token(): patcher = mock.patch.object( @@ -63,6 +72,31 @@ def test_user_agent(oauth_client): ) +@pytest.mark.parametrize( + "header,expected", + [ + (None, 0), + ("", 0), + ("1", 1), + ("-1", 0), + (" 2 ", 2), + (" 2 foo", 0), + ("2 9", 0), + ("foo", 0), + ("Wed, 07 Dec 2022 11:24:16", 0), + ("Wed, 07 Dec 2022 11:29:16", 110), + ("Wed, 77 Dec 2022 11:29:16", 0), + ("foobar", 0), + ], +) +def test_parse_retry_after(oauth_client, mock_utcnow, header, expected): + mock_utcnow.return_value = datetime(2022, 12, 7, 11, 27, 26, 0) + mock_response = mock.Mock(headers={"Retry-After": header}) + result = oauth_client._parse_retry_after(mock_response) + + assert result == expected + + @responses.activate def test_request_exception(oauth_client, caplog): responses.add( @@ -895,68 +929,22 @@ def test_get_one_increased_expiry(self, mock_time, spotify_client): assert 1000 + spotify_client.DEFAULT_EXTRA_EXPIRY == result._expires @responses.activate - def test_get_one_retry_header_invalid(self, spotify_client, caplog): - responses.add( - responses.GET, - url("foo"), - status=429, - adding_headers={"Retry-After": " herpderp "}, - ) - responses.add( - responses.GET, url("foo"), json={"error": "bar"}, status=403 - ) - - result = spotify_client.get_one("foo") - - assert result == {} - assert ( - "Retrying https://api.spotify.com/v1/foo in 0.500 seconds." - in caplog.text - ) - assert "Spotify Web API request failed: bar" in caplog.text - - @responses.activate - def test_get_one_retry_header_date(self, spotify_client, caplog): - t = time() + 2 + def test_get_one_retry_header(self, spotify_client, caplog): + spotify_client._timeout = 0 responses.add( responses.GET, url("foo"), status=429, - adding_headers={"Retry-After": formatdate(t)}, - ) - responses.add( - responses.GET, url("foo"), json={"error": "bar"}, status=403 + adding_headers={"Retry-After": "66"}, ) result = spotify_client.get_one("foo") assert result == {} assert ( - "Retrying https://api.spotify.com/v1/foo in 1.000 seconds." + "Retrying https://api.spotify.com/v1/foo in 66.000 seconds." in caplog.text ) - assert "Spotify Web API request failed: bar" in caplog.text - - @responses.activate - def test_get_one_retry_header_seconds(self, spotify_client, caplog): - responses.add( - responses.GET, - url("foo"), - status=429, - adding_headers={"Retry-After": " 1 "}, - ) - responses.add( - responses.GET, url("foo"), json={"error": "bar"}, status=403 - ) - - result = spotify_client.get_one("foo") - - assert result == {} - assert ( - "Retrying https://api.spotify.com/v1/foo in 1.000 seconds." - in caplog.text - ) - assert "Spotify Web API request failed: bar" in caplog.text @responses.activate def test_get_all(self, spotify_client):