From b2d1e1b4f7ac14bfb1069739b4d2b028f5760254 Mon Sep 17 00:00:00 2001 From: Thomas Kemmer Date: Fri, 1 Jan 2016 20:27:00 +0100 Subject: [PATCH 01/15] m3u: Major refactoring, add default_encoding and default_extension settings. --- mopidy/m3u/__init__.py | 5 +- mopidy/m3u/actor.py | 36 ----- mopidy/m3u/backend.py | 15 ++ mopidy/m3u/ext.conf | 5 + mopidy/m3u/library.py | 19 --- mopidy/m3u/playlists.py | 178 +++++++++++------------ mopidy/m3u/translator.py | 207 +++++++++++++-------------- tests/m3u/test_playlists.py | 49 +++---- tests/m3u/test_translator.py | 264 ++++++++++++++++++----------------- 9 files changed, 369 insertions(+), 409 deletions(-) delete mode 100644 mopidy/m3u/actor.py create mode 100644 mopidy/m3u/backend.py delete mode 100644 mopidy/m3u/library.py diff --git a/mopidy/m3u/__init__.py b/mopidy/m3u/__init__.py index 06825932..df769c88 100644 --- a/mopidy/m3u/__init__.py +++ b/mopidy/m3u/__init__.py @@ -21,10 +21,11 @@ class Extension(ext.Extension): def get_config_schema(self): schema = super(Extension, self).get_config_schema() + schema['default_encoding'] = config.String() + schema['default_extension'] = config.String(choices=['.m3u', '.m3u8']) schema['playlists_dir'] = config.Path(optional=True) return schema def setup(self, registry): - from .actor import M3UBackend - + from .backend import M3UBackend registry.add('backend', M3UBackend) diff --git a/mopidy/m3u/actor.py b/mopidy/m3u/actor.py deleted file mode 100644 index 55257f87..00000000 --- a/mopidy/m3u/actor.py +++ /dev/null @@ -1,36 +0,0 @@ -from __future__ import absolute_import, unicode_literals - -import logging - -import pykka - -from mopidy import backend, m3u -from mopidy.internal import encoding, path -from mopidy.m3u.library import M3ULibraryProvider -from mopidy.m3u.playlists import M3UPlaylistsProvider - - -logger = logging.getLogger(__name__) - - -class M3UBackend(pykka.ThreadingActor, backend.Backend): - uri_schemes = ['m3u'] - - def __init__(self, config, audio): - super(M3UBackend, self).__init__() - - self._config = config - - if config['m3u']['playlists_dir'] is not None: - self._playlists_dir = config['m3u']['playlists_dir'] - try: - path.get_or_create_dir(self._playlists_dir) - except EnvironmentError as error: - logger.warning( - 'Could not create M3U playlists dir: %s', - encoding.locale_decode(error)) - else: - self._playlists_dir = m3u.Extension.get_data_dir(config) - - self.playlists = M3UPlaylistsProvider(backend=self) - self.library = M3ULibraryProvider(backend=self) diff --git a/mopidy/m3u/backend.py b/mopidy/m3u/backend.py new file mode 100644 index 00000000..02719cc7 --- /dev/null +++ b/mopidy/m3u/backend.py @@ -0,0 +1,15 @@ +from __future__ import absolute_import, unicode_literals + +import pykka + +from mopidy import backend + +from . import playlists + + +class M3UBackend(pykka.ThreadingActor, backend.Backend): + uri_schemes = ['m3u'] + + def __init__(self, config, audio): + super(M3UBackend, self).__init__() + self.playlists = playlists.M3UPlaylistsProvider(self, config) diff --git a/mopidy/m3u/ext.conf b/mopidy/m3u/ext.conf index adc0d00a..e4e68ff8 100644 --- a/mopidy/m3u/ext.conf +++ b/mopidy/m3u/ext.conf @@ -1,3 +1,8 @@ [m3u] enabled = true + +default_encoding = latin-1 + +default_extension = .m3u + playlists_dir = diff --git a/mopidy/m3u/library.py b/mopidy/m3u/library.py deleted file mode 100644 index 291a6194..00000000 --- a/mopidy/m3u/library.py +++ /dev/null @@ -1,19 +0,0 @@ -from __future__ import absolute_import, unicode_literals - -import logging - -from mopidy import backend - -logger = logging.getLogger(__name__) - - -class M3ULibraryProvider(backend.LibraryProvider): - - """Library for looking up M3U playlists.""" - - def __init__(self, backend): - super(M3ULibraryProvider, self).__init__(backend) - - def lookup(self, uri): - # TODO Lookup tracks in M3U playlist - return [] diff --git a/mopidy/m3u/playlists.py b/mopidy/m3u/playlists.py index 3567f8aa..e2b35d1d 100644 --- a/mopidy/m3u/playlists.py +++ b/mopidy/m3u/playlists.py @@ -1,117 +1,117 @@ -from __future__ import absolute_import, division, unicode_literals +from __future__ import absolute_import, unicode_literals -import glob +import io +import locale import logging import operator import os -import re -import sys from mopidy import backend -from mopidy.m3u import translator -from mopidy.models import Playlist, Ref +from . import Extension, translator logger = logging.getLogger(__name__) +def log_environment_error(message, error): + if isinstance(error.strerror, bytes): + strerror = error.strerror.decode(locale.getpreferredencoding()) + else: + strerror = error.strerror + logger.error('%s: %s', message, strerror) + + class M3UPlaylistsProvider(backend.PlaylistsProvider): - # TODO: currently this only handles UNIX file systems - _invalid_filename_chars = re.compile(r'[/]') + def __init__(self, backend, config): + super(M3UPlaylistsProvider, self).__init__(backend) - def __init__(self, *args, **kwargs): - super(M3UPlaylistsProvider, self).__init__(*args, **kwargs) - - self._playlists_dir = self.backend._playlists_dir - self._playlists = {} - self.refresh() + ext_config = config[Extension.ext_name] + if ext_config['playlists_dir'] is None: + self._playlists_dir = Extension.get_data_dir(config) + else: + self._playlists_dir = ext_config['playlists_dir'] + self._default_encoding = ext_config['default_encoding'] + self._default_extension = ext_config['default_extension'] def as_list(self): - refs = [ - Ref.playlist(uri=pl.uri, name=pl.name) - for pl in self._playlists.values()] - return sorted(refs, key=operator.attrgetter('name')) - - def get_items(self, uri): - playlist = self._playlists.get(uri) - if playlist is None: - return None - return [Ref.track(uri=t.uri, name=t.name) for t in playlist.tracks] + result = [] + for entry in os.listdir(self._playlists_dir): + if not entry.endswith((b'.m3u', b'.m3u8')): + continue + elif not os.path.isfile(self._abspath(entry)): + continue + else: + result.append(translator.path_to_ref(entry)) + result.sort(key=operator.attrgetter('name')) + return result def create(self, name): - playlist = self._save_m3u(Playlist(name=name)) - self._playlists[playlist.uri] = playlist - logger.info('Created playlist %s', playlist.uri) - return playlist + path = translator.path_from_name(name.strip(), self._default_extension) + try: + with self._open(path, 'w'): + pass + mtime = os.path.getmtime(self._abspath(path)) + except EnvironmentError as e: + log_environment_error('Error creating playlist %s' % name, e) + else: + return translator.playlist(path, [], mtime) def delete(self, uri): - if uri in self._playlists: - path = translator.playlist_uri_to_path(uri, self._playlists_dir) - if os.path.exists(path): - os.remove(path) - else: - logger.warning( - 'Trying to delete missing playlist file %s', path) - del self._playlists[uri] - logger.info('Deleted playlist %s', uri) + path = translator.uri_to_path(uri) + try: + os.remove(self._abspath(path)) + except EnvironmentError as e: + log_environment_error('Error deleting playlist %s' % uri, e) + + def get_items(self, uri): + path = translator.uri_to_path(uri) + try: + with self._open(path, 'r') as fp: + items = translator.load_items(fp, self._playlists_dir) + except EnvironmentError as e: + log_environment_error('Error reading playlist %s' % uri, e) else: - logger.warning('Trying to delete unknown playlist %s', uri) + return items def lookup(self, uri): - return self._playlists.get(uri) + path = translator.uri_to_path(uri) + try: + with self._open(path, 'r') as fp: + items = translator.load_items(fp, self._playlists_dir) + mtime = os.path.getmtime(self._abspath(path)) + except EnvironmentError as e: + log_environment_error('Error reading playlist %s' % uri, e) + else: + return translator.playlist(path, items, mtime) def refresh(self): - playlists = {} - - encoding = sys.getfilesystemencoding() - for path in glob.glob(os.path.join(self._playlists_dir, b'*.m3u*')): - relpath = os.path.basename(path) - uri = translator.path_to_playlist_uri(relpath) - name = os.path.splitext(relpath)[0].decode(encoding, 'replace') - tracks = translator.parse_m3u(path) - playlists[uri] = Playlist(uri=uri, name=name, tracks=tracks) - - self._playlists = playlists - - logger.info( - 'Loaded %d M3U playlists from %s', - len(playlists), self._playlists_dir) - - # TODO Trigger playlists_loaded event? + pass # nothing to do def save(self, playlist): - assert playlist.uri, 'Cannot save playlist without URI' - assert playlist.uri in self._playlists, \ - 'Cannot save playlist with unknown URI: %s' % playlist.uri - - original_uri = playlist.uri - playlist = self._save_m3u(playlist) - if playlist.uri != original_uri and original_uri in self._playlists: - self.delete(original_uri) - self._playlists[playlist.uri] = playlist - return playlist - - def _sanitize_m3u_name(self, name, encoding=sys.getfilesystemencoding()): - name = self._invalid_filename_chars.sub('|', name.strip()) - # make sure we end up with a valid path segment - name = name.encode(encoding, errors='replace') - name = os.path.basename(name) # paranoia? - name = name.decode(encoding) - return name - - def _save_m3u(self, playlist, encoding=sys.getfilesystemencoding()): - if playlist.name: - name = self._sanitize_m3u_name(playlist.name, encoding) - uri = translator.path_to_playlist_uri( - name.encode(encoding) + b'.m3u') - path = translator.playlist_uri_to_path(uri, self._playlists_dir) - elif playlist.uri: - uri = playlist.uri - path = translator.playlist_uri_to_path(uri, self._playlists_dir) - name, _ = os.path.splitext(os.path.basename(path).decode(encoding)) + path = translator.uri_to_path(playlist.uri) + name = translator.name_from_path(path) + try: + with self._open(path, 'w') as fp: + translator.dump_items(playlist.tracks, fp) + if playlist.name and playlist.name != name: + opath, ext = os.path.splitext(path) + path = translator.path_from_name(playlist.name.strip()) + ext + os.rename(self._abspath(opath + ext), self._abspath(path)) + mtime = os.path.getmtime(self._abspath(path)) + except EnvironmentError as e: + log_environment_error('Error saving playlist %s' % playlist.uri, e) else: - raise ValueError('M3U playlist needs name or URI') - translator.save_m3u(path, playlist.tracks, 'latin1') - # assert playlist name matches file name/uri - return playlist.replace(uri=uri, name=name) + return translator.playlist(path, playlist.tracks, mtime) + + def _abspath(self, path): + return os.path.join(self._playlists_dir, path) + + def _open(self, path, mode='r'): + if path.endswith(b'.m3u8'): + encoding = 'utf-8' + else: + encoding = self._default_encoding + if not os.path.isabs(path): + path = os.path.join(self._playlists_dir, path) + return io.open(path, mode, encoding=encoding, errors='replace') diff --git a/mopidy/m3u/translator.py b/mopidy/m3u/translator.py index 764cf84b..da74cc1b 100644 --- a/mopidy/m3u/translator.py +++ b/mopidy/m3u/translator.py @@ -1,130 +1,119 @@ -from __future__ import absolute_import, unicode_literals +from __future__ import absolute_import, print_function, unicode_literals -import codecs -import logging import os -import re -from mopidy import compat -from mopidy.compat import urllib -from mopidy.internal import encoding, path -from mopidy.models import Track +from mopidy import models + +from . import Extension + +try: + from urllib.parse import quote_from_bytes, unquote_to_bytes +except ImportError: + import urllib + + def quote_from_bytes(bytes, safe=b'/'): + # Python 3 returns Unicode string + return urllib.quote(bytes, safe).decode('utf-8') + + def unquote_to_bytes(string): + if isinstance(string, bytes): + return urllib.unquote(string) + else: + return urllib.unquote(string.encode('utf-8')) + +try: + from urllib.parse import urlsplit, urlunsplit +except ImportError: + from urlparse import urlsplit, urlunsplit -M3U_EXTINF_RE = re.compile(r'#EXTINF:(-1|\d+),(.*)') +try: + from os import fsencode, fsdecode +except ImportError: + import sys -logger = logging.getLogger(__name__) + # no 'surrogateescape' in Python 2; 'replace' for backward compatibility + def fsencode(filename, encoding=sys.getfilesystemencoding()): + return filename.encode(encoding, 'replace') + + def fsdecode(filename, encoding=sys.getfilesystemencoding()): + return filename.decode(encoding, 'replace') -def playlist_uri_to_path(uri, playlists_dir): - if not uri.startswith('m3u:'): - raise ValueError('Invalid URI %s' % uri) - file_path = path.uri_to_path(uri) - return os.path.join(playlists_dir, file_path) +def path_to_uri(path, scheme=Extension.ext_name): + """Convert file path to URI.""" + assert isinstance(path, bytes), 'Mopidy paths should be bytes' + uripath = quote_from_bytes(os.path.normpath(path)) + return urlunsplit((scheme, None, uripath, None, None)) -def path_to_playlist_uri(relpath): - """Convert path relative to playlists_dir to M3U URI.""" - if isinstance(relpath, compat.text_type): - relpath = relpath.encode('utf-8') - return b'm3u:%s' % urllib.parse.quote(relpath) +def uri_to_path(uri): + """Convert URI to file path.""" + # TODO: decide on Unicode vs. bytes for URIs + return unquote_to_bytes(urlsplit(uri).path) -def m3u_extinf_to_track(line): - """Convert extended M3U directive to track template.""" - m = M3U_EXTINF_RE.match(line) - if not m: - logger.warning('Invalid extended M3U directive: %s', line) - return Track() - (runtime, title) = m.groups() - if int(runtime) > 0: - return Track(name=title, length=1000 * int(runtime)) - else: - return Track(name=title) - - -def parse_m3u(file_path, media_dir=None): - r""" - Convert M3U file list to list of tracks - - Example M3U data:: - - # This is a comment - Alternative\Band - Song.mp3 - Classical\Other Band - New Song.mp3 - Stuff.mp3 - D:\More Music\Foo.mp3 - http://www.example.com:8000/Listen.pls - http://www.example.com/~user/Mine.mp3 - - Example extended M3U data:: - - #EXTM3U - #EXTINF:123, Sample artist - Sample title - Sample.mp3 - #EXTINF:321,Example Artist - Example title - Greatest Hits\Example.ogg - #EXTINF:-1,Radio XMP - http://mp3stream.example.com:8000/ - - - Relative paths of songs should be with respect to location of M3U. - - Paths are normally platform specific. - - Lines starting with # are ignored, except for extended M3U directives. - - Track.name and Track.length are set from extended M3U directives. - - m3u files are latin-1. - - m3u8 files are utf-8 - """ - # TODO: uris as bytes - file_encoding = 'utf-8' if file_path.endswith(b'.m3u8') else 'latin1' - - tracks = [] +def name_from_path(path): + """Extract name from file path.""" + name, _ = os.path.splitext(os.path.basename(path)) try: - with codecs.open(file_path, 'rb', file_encoding, 'replace') as m3u: - contents = m3u.readlines() - except IOError as error: - logger.warning('Couldn\'t open m3u: %s', encoding.locale_decode(error)) - return tracks + return fsdecode(name) + except UnicodeError: + return None - if not contents: - return tracks - # Strip newlines left by codecs - contents = [line.strip() for line in contents] +def path_from_name(name, ext=None, sep='|'): + """Convert name with optional extension to file path.""" + if ext: + return fsencode(name.replace(os.sep, sep) + ext) + else: + return fsencode(name.replace(os.sep, sep)) - extended = contents[0].startswith('#EXTM3U') - track = Track() - for line in contents: +def path_to_ref(path): + return models.Ref.playlist( + uri=path_to_uri(path), + name=name_from_path(path) + ) + + +def load_items(fp, basedir): + refs = [] + name = None + for line in filter(None, (line.strip() for line in fp)): if line.startswith('#'): - if extended and line.startswith('#EXTINF'): - track = m3u_extinf_to_track(line) + if line.startswith('#EXTINF:'): + name = line.partition(',')[2] continue - if not track.name: - name = os.path.basename(os.path.splitext(line)[0]) - track = track.replace(name=urllib.parse.unquote(name)) - if urllib.parse.urlsplit(line).scheme: - tracks.append(track.replace(uri=line)) - elif os.path.normpath(line) == os.path.abspath(line): - uri = path.path_to_uri(line) - tracks.append(track.replace(uri=uri)) - elif media_dir is not None: - uri = path.path_to_uri(os.path.join(media_dir, line)) - tracks.append(track.replace(uri=uri)) - - track = Track() - return tracks + elif not urlsplit(line).scheme: + path = os.path.join(basedir, fsencode(line)) + if not name: + name = name_from_path(path) + uri = path_to_uri(path, scheme='file') + else: + uri = line # do *not* extract name from (stream?) URI path + refs.append(models.Ref.track(uri=uri, name=name)) + name = None + return refs -def save_m3u(filename, tracks, encoding='latin1', errors='replace'): - extended = any(track.name for track in tracks) - # codecs.open() always uses binary mode, just being explicit here - with codecs.open(filename, 'wb', encoding, errors) as m3u: - if extended: - m3u.write('#EXTM3U' + os.linesep) - for track in tracks: - if extended and track.name: - m3u.write('#EXTINF:%d,%s%s' % ( - track.length // 1000 if track.length else -1, - track.name, - os.linesep)) - m3u.write(track.uri + os.linesep) +def dump_items(items, fp): + if any(item.name for item in items): + print('#EXTM3U', file=fp) + for item in items: + if item.name: + print('#EXTINF:-1,%s' % item.name, file=fp) + # TODO: convert file URIs to (relative) paths? + if isinstance(item.uri, bytes): + print(item.uri.decode('utf-8'), file=fp) + else: + print(item.uri, file=fp) + + +def playlist(path, items=[], mtime=None): + return models.Playlist( + uri=path_to_uri(path), + name=name_from_path(path), + tracks=[models.Track(uri=item.uri, name=item.name) for item in items], + last_modified=(int(mtime * 1000) if mtime else None) + ) diff --git a/tests/m3u/test_playlists.py b/tests/m3u/test_playlists.py index edebe65b..664da9e9 100644 --- a/tests/m3u/test_playlists.py +++ b/tests/m3u/test_playlists.py @@ -7,14 +7,12 @@ import platform import shutil import tempfile import unittest -import urllib import pykka from mopidy import core from mopidy.internal import deprecation -from mopidy.m3u import actor -from mopidy.m3u.translator import playlist_uri_to_path +from mopidy.m3u.backend import M3UBackend from mopidy.models import Playlist, Track from tests import dummy_audio, path_to_data_dir @@ -22,9 +20,12 @@ from tests.m3u import generate_song class M3UPlaylistsProviderTest(unittest.TestCase): - backend_class = actor.M3UBackend + backend_class = M3UBackend config = { 'm3u': { + 'enabled': True, + 'default_encoding': 'latin-1', + 'default_extension': '.m3u', 'playlists_dir': path_to_data_dir(''), } } @@ -34,7 +35,7 @@ class M3UPlaylistsProviderTest(unittest.TestCase): self.playlists_dir = self.config['m3u']['playlists_dir'] audio = dummy_audio.create_proxy() - backend = actor.M3UBackend.start( + backend = M3UBackend.start( config=self.config, audio=audio).proxy() self.core = core.Core(backends=[backend]) @@ -46,7 +47,7 @@ class M3UPlaylistsProviderTest(unittest.TestCase): def test_created_playlist_is_persisted(self): uri = 'm3u:test.m3u' - path = playlist_uri_to_path(uri, self.playlists_dir) + path = os.path.join(self.playlists_dir, b'test.m3u') self.assertFalse(os.path.exists(path)) playlist = self.core.playlists.create('test') @@ -57,7 +58,7 @@ class M3UPlaylistsProviderTest(unittest.TestCase): def test_create_sanitizes_playlist_name(self): playlist = self.core.playlists.create(' ../../test FOO baR ') self.assertEqual('..|..|test FOO baR', playlist.name) - path = playlist_uri_to_path(playlist.uri, self.playlists_dir) + path = os.path.join(self.playlists_dir, b'..|..|test FOO baR.m3u') self.assertEqual(self.playlists_dir, os.path.dirname(path)) self.assertTrue(os.path.exists(path)) @@ -65,8 +66,8 @@ class M3UPlaylistsProviderTest(unittest.TestCase): uri1 = 'm3u:test1.m3u' uri2 = 'm3u:test2.m3u' - path1 = playlist_uri_to_path(uri1, self.playlists_dir) - path2 = playlist_uri_to_path(uri2, self.playlists_dir) + path1 = os.path.join(self.playlists_dir, b'test1.m3u') + path2 = os.path.join(self.playlists_dir, b'test2.m3u') playlist = self.core.playlists.create('test1') self.assertEqual('test1', playlist.name) @@ -82,7 +83,7 @@ class M3UPlaylistsProviderTest(unittest.TestCase): def test_deleted_playlist_is_removed(self): uri = 'm3u:test.m3u' - path = playlist_uri_to_path(uri, self.playlists_dir) + path = os.path.join(self.playlists_dir, b'test.m3u') self.assertFalse(os.path.exists(path)) @@ -98,7 +99,7 @@ class M3UPlaylistsProviderTest(unittest.TestCase): track = Track(uri=generate_song(1)) playlist = self.core.playlists.create('test') playlist = self.core.playlists.save(playlist.replace(tracks=[track])) - path = playlist_uri_to_path(playlist.uri, self.playlists_dir) + path = os.path.join(self.playlists_dir, b'test.m3u') with open(path) as f: contents = f.read() @@ -109,32 +110,32 @@ class M3UPlaylistsProviderTest(unittest.TestCase): track = Track(uri=generate_song(1), name='Test', length=60000) playlist = self.core.playlists.create('test') playlist = self.core.playlists.save(playlist.replace(tracks=[track])) - path = playlist_uri_to_path(playlist.uri, self.playlists_dir) + path = os.path.join(self.playlists_dir, b'test.m3u') with open(path) as f: m3u = f.read().splitlines() - self.assertEqual(['#EXTM3U', '#EXTINF:60,Test', track.uri], m3u) + self.assertEqual(['#EXTM3U', '#EXTINF:-1,Test', track.uri], m3u) def test_latin1_playlist_contents_is_written_to_disk(self): track = Track(uri=generate_song(1), name='Test\x9f', length=60000) playlist = self.core.playlists.create('test') playlist = self.core.playlists.save(playlist.copy(tracks=[track])) - path = playlist_uri_to_path(playlist.uri, self.playlists_dir) + path = os.path.join(self.playlists_dir, b'test.m3u') with open(path, 'rb') as f: m3u = f.read().splitlines() - self.assertEqual([b'#EXTM3U', b'#EXTINF:60,Test\x9f', track.uri], m3u) + self.assertEqual([b'#EXTM3U', b'#EXTINF:-1,Test\x9f', track.uri], m3u) def test_utf8_playlist_contents_is_replaced_and_written_to_disk(self): track = Track(uri=generate_song(1), name='Test\u07b4', length=60000) playlist = self.core.playlists.create('test') playlist = self.core.playlists.save(playlist.copy(tracks=[track])) - path = playlist_uri_to_path(playlist.uri, self.playlists_dir) + path = os.path.join(self.playlists_dir, b'test.m3u') with open(path, 'rb') as f: m3u = f.read().splitlines() - self.assertEqual([b'#EXTM3U', b'#EXTINF:60,Test?', track.uri], m3u) + self.assertEqual([b'#EXTM3U', b'#EXTINF:-1,Test?', track.uri], m3u) def test_playlists_are_loaded_at_startup(self): track = Track(uri='dummy:track:path2') @@ -149,8 +150,7 @@ class M3UPlaylistsProviderTest(unittest.TestCase): self.assertEqual(track.uri, result.tracks[0].uri) def test_load_playlist_with_nonfilesystem_encoding_of_filename(self): - uri = 'm3u:%s.m3u' % urllib.quote('øæå'.encode('latin-1')) - path = playlist_uri_to_path(uri, self.playlists_dir) + path = os.path.join(self.playlists_dir, 'øæå.m3u'.encode('latin-1')) with open(path, 'wb+') as f: f.write(b'#EXTM3U\n') @@ -198,7 +198,7 @@ class M3UPlaylistsProviderTest(unittest.TestCase): playlist = self.core.playlists.create('test') self.assertEqual(playlist, self.core.playlists.lookup(playlist.uri)) - path = playlist_uri_to_path(playlist.uri, self.playlists_dir) + path = os.path.join(self.playlists_dir, b'test.m3u') self.assertTrue(os.path.exists(path)) os.remove(path) @@ -245,12 +245,9 @@ class M3UPlaylistsProviderTest(unittest.TestCase): def test_save_playlist_with_new_uri(self): uri = 'm3u:test.m3u' - - with self.assertRaises(AssertionError): - self.core.playlists.save(Playlist(uri=uri)) - - path = playlist_uri_to_path(uri, self.playlists_dir) - self.assertFalse(os.path.exists(path)) + self.core.playlists.save(Playlist(uri=uri)) + path = os.path.join(self.playlists_dir, b'test.m3u') + self.assertTrue(os.path.exists(path)) def test_playlist_with_unknown_track(self): track = Track(uri='file:///dev/null') diff --git a/tests/m3u/test_translator.py b/tests/m3u/test_translator.py index 88387cb3..35efed4c 100644 --- a/tests/m3u/test_translator.py +++ b/tests/m3u/test_translator.py @@ -2,137 +2,145 @@ from __future__ import absolute_import, unicode_literals -import os -import tempfile -import unittest +import io -from mopidy.internal import path from mopidy.m3u import translator -from mopidy.models import Track - -from tests import path_to_data_dir - -data_dir = path_to_data_dir('') -song1_path = path_to_data_dir('song1.mp3') -song2_path = path_to_data_dir('song2.mp3') -song3_path = path_to_data_dir('φοο.mp3') -encoded_path = path_to_data_dir('æøå.mp3') -song1_uri = path.path_to_uri(song1_path) -song2_uri = path.path_to_uri(song2_path) -song3_uri = path.path_to_uri(song3_path) -song4_uri = 'http://example.com/foo%20bar.mp3' -encoded_uri = path.path_to_uri(encoded_path) -song1_track = Track(name='song1', uri=song1_uri) -song2_track = Track(name='song2', uri=song2_uri) -song3_track = Track(name='φοο', uri=song3_uri) -song4_track = Track(name='foo bar', uri=song4_uri) -encoded_track = Track(name='æøå', uri=encoded_uri) -song1_ext_track = song1_track.replace(name='Song #1') -song2_ext_track = song2_track.replace(name='Song #2', length=60000) -encoded_ext_track = encoded_track.replace(name='æøå') +from mopidy.models import Playlist, Ref, Track -# FIXME use mock instead of tempfile.NamedTemporaryFile - -class M3UToUriTest(unittest.TestCase): - - def parse(self, name): - return translator.parse_m3u(name, data_dir) - - def test_empty_file(self): - tracks = self.parse(path_to_data_dir('empty.m3u')) - self.assertEqual([], tracks) - - def test_basic_file(self): - tracks = self.parse(path_to_data_dir('one.m3u')) - self.assertEqual([song1_track], tracks) - - def test_file_with_comment(self): - tracks = self.parse(path_to_data_dir('comment.m3u')) - self.assertEqual([song1_track], tracks) - - def test_file_is_relative_to_correct_dir(self): - with tempfile.NamedTemporaryFile(delete=False) as tmp: - tmp.write('song1.mp3') - try: - tracks = self.parse(tmp.name) - self.assertEqual([song1_track], tracks) - finally: - if os.path.exists(tmp.name): - os.remove(tmp.name) - - def test_file_with_absolute_files(self): - with tempfile.NamedTemporaryFile(delete=False) as tmp: - tmp.write(song1_path) - try: - tracks = self.parse(tmp.name) - self.assertEqual([song1_track], tracks) - finally: - if os.path.exists(tmp.name): - os.remove(tmp.name) - - def test_file_with_multiple_absolute_files(self): - with tempfile.NamedTemporaryFile(delete=False) as tmp: - tmp.write(song1_path + '\n') - tmp.write('# comment \n') - tmp.write(song2_path) - try: - tracks = self.parse(tmp.name) - self.assertEqual([song1_track, song2_track], tracks) - finally: - if os.path.exists(tmp.name): - os.remove(tmp.name) - - def test_file_with_uri(self): - with tempfile.NamedTemporaryFile(delete=False) as tmp: - tmp.write(song1_uri) - tmp.write('\n') - tmp.write(song4_uri) - try: - tracks = self.parse(tmp.name) - self.assertEqual([song1_track, song4_track], tracks) - finally: - if os.path.exists(tmp.name): - os.remove(tmp.name) - - def test_encoding_is_latin1(self): - tracks = self.parse(path_to_data_dir('encoding.m3u')) - self.assertEqual([encoded_track], tracks) - - def test_open_missing_file(self): - tracks = self.parse(path_to_data_dir('non-existant.m3u')) - self.assertEqual([], tracks) - - def test_empty_ext_file(self): - tracks = self.parse(path_to_data_dir('empty-ext.m3u')) - self.assertEqual([], tracks) - - def test_basic_ext_file(self): - tracks = self.parse(path_to_data_dir('one-ext.m3u')) - self.assertEqual([song1_ext_track], tracks) - - def test_multi_ext_file(self): - tracks = self.parse(path_to_data_dir('two-ext.m3u')) - self.assertEqual([song1_ext_track, song2_ext_track], tracks) - - def test_ext_file_with_comment(self): - tracks = self.parse(path_to_data_dir('comment-ext.m3u')) - self.assertEqual([song1_ext_track], tracks) - - def test_ext_encoding_is_latin1(self): - tracks = self.parse(path_to_data_dir('encoding-ext.m3u')) - self.assertEqual([encoded_ext_track], tracks) - - def test_m3u8_file(self): - with tempfile.NamedTemporaryFile(suffix='.m3u8', delete=False) as tmp: - tmp.write(song3_path) - try: - tracks = self.parse(tmp.name) - self.assertEqual([song3_track], tracks) - finally: - if os.path.exists(tmp.name): - os.remove(tmp.name) +def loads(s, basedir=b'.'): + return translator.load_items(io.StringIO(s), basedir=basedir) -class URItoM3UTest(unittest.TestCase): - pass +def dumps(items): + fp = io.StringIO() + translator.dump_items(items, fp) + return fp.getvalue() + + +def test_path_to_uri(): + from mopidy.m3u.translator import path_to_uri + + assert path_to_uri(b'test') == 'm3u:test' + assert path_to_uri(b'test.m3u') == 'm3u:test.m3u' + assert path_to_uri(b'./test.m3u') == 'm3u:test.m3u' + assert path_to_uri(b'foo/../test.m3u') == 'm3u:test.m3u' + assert path_to_uri(b'Test Playlist.m3u') == 'm3u:Test%20Playlist.m3u' + assert path_to_uri(b'test.mp3', scheme='file') == 'file:///test.mp3' + + +def test_latin1_path_to_uri(): + path = 'æøå.m3u'.encode('latin-1') + assert translator.path_to_uri(path) == 'm3u:%E6%F8%E5.m3u' + + +def test_utf8_path_to_uri(): + path = 'æøå.m3u'.encode('utf-8') + assert translator.path_to_uri(path) == 'm3u:%C3%A6%C3%B8%C3%A5.m3u' + + +def test_uri_to_path(): + from mopidy.m3u.translator import uri_to_path + + assert uri_to_path('m3u:test.m3u') == b'test.m3u' + assert uri_to_path(b'm3u:test.m3u') == b'test.m3u' + assert uri_to_path('m3u:Test%20Playlist.m3u') == b'Test Playlist.m3u' + assert uri_to_path(b'm3u:Test%20Playlist.m3u') == b'Test Playlist.m3u' + assert uri_to_path('m3u:%E6%F8%E5.m3u') == b'\xe6\xf8\xe5.m3u' + assert uri_to_path(b'm3u:%E6%F8%E5.m3u') == b'\xe6\xf8\xe5.m3u' + assert uri_to_path('file:///test.mp3') == b'/test.mp3' + assert uri_to_path(b'file:///test.mp3') == b'/test.mp3' + + +def test_name_from_path(): + from mopidy.m3u.translator import name_from_path + + assert name_from_path(b'test') == 'test' + assert name_from_path(b'test.m3u') == 'test' + assert name_from_path(b'../test.m3u') == 'test' + + +def test_path_from_name(): + from mopidy.m3u.translator import path_from_name + + assert path_from_name('test') == b'test' + assert path_from_name('test', '.m3u') == b'test.m3u' + assert path_from_name('foo/bar', sep='-') == b'foo-bar' + + +def test_path_to_ref(): + from mopidy.m3u.translator import path_to_ref + + assert path_to_ref(b'test.m3u') == Ref.playlist( + uri='m3u:test.m3u', name='test' + ) + assert path_to_ref(b'Test Playlist.m3u') == Ref.playlist( + uri='m3u:Test%20Playlist.m3u', name='Test Playlist' + ) + + +def test_load_items(): + assert loads('') == [] + + assert loads('test.mp3', basedir=b'/playlists') == [ + Ref.track(uri='file:///playlists/test.mp3', name='test') + ] + assert loads('../test.mp3', basedir=b'/playlists') == [ + Ref.track(uri='file:///test.mp3', name='test') + ] + assert loads('/test.mp3') == [ + Ref.track(uri='file:///test.mp3', name='test') + ] + assert loads('file:///test.mp3') == [ + Ref.track(uri='file:///test.mp3') + ] + assert loads('http://example.com/stream') == [ + Ref.track(uri='http://example.com/stream') + ] + + assert loads('#EXTM3U\n#EXTINF:42,Test\nfile:///test.mp3\n') == [ + Ref.track(uri='file:///test.mp3', name='Test') + ] + assert loads('#EXTM3U\n#EXTINF:-1,Test\nhttp://example.com/stream\n') == [ + Ref.track(uri='http://example.com/stream', name='Test') + ] + + +def test_dump_items(): + assert dumps([]) == '' + assert dumps([Ref.track(uri='file:///test.mp3')]) == ( + 'file:///test.mp3\n' + ) + assert dumps([Ref.track(uri='file:///test.mp3', name='test')]) == ( + '#EXTM3U\n' + '#EXTINF:-1,test\n' + 'file:///test.mp3\n' + ) + assert dumps([Track(uri='file:///test.mp3', name='test', length=42)]) == ( + '#EXTM3U\n' + '#EXTINF:-1,test\n' + 'file:///test.mp3\n' + ) + assert dumps([Track(uri='http://example.com/stream')]) == ( + 'http://example.com/stream\n' + ) + assert dumps([Track(uri='http://example.com/stream', name='Test')]) == ( + '#EXTM3U\n' + '#EXTINF:-1,Test\n' + 'http://example.com/stream\n' + ) + + +def test_playlist(): + from mopidy.m3u.translator import playlist + + assert playlist(b'test.m3u') == Playlist( + uri='m3u:test.m3u', + name='test' + ) + assert playlist(b'test.m3u', [Ref(uri='file:///test.mp3')], 1) == Playlist( + uri='m3u:test.m3u', + name='test', + tracks=[Track(uri='file:///test.mp3')], + last_modified=1000 + ) From 2b8508d3c7bb2a907e1c3bc221edd125c418b433 Mon Sep 17 00:00:00 2001 From: Thomas Kemmer Date: Sat, 9 Jan 2016 07:00:57 +0100 Subject: [PATCH 02/15] m3u: Implement write-replace context manager. --- mopidy/m3u/playlists.py | 34 +++++++++++++++++++++++++++++++++- 1 file changed, 33 insertions(+), 1 deletion(-) diff --git a/mopidy/m3u/playlists.py b/mopidy/m3u/playlists.py index e2b35d1d..7e4e39ff 100644 --- a/mopidy/m3u/playlists.py +++ b/mopidy/m3u/playlists.py @@ -1,10 +1,12 @@ from __future__ import absolute_import, unicode_literals +import contextlib import io import locale import logging import operator import os +import tempfile from mopidy import backend @@ -21,6 +23,33 @@ def log_environment_error(message, error): logger.error('%s: %s', message, strerror) +@contextlib.contextmanager +def replace(path, mode='w+b', encoding=None, errors=None): + try: + (fd, tempname) = tempfile.mkstemp(dir=os.path.dirname(path)) + except TypeError: + # Python 3 requires dir to be of type str until v3.5 + import sys + path = path.decode(sys.getfilesystemencoding()) + (fd, tempname) = tempfile.mkstemp(dir=os.path.dirname(path)) + try: + fp = io.open(fd, mode, encoding=encoding, errors=errors) + except: + os.remove(tempname) + os.close(fd) + raise + try: + yield fp + fp.flush() + os.fsync(fd) + os.rename(tempname, path) + except: + os.remove(tempname) + raise + finally: + fp.close() + + class M3UPlaylistsProvider(backend.PlaylistsProvider): def __init__(self, backend, config): @@ -114,4 +143,7 @@ class M3UPlaylistsProvider(backend.PlaylistsProvider): encoding = self._default_encoding if not os.path.isabs(path): path = os.path.join(self._playlists_dir, path) - return io.open(path, mode, encoding=encoding, errors='replace') + if 'w' in mode: + return replace(path, mode, encoding=encoding, errors='replace') + else: + return io.open(path, mode, encoding=encoding, errors='replace') From 2bcf1a6b0079dc58fff4468390ce04ac362bd6ed Mon Sep 17 00:00:00 2001 From: Thomas Kemmer Date: Sun, 10 Jan 2016 19:23:14 +0100 Subject: [PATCH 03/15] m3u: Change default_extension to m3u8. --- mopidy/m3u/ext.conf | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/mopidy/m3u/ext.conf b/mopidy/m3u/ext.conf index e4e68ff8..862bc6f7 100644 --- a/mopidy/m3u/ext.conf +++ b/mopidy/m3u/ext.conf @@ -1,8 +1,5 @@ [m3u] enabled = true - -default_encoding = latin-1 - -default_extension = .m3u - playlists_dir = +default_encoding = latin-1 +default_extension = .m3u8 From 1715756b14fe37112072ba5dbbb6330818b9e136 Mon Sep 17 00:00:00 2001 From: Thomas Kemmer Date: Sun, 10 Jan 2016 19:45:00 +0100 Subject: [PATCH 04/15] m3u: Update docs. --- docs/ext/m3u.rst | 11 +++++++++++ mopidy/backend.py | 5 +++-- 2 files changed, 14 insertions(+), 2 deletions(-) diff --git a/docs/ext/m3u.rst b/docs/ext/m3u.rst index 2b86b73a..37dc60be 100644 --- a/docs/ext/m3u.rst +++ b/docs/ext/m3u.rst @@ -54,3 +54,14 @@ See :ref:`config` for general help on configuring Mopidy. Path to directory with M3U files. Unset by default, in which case the extension's data dir is used to store playlists. + +.. confval:: m3u/default_encoding + + Text encoding used for files with extension ``.m3u``. Default is + ``latin-1``. Note that files with extension ``.m3u8`` are always + expected to be UTF-8 encoded. + +.. confval:: m3u/default_extension + + The file extension for M3U playlists created using the core playlist + API. Default is ``.m3u8``. diff --git a/mopidy/backend.py b/mopidy/backend.py index 8616ae96..7412ccc6 100644 --- a/mopidy/backend.py +++ b/mopidy/backend.py @@ -347,13 +347,14 @@ class PlaylistsProvider(object): """ Create a new empty playlist with the given name. - Returns a new playlist with the given name and an URI. + Returns a new playlist with the given name and an URI, or :class:`None` + on failure. *MUST be implemented by subclass.* :param name: name of the new playlist :type name: string - :rtype: :class:`mopidy.models.Playlist` + :rtype: :class:`mopidy.models.Playlist` or :class:`None` """ raise NotImplementedError From 60b071dbbdf45e7905cfbb06a54df5f0f71bf27e Mon Sep 17 00:00:00 2001 From: Thomas Kemmer Date: Sun, 10 Jan 2016 20:08:20 +0100 Subject: [PATCH 05/15] m3u: Update changelog for PR #1386. --- docs/changelog.rst | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/docs/changelog.rst b/docs/changelog.rst index ed382701..cdf6740e 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -48,6 +48,23 @@ M3U backend - Derive track name from file name for non-extended M3U playlists. (Fixes: :issue:`1364`, PR: :issue:`1369`) +- Major refactoring of the M3U playlist extension. (Fixes: + :issue:`1370` PR: :issue:`1386`) + + - Add :confval:`m3u/default_encoding` and :confval:`m3u/default_extension` + config values for improved text encoding support. + + - No longer scan playlist directory and parse playlists at startup or refresh. + Similarly to the file extension, this now happens on request. + + - Use :class:`mopidy.models.Ref` instances when reading and writing + playlists. Therefore, ``Track.length`` is no longer stored in + extended M3U playlists and ``#EXTINF`` runtime is always set to + -1. + + - Improve reliability of playlist updates using the core playlist API by + applying the write-replace pattern for file updates. + MPD frontend ------------ From ea89a85b5e93e6f02abd98878a53264fc5d3a484 Mon Sep 17 00:00:00 2001 From: jcass Date: Wed, 20 Jan 2016 00:07:15 +0200 Subject: [PATCH 06/15] docs:add section with some background and pointers on how to test extensions. --- docs/extensiondev.rst | 225 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 225 insertions(+) diff --git a/docs/extensiondev.rst b/docs/extensiondev.rst index f797368f..4b96d2e7 100644 --- a/docs/extensiondev.rst +++ b/docs/extensiondev.rst @@ -542,3 +542,228 @@ your HTTP requests:: For further details, see Requests' docs on `session objects `__. + +Testing Extensions +================== + +Creating test cases for your extensions makes them much simpler to maintain +over the long term. It can also make it easier for you to review and accept +pull requests from other contributors knowing that they will not break the +extension in some unanticipated way. + +Before getting started, it is important to familiarize yourself with the +Python `mock library `_. +When it comes to running tests, Mopidy typically makes use of testing tools +like `tox `_ and +`pytest `_. + +Testing Approach +---------------- + +To a large extent the testing approach to follow depends on how your extension +is structured, which parts of Mopidy it interacts with, and if it uses any 3rd +party APIs or makes any HTTP requests to the outside world. + +The sections that follow contain code extracts that highlight some of the +key areas that should be tested. For more exhaustive examples, you may want to +take a look at the test cases that ship with Mopidy itself which covers +everything from instantiating various controllers, reading configuration files, +and simulating events that your extension can listen to. + +In general your tests should cover the extension definition, the relevant +Mopidy controllers, and the Pykka backend and / or frontend actors that form +part of the extension. + +Testing the Extension Definition +-------------------------------- +Test cases for checking the definition of the extension should ensure that: +- the extension provides a ``ext.conf`` configuration file containing the + relevant parameters with their default values, +- that the config schema is fully defined, and +- that the extension's actor(s) are added to the Mopidy registry on setup. + +An example of what these tests could look like is provided below:: + + def test_get_default_config(self): + ext = Extension() + config = ext.get_default_config() + + self.assertIn('[my_extension]', config) + self.assertIn('enabled = true', config) + self.assertIn('param_1 = value_1', config) + self.assertIn('param_2 = value_2', config) + self.assertIn('param_n = value_n', config) + + def test_get_config_schema(self): + ext = Extension() + schema = ext.get_config_schema() + + self.assertIn('enabled', schema) + self.assertIn('param_1', schema) + self.assertIn('param_2', schema) + self.assertIn('param_n', schema) + + def test_setup(self): + registry = mock.Mock() + + ext = Extension() + ext.setup(registry) + calls = [mock.call('frontend', frontend_lib.MyFrontend), + mock.call('backend', backend_lib.MyBackend)] + registry.add.assert_has_calls(calls, any_order=True) + + +Testing Backend Actors +---------------------- +Backends can usually be constructed with a small mockup of the configuration +file, and mocking the audio actor:: + + @pytest.fixture() + def config(): + return { + 'http': { + 'hostname': '127.0.0.1', + 'port': '6680' + }, + 'proxy': { + 'hostname': 'host_mock', + 'port': 'port_mock' + }, + 'my_extension': { + 'enabled': True, + 'param_1': 'value_1', + 'param_2': 'value_2', + 'param_n': 'value_n', + } + } + + def get_backend(config): + return backend.MyBackend(config=config, audio=mock.Mock()) + + +You'll probably want to patch ``requests`` or any other web API's that you use +to avoid any unintended HTTP requests from being made by your backend during +testing:: + + from mock import patch + @mock.patch('requests.get', + mock.Mock(side_effect=Exception('Intercepted unintended HTTP call'))) + + +Backend tests should also ensure that: +- the backend provides a unique URI scheme, +- that it sets up the various providers (e.g. library, playback, etc.):: + + def test_uri_schemes(config): + backend = get_backend(config) + + assert 'my_scheme' in backend.uri_schemes + + + def test_init_sets_up_the_providers(config): + backend = get_backend(config) + + assert isinstance(backend.library, library.MyLibraryProvider) + assert isinstance(backend.playback, playback.MyPlaybackProvider) + + +Once you have a backend instance to work with, testing the various playback, +library, and other providers is straight forward and should not require any +special setup or processing. + +Testing Libraries +----------------- +Library test cases should cover the implementations of the standard Mopidy +API (e.g. ``browse``, ``lookup``, ``refresh``, ``get_images``, ``search``, +etc.) + +Testing Playback Controllers +---------------------------- +Testing ``change_track`` and ``translate_uri`` is probably the highest +priority, since these methods are used to prepare the track and provide its +audio URL to Mopidy's core for playback. + +Testing Frontends +----------------- +Because most frontends will interact with the Mopidy core, it will most likely +be necessary to have a full core running for testing purposes:: + + self.core = core.Core.start( + config, backends=[get_backend(config]).proxy() + + +It may be advisable to take a quick look at the +`Pykka API `_ at this point to make sure that +you are familiar with ``ThreadingActor``, ``ThreadingFuture``, and the +``proxies`` that allow you to access the attributes and methods of the actor +directly. + +You'll also need a list of ``models.Track`` and a list of URIs in order to +populate the core with some simple tracks that can be used for testing:: + + class BaseTest(unittest.TestCase): + tracks = [ + models.Track(uri='my_scheme:track:id1', length=40000), # Regular track + models.Track(uri='my_scheme:track:id2', length=None), # No duration + ] + + uris = [ 'my_scheme:track:id1', 'my_scheme:track:id2'] + + +In the ``setup()`` method of your test class, you will then probably need to +monkey patch looking up tracks in the library (so that it will always use the +lists that you defined), and then populate the core's tracklist:: + + def lookup(uris): + result = {uri: [] for uri in uris} + for track in self.tracks: + if track.uri in result: + result[track.uri].append(track) + return result + + self.core.library.lookup = lookup + self.tl_tracks = self.core.tracklist.add(uris=self.uris).get() + + +With all of that done you should finally be ready to instantiate your frontend:: + + self.frontend = frontend.MyFrontend.start(config(), self.core).proxy() + + +...and then just remember that the normal core and frontend methods will usually +return ``pykka.ThreadingFuture`` objects, so you will need to add ``.get()`` at +the end of most method calls in order to get to the actual return values. + +Triggering Events +----------------- +There may be test case scenarios that require simulating certain event triggers +that your extension's actors can listen for and respond on. An example for +patching the listener to store these events, and then play them back for your +actor, may look something like this:: + + self.events = [] + self.patcher = mock.patch('mopidy.listener.send') + self.send_mock = self.patcher.start() + + def send(cls, event, **kwargs): + self.events.append((event, kwargs)) + + self.send_mock.side_effect = send + + +...and then just call ``replay_events()`` at the relevant points in your code +to have the events fire:: + + def replay_events(self, my_actor, until=None): + while self.events: + if self.events[0][0] == until: + break + event, kwargs = self.events.pop(0) + frontend.on_event(event, **kwargs).get() + + +Further Reading +--------------- +The `/tests `_ +directory on the Mopidy development branch contains hundreds of sample test +cases that cover virtually every aspect of using the server. \ No newline at end of file From 7f03b2125840ecc495fa88d1e4709cbc8921767e Mon Sep 17 00:00:00 2001 From: jcass Date: Wed, 20 Jan 2016 00:19:59 +0200 Subject: [PATCH 07/15] docs:align case of headings with rest of section. Remove fragmented sentences. --- docs/extensiondev.rst | 33 ++++++++++++++++----------------- 1 file changed, 16 insertions(+), 17 deletions(-) diff --git a/docs/extensiondev.rst b/docs/extensiondev.rst index 4b96d2e7..8e6cc106 100644 --- a/docs/extensiondev.rst +++ b/docs/extensiondev.rst @@ -543,7 +543,7 @@ your HTTP requests:: For further details, see Requests' docs on `session objects `__. -Testing Extensions +Testing extensions ================== Creating test cases for your extensions makes them much simpler to maintain @@ -557,7 +557,7 @@ When it comes to running tests, Mopidy typically makes use of testing tools like `tox `_ and `pytest `_. -Testing Approach +Testing approach ---------------- To a large extent the testing approach to follow depends on how your extension @@ -574,7 +574,7 @@ In general your tests should cover the extension definition, the relevant Mopidy controllers, and the Pykka backend and / or frontend actors that form part of the extension. -Testing the Extension Definition +Testing the extension definition -------------------------------- Test cases for checking the definition of the extension should ensure that: - the extension provides a ``ext.conf`` configuration file containing the @@ -613,7 +613,7 @@ An example of what these tests could look like is provided below:: registry.add.assert_has_calls(calls, any_order=True) -Testing Backend Actors +Testing backend actors ---------------------- Backends can usually be constructed with a small mockup of the configuration file, and mocking the audio actor:: @@ -671,19 +671,19 @@ Once you have a backend instance to work with, testing the various playback, library, and other providers is straight forward and should not require any special setup or processing. -Testing Libraries +Testing libraries ----------------- Library test cases should cover the implementations of the standard Mopidy API (e.g. ``browse``, ``lookup``, ``refresh``, ``get_images``, ``search``, etc.) -Testing Playback Controllers +Testing playback controllers ---------------------------- Testing ``change_track`` and ``translate_uri`` is probably the highest priority, since these methods are used to prepare the track and provide its audio URL to Mopidy's core for playback. -Testing Frontends +Testing frontends ----------------- Because most frontends will interact with the Mopidy core, it will most likely be necessary to have a full core running for testing purposes:: @@ -730,11 +730,11 @@ With all of that done you should finally be ready to instantiate your frontend:: self.frontend = frontend.MyFrontend.start(config(), self.core).proxy() -...and then just remember that the normal core and frontend methods will usually -return ``pykka.ThreadingFuture`` objects, so you will need to add ``.get()`` at +Keep in mind that the normal core and frontend methods will usually return +``pykka.ThreadingFuture`` objects, so you will need to add ``.get()`` at the end of most method calls in order to get to the actual return values. -Triggering Events +Triggering events ----------------- There may be test case scenarios that require simulating certain event triggers that your extension's actors can listen for and respond on. An example for @@ -751,8 +751,9 @@ actor, may look something like this:: self.send_mock.side_effect = send -...and then just call ``replay_events()`` at the relevant points in your code -to have the events fire:: +Once all of the events have been captured, a method like +``replay_events()`` can be called at the relevant points in the code to have +the events fire:: def replay_events(self, my_actor, until=None): while self.events: @@ -762,8 +763,6 @@ to have the events fire:: frontend.on_event(event, **kwargs).get() -Further Reading ---------------- -The `/tests `_ -directory on the Mopidy development branch contains hundreds of sample test -cases that cover virtually every aspect of using the server. \ No newline at end of file +For further details and examples, refer to the +`/tests `_ +directory on the Mopidy development branch. \ No newline at end of file From f62057a9ad57bbbc060028f83774efe0639a4271 Mon Sep 17 00:00:00 2001 From: Stein Magnus Jodal Date: Sun, 17 Jan 2016 07:55:34 +0100 Subject: [PATCH 08/15] flake8: Fix compat with pep8 1.7.0 (cherry picked from commit 18b609fa6ed3c06c0dc3156cbb7409c9494c0bc2) --- mopidy/audio/scan.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/mopidy/audio/scan.py b/mopidy/audio/scan.py index ca2c308c..fd5d2d49 100644 --- a/mopidy/audio/scan.py +++ b/mopidy/audio/scan.py @@ -141,8 +141,9 @@ def _process(pipeline, timeout_ms): have_audio = False missing_message = None - types = (gst.MESSAGE_ELEMENT | gst.MESSAGE_APPLICATION | gst.MESSAGE_ERROR - | gst.MESSAGE_EOS | gst.MESSAGE_ASYNC_DONE | gst.MESSAGE_TAG) + types = ( + gst.MESSAGE_ELEMENT | gst.MESSAGE_APPLICATION | gst.MESSAGE_ERROR | + gst.MESSAGE_EOS | gst.MESSAGE_ASYNC_DONE | gst.MESSAGE_TAG) previous = clock.get_time() while timeout > 0: From 05729d3dc068e92da0e0f080b780b8a298417bf7 Mon Sep 17 00:00:00 2001 From: jcass Date: Wed, 20 Jan 2016 09:36:07 +0200 Subject: [PATCH 09/15] docs:fix bullet list formatting. --- docs/extensiondev.rst | 2 ++ 1 file changed, 2 insertions(+) diff --git a/docs/extensiondev.rst b/docs/extensiondev.rst index 8e6cc106..c208a092 100644 --- a/docs/extensiondev.rst +++ b/docs/extensiondev.rst @@ -577,6 +577,7 @@ part of the extension. Testing the extension definition -------------------------------- Test cases for checking the definition of the extension should ensure that: + - the extension provides a ``ext.conf`` configuration file containing the relevant parameters with their default values, - that the config schema is fully defined, and @@ -651,6 +652,7 @@ testing:: Backend tests should also ensure that: + - the backend provides a unique URI scheme, - that it sets up the various providers (e.g. library, playback, etc.):: From edc3929dafe73dfb4f7d2b59711c7ab57b2df618 Mon Sep 17 00:00:00 2001 From: jcass Date: Wed, 20 Jan 2016 11:49:29 +0200 Subject: [PATCH 10/15] docs:address PR review comments. --- docs/extensiondev.rst | 48 ++++++++++++++++++++++++++++--------------- 1 file changed, 32 insertions(+), 16 deletions(-) diff --git a/docs/extensiondev.rst b/docs/extensiondev.rst index c208a092..348082fd 100644 --- a/docs/extensiondev.rst +++ b/docs/extensiondev.rst @@ -576,6 +576,7 @@ part of the extension. Testing the extension definition -------------------------------- + Test cases for checking the definition of the extension should ensure that: - the extension provides a ``ext.conf`` configuration file containing the @@ -589,20 +590,20 @@ An example of what these tests could look like is provided below:: ext = Extension() config = ext.get_default_config() - self.assertIn('[my_extension]', config) - self.assertIn('enabled = true', config) - self.assertIn('param_1 = value_1', config) - self.assertIn('param_2 = value_2', config) - self.assertIn('param_n = value_n', config) + assert '[my_extension]' in config + assert 'enabled = true' in config + assert 'param_1 = value_1' in config + assert 'param_2 = value_2' in config + assert 'param_n = value_n' in config def test_get_config_schema(self): ext = Extension() schema = ext.get_config_schema() - self.assertIn('enabled', schema) - self.assertIn('param_1', schema) - self.assertIn('param_2', schema) - self.assertIn('param_n', schema) + assert 'enabled' in schema + assert 'param_1' in schema + assert 'param_2' in schema + assert 'param_n' in schema def test_setup(self): registry = mock.Mock() @@ -616,10 +617,11 @@ An example of what these tests could look like is provided below:: Testing backend actors ---------------------- + Backends can usually be constructed with a small mockup of the configuration file, and mocking the audio actor:: - @pytest.fixture() + @pytest.fixture def config(): return { 'http': { @@ -641,10 +643,17 @@ file, and mocking the audio actor:: def get_backend(config): return backend.MyBackend(config=config, audio=mock.Mock()) +The following libraries might be useful for mocking any HTTP requests that +your extension makes: -You'll probably want to patch ``requests`` or any other web API's that you use -to avoid any unintended HTTP requests from being made by your backend during -testing:: +- `responses `_ - A utility library for + mocking out the requests Python library. +- `vcrpy `_ - Automatically mock your HTTP + interactions to simplify and speed up testing. + +At the very least, you'll probably want to patch ``requests`` or any other web +API's that you use to avoid any unintended HTTP requests from being made by +your backend during testing:: from mock import patch @mock.patch('requests.get', @@ -654,7 +663,9 @@ testing:: Backend tests should also ensure that: - the backend provides a unique URI scheme, -- that it sets up the various providers (e.g. library, playback, etc.):: +- that it sets up the various providers (e.g. library, playback, etc.) + +:: def test_uri_schemes(config): backend = get_backend(config) @@ -675,18 +686,21 @@ special setup or processing. Testing libraries ----------------- + Library test cases should cover the implementations of the standard Mopidy API (e.g. ``browse``, ``lookup``, ``refresh``, ``get_images``, ``search``, etc.) Testing playback controllers ---------------------------- + Testing ``change_track`` and ``translate_uri`` is probably the highest priority, since these methods are used to prepare the track and provide its audio URL to Mopidy's core for playback. Testing frontends ----------------- + Because most frontends will interact with the Mopidy core, it will most likely be necessary to have a full core running for testing purposes:: @@ -700,8 +714,9 @@ you are familiar with ``ThreadingActor``, ``ThreadingFuture``, and the ``proxies`` that allow you to access the attributes and methods of the actor directly. -You'll also need a list of ``models.Track`` and a list of URIs in order to -populate the core with some simple tracks that can be used for testing:: +You'll also need a list of :class:`~mopidy.models.Track` and a list of URIs in +order to populate the core with some simple tracks that can be used for +testing:: class BaseTest(unittest.TestCase): tracks = [ @@ -738,6 +753,7 @@ the end of most method calls in order to get to the actual return values. Triggering events ----------------- + There may be test case scenarios that require simulating certain event triggers that your extension's actors can listen for and respond on. An example for patching the listener to store these events, and then play them back for your From 239a7be7082befae844a983a110d03c577131809 Mon Sep 17 00:00:00 2001 From: jcass Date: Wed, 20 Jan 2016 15:41:58 +0200 Subject: [PATCH 11/15] fix: ensure that tl_track information is included in event trigger when consume mode is enabled. --- mopidy/core/playback.py | 5 +++-- tests/core/test_playback.py | 28 ++++++++++++++++++++++++++++ 2 files changed, 31 insertions(+), 2 deletions(-) diff --git a/mopidy/core/playback.py b/mopidy/core/playback.py index 89bd92ee..7170969e 100644 --- a/mopidy/core/playback.py +++ b/mopidy/core/playback.py @@ -516,7 +516,8 @@ class PlaybackController(object): listener.CoreListener.send('track_playback_started', tl_track=tl_track) def _trigger_track_playback_ended(self, time_position_before_stop): - if self.get_current_tl_track() is None: + tl_track = self.get_current_tl_track() + if tl_track is None: return logger.debug('Triggering track playback ended event') @@ -528,7 +529,7 @@ class PlaybackController(object): # TODO: Use the lowest of track duration and position. listener.CoreListener.send( 'track_playback_ended', - tl_track=self.get_current_tl_track(), + tl_track=tl_track, time_position=time_position_before_stop) def _trigger_playback_state_changed(self, old_state, new_state): diff --git a/tests/core/test_playback.py b/tests/core/test_playback.py index 06d516ac..2fb95437 100644 --- a/tests/core/test_playback.py +++ b/tests/core/test_playback.py @@ -491,6 +491,34 @@ class EventEmissionTest(BaseTest): ], listener_mock.send.mock_calls) + def test_next_emits_events_when_consume_mode_is_enabled(self, listener_mock): + tl_tracks = self.core.tracklist.get_tl_tracks() + + self.core.tracklist.set_consume(True) + self.core.playback.play(tl_tracks[0]) + self.replay_events() + self.core.playback.seek(1000) + self.replay_events() + listener_mock.reset_mock() + + self.core.playback.next() + self.replay_events() + + self.assertListEqual( + [ + mock.call( + 'tracklist_changed'), + mock.call( + 'track_playback_ended', + tl_track=tl_tracks[0], time_position=mock.ANY), + mock.call( + 'playback_state_changed', + old_state='playing', new_state='playing'), + mock.call( + 'track_playback_started', tl_track=tl_tracks[1]), + ], + listener_mock.send.mock_calls) + def test_gapless_track_change_emits_events(self, listener_mock): tl_tracks = self.core.tracklist.get_tl_tracks() From dee7eb7e20769e55e4e44f42426ca9eea02cbb60 Mon Sep 17 00:00:00 2001 From: jcass Date: Wed, 20 Jan 2016 15:55:02 +0200 Subject: [PATCH 12/15] tests:fix pep8 violation. --- tests/core/test_playback.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/core/test_playback.py b/tests/core/test_playback.py index 2fb95437..1f25a4fa 100644 --- a/tests/core/test_playback.py +++ b/tests/core/test_playback.py @@ -491,7 +491,8 @@ class EventEmissionTest(BaseTest): ], listener_mock.send.mock_calls) - def test_next_emits_events_when_consume_mode_is_enabled(self, listener_mock): + def test_next_emits_events_when_consume_mode_is_enabled(self, + listener_mock): tl_tracks = self.core.tracklist.get_tl_tracks() self.core.tracklist.set_consume(True) From c55a82b150fec9165b062a364ab4181a62fcaddf Mon Sep 17 00:00:00 2001 From: jcass Date: Thu, 21 Jan 2016 05:38:09 +0200 Subject: [PATCH 13/15] docs:fix syntax error in code sample. --- docs/extensiondev.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/extensiondev.rst b/docs/extensiondev.rst index 348082fd..72747e28 100644 --- a/docs/extensiondev.rst +++ b/docs/extensiondev.rst @@ -705,7 +705,7 @@ Because most frontends will interact with the Mopidy core, it will most likely be necessary to have a full core running for testing purposes:: self.core = core.Core.start( - config, backends=[get_backend(config]).proxy() + config, backends=[get_backend(config)]).proxy() It may be advisable to take a quick look at the From 2fcbc691c0dbae8e8de5ee0951f896cade424eb7 Mon Sep 17 00:00:00 2001 From: jcass Date: Thu, 21 Jan 2016 05:55:37 +0200 Subject: [PATCH 14/15] fix:add changelog entry and fix line indentation. --- docs/changelog.rst | 6 +++++- tests/core/test_playback.py | 5 +++-- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/docs/changelog.rst b/docs/changelog.rst index cdf6740e..0a378942 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -23,6 +23,10 @@ Core API - Add :meth:`mopidy.core.PlaylistsController.get_uri_schemes`. (PR: :issue:`1362`) +- The ``track_playback_ended`` event now includes the correct ``tl_track`` + reference when changing to the next track in consume mode. (Fixes: + :issue:`1402` PR: :issue:`1403`) + Models ------ @@ -37,7 +41,7 @@ Extension support we let Mopidy crash if an extension's setup crashed. (PR: :issue:`1337`) Local backend --------------- +------------- - Made :confval:`local/data_dir` really deprecated. This change breaks older versions of Mopidy-Local-SQLite and Mopidy-Local-Images. diff --git a/tests/core/test_playback.py b/tests/core/test_playback.py index 1f25a4fa..3ddc51f3 100644 --- a/tests/core/test_playback.py +++ b/tests/core/test_playback.py @@ -491,8 +491,9 @@ class EventEmissionTest(BaseTest): ], listener_mock.send.mock_calls) - def test_next_emits_events_when_consume_mode_is_enabled(self, - listener_mock): + def test_next_emits_events_when_consume_mode_is_enabled( + self, + listener_mock): tl_tracks = self.core.tracklist.get_tl_tracks() self.core.tracklist.set_consume(True) From 2b8cd9f24fab31c98ba5958bf786404cd148d7bd Mon Sep 17 00:00:00 2001 From: jcass Date: Thu, 21 Jan 2016 05:59:57 +0200 Subject: [PATCH 15/15] fix:add reference to PR in changelog. --- docs/changelog.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/changelog.rst b/docs/changelog.rst index 0a378942..bdda493d 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -25,7 +25,7 @@ Core API - The ``track_playback_ended`` event now includes the correct ``tl_track`` reference when changing to the next track in consume mode. (Fixes: - :issue:`1402` PR: :issue:`1403`) + :issue:`1402` PR: :issue:`1403` PR: :issue:`1406`) Models ------