Merge lp:~simpoir/landscape-client/fix_tempfile_cleaning into lp:~landscape/landscape-client/trunk

[Simon Poirier]

This branches does some cleanup in test tempfiles and fixes their broken
removal in test helpers.

This branches does some cleanup in test tempfiles and fixes their broken
removal in test helpers.

Testing instructions:

export TMP=$(mktemp -d)
trial -j8 landscape
ls $TMP

[🤖 Landscape Builder (landscape-builder)]

review: Abstain (executing tests)

[🤖 Landscape Builder (landscape-builder)]

Command: TRIAL_ARGS=-j4 make check
Result: Success
Revno: 918
Branch: lp:~simpoir/landscape-client/fix_tempfile_cleaning
Jenkins: https://ci.lscape.net/job/latch-test/7857/

review: Approve (test results)

[Benji York (benji)]

Other than the inline, preexisting trivial, this branch looks good.

review: Approve

[Chris Glass (tribaal)]

Nice! +1

(agree with Benji's point FWIW)

review: Approve

[lp:~simpoir/landscape-client/fix_tempfile_cleaning]

919. By Simon Poirier on 2016-08-19

address review nitpick

[🤖 Landscape Builder (landscape-builder)]

review: Abstain (executing tests)

[🤖 Landscape Builder (landscape-builder)]

Command: TRIAL_ARGS=-j4 make check
Result: Success
Revno: 919
Branch: lp:~simpoir/landscape-client/fix_tempfile_cleaning
Jenkins: https://ci.lscape.net/job/latch-test/7889/

review: Approve (test results)

=== modified file 'landscape/broker/tests/test_store.py'
--- landscape/broker/tests/test_store.py	2016-06-15 23:00:54 +0000
+++ landscape/broker/tests/test_store.py	2016-08-19 18:33:45 +0000
@@ -1,5 +1,3 @@
-import tempfile
-import shutil
 import os
 import mock
 
@@ -14,8 +12,8 @@
 
     def setUp(self):
         super(MessageStoreTest, self).setUp()
-        self.temp_dir = tempfile.mkdtemp()
-        self.persist_filename = tempfile.mktemp()
+        self.temp_dir = self.makeDir()
+        self.persist_filename = self.makeFile()
         self.store = self.create_store()
 
     def create_store(self):
@@ -29,12 +27,6 @@
         store.add_schema(Message("resynchronize", {}))
         return store
 
-    def tearDown(self):
-        super(MessageStoreTest, self).tearDown()
-        shutil.rmtree(self.temp_dir)
-        if os.path.isfile(self.persist_filename):
-            os.unlink(self.persist_filename)
-
     def test_get_set_sequence(self):
         self.assertEqual(self.store.get_sequence(), 0)
         self.store.set_sequence(3)

=== modified file 'landscape/lib/tests/test_persist.py'
--- landscape/lib/tests/test_persist.py	2013-04-01 17:53:37 +0000
+++ landscape/lib/tests/test_persist.py	2016-08-19 18:33:45 +0000
@@ -324,7 +324,7 @@
         for path in self.set_result:
             self.persist.set(path, self.set_result[path])
 
-        filename = self.makeFile()
+        filename = self.makePersistFile()
         self.persist.save(filename)
 
         persist = self.build_persist()
@@ -335,7 +335,7 @@
                          self.format(result, self.set_result))
 
     def test_save_on_unexistent_dir(self):
-        dirname = self.makeFile()
+        dirname = self.makePersistFile()
         filename = os.path.join(dirname, "foobar")
 
         self.assertFalse(os.path.exists(dirname))
@@ -355,7 +355,7 @@
         Persist can be constructed with a filename, and Persist.save with no
         arguments will write to that filename.
         """
-        filename = self.makeFile()
+        filename = self.makePersistFile()
         persist = self.build_persist(filename=filename)
         self.assertFalse(os.path.exists(filename))
         persist.save()
@@ -373,7 +373,7 @@
         If a Persist is created with a default filename, and the filename
         exists, it will be loaded.
         """
-        filename = self.makeFile()
+        filename = self.makePersistFile()
         persist = self.build_persist(filename=filename)
         persist.set("foo", "bar")
         persist.save()
@@ -394,7 +394,7 @@
         self.assertEqual(persist.get("a"), 1)
 
     def test_load_empty_files_wont_break(self):
-        filename = self.makeFile("")
+        filename = self.makePersistFile("")
         self.persist.load(filename)
 
     def test_load_empty_files_restore_backup(self):
@@ -402,7 +402,7 @@
         If the current file is empty, it tries to load the old one if it
         exists.
         """
-        filename = self.makeFile("")
+        filename = self.makePersistFile("")
         filename_old = filename + ".old"
 
         self.persist.set("a", 1)
@@ -425,7 +425,7 @@
         If the file doesn't exist, it tries to load the old one if present and
         valid.
         """
