Merge lp:~elopio/snapcraft/fix1477638-format_strings-2 into lp:~snappy-dev/snapcraft/core

[Leo Arias]

Use the format string in plugin.py

[lp:~elopio/snapcraft/fix1477638-format_strings-2]

113. By Leo Arias on 2015-07-23

Prettify the code.

114. By Leo Arias on 2015-07-23

Typo

115. By Leo Arias on 2015-07-23

Use the format method in load_plugin.

116. By Leo Arias on 2015-07-24

Removed the !r as suggested by mterry

[Michael Terry (mterry)]

I'm fine with content, but you have a conflict with trunk

review: Approve

[lp:~elopio/snapcraft/fix1477638-format_strings-2]

117. By Leo Arias on 2015-07-29

Merged with trunk.

[Leo Arias (elopio)]

There's no conflict anymore, so I'll top approve.

=== modified file 'snapcraft/plugin.py'
--- snapcraft/plugin.py	2015-07-23 17:19:25 +0000
+++ snapcraft/plugin.py	2015-07-29 15:07:56 +0000
@@ -69,14 +69,14 @@
             # only set to valid if it loads without PluginError
             self.part_names.append(part_name)
             self.valid = True
-        except PluginError:
+        except PluginError as e:
+            logger.error(str(e))
             return
 
     def _load_config(self, name):
         configPath = os.path.join(plugindir(name), name + ".yaml")
         if not os.path.exists(configPath):
-            logger.error("Unknown plugin %s" % name)
-            raise PluginError()
+            raise PluginError('Unknown plugin: {}'.format(name))
         with open(configPath, 'r') as fp:
             self.config = yaml.load(fp) or {}
 
@@ -92,8 +92,7 @@
                 setattr(options, attrname, properties[opt])
             else:
                 if opt_parameters.get('required', False):
-                    logger.error("Required field %s missing on part %s" % (opt, name))
-                    raise PluginError()
+                    raise PluginError('Required field {} missing on part {}'.format(opt, name))
                 setattr(options, attrname, None)
 
         return options
@@ -230,7 +229,7 @@
         # validate
         for d in includes + excludes:
             if os.path.isabs(d):
-                raise PluginError("path '%s' must be relative" % d)
+                raise PluginError("path '{}' must be relative".format(d))
 
         sourceFiles = set()
         for root, dirs, files in os.walk(self.installdir):
@@ -280,6 +279,6 @@
 def load_plugin(part_name, plugin_name, properties={}, load_code=True):
     part = PluginHandler(plugin_name, part_name, properties, load_code=load_code)
     if not part.is_valid():
-        logger.error("Could not load part %s" % plugin_name)
+        logger.error('Could not load part {}'.format(plugin_name))
         sys.exit(1)
     return part

=== modified file 'snapcraft/tests/test_plugin.py'
--- snapcraft/tests/test_plugin.py	2015-07-23 14:23:29 +0000
+++ snapcraft/tests/test_plugin.py	2015-07-29 15:07:56 +0000
@@ -25,15 +25,31 @@
     patch,
 )
 
-import snapcraft.tests.mock_plugin
-from snapcraft.plugin import PluginHandler, PluginError
-from snapcraft.tests import TestCase
-
-
-class TestPlugin(TestCase):
-
-    def get_test_plugin(self):
-        return PluginHandler('mock', 'mock-part', {}, load_config=False, load_code=False)
+from snapcraft import (
+    plugin,
+    tests
+)
+from snapcraft.tests import mock_plugin
+
+
+class TestPlugin(tests.TestCase):
+
+    def get_test_plugin(self, name='mock', part_name='mock-part',
+                        properties=None, load_code=False, load_config=False):
+        if properties is None:
+            properties = {}
+        return plugin.PluginHandler(
+            name, part_name, properties, load_code=load_code,
+            load_config=load_config)
+
+    def test_init_unknown_plugin_must_log_error(self):
+        fake_logger = fixtures.FakeLogger(level=logging.ERROR)
+        self.useFixture(fake_logger)
+
+        self.get_test_plugin('test_unexisting_name', load_config=True)
+
+        self.assertEqual(
+            'Unknown plugin: test_unexisting_name\n', fake_logger.output)
 
     def test_is_dirty(self):
         p = self.get_test_plugin()
@@ -100,8 +116,8 @@
         fake_logger = fixtures.FakeLogger(level=logging.INFO)
         self.useFixture(fake_logger)
 
-        plugin = self.get_test_plugin()
-        plugin.notify_stage('test stage')
+        p = self.get_test_plugin()
+        p.notify_stage('test stage')
 
         self.assertEqual('test stage mock-part\n', fake_logger.output)
 
@@ -111,9 +127,9 @@
             # called with the name only and sys.path set
             self.assertEqual(module_name, "x-mock")
             self.assertTrue(sys.path[0].endswith("parts/plugins"))
-            return snapcraft.tests.mock_plugin
+            return mock_plugin
         with patch("importlib.import_module", side_effect=mock_import_modules):
-            PluginHandler(
+            plugin.PluginHandler(
                 "x-mock", "mock-part", {}, load_config=False, load_code=True)
         # sys.path is cleaned afterwards
         self.assertFalse(sys.path[0].endswith("parts/plugins"))
@@ -123,15 +139,37 @@
         def mock_import_modules(module_name):
             # called with the full snapcraft path
             self.assertEqual(module_name, "snapcraft.plugins.mock")
-            return snapcraft.tests.mock_plugin
+            return mock_plugin
         with patch("importlib.import_module", side_effect=mock_import_modules):
-            PluginHandler(
+            plugin.PluginHandler(
                 "mock", "mock-part", {}, load_config=False, load_code=True)
 
-    def test_collect_snap_files_with_abs_path_raises(self):
-        # ensure that absolute path raise an error
-        # (os.path.join will throw an error otherwise)
-        with patch("importlib.import_module", return_value=snapcraft.tests.mock_plugin):
-            p = PluginHandler("mock", "mock-part", {}, load_config=False)
-        with self.assertRaises(PluginError):
-            p.collect_snap_files(['*'], ['/1'])
+    def test_collect_snap_files_with_absolute_includes_must_raise_error(self):
+        p = self.get_test_plugin()
+        with self.assertRaises(plugin.PluginError) as raised:
+            p.collect_snap_files(includes=['rel', '/abs/include'], excludes=[])
+
+        self.assertEqual(
+            "path '/abs/include' must be relative", str(raised.exception))
+
+    def test_collect_snap_files_with_absolute_excludes_must_raise_error(self):
+        p = self.get_test_plugin()
+        with self.assertRaises(plugin.PluginError) as raised:
+            p.collect_snap_files(includes=[], excludes=['rel', '/abs/exclude'])
+
+        self.assertEqual(
+            "path '/abs/exclude' must be relative", str(raised.exception))
+
+    def test_load_plugin_with_invalid_part_must_exit_with_error(self):
+        fake_logger = fixtures.FakeLogger(level=logging.ERROR)
+        self.useFixture(fake_logger)
+
+        with self.assertRaises(SystemExit) as raised:
+            plugin.load_plugin(
+                'dummy-part', 'test_unexisting_name', load_code=False)
+
+        self.assertEqual(raised.exception.code, 1, 'Wrong exit code returned.')
+        self.assertEqual(
+            'Unknown plugin: test_unexisting_name\n'
+            'Could not load part test_unexisting_name\n',
+            fake_logger.output)