diff --git a/docs/changelog.rst b/docs/changelog.rst index 0a19e834..182db916 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -30,6 +30,8 @@ Core API - Add :meth:`mopidy.core.playback.PlaybackController.get_current_tlid`. (Part of: :issue:`1137`) +- Update core to handle backend crashes and bad data. (Fixes: :issue:`1161`) + Models ------ diff --git a/mopidy/core/library.py b/mopidy/core/library.py index d9803d3f..8e6e4015 100644 --- a/mopidy/core/library.py +++ b/mopidy/core/library.py @@ -1,16 +1,32 @@ from __future__ import absolute_import, unicode_literals import collections +import contextlib import logging import operator import urlparse +from mopidy import compat, exceptions, models from mopidy.utils import deprecation, validation logger = logging.getLogger(__name__) +@contextlib.contextmanager +def _backend_error_handling(backend, reraise=None): + try: + yield + except exceptions.ValidationError as e: + logger.error('%s backend returned bad data: %s', + backend.actor_ref.actor_class.__name__, e) + except Exception as e: + if reraise and isinstance(e, reraise): + raise + logger.exception('%s backend caused an exception.', + backend.actor_ref.actor_class.__name__) + + class LibraryController(object): pykka_traversable = True @@ -79,22 +95,24 @@ class LibraryController(object): backends = self.backends.with_library_browse.values() futures = {b: b.library.root_directory for b in backends} for backend, future in futures.items(): - try: - directories.add(future.get()) - except Exception: - logger.exception('%s backend caused an exception.', - backend.actor_ref.actor_class.__name__) + with _backend_error_handling(backend): + root = future.get() + validation.check_instance(root, models.Ref) + directories.add(root) return sorted(directories, key=operator.attrgetter('name')) def _browse(self, uri): scheme = urlparse.urlparse(uri).scheme backend = self.backends.with_library_browse.get(scheme) - try: - if backend: - return backend.library.browse(uri).get() - except Exception: - logger.exception('%s backend caused an exception.', - backend.actor_ref.actor_class.__name__) + + if not backend: + return [] + + with _backend_error_handling(backend): + result = backend.library.browse(uri).get() + validation.check_instances(result, models.Ref) + return result + return [] def get_distinct(self, field, query=None): @@ -120,11 +138,11 @@ class LibraryController(object): futures = {b: b.library.get_distinct(field, query) for b in self.backends.with_library.values()} for backend, future in futures.items(): - try: - result.update(future.get()) - except Exception: - logger.exception('%s backend caused an exception.', - backend.actor_ref.actor_class.__name__) + with _backend_error_handling(backend): + values = future.get() + if values is not None: + validation.check_instances(values, compat.text_type) + result.update(values) return result def get_images(self, uris): @@ -152,12 +170,16 @@ class LibraryController(object): results = {uri: tuple() for uri in uris} for backend, future in futures.items(): - try: + with _backend_error_handling(backend): + if future.get() is None: + continue + validation.check_instance(future.get(), collections.Mapping) for uri, images in future.get().items(): + if uri not in uris: + raise exceptions.ValidationError( + 'Got unknown image URI: %s' % uri) + validation.check_instances(images, models.Image) results[uri] += tuple(images) - except Exception: - logger.exception('%s backend caused an exception.', - backend.actor_ref.actor_class.__name__) return results def find_exact(self, query=None, uris=None, **kwargs): @@ -202,7 +224,7 @@ class LibraryController(object): uris = [uri] futures = {} - result = {u: [] for u in uris} + results = {u: [] for u in uris} # TODO: lookup(uris) to backend APIs for backend, backend_uris in self._get_backends_to_uris(uris).items(): @@ -210,15 +232,15 @@ class LibraryController(object): futures[(backend, u)] = backend.library.lookup(u) for (backend, u), future in futures.items(): - try: - result[u] = future.get() - except Exception: - logger.exception('%s backend caused an exception.', - backend.actor_ref.actor_class.__name__) + with _backend_error_handling(backend): + result = future.get() + if result is not None: + validation.check_instances(result, models.Track) + results[u] = result if uri: - return result[uri] - return result + return results[uri] + return results def refresh(self, uri=None): """ @@ -241,11 +263,8 @@ class LibraryController(object): futures[backend] = backend.library.refresh(uri) for backend, future in futures.items(): - try: + with _backend_error_handling(backend): future.get() - except Exception: - logger.exception('%s backend caused an exception.', - backend.actor_ref.actor_class.__name__) def search(self, query=None, uris=None, exact=False, **kwargs): """ @@ -311,25 +330,26 @@ class LibraryController(object): futures[backend] = backend.library.search( query=query, uris=backend_uris, exact=exact) + # Some of our tests check for LookupError to catch bad queries. This is + # silly and should be replaced with query validation before passing it + # to the backends. + reraise = (TypeError, LookupError) + results = [] for backend, future in futures.items(): try: - results.append(future.get()) + with _backend_error_handling(backend, reraise=reraise): + result = future.get() + if result is not None: + validation.check_instance(result, models.SearchResult) + results.append(result) except TypeError: backend_name = backend.actor_ref.actor_class.__name__ logger.warning( '%s does not implement library.search() with "exact" ' 'support. Please upgrade it.', backend_name) - except LookupError: - # Some of our tests check for this to catch bad queries. This - # is silly and should be replaced with query validation before - # passing it to the backends. - raise - except Exception: - logger.exception('%s backend caused an exception.', - backend.actor_ref.actor_class.__name__) - return [r for r in results if r] + return results def _normalize_query(query): diff --git a/mopidy/core/mixer.py b/mopidy/core/mixer.py index fde7ee5a..2815fb0c 100644 --- a/mopidy/core/mixer.py +++ b/mopidy/core/mixer.py @@ -1,13 +1,27 @@ from __future__ import absolute_import, unicode_literals +import contextlib import logging +from mopidy import exceptions from mopidy.utils import validation logger = logging.getLogger(__name__) +@contextlib.contextmanager +def _mixer_error_handling(mixer): + try: + yield + except exceptions.ValidationError as e: + logger.error('%s mixer returned bad data: %s', + mixer.actor_ref.actor_class.__name__, e) + except Exception: + logger.exception('%s mixer caused an exception.', + mixer.actor_ref.actor_class.__name__) + + class MixerController(object): pykka_traversable = True @@ -21,8 +35,15 @@ class MixerController(object): The volume scale is linear. """ - if self._mixer is not None: - return self._mixer.get_volume().get() + if self._mixer is None: + return None + + with _mixer_error_handling(self._mixer): + volume = self._mixer.get_volume().get() + volume is None or validation.check_integer(volume, min=0, max=100) + return volume + + return None def set_volume(self, volume): """Set the volume. @@ -36,9 +57,14 @@ class MixerController(object): validation.check_integer(volume, min=0, max=100) if self._mixer is None: - return False - else: - return self._mixer.set_volume(volume).get() + return False # TODO: 2.0 return None + + with _mixer_error_handling(self._mixer): + result = self._mixer.set_volume(volume).get() + validation.check_instance(result, bool) + return result + + return False def get_mute(self): """Get mute state. @@ -46,8 +72,15 @@ class MixerController(object): :class:`True` if muted, :class:`False` unmuted, :class:`None` if unknown. """ - if self._mixer is not None: - return self._mixer.get_mute().get() + if self._mixer is None: + return None + + with _mixer_error_handling(self._mixer): + mute = self._mixer.get_mute().get() + mute is None or validation.check_instance(mute, bool) + return mute + + return None def set_mute(self, mute): """Set mute state. @@ -58,6 +91,11 @@ class MixerController(object): """ validation.check_boolean(mute) if self._mixer is None: - return False - else: - return self._mixer.set_mute(bool(mute)).get() + return False # TODO: 2.0 return None + + with _mixer_error_handling(self._mixer): + result = self._mixer.set_mute(bool(mute)).get() + validation.check_instance(result, bool) + return result + + return False diff --git a/mopidy/core/playlists.py b/mopidy/core/playlists.py index fc8951e7..c5e6d0cc 100644 --- a/mopidy/core/playlists.py +++ b/mopidy/core/playlists.py @@ -1,15 +1,31 @@ from __future__ import absolute_import, unicode_literals +import contextlib import logging import urlparse +from mopidy import exceptions from mopidy.core import listener -from mopidy.models import Playlist +from mopidy.models import Playlist, Ref from mopidy.utils import deprecation, validation logger = logging.getLogger(__name__) +@contextlib.contextmanager +def _backend_error_handling(backend, reraise=None): + try: + yield + except exceptions.ValidationError as e: + logger.error('%s backend returned bad data: %s', + backend.actor_ref.actor_class.__name__, e) + except Exception as e: + if reraise and isinstance(e, reraise): + raise + logger.exception('%s backend caused an exception.', + backend.actor_ref.actor_class.__name__) + + class PlaylistsController(object): pykka_traversable = True @@ -34,17 +50,18 @@ class PlaylistsController(object): for backend in set(self.backends.with_playlists.values())} results = [] - for backend, future in futures.items(): + for b, future in futures.items(): try: - results.extend(future.get()) + with _backend_error_handling(b, reraise=NotImplementedError): + playlists = future.get() + if playlists is not None: + validation.check_instances(playlists, Ref) + results.extend(playlists) except NotImplementedError: - backend_name = backend.actor_ref.actor_class.__name__ + backend_name = b.actor_ref.actor_class.__name__ logger.warning( '%s does not implement playlists.as_list(). ' 'Please upgrade it.', backend_name) - except Exception: - logger.exception('%s backend caused an exception.', - backend.actor_ref.actor_class.__name__) return results @@ -66,8 +83,16 @@ class PlaylistsController(object): uri_scheme = urlparse.urlparse(uri).scheme backend = self.backends.with_playlists.get(uri_scheme, None) - if backend: - return backend.playlists.get_items(uri).get() + + if not backend: + return None + + with _backend_error_handling(backend): + items = backend.playlists.get_items(uri).get() + items is None or validation.check_instances(items, Ref) + return items + + return None def get_playlists(self, include_tracks=True): """ @@ -120,22 +145,23 @@ class PlaylistsController(object): :type name: string :param uri_scheme: use the backend matching the URI scheme :type uri_scheme: string - :rtype: :class:`mopidy.models.Playlist` + :rtype: :class:`mopidy.models.Playlist` or :class:`None` """ if uri_scheme in self.backends.with_playlists: backends = [self.backends.with_playlists[uri_scheme]] else: backends = self.backends.with_playlists.values() + for backend in backends: - try: - playlist = backend.playlists.create(name).get() - except Exception: - playlist = None - # Workaround for playlist providers that return None from create() - if not playlist: - continue - listener.CoreListener.send('playlist_changed', playlist=playlist) - return playlist + with _backend_error_handling(backend): + result = backend.playlists.create(name).get() + if result is None: + continue + validation.check_instance(result, Playlist) + listener.CoreListener.send('playlist_changed', playlist=result) + return result + + return None def delete(self, uri): """ @@ -151,8 +177,14 @@ class PlaylistsController(object): uri_scheme = urlparse.urlparse(uri).scheme backend = self.backends.with_playlists.get(uri_scheme, None) - if backend: + if not backend: + return + + with _backend_error_handling(backend): backend.playlists.delete(uri).get() + # TODO: emit playlist changed? + + # TODO: return value? def filter(self, criteria=None, **kwargs): """ @@ -198,11 +230,16 @@ class PlaylistsController(object): """ uri_scheme = urlparse.urlparse(uri).scheme backend = self.backends.with_playlists.get(uri_scheme, None) - if backend: - return backend.playlists.lookup(uri).get() - else: + if not backend: return None + with _backend_error_handling(backend): + playlist = backend.playlists.lookup(uri).get() + playlist is None or validation.check_instance(playlist, Playlist) + return playlist + + return None + # TODO: there is an inconsistency between library.refresh(uri) and this # call, not sure how to sort this out. def refresh(self, uri_scheme=None): @@ -231,12 +268,9 @@ class PlaylistsController(object): futures[backend] = backend.playlists.refresh() for backend, future in futures.items(): - try: + with _backend_error_handling(backend): future.get() playlists_loaded = True - except Exception: - logger.exception('%s backend caused an exception.', - backend.actor_ref.actor_class.__name__) if playlists_loaded: listener.CoreListener.send('playlists_loaded') @@ -270,7 +304,16 @@ class PlaylistsController(object): uri_scheme = urlparse.urlparse(playlist.uri).scheme backend = self.backends.with_playlists.get(uri_scheme, None) - if backend: + if not backend: + return None + + # TODO: we let AssertionError error through due to legacy tests :/ + with _backend_error_handling(backend, reraise=AssertionError): playlist = backend.playlists.save(playlist).get() - listener.CoreListener.send('playlist_changed', playlist=playlist) + playlist is None or validation.check_instance(playlist, Playlist) + if playlist: + listener.CoreListener.send( + 'playlist_changed', playlist=playlist) return playlist + + return None diff --git a/tests/core/test_library.py b/tests/core/test_library.py index 4a8937c3..2f244ce7 100644 --- a/tests/core/test_library.py +++ b/tests/core/test_library.py @@ -15,6 +15,7 @@ class BaseCoreLibraryTest(unittest.TestCase): dummy1_root = Ref.directory(uri='dummy1:directory', name='dummy1') self.backend1 = mock.Mock() self.backend1.uri_schemes.get.return_value = ['dummy1'] + self.backend1.actor_ref.actor_class.__name__ = 'DummyBackend1' self.library1 = mock.Mock(spec=backend.LibraryProvider) self.library1.get_images.return_value.get.return_value = {} self.library1.root_directory.get.return_value = dummy1_root @@ -23,6 +24,7 @@ class BaseCoreLibraryTest(unittest.TestCase): dummy2_root = Ref.directory(uri='dummy2:directory', name='dummy2') self.backend2 = mock.Mock() self.backend2.uri_schemes.get.return_value = ['dummy2', 'du2'] + self.backend2.actor_ref.actor_class.__name__ = 'DummyBackend2' self.library2 = mock.Mock(spec=backend.LibraryProvider) self.library2.get_images.return_value.get.return_value = {} self.library2.root_directory.get.return_value = dummy2_root @@ -31,6 +33,7 @@ class BaseCoreLibraryTest(unittest.TestCase): # A backend without the optional library provider self.backend3 = mock.Mock() self.backend3.uri_schemes.get.return_value = ['dummy3'] + self.backend3.actor_ref.actor_class.__name__ = 'DummyBackend3' self.backend3.has_library().get.return_value = False self.backend3.has_library_browse().get.return_value = False @@ -148,11 +151,14 @@ class CoreLibraryTest(BaseCoreLibraryTest): self.core.library.lookup('dummy1:a', ['dummy2:a']) def test_lookup_can_handle_uris(self): - self.library1.lookup().get.return_value = [1234] - self.library2.lookup().get.return_value = [5678] + track1 = Track(name='abc') + track2 = Track(name='def') + + self.library1.lookup().get.return_value = [track1] + self.library2.lookup().get.return_value = [track2] result = self.core.library.lookup(uris=['dummy1:a', 'dummy2:a']) - self.assertEqual(result, {'dummy2:a': [5678], 'dummy1:a': [1234]}) + self.assertEqual(result, {'dummy2:a': [track2], 'dummy1:a': [track1]}) def test_lookup_uris_returns_empty_list_for_dummy3_track(self): result = self.core.library.lookup(uris=['dummy3:a']) @@ -349,12 +355,14 @@ class DeprecatedLookupCoreLibraryTest(BaseCoreLibraryTest): return super(DeprecatedLookupCoreLibraryTest, self).run(result) def test_lookup_selects_dummy1_backend(self): + self.library1.lookup.return_value.get.return_value = [] self.core.library.lookup('dummy1:a') self.library1.lookup.assert_called_once_with('dummy1:a') self.assertFalse(self.library2.lookup.called) def test_lookup_selects_dummy2_backend(self): + self.library2.lookup.return_value.get.return_value = [] self.core.library.lookup('dummy2:a') self.assertFalse(self.library1.lookup.called) @@ -407,8 +415,7 @@ class LegacyFindExactToSearchLibraryTest(unittest.TestCase): # We are just testing that this doesn't fail. -@mock.patch('mopidy.core.library.logger') -class BackendFailuresCoreLibraryTest(unittest.TestCase): +class MockBackendCoreLibraryBase(unittest.TestCase): def setUp(self): # noqa: N802 dummy_root = Ref.directory(uri='dummy:directory', name='dummy') @@ -423,52 +430,182 @@ class BackendFailuresCoreLibraryTest(unittest.TestCase): self.core = core.Core(mixer=None, backends=[self.backend]) - def test_browse_backend_get_root_exception_gets_ignored(self, logger): + +@mock.patch('mopidy.core.library.logger') +class BrowseBadBackendTest(MockBackendCoreLibraryBase): + + def test_backend_raises_exception_for_root(self, logger): # Might happen if root_directory is a property for some weird reason. self.library.root_directory.get.side_effect = Exception self.assertEqual([], self.core.library.browse(None)) logger.exception.assert_called_with(mock.ANY, 'DummyBackend') - def test_browse_backend_browse_uri_exception_gets_ignored(self, logger): + def test_backend_returns_none_for_root(self, logger): + self.library.root_directory.get.return_value = None + self.assertEqual([], self.core.library.browse(None)) + logger.error.assert_called_with(mock.ANY, 'DummyBackend', mock.ANY) + + def test_backend_returns_wrong_type_for_root(self, logger): + self.library.root_directory.get.return_value = 123 + self.assertEqual([], self.core.library.browse(None)) + logger.error.assert_called_with(mock.ANY, 'DummyBackend', mock.ANY) + + def test_backend_raises_exception_for_browse(self, logger): self.library.browse.return_value.get.side_effect = Exception self.assertEqual([], self.core.library.browse('dummy:directory')) logger.exception.assert_called_with(mock.ANY, 'DummyBackend') - def test_get_distinct_backend_exception_gets_ignored(self, logger): + def test_backend_returns_wrong_type_for_browse(self, logger): + self.library.browse.return_value.get.return_value = [123] + self.assertEqual([], self.core.library.browse('dummy:directory')) + logger.error.assert_called_with(mock.ANY, 'DummyBackend', mock.ANY) + + +@mock.patch('mopidy.core.library.logger') +class GetDistinctBadBackendTest(MockBackendCoreLibraryBase): + + def test_backend_raises_exception(self, logger): self.library.get_distinct.return_value.get.side_effect = Exception self.assertEqual(set(), self.core.library.get_distinct('artist')) logger.exception.assert_called_with(mock.ANY, 'DummyBackend') - def test_get_images_backend_exception_get_ignored(self, logger): + def test_backend_returns_none(self, logger): + self.library.get_distinct.return_value.get.return_value = None + self.assertEqual(set(), self.core.library.get_distinct('artist')) + self.assertFalse(logger.error.called) + + def test_backend_returns_wrong_type(self, logger): + self.library.get_distinct.return_value.get.return_value = 'abc' + self.assertEqual(set(), self.core.library.get_distinct('artist')) + logger.error.assert_called_with(mock.ANY, 'DummyBackend', mock.ANY) + + def test_backend_returns_iterable_containing_wrong_types(self, logger): + self.library.get_distinct.return_value.get.return_value = [1, 2, 3] + self.assertEqual(set(), self.core.library.get_distinct('artist')) + logger.error.assert_called_with(mock.ANY, 'DummyBackend', mock.ANY) + + +@mock.patch('mopidy.core.library.logger') +class GetImagesBadBackendTest(MockBackendCoreLibraryBase): + + def test_backend_raises_exception(self, logger): + uri = 'dummy:/1' self.library.get_images.return_value.get.side_effect = Exception - self.assertEqual( - {'dummy:/1': tuple()}, self.core.library.get_images(['dummy:/1'])) + self.assertEqual({uri: tuple()}, self.core.library.get_images([uri])) logger.exception.assert_called_with(mock.ANY, 'DummyBackend') - def test_lookup_backend_exceptiosn_gets_ignores(self, logger): + def test_backend_returns_none(self, logger): + uri = 'dummy:/1' + self.library.get_images.return_value.get.return_value = None + self.assertEqual({uri: tuple()}, self.core.library.get_images([uri])) + self.assertFalse(logger.error.called) + + def test_backend_returns_wrong_type(self, logger): + uri = 'dummy:/1' + self.library.get_images.return_value.get.return_value = 'abc' + self.assertEqual({uri: tuple()}, self.core.library.get_images([uri])) + logger.error.assert_called_with(mock.ANY, 'DummyBackend', mock.ANY) + + def test_backend_returns_mapping_containing_wrong_types(self, logger): + uri = 'dummy:/1' + self.library.get_images.return_value.get.return_value = {uri: 'abc'} + self.assertEqual({uri: tuple()}, self.core.library.get_images([uri])) + logger.error.assert_called_with(mock.ANY, 'DummyBackend', mock.ANY) + + def test_backend_returns_mapping_containing_none(self, logger): + uri = 'dummy:/1' + self.library.get_images.return_value.get.return_value = {uri: None} + self.assertEqual({uri: tuple()}, self.core.library.get_images([uri])) + logger.error.assert_called_with(mock.ANY, 'DummyBackend', mock.ANY) + + def test_backend_returns_unknown_uri(self, logger): + uri = 'dummy:/1' + self.library.get_images.return_value.get.return_value = {'foo': []} + self.assertEqual({uri: tuple()}, self.core.library.get_images([uri])) + logger.error.assert_called_with(mock.ANY, 'DummyBackend', mock.ANY) + + +@mock.patch('mopidy.core.library.logger') +class LookupByUrisBadBackendTest(MockBackendCoreLibraryBase): + + def test_backend_raises_exception(self, logger): + uri = 'dummy:/1' self.library.lookup.return_value.get.side_effect = Exception - self.assertEqual( - {'dummy:/1': []}, self.core.library.lookup(uris=['dummy:/1'])) + self.assertEqual({uri: []}, self.core.library.lookup(uris=[uri])) logger.exception.assert_called_with(mock.ANY, 'DummyBackend') - def test_refresh_backend_exception_gets_ignored(self, logger): + def test_backend_returns_none(self, logger): + uri = 'dummy:/1' + self.library.lookup.return_value.get.return_value = None + self.assertEqual({uri: []}, self.core.library.lookup(uris=[uri])) + self.assertFalse(logger.error.called) + + def test_backend_returns_wrong_type(self, logger): + uri = 'dummy:/1' + self.library.lookup.return_value.get.return_value = 'abc' + self.assertEqual({uri: []}, self.core.library.lookup(uris=[uri])) + logger.error.assert_called_with(mock.ANY, 'DummyBackend', mock.ANY) + + def test_backend_returns_iterable_containing_wrong_types(self, logger): + uri = 'dummy:/1' + self.library.lookup.return_value.get.return_value = [123] + self.assertEqual({uri: []}, self.core.library.lookup(uris=[uri])) + logger.error.assert_called_with(mock.ANY, 'DummyBackend', mock.ANY) + + def test_backend_returns_none_with_uri(self, logger): + uri = 'dummy:/1' + self.library.lookup.return_value.get.return_value = None + self.assertEqual([], self.core.library.lookup(uri)) + self.assertFalse(logger.error.called) + + def test_backend_returns_wrong_type_with_uri(self, logger): + uri = 'dummy:/1' + self.library.lookup.return_value.get.return_value = 'abc' + self.assertEqual([], self.core.library.lookup(uri)) + logger.error.assert_called_with(mock.ANY, 'DummyBackend', mock.ANY) + + def test_backend_returns_iterable_wrong_types_with_uri(self, logger): + uri = 'dummy:/1' + self.library.lookup.return_value.get.return_value = [123] + self.assertEqual([], self.core.library.lookup(uri)) + logger.error.assert_called_with(mock.ANY, 'DummyBackend', mock.ANY) + + +@mock.patch('mopidy.core.library.logger') +class RefreshBadBackendTest(MockBackendCoreLibraryBase): + + def test_backend_raises_exception(self, logger): self.library.refresh.return_value.get.side_effect = Exception self.core.library.refresh() logger.exception.assert_called_with(mock.ANY, 'DummyBackend') - def test_refresh_uri_backend_exception_gets_ignored(self, logger): + def test_backend_raises_exception_with_uri(self, logger): self.library.refresh.return_value.get.side_effect = Exception self.core.library.refresh('dummy:/1') logger.exception.assert_called_with(mock.ANY, 'DummyBackend') - def test_search_backend_exception_gets_ignored(self, logger): + +@mock.patch('mopidy.core.library.logger') +class SearchBadBackendTest(MockBackendCoreLibraryBase): + + def test_backend_raises_exception(self, logger): self.library.search.return_value.get.side_effect = Exception self.assertEqual([], self.core.library.search(query={'any': ['foo']})) logger.exception.assert_called_with(mock.ANY, 'DummyBackend') - def test_search_backend_lookup_error_gets_through(self, logger): + def test_backend_raises_lookuperror(self, logger): # TODO: is this behavior desired? Do we need to continue handling # LookupError case specially. self.library.search.return_value.get.side_effect = LookupError with self.assertRaises(LookupError): self.core.library.search(query={'any': ['foo']}) + + def test_backend_returns_none(self, logger): + self.library.search.return_value.get.return_value = None + self.assertEqual([], self.core.library.search(query={'any': ['foo']})) + self.assertFalse(logger.error.called) + + def test_backend_returns_wrong_type(self, logger): + self.library.search.return_value.get.return_value = 'abc' + self.assertEqual([], self.core.library.search(query={'any': ['foo']})) + logger.error.assert_called_with(mock.ANY, 'DummyBackend', mock.ANY) diff --git a/tests/core/test_mixer.py b/tests/core/test_mixer.py index c4ef7fe9..45241fec 100644 --- a/tests/core/test_mixer.py +++ b/tests/core/test_mixer.py @@ -23,6 +23,7 @@ class CoreMixerTest(unittest.TestCase): self.mixer.get_volume.assert_called_once_with() def test_set_volume(self): + self.mixer.set_volume.return_value.get.return_value = True self.core.mixer.set_volume(30) self.mixer.set_volume.assert_called_once_with(30) @@ -34,6 +35,7 @@ class CoreMixerTest(unittest.TestCase): self.mixer.get_mute.assert_called_once_with() def test_set_mute(self): + self.mixer.set_mute.return_value.get.return_value = True self.core.mixer.set_mute(True) self.mixer.set_mute.assert_called_once_with(True) @@ -92,3 +94,63 @@ class CoreNoneMixerListenerTest(unittest.TestCase): def test_forwards_mixer_mute_changed_event_to_frontends(self, send): self.core.mixer.set_mute(mute=True) self.assertEqual(send.call_count, 0) + + +class MockBackendCoreMixerBase(unittest.TestCase): + + def setUp(self): # noqa: N802 + self.mixer = mock.Mock() + self.mixer.actor_ref.actor_class.__name__ = 'DummyMixer' + self.core = core.Core(mixer=self.mixer, backends=[]) + + +class GetVolumeBadBackendTest(MockBackendCoreMixerBase): + + def test_backend_raises_exception(self): + self.mixer.get_volume.return_value.get.side_effect = Exception + self.assertEqual(self.core.mixer.get_volume(), None) + + def test_backend_returns_too_small_value(self): + self.mixer.get_volume.return_value.get.return_value = -1 + self.assertEqual(self.core.mixer.get_volume(), None) + + def test_backend_returns_too_large_value(self): + self.mixer.get_volume.return_value.get.return_value = 1000 + self.assertEqual(self.core.mixer.get_volume(), None) + + def test_backend_returns_wrong_type(self): + self.mixer.get_volume.return_value.get.return_value = '12' + self.assertEqual(self.core.mixer.get_volume(), None) + + +class SetVolumeBadBackendTest(MockBackendCoreMixerBase): + + def test_backend_raises_exception(self): + self.mixer.set_volume.return_value.get.side_effect = Exception + self.assertFalse(self.core.mixer.set_volume(30)) + + def test_backend_returns_wrong_type(self): + self.mixer.set_volume.return_value.get.return_value = 'done' + self.assertFalse(self.core.mixer.set_volume(30)) + + +class GetMuteBadBackendTest(MockBackendCoreMixerBase): + + def test_backend_raises_exception(self): + self.mixer.get_mute.return_value.get.side_effect = Exception + self.assertEqual(self.core.mixer.get_mute(), None) + + def test_backend_returns_wrong_type(self): + self.mixer.get_mute.return_value.get.return_value = '12' + self.assertEqual(self.core.mixer.get_mute(), None) + + +class SetMuteBadBackendTest(MockBackendCoreMixerBase): + + def test_backend_raises_exception(self): + self.mixer.set_mute.return_value.get.side_effect = Exception + self.assertFalse(self.core.mixer.set_mute(True)) + + def test_backend_returns_wrong_type(self): + self.mixer.set_mute.return_value.get.return_value = 'done' + self.assertFalse(self.core.mixer.set_mute(True)) diff --git a/tests/core/test_playlists.py b/tests/core/test_playlists.py index 063b4057..2dabf93b 100644 --- a/tests/core/test_playlists.py +++ b/tests/core/test_playlists.py @@ -298,8 +298,7 @@ class DeprecatedGetPlaylistsTest(BasePlaylistsTest): self.assertEqual(len(result[1].tracks), 0) -@mock.patch('mopidy.core.playlists.logger') -class BackendFailuresCorePlaylistsTest(unittest.TestCase): +class MockBackendCorePlaylistsBase(unittest.TestCase): def setUp(self): # noqa: N802 self.playlists = mock.Mock(spec=backend.PlaylistsProvider) @@ -311,27 +310,127 @@ class BackendFailuresCorePlaylistsTest(unittest.TestCase): self.core = core.Core(mixer=None, backends=[self.backend]) - def test_as_list_backend_exception_gets_ignored(self, logger): - self.playlists.as_list.get.side_effect = Exception + +@mock.patch('mopidy.core.playlists.logger') +class AsListBadBackendsTest(MockBackendCorePlaylistsBase): + + def test_backend_raises_exception(self, logger): + self.playlists.as_list.return_value.get.side_effect = Exception self.assertEqual([], self.core.playlists.as_list()) logger.exception.assert_called_with(mock.ANY, 'DummyBackend') - def test_get_items_backend_exception_gets_through(self, logger): - # TODO: is this behavior desired? + def test_backend_returns_none(self, logger): + self.playlists.as_list.return_value.get.return_value = None + self.assertEqual([], self.core.playlists.as_list()) + self.assertFalse(logger.error.called) + + def test_backend_returns_wrong_type(self, logger): + self.playlists.as_list.return_value.get.return_value = 'abc' + self.assertEqual([], self.core.playlists.as_list()) + logger.error.assert_called_with(mock.ANY, 'DummyBackend', mock.ANY) + + +@mock.patch('mopidy.core.playlists.logger') +class GetItemsBadBackendsTest(MockBackendCorePlaylistsBase): + + def test_backend_raises_exception(self, logger): self.playlists.get_items.return_value.get.side_effect = Exception - with self.assertRaises(Exception): - self.core.playlists.get_items('dummy:/1') + self.assertIsNone(self.core.playlists.get_items('dummy:/1')) + logger.exception.assert_called_with(mock.ANY, 'DummyBackend') + + def test_backend_returns_none(self, logger): + self.playlists.get_items.return_value.get.return_value = None + self.assertIsNone(self.core.playlists.get_items('dummy:/1')) + self.assertFalse(logger.error.called) + + def test_backend_returns_wrong_type(self, logger): + self.playlists.get_items.return_value.get.return_value = 'abc' + self.assertIsNone(self.core.playlists.get_items('dummy:/1')) + logger.error.assert_called_with(mock.ANY, 'DummyBackend', mock.ANY) + + +@mock.patch('mopidy.core.playlists.logger') +class CreateBadBackendsTest(MockBackendCorePlaylistsBase): + + def test_backend_raises_exception(self, logger): + self.playlists.create.return_value.get.side_effect = Exception + self.assertIsNone(self.core.playlists.create('foobar')) + logger.exception.assert_called_with(mock.ANY, 'DummyBackend') + + def test_backend_returns_none(self, logger): + self.playlists.create.return_value.get.return_value = None + self.assertIsNone(self.core.playlists.create('foobar')) + self.assertFalse(logger.error.called) + + def test_backend_returns_wrong_type(self, logger): + self.playlists.create.return_value.get.return_value = 'abc' + self.assertIsNone(self.core.playlists.create('foobar')) + logger.error.assert_called_with(mock.ANY, 'DummyBackend', mock.ANY) + + +@mock.patch('mopidy.core.playlists.logger') +class DeleteBadBackendsTest(MockBackendCorePlaylistsBase): + + def test_backend_raises_exception(self, logger): + self.playlists.delete.return_value.get.side_effect = Exception + self.assertIsNone(self.core.playlists.delete('dummy:/1')) + logger.exception.assert_called_with(mock.ANY, 'DummyBackend') + + +@mock.patch('mopidy.core.playlists.logger') +class LookupBadBackendsTest(MockBackendCorePlaylistsBase): + + def test_backend_raises_exception(self, logger): + self.playlists.lookup.return_value.get.side_effect = Exception + self.assertIsNone(self.core.playlists.lookup('dummy:/1')) + logger.exception.assert_called_with(mock.ANY, 'DummyBackend') + + def test_backend_returns_none(self, logger): + self.playlists.lookup.return_value.get.return_value = None + self.assertIsNone(self.core.playlists.lookup('dummy:/1')) + self.assertFalse(logger.error.called) + + def test_backend_returns_wrong_type(self, logger): + self.playlists.lookup.return_value.get.return_value = 'abc' + self.assertIsNone(self.core.playlists.lookup('dummy:/1')) + logger.error.assert_called_with(mock.ANY, 'DummyBackend', mock.ANY) + + +@mock.patch('mopidy.core.playlists.logger') +class RefreshBadBackendsTest(MockBackendCorePlaylistsBase): @mock.patch('mopidy.core.listener.CoreListener.send') - def test_refresh_backend_exception_gets_ignored(self, send, logger): + def test_backend_raises_exception(self, send, logger): self.playlists.refresh.return_value.get.side_effect = Exception self.core.playlists.refresh() self.assertFalse(send.called) logger.exception.assert_called_with(mock.ANY, 'DummyBackend') @mock.patch('mopidy.core.listener.CoreListener.send') - def test_refresh_uri_backend_exception_gets_ignored(self, send, logger): + def test_backend_raises_exception_called_with_uri(self, send, logger): self.playlists.refresh.return_value.get.side_effect = Exception self.core.playlists.refresh('dummy') self.assertFalse(send.called) logger.exception.assert_called_with(mock.ANY, 'DummyBackend') + + +@mock.patch('mopidy.core.playlists.logger') +class SaveBadBackendsTest(MockBackendCorePlaylistsBase): + + def test_backend_raises_exception(self, logger): + playlist = Playlist(uri='dummy:/1') + self.playlists.save.return_value.get.side_effect = Exception + self.assertIsNone(self.core.playlists.save(playlist)) + logger.exception.assert_called_with(mock.ANY, 'DummyBackend') + + def test_backend_returns_none(self, logger): + playlist = Playlist(uri='dummy:/1') + self.playlists.save.return_value.get.return_value = None + self.assertIsNone(self.core.playlists.save(playlist)) + self.assertFalse(logger.error.called) + + def test_backend_returns_wrong_type(self, logger): + playlist = Playlist(uri='dummy:/1') + self.playlists.save.return_value.get.return_value = 'abc' + self.assertIsNone(self.core.playlists.save(playlist)) + logger.error.assert_called_with(mock.ANY, 'DummyBackend', mock.ANY)