-        filename = self.makeFile("")
+        filename = self.makePersistFile("")
         filename_old = filename + ".old"
         os.unlink(filename)
 

=== modified file 'landscape/manager/aptsources.py'
--- landscape/manager/aptsources.py	2013-04-01 09:25:31 +0000
+++ landscape/manager/aptsources.py	2016-08-19 18:33:45 +0000
@@ -103,7 +103,12 @@
         fd, path = tempfile.mkstemp()
         os.close(fd)
         new_sources = file(path, "w")
-        for line in file(self.SOURCES_LIST):
+        try:
+            source_file = open(self.SOURCES_LIST)
+        except:
+            os.unlink(path)
+            raise
+        for line in source_file:
             stripped_line = line.strip()
             if not stripped_line or stripped_line.startswith("#"):
                 new_sources.write(line)

=== modified file 'landscape/manager/scriptexecution.py'
--- landscape/manager/scriptexecution.py	2013-05-17 22:32:49 +0000
+++ landscape/manager/scriptexecution.py	2016-08-19 18:33:45 +0000
@@ -205,9 +205,9 @@
             return self._respond(FAILED, str(failure), opid)
 
     @inlineCallbacks
-    def _save_attachments(self, attachments, uid, gid, computer_id):
+    def _save_attachments(self, attachments, uid, gid, computer_id, env):
         root_path = self.registry.config.url.rsplit("/", 1)[0] + "/attachment/"
-        attachment_dir = tempfile.mkdtemp()
+        env["LANDSCAPE_ATTACHMENTS"] = attachment_dir = tempfile.mkdtemp()
         headers = {"User-Agent": "landscape-client/%s" % VERSION,
                    "Content-Type": "application/octet-stream",
                    "X-Computer-ID": computer_id}
@@ -277,15 +277,12 @@
                                       "broker.bpickle"))
             persist = persist.root_at("registration")
             computer_id = persist.get("secure-id")
-            d = self._save_attachments(attachments, uid, gid, computer_id)
+            d = self._save_attachments(attachments, uid, gid, computer_id, env)
         else:
             d = succeed(None)
 
         def prepare_script(attachment_dir):
 
-            if attachment_dir is not None:
-                env["LANDSCAPE_ATTACHMENTS"] = attachment_dir
-
             return self._run_script(
                 filename, uid, gid, path, env, time_limit)
 

=== modified file 'landscape/monitor/tests/test_cephusage.py'
--- landscape/monitor/tests/test_cephusage.py	2016-06-15 16:11:17 +0000
+++ landscape/monitor/tests/test_cephusage.py	2016-08-19 18:33:45 +0000
@@ -1,5 +1,4 @@
 import mock
-import tempfile
 from landscape.lib.fs import touch_file
 from landscape.tests.helpers import LandscapeTest, MonitorHelper
 from landscape.monitor.cephusage import CephUsage
@@ -110,8 +109,7 @@
         plugin._ceph_config = None
         self.assertFalse(plugin._should_run())
 
-   
-    @mock.patch("logging.info") 
+    @mock.patch("logging.info")
     def test_wb_should_run_no_rados(self, logging):
         """
         If the Rados library cannot be imported (CephUsage._has_rados is False)
@@ -131,7 +129,7 @@
         """
         plugin = CephUsage()
         plugin._has_rados = True
-        plugin._ceph_config = tempfile.mktemp()
+        plugin._ceph_config = self.makeFile()
         touch_file(plugin._ceph_config)
         self.assertTrue(plugin._should_run())
 

=== modified file 'landscape/monitor/tests/test_usermonitor.py'
--- landscape/monitor/tests/test_usermonitor.py	2016-06-15 20:34:35 +0000
+++ landscape/monitor/tests/test_usermonitor.py	2016-08-19 18:33:45 +0000
@@ -1,5 +1,4 @@
 import os
-import tempfile
 
 from mock import Mock, ANY
 from twisted.internet.defer import fail
@@ -359,7 +358,7 @@
 
         # Create the (temporary, test) user update flag file.
         landscape.monitor.usermonitor.USER_UPDATE_FLAG_FILE = \
-            update_flag_file = tempfile.mkstemp()[1]
+            update_flag_file = self.makeFile("")
         self.addCleanup(lambda: os.remove(update_flag_file))
 
         # Trigger a detect changes.
@@ -381,7 +380,7 @@
 
         # Create the (temporary, test) user update flag file.
         landscape.monitor.usermonitor.USER_UPDATE_FLAG_FILE = \
