ext: Update validate_extension to validate_extension_data

Adds more checks to catch extension errors and importantly tests for exercise
these code paths.

mopidy/__main__.py

@@ -95,7 +95,7 @@ def main():
             extension = data.extension
 
             # TODO: factor out all of this to a helper that can be tested
-            if not ext.validate_extension(data.extension, data.entry_point):
+            if not ext.validate_extension_data(data):
                 config[extension.ext_name] = {'enabled': False}
                 config_errors[extension.ext_name] = {
                     'enabled': 'extension disabled by self check.'}

mopidy/ext.py

@@ -184,47 +184,70 @@ def load_extensions():
     return installed_extensions
 
 
-def validate_extension(extension, entry_point):
+def validate_extension_data(data):
     """Verify extension's dependencies and environment.
 
     :param extensions: an extension to check
     :returns: if extension should be run
     """
 
-    logger.debug('Validating extension: %s', extension.ext_name)
+    logger.debug('Validating extension: %s', data.extension.ext_name)
 
-    if extension.ext_name != entry_point.name:
+    if data.extension.ext_name != data.entry_point.name:
         logger.warning(
             'Disabled extension %(ep)s: entry point name (%(ep)s) '
             'does not match extension name (%(ext)s)',
-            {'ep': entry_point.name, 'ext': extension.ext_name})
+            {'ep': data.entry_point.name, 'ext': data.extension.ext_name})
         return False
 
     try:
-        entry_point.require()
+        data.entry_point.require()
     except pkg_resources.DistributionNotFound as ex:
         logger.info(
             'Disabled extension %s: Dependency %s not found',
-            extension.ext_name, ex)
+            data.extension.ext_name, ex)
         return False
     except pkg_resources.VersionConflict as ex:
         if len(ex.args) == 2:
             found, required = ex.args
             logger.info(
                 'Disabled extension %s: %s required, but found %s at %s',
-                extension.ext_name, required, found, found.location)
+                data.extension.ext_name, required, found, found.location)
         else:
             logger.info(
-                'Disabled extension %s: %s', extension.ext_name, ex)
+                'Disabled extension %s: %s', data.extension.ext_name, ex)
         return False
 
     try:
-        extension.validate_environment()
+        data.extension.validate_environment()
     except exceptions.ExtensionError as ex:
         logger.info(
-            'Disabled extension %s: %s', extension.ext_name, ex.message)
+            'Disabled extension %s: %s', data.extension.ext_name, ex.message)
         return False
     except Exception:
-        return False  # TODO: log
+        logger.exception('Validating extension %s failed with an exception.',
+                         data.extension.ext_name)
+        return False
+
+    if not data.config_schema:
+        logger.error('Extension %s does not have a config schema, disabling.',
+                     data.extension.ext_name)
+        return False
+    elif not isinstance(data.config_schema.get('enabled'), config_lib.Boolean):
+        logger.error('Extension %s does not have the required "enabled" config'
+                     ' option, disabling.', data.extension.ext_name)
+        return False
+
+    for key, value in data.config_schema.items():
+        if not isinstance(value, config_lib.ConfigValue):
+            logger.error('Extension %s config schema contains an invalid value'
+                         ' for the option "%s", disabling.',
+                         data.extension.ext_name, key)
+            return False
+
+    if not data.config_defaults:
+        logger.error('Extension %s does not have a default config, disabling.',
+                     data.extension.ext_name)
+        return False
 
     return True

tests/test_ext.py

@@ -160,7 +160,7 @@ def test_load_extensions_get_command_fails(iter_entry_points_mock):
         get_command.assert_called_once_with()
 
 
-# ext.validate_extension
+# ext.validate_extension_data
 
 @pytest.fixture
 def ext_data():
@@ -178,18 +178,18 @@ def ext_data():
 
 def test_validate_extension_name_mismatch(ext_data):
     ext_data.entry_point.name = 'barfoo'
-    assert not ext.validate_extension(ext_data.extension, ext_data.entry_point)
+    assert not ext.validate_extension_data(ext_data)
 
 
 def test_validate_extension_distribution_not_found(ext_data):
     error = pkg_resources.DistributionNotFound
     ext_data.entry_point.require.side_effect = error
-    assert not ext.validate_extension(ext_data.extension, ext_data.entry_point)
+    assert not ext.validate_extension_data(ext_data)
 
 
 def test_validate_extension_version_conflict(ext_data):
     ext_data.entry_point.require.side_effect = pkg_resources.VersionConflict
-    assert not ext.validate_extension(ext_data.extension, ext_data.entry_point)
+    assert not ext.validate_extension_data(ext_data)
 
 
 def test_validate_extension_exception(ext_data):
@@ -197,8 +197,7 @@ def test_validate_extension_exception(ext_data):
 
     # We trust that entry points are well behaved, so exception will bubble.
     with pytest.raises(Exception):
-        assert not ext.validate_extension(
+        assert not ext.validate_extension_data(ext_data)
-            ext_data.extension, ext_data.entry_point)
 
 
 def test_validate_extension_instance_validate_env_ext_error(ext_data):
@@ -206,8 +205,7 @@ def test_validate_extension_instance_validate_env_ext_error(ext_data):
     with mock.patch.object(extension, 'validate_environment') as validate:
         validate.side_effect = exceptions.ExtensionError('error')
 
-        assert not ext.validate_extension(
+        assert not ext.validate_extension_data(ext_data)
-            ext_data.extension, ext_data.entry_point)
         validate.assert_called_once_with()
 
 
@@ -216,6 +214,31 @@ def test_validate_extension_instance_validate_env_exception(ext_data):
     with mock.patch.object(extension, 'validate_environment') as validate:
         validate.side_effect = Exception
 
-        assert not ext.validate_extension(
+        assert not ext.validate_extension_data(ext_data)
-            ext_data.extension, ext_data.entry_point)
         validate.assert_called_once_with()
+
+
+def test_validate_extension_with_missing_schema(ext_data):
+    ext_data = ext_data._replace(config_schema=None)
+    assert not ext.validate_extension_data(ext_data)
+
+
+def test_validate_extension_with_schema_that_is_missing_enabled(ext_data):
+    del ext_data.config_schema['enabled']
+    ext_data.config_schema['baz'] = config.String()
+    assert not ext.validate_extension_data(ext_data)
+
+
+def test_validate_extension_with_schema_with_wrong_types(ext_data):
+    ext_data.config_schema['enabled'] = 123
+    assert not ext.validate_extension_data(ext_data)
+
+
+def test_validate_extension_with_schema_with_invalid_type(ext_data):
+    ext_data.config_schema['baz'] = 123
+    assert not ext.validate_extension_data(ext_data)
+
+
+def test_validate_extension_with_no_defaults(ext_data):
+    ext_data = ext_data._replace(config_defaults=None)
+    assert not ext.validate_extension_data(ext_data)