Merge lp:~elopio/snapcraft/makedirs into lp:~snappy-dev/snapcraft/core

[Leo Arias]

Refactored the plugin makedirs, with tests. Added python3 >= 3.2 as a dependency, needed by makedirs exist_ok argument.

[Michael Terry (mterry)]

LGTM, thanks Leo!

review: Approve

[Snappy Tarmac (snappydevtarmac)]

The attempt to merge lp:~elopio/snapcraft/makedirs into lp:snapcraft failed. Below is the output from the failed tests.

wget -c http://0.0.0.0:46923/test.tar

parts:
cp --preserve=all zzz /tmp/tmpwbwvn8c3/parts/copy/install/zzz
cp --preserve=all src /tmp/tmp548v9y6s/parts/copy/install/dst
cp --preserve=all src /tmp/tmp7m_60hbh/parts/copy/install/dir/dst

.........--2015-08-05 14:20:49-- http://0.0.0.0:46923/test.tar
Connecting to 0.0.0.0:46923... connected.
HTTP request sent, awaiting response... 200 OK
Length: 22 [text/html]
Saving to: ‘test.tar’

     0K 100% 313M=0s

2015-08-05 14:20:49 (313 MB/s) - ‘test.tar’ saved [22/22]

....Warning: unable to find "test_relexepath" in the path
...........EE....EE.......
======================================================================
ERROR: test_makedirs_with_existing_dirs (snapcraft.tests.test_plugin.PluginMakedirsTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/tmp/tarmac/branch.Edc9Te/snapcraft/tests/test_plugin.py", line 205, in test_makedirs_with_existing_dirs
    p.makedirs()
  File "/tmp/tarmac/branch.Edc9Te/snapcraft/plugin.py", line 133, in makedirs
    os.makedirs(d, exists_ok=True)
TypeError: makedirs() got an unexpected keyword argument 'exists_ok'

======================================================================
ERROR: test_makedirs_with_existing_dirs (snapcraft.tests.test_plugin.PluginMakedirsTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/tmp/tarmac/branch.Edc9Te/snapcraft/tests/test_plugin.py", line 205, in test_makedirs_with_existing_dirs
    p.makedirs()
  File "/tmp/tarmac/branch.Edc9Te/snapcraft/plugin.py", line 133, in makedirs
    os.makedirs(d, exists_ok=True)
TypeError: makedirs() got an unexpected keyword argument 'exists_ok'

======================================================================
ERROR: test_is_dirty (snapcraft.tests.test_plugin.PluginTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/tmp/tarmac/branch.Edc9Te/snapcraft/tests/test_plugin.py", line 61, in test_is_dirty
    p.pull()
  File "/tmp/tarmac/branch.Edc9Te/snapcraft/plugin.py", line 165, in pull
    self.makedirs()
  File "/tmp/tarmac/branch.Edc9Te/snapcraft/plugin.py", line 133, in makedirs
    os.makedirs(d, exists_ok=True)
TypeError: makedirs() got an unexpected keyword argument 'exists_ok'

======================================================================
ERROR: test_is_dirty (snapcraft.tests.test_plugin.PluginTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/usr/lib/python3.4/unittest/case.py", line 57, in testPartExecutor
    yield
  File "/usr/lib/python3.4/unittest/case.py", line 615, in doCleanups
    function(*args, **kwargs)
FileNotFoundError: [Errno 2] No such file or directory: '/tmp/tmpjmz3ufkk'

----------------------------------------------------------------------
Ran 38 tests in 0.698s

FAILED (errors=4)

[lp:~elopio/snapcraft/makedirs]

121. By Leo Arias on 2015-08-05

Fixed typo on the exist argument.

[Leo Arias (elopio)]

Sorry, a typo. It's exist_ok instead of exists_ok. I must have done something stupid when running the tests after the last push.

=== modified file 'debian/control'
--- debian/control	2015-08-04 21:37:07 +0000
+++ debian/control	2015-08-05 14:26:20 +0000
@@ -11,7 +11,7 @@
                pep8,
                plainbox,
                pyflakes,
-               python3,
+               python3 (>= 3.2),
                python3-apt,
                python3-fixtures,
                python3-setuptools,
 
=== modified file 'snapcraft/plugin.py'
--- snapcraft/plugin.py	2015-07-24 16:55:44 +0000
+++ snapcraft/plugin.py	2015-08-05 14:26:20 +0000
@@ -54,12 +54,13 @@
         self.deps = []
         self.plugin_name = name
 
-        self.sourcedir = os.path.join(os.getcwd(), "parts", part_name, "src")
-        self.builddir = os.path.join(os.getcwd(), "parts", part_name, "build")
-        self.installdir = os.path.join(os.getcwd(), "parts", part_name, "install")
-        self.stagedir = os.path.join(os.getcwd(), "stage")
-        self.snapdir = os.path.join(os.getcwd(), "snap")
-        self.statefile = os.path.join(os.getcwd(), "parts", part_name, "state")
+        parts_dir = os.path.join(os.getcwd(), 'parts')
+        self.sourcedir = os.path.join(parts_dir, part_name, 'src')
+        self.builddir = os.path.join(parts_dir, part_name, 'build')
+        self.installdir = os.path.join(parts_dir, part_name, 'install')
+        self.stagedir = os.path.join(os.getcwd(), 'stage')
+        self.snapdir = os.path.join(os.getcwd(), 'snap')
+        self.statefile = os.path.join(parts_dir, part_name, 'state')
 
         try:
             if load_config:
@@ -124,26 +125,12 @@
         return self.part_names[0]
 
     def makedirs(self):
-        try:
-            os.makedirs(self.sourcedir)
-        except FileExistsError:
-            pass
-        try:
-            os.makedirs(self.builddir)
-        except FileExistsError:
-            pass
-        try:
-            os.makedirs(self.installdir)
-        except FileExistsError:
-            pass
-        try:
-            os.makedirs(self.stagedir)
-        except FileExistsError:
-            pass
-        try:
-            os.makedirs(self.snapdir)
-        except FileExistsError:
-            pass
+        dirs = [
+            self.sourcedir, self.builddir, self.installdir, self.stagedir,
+            self.snapdir
+        ]
+        for d in dirs:
+            os.makedirs(d, exist_ok=True)
 
     def is_valid(self):
         return self.valid
 
=== modified file 'snapcraft/tests/test_plugin.py'
--- snapcraft/tests/test_plugin.py	2015-07-23 18:30:21 +0000
+++ snapcraft/tests/test_plugin.py	2015-08-05 14:26:20 +0000
@@ -32,27 +32,28 @@
 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 get_test_plugin(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)
+
+
+class PluginTestCase(tests.TestCase):
 
     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)
+        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()
+        p = get_test_plugin()
         p.statefile = tempfile.NamedTemporaryFile().name
         self.addCleanup(os.remove, p.statefile)
         p.code = Mock()
@@ -65,7 +66,7 @@
         self.assertFalse(p.code.pull.called)
 
     def test_collect_snap_files(self):
-        p = self.get_test_plugin()
+        p = get_test_plugin()
 
         tmpdirObject = tempfile.TemporaryDirectory()
         self.addCleanup(tmpdirObject.cleanup)
@@ -116,7 +117,7 @@
         fake_logger = fixtures.FakeLogger(level=logging.INFO)
         self.useFixture(fake_logger)
 
-        p = self.get_test_plugin()
+        p = get_test_plugin()
         p.notify_stage('test stage')
 
         self.assertEqual('test stage mock-part\n', fake_logger.output)
@@ -145,7 +146,7 @@
                 "mock", "mock-part", {}, load_config=False, load_code=True)
 
     def test_collect_snap_files_with_absolute_includes_must_raise_error(self):
-        p = self.get_test_plugin()
+        p = get_test_plugin()
         with self.assertRaises(plugin.PluginError) as raised:
             p.collect_snap_files(includes=['rel', '/abs/include'], excludes=[])
 
@@ -153,7 +154,7 @@
             "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()
+        p = get_test_plugin()
         with self.assertRaises(plugin.PluginError) as raised:
             p.collect_snap_files(includes=[], excludes=['rel', '/abs/exclude'])
 
@@ -173,3 +174,34 @@
             'Unknown plugin: test_unexisting_name\n'
             'Could not load part test_unexisting_name\n',
             fake_logger.output)
+
+
+class PluginMakedirsTestCase(tests.TestCase):
+
+    scenarios = [
+        ('existing_dirs', {'make_dirs': True}),
+        ('unexisting_dirs', {'make_dirs': False})
+    ]
+
+    def get_plugin_dirs(self, part_name):
+        parts_dir = os.path.join(self.path, 'parts')
+        return [
+            os.path.join(parts_dir, part_name, 'src'),
+            os.path.join(parts_dir, part_name, 'build'),
+            os.path.join(parts_dir, part_name, 'install'),
+            os.path.join(self.path, 'stage'),
+            os.path.join(self.path, 'snap')
+        ]
+
+    def test_makedirs_with_existing_dirs(self):
+        part_name = 'test_part'
+        dirs = self.get_plugin_dirs(part_name)
+        if self.make_dirs:
+            os.makedirs(os.path.join('parts', part_name))
+            for d in dirs:
+                os.mkdir(d)
+
+        p = get_test_plugin(part_name=part_name)
+        p.makedirs()
+        for d in dirs:
+            self.assertTrue(os.path.exists(d), '{} does not exist'.format(d))