-            update_flag_file = tempfile.mkstemp()[1]
+            update_flag_file = self.makeFile("")
 
         self.broker_service.message_store.set_accepted_types(["users"])
         self.provider.users = [("jdoe", "x", 1000, 1000, "JD,,,,",

=== modified file 'landscape/package/tests/test_changer.py'
--- landscape/package/tests/test_changer.py	2016-06-16 21:13:41 +0000
+++ landscape/package/tests/test_changer.py	2016-08-19 18:33:45 +0000
@@ -881,7 +881,7 @@
         from previous runs.
         """
         existing_deb_path = os.path.join(self.config.binaries_path, "123.deb")
-        self.makeFile(basename=existing_deb_path, content="foo")
+        self.makeFile(path=existing_deb_path, content="foo")
         self.changer.init_channels([])
         self.assertFalse(os.path.exists(existing_deb_path))
 

=== modified file 'landscape/tests/helpers.py'
--- landscape/tests/helpers.py	2016-06-17 00:17:50 +0000
+++ landscape/tests/helpers.py	2016-08-19 18:33:45 +0000
@@ -94,7 +94,7 @@
                                         "%s" % (diff, extra))
 
 
-class LandscapeTest(MessageTestCase, HelperTestCase, TestCase):
+class LandscapeTest(MessageTestCase, HelperTestCase, TestCase, ):
 
     def setUp(self):
         self._old_config_filenames = BaseConfiguration.default_config_filenames
@@ -227,16 +227,12 @@
 
         The file is removed after the test runs.
         """
-        if path is not None:
-            self.addCleanup(shutil.rmtree, path, ignore_errors=True)
-        elif basename is not None:
+        if basename is not None:
             if dirname is None:
                 dirname = tempfile.mkdtemp()
-                self.addCleanup(shutil.rmtree, dirname, ignore_errors=True)
             path = os.path.join(dirname, basename)
-        else:
+        elif path is None:
             fd, path = tempfile.mkstemp(suffix, prefix, dirname)
-            self.addCleanup(shutil.rmtree, path, ignore_errors=True)
             os.close(fd)
             if content is None:
                 os.unlink(path)
@@ -244,8 +240,22 @@
             file = open(path, "w")
             file.write(content)
             file.close()
+        self.addCleanup(self._clean_file, path)
         return path
 
+    def _clean_file(self, path):
+        """Try to remove a filesystem path, whether it's a directory or file.
+
+        @param path: the path to remove
+        """
+        try:
+            if os.path.isdir(path):
+                shutil.rmtree(path)
+            else:
+                os.unlink(path)
+        except OSError:
+            pass
+
     def makeDir(self, suffix="", prefix="tmp", dirname=None, path=None):
         """Create a temporary directory and return the path to it.
 
@@ -259,7 +269,7 @@
             os.makedirs(path)
         else:
             path = tempfile.mkdtemp(suffix, prefix, dirname)
-        self.addCleanup(shutil.rmtree, path, ignore_errors=True)
+        self.addCleanup(self._clean_file, path)
         return path
 
 

=== modified file 'landscape/tests/test_service.py'
--- landscape/tests/test_service.py	2016-06-15 17:07:39 +0000
+++ landscape/tests/test_service.py	2016-08-19 18:33:45 +0000
@@ -40,7 +40,7 @@
         """
 
         class PersistService(TestService):
-            persist_filename = self.makeFile(content="")
+            persist_filename = self.makePersistFile(content="")
 
         service = PersistService(self.config)
         self.assertEqual(service.persist.filename, service.persist_filename)

=== modified file 'landscape/tests/test_watchdog.py'
--- landscape/tests/test_watchdog.py	2016-06-16 23:27:52 +0000
+++ landscape/tests/test_watchdog.py	2016-08-19 18:33:45 +0000
@@ -1305,7 +1305,7 @@
             "landscape", None, os.getuid(), None, None, None, None)
         reactor = FakeReactor()
         with mock.patch("landscape.watchdog.pwd", new=self.fake_pwd):
-            run(["--log-dir", self.makeFile()], reactor=reactor)
+            run(["--log-dir", self.makeDir()], reactor=reactor)
         self.assertTrue(reactor.running)
 
     def test_no_landscape_user(self):
@@ -1328,7 +1328,7 @@
 
         reactor = FakeReactor()
         with mock.patch("landscape.watchdog.pwd", new=self.fake_pwd):
-            run(["--log-dir", self.makeFile()], reactor=reactor)
+            run(["--log-dir", self.makeDir()], reactor=reactor)
         self.assertNotIn("DEBIAN_YO", os.environ)
         self.assertNotIn("DEBCONF_YO", os.environ)
         self.assertNotIn("LANDSCAPE_ATTACHMENTS", os.environ)