cloud-init-dev team mailing list archive

[Scott Moser <ssmoser2+ubuntu@xxxxxxxxx>]

Scott Moser has proposed merging ~smoser/cloud-init:fix/fix-unittest-use-of-subp into cloud-init:master.

Commit message:
tests: Disallow use of util.subp except for where needed.

In many cases, cloud-init uses 'util.subp' to run a subprocess.
This is not really desirable in our unit tests as it makes the tests
dependent upon existance of those utilities.

The change here is to modify the base test case class (CiTestCase) to
raise exception any time subp is called.  Then, fix all callers.
For cases where subp is necessary or actually desired, we can use it
via 
  a.) context hander CiTestCase.allow_subp(value)
  b.) class level self.allowed_subp = value

Both cases the value is a list of acceptable executable names that
will be called (essentially argv[0]).


Requested reviews:
  cloud-init commiters (cloud-init-dev)

For more details, see:
https://code.launchpad.net/~smoser/cloud-init/+git/cloud-init/+merge/354001

see commit message
-- 
Your team cloud-init commiters is requested to review the proposed merge of ~smoser/cloud-init:fix/fix-unittest-use-of-subp into cloud-init:master.

diff --git a/cloudinit/analyze/tests/test_dump.py b/cloudinit/analyze/tests/test_dump.py
index f4c4284..48ff2f7 100644
--- a/cloudinit/analyze/tests/test_dump.py
+++ b/cloudinit/analyze/tests/test_dump.py
@@ -2,11 +2,12 @@
 
 from datetime import datetime
 from textwrap import dedent
+import mock
 
 from cloudinit.analyze.dump import (
     dump_events, parse_ci_logline, parse_timestamp)
-from cloudinit.util import subp, write_file
-from cloudinit.tests.helpers import CiTestCase
+from cloudinit.util import which, write_file
+from cloudinit.tests.helpers import CiTestCase, skipIf
 
 
 class TestParseTimestamp(CiTestCase):
@@ -15,21 +16,9 @@ class TestParseTimestamp(CiTestCase):
         """Logs with cloud-init detailed formats will be properly parsed."""
         trusty_fmt = '%Y-%m-%d %H:%M:%S,%f'
         trusty_stamp = '2016-09-12 14:39:20,839'
-
-        parsed = parse_timestamp(trusty_stamp)
-
-        # convert ourselves
         dt = datetime.strptime(trusty_stamp, trusty_fmt)
-        expected = float(dt.strftime('%s.%f'))
-
-        # use date(1)
-        out, _err = subp(['date', '+%s.%3N', '-d', trusty_stamp])
-        timestamp = out.strip()
-        date_ts = float(timestamp)
-
-        self.assertEqual(expected, parsed)
-        self.assertEqual(expected, date_ts)
-        self.assertEqual(date_ts, parsed)
+        self.assertEqual(
+            float(dt.strftime('%s.%f')), parse_timestamp(trusty_stamp))
 
     def test_parse_timestamp_handles_syslog_adding_year(self):
         """Syslog timestamps lack a year. Add year and properly parse."""
@@ -39,17 +28,9 @@ class TestParseTimestamp(CiTestCase):
         # convert stamp ourselves by adding the missing year value
         year = datetime.now().year
         dt = datetime.strptime(syslog_stamp + " " + str(year), syslog_fmt)
-        expected = float(dt.strftime('%s.%f'))
-        parsed = parse_timestamp(syslog_stamp)
-
-        # use date(1)
-        out, _ = subp(['date', '+%s.%3N', '-d', syslog_stamp])
-        timestamp = out.strip()
-        date_ts = float(timestamp)
-
-        self.assertEqual(expected, parsed)
-        self.assertEqual(expected, date_ts)
-        self.assertEqual(date_ts, parsed)
+        self.assertEqual(
+            float(dt.strftime('%s.%f')),
+            parse_timestamp(syslog_stamp))
 
     def test_parse_timestamp_handles_journalctl_format_adding_year(self):
         """Journalctl precise timestamps lack a year. Add year and parse."""
@@ -59,37 +40,22 @@ class TestParseTimestamp(CiTestCase):
         # convert stamp ourselves by adding the missing year value
         year = datetime.now().year
         dt = datetime.strptime(journal_stamp + " " + str(year), journal_fmt)
-        expected = float(dt.strftime('%s.%f'))
-        parsed = parse_timestamp(journal_stamp)
-
-        # use date(1)
-        out, _ = subp(['date', '+%s.%6N', '-d', journal_stamp])
-        timestamp = out.strip()
-        date_ts = float(timestamp)
-
-        self.assertEqual(expected, parsed)
-        self.assertEqual(expected, date_ts)
-        self.assertEqual(date_ts, parsed)
+        self.assertEqual(
+            float(dt.strftime('%s.%f')), parse_timestamp(journal_stamp))
 
+    @skipIf(not which("date"), "'date' command not available.")
     def test_parse_unexpected_timestamp_format_with_date_command(self):
-        """Dump sends unexpected timestamp formats to data for processing."""
+        """Dump sends unexpected timestamp formats to date for processing."""
         new_fmt = '%H:%M %m/%d %Y'
         new_stamp = '17:15 08/08'
-
         # convert stamp ourselves by adding the missing year value
         year = datetime.now().year
         dt = datetime.strptime(new_stamp + " " + str(year), new_fmt)
-        expected = float(dt.strftime('%s.%f'))
-        parsed = parse_timestamp(new_stamp)
 
         # use date(1)
-        out, _ = subp(['date', '+%s.%6N', '-d', new_stamp])
-        timestamp = out.strip()
-        date_ts = float(timestamp)
-
-        self.assertEqual(expected, parsed)
-        self.assertEqual(expected, date_ts)
-        self.assertEqual(date_ts, parsed)
+        with self.allow_subp(["date"]):
+            self.assertEqual(
+                float(dt.strftime('%s.%f')), parse_timestamp(new_stamp))
 
 
 class TestParseCILogLine(CiTestCase):
@@ -185,10 +151,13 @@ class TestDumpEvents(CiTestCase):
         self.assertEqual(expected_events, events)
         self.assertEqual(expected_data, data)
 
-    def test_dump_events_with_cisource(self):
+    @mock.patch("cloudinit.analyze.dump.parse_timestamp_from_date")
+    def test_dump_events_with_cisource(self, m_parse_from_date):
         """Cisource file is read and parsed into a tuple of events and data."""
         tmpfile = self.tmp_path('logfile')
         write_file(tmpfile, SAMPLE_LOGS)
+        m_parse_from_date.return_value = 1472594005.972
+
         events, data = dump_events(cisource=open(tmpfile))
         year = datetime.now().year
         dt1 = datetime.strptime(
@@ -208,3 +177,5 @@ class TestDumpEvents(CiTestCase):
             'timestamp': 1472594005.972}]
         self.assertEqual(expected_events, events)
         self.assertEqual(SAMPLE_LOGS.splitlines(), [d.strip() for d in data])
+        m_parse_from_date.assert_has_calls(
+            [mock.call("2016-08-30 21:53:25.972325+00:00")])
diff --git a/cloudinit/cmd/tests/test_status.py b/cloudinit/cmd/tests/test_status.py
index 37a8993..aded858 100644
--- a/cloudinit/cmd/tests/test_status.py
+++ b/cloudinit/cmd/tests/test_status.py
@@ -39,7 +39,8 @@ class TestStatus(CiTestCase):
         ensure_file(self.disable_file)  # Create the ignored disable file
         (is_disabled, reason) = wrap_and_call(
             'cloudinit.cmd.status',
-            {'uses_systemd': False},
+            {'uses_systemd': False,
+             'get_cmdline': "root=/dev/my-root not-important"},
             status._is_cloudinit_disabled, self.disable_file, self.paths)
         self.assertFalse(
             is_disabled, 'expected enabled cloud-init on sysvinit')
@@ -50,7 +51,8 @@ class TestStatus(CiTestCase):
         ensure_file(self.disable_file)  # Create observed disable file
         (is_disabled, reason) = wrap_and_call(
             'cloudinit.cmd.status',
-            {'uses_systemd': True},
+            {'uses_systemd': True,
+             'get_cmdline': "root=/dev/my-root not-important"},
             status._is_cloudinit_disabled, self.disable_file, self.paths)
         self.assertTrue(is_disabled, 'expected disabled cloud-init')
         self.assertEqual(
diff --git a/cloudinit/config/tests/test_snap.py b/cloudinit/config/tests/test_snap.py
index 34c80f1..3c47289 100644
--- a/cloudinit/config/tests/test_snap.py
+++ b/cloudinit/config/tests/test_snap.py
@@ -162,6 +162,7 @@ class TestAddAssertions(CiTestCase):
 class TestRunCommands(CiTestCase):
 
     with_logs = True
+    allowed_subp = [CiTestCase.SUBP_SHELL_TRUE]
 
     def setUp(self):
         super(TestRunCommands, self).setUp()
@@ -424,8 +425,10 @@ class TestHandle(CiTestCase):
             'snap': {'commands': ['echo "HI" >> %s' % outfile,
                                   'echo "MOM" >> %s' % outfile]}}
         mock_path = 'cloudinit.config.cc_snap.sys.stderr'
-        with mock.patch(mock_path, new_callable=StringIO):
-            handle('snap', cfg=cfg, cloud=None, log=self.logger, args=None)
+        with self.allow_subp([CiTestCase.SUBP_SHELL_TRUE]):
+            with mock.patch(mock_path, new_callable=StringIO):
+                handle('snap', cfg=cfg, cloud=None, log=self.logger, args=None)
+
         self.assertEqual('HI\nMOM\n', util.load_file(outfile))
 
     @mock.patch('cloudinit.config.cc_snap.util.subp')
diff --git a/cloudinit/config/tests/test_ubuntu_advantage.py b/cloudinit/config/tests/test_ubuntu_advantage.py
index f1beeff..b7cf9be 100644
--- a/cloudinit/config/tests/test_ubuntu_advantage.py
+++ b/cloudinit/config/tests/test_ubuntu_advantage.py
@@ -23,6 +23,7 @@ class FakeCloud(object):
 class TestRunCommands(CiTestCase):
 
     with_logs = True
+    allowed_subp = [CiTestCase.SUBP_SHELL_TRUE]
 
     def setUp(self):
         super(TestRunCommands, self).setUp()
@@ -234,8 +235,10 @@ class TestHandle(CiTestCase):
             'ubuntu-advantage': {'commands': ['echo "HI" >> %s' % outfile,
                                               'echo "MOM" >> %s' % outfile]}}
         mock_path = '%s.sys.stderr' % MPATH
-        with mock.patch(mock_path, new_callable=StringIO):
-            handle('nomatter', cfg=cfg, cloud=None, log=self.logger, args=None)
+        with self.allow_subp([CiTestCase.SUBP_SHELL_TRUE]):
+            with mock.patch(mock_path, new_callable=StringIO):
+                handle('nomatter', cfg=cfg, cloud=None, log=self.logger,
+                       args=None)
         self.assertEqual('HI\nMOM\n', util.load_file(outfile))
 
 
diff --git a/cloudinit/net/tests/test_init.py b/cloudinit/net/tests/test_init.py
index 5c017d1..8b444f1 100644
--- a/cloudinit/net/tests/test_init.py
+++ b/cloudinit/net/tests/test_init.py
@@ -199,6 +199,8 @@ class TestGenerateFallbackConfig(CiTestCase):
         self.sysdir = self.tmp_dir() + '/'
         self.m_sys_path.return_value = self.sysdir
         self.addCleanup(sys_mock.stop)
+        self.add_patch('cloudinit.net.util.is_container', 'm_is_container',
+                       return_value=False)
         self.add_patch('cloudinit.net.util.udevadm_settle', 'm_settle')
 
     def test_generate_fallback_finds_connected_eth_with_mac(self):
diff --git a/cloudinit/sources/DataSourceAltCloud.py b/cloudinit/sources/DataSourceAltCloud.py
index 24fd65f..8dc63dc 100644
--- a/cloudinit/sources/DataSourceAltCloud.py
+++ b/cloudinit/sources/DataSourceAltCloud.py
@@ -181,27 +181,21 @@ class DataSourceAltCloud(sources.DataSource):
 
         # modprobe floppy
         try:
-            cmd = CMD_PROBE_FLOPPY
-            (cmd_out, _err) = util.subp(cmd)
-            LOG.debug('Command: %s\nOutput%s', ' '.join(cmd), cmd_out)
+            modprobe_floppy()
         except ProcessExecutionError as e:
-            util.logexc(LOG, 'Failed command: %s\n%s', ' '.join(cmd), e)
+            util.logexc(LOG, 'Failed modprobe: %s', e)
             return False
         except OSError as e:
-            util.logexc(LOG, 'Failed command: %s\n%s', ' '.join(cmd), e)
+            util.logexc(LOG, 'No modprobe: %s', e)
             return False
 
         floppy_dev = '/dev/fd0'
 
         # udevadm settle for floppy device
         try:
-            (cmd_out, _err) = util.udevadm_settle(exists=floppy_dev, timeout=5)
-            LOG.debug('Command: %s\nOutput%s', ' '.join(cmd), cmd_out)
-        except ProcessExecutionError as e:
-            util.logexc(LOG, 'Failed command: %s\n%s', ' '.join(cmd), e)
-            return False
-        except OSError as e:
-            util.logexc(LOG, 'Failed command: %s\n%s', ' '.join(cmd), e)
+            util.udevadm_settle(exists=floppy_dev, timeout=5)
+        except (ProcessExecutionError, OSError) as e:
+            util.logexc(LOG, 'Failed udevadm_settle: %s\n', e)
             return False
 
         try:
@@ -258,6 +252,11 @@ class DataSourceAltCloud(sources.DataSource):
             return False
 
 
+def modprobe_floppy():
+    out, _err = util.subp(CMD_PROBE_FLOPPY)
+    LOG.debug('Command: %s\nOutput%s', ' '.join(CMD_PROBE_FLOPPY), out)
+
+
 # Used to match classes to dependencies
 # Source DataSourceAltCloud does not really depend on networking.
 # In the future 'dsmode' like behavior can be added to offer user
diff --git a/cloudinit/sources/DataSourceSmartOS.py b/cloudinit/sources/DataSourceSmartOS.py
index ad8cfb9..fa72fc6 100644
--- a/cloudinit/sources/DataSourceSmartOS.py
+++ b/cloudinit/sources/DataSourceSmartOS.py
@@ -683,6 +683,19 @@ def jmc_client_factory(
     raise ValueError("Unknown value for smartos_type: %s" % smartos_type)
 
 
+def identify_file(content_f):
+    cmd = ["file", "--brief", "--mime-type", content_f]
+    f_type = None
+    try:
+        (f_type, _err) = util.subp(cmd)
+        LOG.debug("script %s mime type is %s", content_f, f_type)
+    except util.ProcessExecutionError as e:
+        util.logexc(
+            LOG, ("Failed to identify script type for %s" % content_f, e))
+    return None if f_type is None else f_type.strip()
+
+
+
 def write_boot_content(content, content_f, link=None, shebang=False,
                        mode=0o400):
     """
@@ -715,18 +728,11 @@ def write_boot_content(content, content_f, link=None, shebang=False,
     util.write_file(content_f, content, mode=mode)
 
     if shebang and not content.startswith("#!"):
-        try:
-            cmd = ["file", "--brief", "--mime-type", content_f]
-            (f_type, _err) = util.subp(cmd)
-            LOG.debug("script %s mime type is %s", content_f, f_type)
-            if f_type.strip() == "text/plain":
-                new_content = "\n".join(["#!/bin/bash", content])
-                util.write_file(content_f, new_content, mode=mode)
-                LOG.debug("added shebang to file %s", content_f)
-
-        except Exception as e:
-            util.logexc(LOG, ("Failed to identify script type for %s" %
-                              content_f, e))
+        f_type = identify_file(content_f)
+        if f_type == "text/plain":
+            util.write_file(
+                content_f, "\n".join(["#!/bin/bash", content]), mode=mode)
+            LOG.debug("added shebang to file %s", content_f)
 
     if link:
         try:
diff --git a/cloudinit/tests/helpers.py b/cloudinit/tests/helpers.py
index 5bfe7fa..518a632 100644
--- a/cloudinit/tests/helpers.py
+++ b/cloudinit/tests/helpers.py
@@ -17,9 +17,9 @@ import six
 import unittest2
 
 try:
-    from contextlib import ExitStack
+    from contextlib import ExitStack, contextmanager
 except ImportError:
-    from contextlib2 import ExitStack
+    from contextlib2 import ExitStack, contextmanager
 
 try:
     from configparser import ConfigParser
@@ -31,6 +31,8 @@ from cloudinit.config.schema import (
 from cloudinit import helpers as ch
 from cloudinit import util
 
+_real_subp = util.subp
+
 # Used for skipping tests
 SkipTest = unittest2.SkipTest
 
@@ -140,6 +142,17 @@ class CiTestCase(TestCase):
     # Subclass overrides for specific test behavior
     # Whether or not a unit test needs logfile setup
     with_logs = False
+    allowed_subp = False
+    SUBP_SHELL_TRUE = "shell=true"
+
+    @contextmanager
+    def allow_subp(self, allowed_subp):
+        orig = self.allowed_subp
+        try:
+            self.allowed_subp = allowed_subp
+            yield
+        finally:
+            self.allowed_subp = orig
 
     def setUp(self):
         super(CiTestCase, self).setUp()
@@ -152,11 +165,41 @@ class CiTestCase(TestCase):
             handler.setFormatter(formatter)
             self.old_handlers = self.logger.handlers
             self.logger.handlers = [handler]
+        if self.allowed_subp is True:
+            util.subp = _real_subp
+        else:
+            util.subp = self._fake_subp
+
+    def _fake_subp(self, *args, **kwargs):
+        if 'args' in kwargs:
+            cmd = kwargs['args']
+        else:
+            if isinstance(args[0], six.string_types):
+                cmd = args[0]
+            else:
+                cmd = args[0][0]
+        pass_through = False
+        if not isinstance(self.allowed_subp, (list, bool)):
+            raise TypeError("self.allowed_subp supports list or bool.")
+        if isinstance(self.allowed_subp, bool):
+            pass_through = self.allowed_subp
+        else:
+            pass_through = (
+                (cmd in self.allowed_subp) or
+                (self.SUBP_SHELL_TRUE in self.allowed_subp and
+                 kwargs.get('shell')))
+        if pass_through:
+            return _real_subp(*args, **kwargs)
+        raise Exception(
+            "called subp. set self.allowed_subp=True to allow\n subp(%s)" %
+            ', '.join([str(repr(a)) for a in args] +
+                      ["%s=%s" % (k, repr(v)) for k, v in kwargs.items()]))
 
     def tearDown(self):
         if self.with_logs:
             # Remove the handler we setup
             logging.getLogger().handlers = self.old_handlers
+        util.subp = _real_subp
         super(CiTestCase, self).tearDown()
 
     def tmp_dir(self, dir=None, cleanup=True):
@@ -188,6 +231,13 @@ class CiTestCase(TestCase):
         raise SystemExit(code)
 
 
+class TestMeOne(CiTestCase):
+    allowed_subp = False
+
+    def test_me(self):
+        print("hello, subp=%s" % util.subp)
+
+
 class ResourceUsingTestCase(CiTestCase):
 
     def setUp(self):
diff --git a/tests/unittests/test_datasource/test_altcloud.py b/tests/unittests/test_datasource/test_altcloud.py
index 3253f3a..81d8936 100644
--- a/tests/unittests/test_datasource/test_altcloud.py
+++ b/tests/unittests/test_datasource/test_altcloud.py
@@ -262,64 +262,55 @@ class TestUserDataRhevm(CiTestCase):
     '''
     Test to exercise method: DataSourceAltCloud.user_data_rhevm()
     '''
-    cmd_pass = ['true']
-    cmd_fail = ['false']
-    cmd_not_found = ['bogus bad command']
-
     def setUp(self):
         '''Set up.'''
         self.paths = helpers.Paths({'cloud_dir': '/tmp'})
-        self.mount_dir = tempfile.mkdtemp()
+        self.mount_dir = self.tmp_dir()
         _write_user_data_files(self.mount_dir, 'test user data')
-
-    def tearDown(self):
-        # Reset
-
-        _remove_user_data_files(self.mount_dir)
-
-        # Attempt to remove the temp dir ignoring errors
-        try:
-            shutil.rmtree(self.mount_dir)
-        except OSError:
-            pass
-
-        dsac.CLOUD_INFO_FILE = '/etc/sysconfig/cloud-info'
-        dsac.CMD_PROBE_FLOPPY = ['modprobe', 'floppy']
-        dsac.CMD_UDEVADM_SETTLE = ['udevadm', 'settle',
-                                   '--quiet', '--timeout=5']
+        self.add_patch(
+            'cloudinit.sources.DataSourceAltCloud.modprobe_floppy',
+            'm_modprobe_floppy', return_value=None)
+        self.add_patch(
+            'cloudinit.sources.DataSourceAltCloud.util.udevadm_settle',
+            'm_udevadm_settle', return_value=('',''))
+        self.add_patch(
+            'cloudinit.sources.DataSourceAltCloud.util.mount_cb',
+            'm_mount_cb')
 
     def test_mount_cb_fails(self):
         '''Test user_data_rhevm() where mount_cb fails.'''
 
-        dsac.CMD_PROBE_FLOPPY = self.cmd_pass
+        self.m_mount_cb.side_effect = util.MountFailedError("Failed Mount")
         dsrc = dsac.DataSourceAltCloud({}, None, self.paths)
         self.assertEqual(False, dsrc.user_data_rhevm())
 
     def test_modprobe_fails(self):
         '''Test user_data_rhevm() where modprobe fails.'''
 
-        dsac.CMD_PROBE_FLOPPY = self.cmd_fail
+        self.m_modprobe_floppy.side_effect = util.ProcessExecutionError(
+            "Failed modprobe")
         dsrc = dsac.DataSourceAltCloud({}, None, self.paths)
         self.assertEqual(False, dsrc.user_data_rhevm())
 
     def test_no_modprobe_cmd(self):
         '''Test user_data_rhevm() with no modprobe command.'''
 
-        dsac.CMD_PROBE_FLOPPY = self.cmd_not_found
+        self.m_modprobe_floppy.side_effect = OSError("No such file or dir")
         dsrc = dsac.DataSourceAltCloud({}, None, self.paths)
         self.assertEqual(False, dsrc.user_data_rhevm())
 
     def test_udevadm_fails(self):
         '''Test user_data_rhevm() where udevadm fails.'''
 
-        dsac.CMD_UDEVADM_SETTLE = self.cmd_fail
+        self.m_udevadm_settle.side_effect = util.ProcessExecutionError(
+            "Failed settle.")
         dsrc = dsac.DataSourceAltCloud({}, None, self.paths)
         self.assertEqual(False, dsrc.user_data_rhevm())
 
     def test_no_udevadm_cmd(self):
         '''Test user_data_rhevm() with no udevadm command.'''
 
-        dsac.CMD_UDEVADM_SETTLE = self.cmd_not_found
+        self.m_udevadm_settle.side_effect = OSError("No such file or dir")
         dsrc = dsac.DataSourceAltCloud({}, None, self.paths)
         self.assertEqual(False, dsrc.user_data_rhevm())
 
diff --git a/tests/unittests/test_datasource/test_cloudsigma.py b/tests/unittests/test_datasource/test_cloudsigma.py
index f6a59b6..380ad1b 100644
--- a/tests/unittests/test_datasource/test_cloudsigma.py
+++ b/tests/unittests/test_datasource/test_cloudsigma.py
@@ -42,6 +42,9 @@ class CepkoMock(Cepko):
 class DataSourceCloudSigmaTest(test_helpers.CiTestCase):
     def setUp(self):
         super(DataSourceCloudSigmaTest, self).setUp()
+        self.add_patch(
+            "cloudinit.sources.DataSourceCloudSigma.util.is_container",
+            "m_is_container", return_value=False)
         self.paths = helpers.Paths({'run_dir': self.tmp_dir()})
         self.datasource = DataSourceCloudSigma.DataSourceCloudSigma(
             "", "", paths=self.paths)
diff --git a/tests/unittests/test_datasource/test_configdrive.py b/tests/unittests/test_datasource/test_configdrive.py
index 68400f2..af633b5 100644
--- a/tests/unittests/test_datasource/test_configdrive.py
+++ b/tests/unittests/test_datasource/test_configdrive.py
@@ -224,6 +224,9 @@ class TestConfigDriveDataSource(CiTestCase):
 
     def setUp(self):
         super(TestConfigDriveDataSource, self).setUp()
+        self.add_patch(
+            "cloudinit.sources.DataSourceConfigDrive.util.find_devs_with",
+            "m_find_devs_with", return_value=[])
         self.tmp = self.tmp_dir()
 
     def test_ec2_metadata(self):
diff --git a/tests/unittests/test_datasource/test_nocloud.py b/tests/unittests/test_datasource/test_nocloud.py
index cdbd1e1..21931eb 100644
--- a/tests/unittests/test_datasource/test_nocloud.py
+++ b/tests/unittests/test_datasource/test_nocloud.py
@@ -25,6 +25,8 @@ class TestNoCloudDataSource(CiTestCase):
 
         self.mocks.enter_context(
             mock.patch.object(util, 'get_cmdline', return_value=self.cmdline))
+        self.mocks.enter_context(
+            mock.patch.object(util, 'read_dmi_data', return_value=None))
 
     def test_nocloud_seed_dir(self):
         md = {'instance-id': 'IID', 'dsmode': 'local'}
diff --git a/tests/unittests/test_datasource/test_opennebula.py b/tests/unittests/test_datasource/test_opennebula.py
index 36b4d77..6159101 100644
--- a/tests/unittests/test_datasource/test_opennebula.py
+++ b/tests/unittests/test_datasource/test_opennebula.py
@@ -43,6 +43,7 @@ DS_PATH = "cloudinit.sources.DataSourceOpenNebula"
 
 class TestOpenNebulaDataSource(CiTestCase):
     parsed_user = None
+    allowed_subp = ['bash']
 
     def setUp(self):
         super(TestOpenNebulaDataSource, self).setUp()
diff --git a/tests/unittests/test_datasource/test_ovf.py b/tests/unittests/test_datasource/test_ovf.py
index fc4eb36..9d52eb9 100644
--- a/tests/unittests/test_datasource/test_ovf.py
+++ b/tests/unittests/test_datasource/test_ovf.py
@@ -124,7 +124,9 @@ class TestDatasourceOVF(CiTestCase):
         ds = self.datasource(sys_cfg={}, distro={}, paths=paths)
         retcode = wrap_and_call(
             'cloudinit.sources.DataSourceOVF',
-            {'util.read_dmi_data': None},
+            {'util.read_dmi_data': None,
+             'transport_iso9660': (False, None, None),
+             'transport_vmware_guestd': (False, None, None)},
             ds.get_data)
         self.assertFalse(retcode, 'Expected False return from ds.get_data')
         self.assertIn(
@@ -138,7 +140,9 @@ class TestDatasourceOVF(CiTestCase):
             paths=paths)
         retcode = wrap_and_call(
             'cloudinit.sources.DataSourceOVF',
-            {'util.read_dmi_data': 'vmware'},
+            {'util.read_dmi_data': 'vmware',
+             'transport_iso9660': (False, None, None),
+             'transport_vmware_guestd': (False, None, None)},
             ds.get_data)
         self.assertFalse(retcode, 'Expected False return from ds.get_data')
         self.assertIn(
diff --git a/tests/unittests/test_datasource/test_smartos.py b/tests/unittests/test_datasource/test_smartos.py
index dca0b3d..4c48df7 100644
--- a/tests/unittests/test_datasource/test_smartos.py
+++ b/tests/unittests/test_datasource/test_smartos.py
@@ -20,10 +20,8 @@ import multiprocessing
 import os
 import os.path
 import re
-import shutil
 import signal
 import stat
-import tempfile
 import unittest2
 import uuid
 
@@ -31,15 +29,19 @@ from cloudinit import serial
 from cloudinit.sources import DataSourceSmartOS
 from cloudinit.sources.DataSourceSmartOS import (
     convert_smartos_network_data as convert_net,
-    SMARTOS_ENV_KVM, SERIAL_DEVICE, get_smartos_environ)
+    SMARTOS_ENV_KVM, SERIAL_DEVICE, get_smartos_environ,
+    identify_file)
 
 import six
 
 from cloudinit import helpers as c_helpers
-from cloudinit.util import (b64e, subp)
+from cloudinit.util import (
+    b64e, subp, ProcessExecutionError, which, write_file)
 
-from cloudinit.tests.helpers import mock, FilesystemMockingTestCase, TestCase
+from cloudinit.tests.helpers import (
+    CiTestCase, mock, FilesystemMockingTestCase, skipIf)
 
+DSMOS = 'cloudinit.sources.DataSourceSmartOS'
 SDC_NICS = json.loads("""
 [
     {
@@ -366,37 +368,34 @@ class PsuedoJoyentClient(object):
 
 
 class TestSmartOSDataSource(FilesystemMockingTestCase):
+    allowed_subp = True
+    jmc_cfact = None
+    get_smartos_environ = None
+
     def setUp(self):
         super(TestSmartOSDataSource, self).setUp()
 
-        dsmos = 'cloudinit.sources.DataSourceSmartOS'
-        patcher = mock.patch(dsmos + ".jmc_client_factory")
-        self.jmc_cfact = patcher.start()
-        self.addCleanup(patcher.stop)
-        patcher = mock.patch(dsmos + ".get_smartos_environ")
-        self.get_smartos_environ = patcher.start()
-        self.addCleanup(patcher.stop)
-
-        self.tmp = tempfile.mkdtemp()
-        self.addCleanup(shutil.rmtree, self.tmp)
-        self.paths = c_helpers.Paths(
-            {'cloud_dir': self.tmp, 'run_dir': self.tmp})
-
-        self.legacy_user_d = os.path.join(self.tmp, 'legacy_user_tmp')
+        self.add_patch(DSMOS + ".get_smartos_environ", "get_smartos_environ")
+        self.add_patch(DSMOS + ".jmc_client_factory", "jmc_cfact")
+        self.legacy_user_d = self.tmp_path('legacy_user_tmp')
         os.mkdir(self.legacy_user_d)
-
-        self.orig_lud = DataSourceSmartOS.LEGACY_USER_D
-        DataSourceSmartOS.LEGACY_USER_D = self.legacy_user_d
-
-    def tearDown(self):
-        DataSourceSmartOS.LEGACY_USER_D = self.orig_lud
-        super(TestSmartOSDataSource, self).tearDown()
+        self.add_patch(DSMOS + ".LEGACY_USER_D", "m_legacy_user_d",
+                       autospec=False, new=self.legacy_user_d)
+        self.add_patch(DSMOS + ".identify_file", "m_identify_file",
+                       return_value="text/plain")
 
     def _get_ds(self, mockdata=None, mode=DataSourceSmartOS.SMARTOS_ENV_KVM,
                 sys_cfg=None, ds_cfg=None):
         self.jmc_cfact.return_value = PsuedoJoyentClient(mockdata)
         self.get_smartos_environ.return_value = mode
 
+        tmpd = self.tmp_dir()
+        dirs = {'cloud_dir': self.tmp_path('cloud_dir', tmpd),
+                'run_dir': self.tmp_path('run_dir')}
+        for d in dirs.values():
+            os.mkdir(d)
+        paths = c_helpers.Paths(dirs)
+
         if sys_cfg is None:
             sys_cfg = {}
 
@@ -405,7 +404,7 @@ class TestSmartOSDataSource(FilesystemMockingTestCase):
             sys_cfg['datasource']['SmartOS'] = ds_cfg
 
         return DataSourceSmartOS.DataSourceSmartOS(
-            sys_cfg, distro=None, paths=self.paths)
+            sys_cfg, distro=None, paths=paths)
 
     def test_no_base64(self):
         ds_cfg = {'no_base64_decode': ['test_var1'], 'all_base': True}
@@ -493,6 +492,7 @@ class TestSmartOSDataSource(FilesystemMockingTestCase):
                          dsrc.metadata['user-script'])
 
         legacy_script_f = "%s/user-script" % self.legacy_user_d
+        print("legacy_script_f=%s" % legacy_script_f)
         self.assertTrue(os.path.exists(legacy_script_f))
         self.assertTrue(os.path.islink(legacy_script_f))
         user_script_perm = oct(os.stat(legacy_script_f)[stat.ST_MODE])[-3:]
@@ -640,6 +640,27 @@ class TestSmartOSDataSource(FilesystemMockingTestCase):
                          mydscfg['disk_aliases']['FOO'])
 
 
+class TestIdentifyFile(CiTestCase):
+    @skipIf(not which("file"), "command 'file' not available.")
+    def test_file_happy_path(self):
+        """Test file is available and functional on plain text."""
+        fname = self.tmp_path("myfile")
+        write_file(fname, "plain text content here\n")
+        with self.allow_subp(["file"]):
+            self.assertEqual("text/plain", identify_file(fname))
+
+    @mock.patch(DSMOS + ".util.subp")
+    def test_returns_none_on_error(self, m_subp):
+        """On 'file' execution error, None should be returned."""
+        m_subp.side_effect = ProcessExecutionError("FILE_FAILED", exit_code=99)
+        fname = self.tmp_path("myfile")
+        write_file(fname, "plain text content here\n")
+        self.assertEqual(None, identify_file(fname))
+        self.assertEqual(
+            [mock.call(["file", "--brief", "--mime-type", fname])],
+            m_subp.call_args_list)
+
+
 class ShortReader(object):
     """Implements a 'read' interface for bytes provided.
     much like io.BytesIO but the 'endbyte' acts as if EOF.
@@ -893,7 +914,7 @@ class TestJoyentMetadataClient(FilesystemMockingTestCase):
         self.assertEqual(client.list(), [])
 
 
-class TestNetworkConversion(TestCase):
+class TestNetworkConversion(CiTestCase):
     def test_convert_simple(self):
         expected = {
             'version': 1,
@@ -1058,7 +1079,7 @@ class TestNetworkConversion(TestCase):
                       "Only supported on KVM and bhyve guests under SmartOS")
 @unittest2.skipUnless(os.access(SERIAL_DEVICE, os.W_OK),
                       "Requires write access to " + SERIAL_DEVICE)
-class TestSerialConcurrency(TestCase):
+class TestSerialConcurrency(CiTestCase):
     """
        This class tests locking on an actual serial port, and as such can only
        be run in a kvm or bhyve guest running on a SmartOS host.  A test run on
@@ -1067,6 +1088,8 @@ class TestSerialConcurrency(TestCase):
        absence of proper locking multiple processes opening the same serial
        port can corrupt each others' exchanges with the metadata server.
     """
+    allowed_subp = True
+
     def setUp(self):
         self.mdata_proc = multiprocessing.Process(target=self.start_mdata_loop)
         self.mdata_proc.start()
diff --git a/tests/unittests/test_ds_identify.py b/tests/unittests/test_ds_identify.py
index e08e790..46778e9 100644
--- a/tests/unittests/test_ds_identify.py
+++ b/tests/unittests/test_ds_identify.py
@@ -89,6 +89,7 @@ CallReturn = namedtuple('CallReturn',
 
 class DsIdentifyBase(CiTestCase):
     dsid_path = os.path.realpath('tools/ds-identify')
+    allowed_subp = ['sh']
 
     def call(self, rootd=None, mocks=None, func="main", args=None, files=None,
              policy_dmi=DI_DEFAULT_POLICY,
diff --git a/tests/unittests/test_handler/test_handler_apt_source_v3.py b/tests/unittests/test_handler/test_handler_apt_source_v3.py
index 7a64c23..a81c67c 100644
--- a/tests/unittests/test_handler/test_handler_apt_source_v3.py
+++ b/tests/unittests/test_handler/test_handler_apt_source_v3.py
@@ -48,6 +48,10 @@ ADD_APT_REPO_MATCH = r"^[\w-]+:\w"
 
 TARGET = None
 
+MOCK_LSB_RELEASE_DATA = {
+    'id': 'Ubuntu', 'description': 'Ubuntu 18.04.1 LTS',
+    'release': '18.04', 'codename': 'bionic'}
+
 
 class TestAptSourceConfig(t_help.FilesystemMockingTestCase):
     """TestAptSourceConfig
@@ -64,6 +68,9 @@ class TestAptSourceConfig(t_help.FilesystemMockingTestCase):
         self.aptlistfile3 = os.path.join(self.tmp, "single-deb3.list")
         self.join = os.path.join
         self.matcher = re.compile(ADD_APT_REPO_MATCH).search
+        self.add_patch(
+            'cloudinit.config.cc_apt_configure.util.lsb_release',
+            'm_lsb_release', return_value=MOCK_LSB_RELEASE_DATA.copy())
 
     @staticmethod
     def _add_apt_sources(*args, **kwargs):
@@ -76,7 +83,7 @@ class TestAptSourceConfig(t_help.FilesystemMockingTestCase):
         Get the most basic default mrror and release info to be used in tests
         """
         params = {}
-        params['RELEASE'] = util.lsb_release()['codename']
+        params['RELEASE'] = MOCK_LSB_RELEASE_DATA['release']
         arch = 'amd64'
         params['MIRROR'] = cc_apt_configure.\
             get_default_mirrors(arch)["PRIMARY"]
@@ -464,7 +471,7 @@ class TestAptSourceConfig(t_help.FilesystemMockingTestCase):
                              'uri':
                              'http://testsec.ubuntu.com/%s/' % component}]}
         post = ("%s_dists_%s-updates_InRelease" %
-                (component, util.lsb_release()['codename']))
+                (component, MOCK_LSB_RELEASE_DATA['codename']))
         fromfn = ("%s/%s_%s" % (pre, archive, post))
         tofn = ("%s/test.ubuntu.com_%s" % (pre, post))
 
diff --git a/tests/unittests/test_handler/test_handler_bootcmd.py b/tests/unittests/test_handler/test_handler_bootcmd.py
index b137526..9672fdb 100644
--- a/tests/unittests/test_handler/test_handler_bootcmd.py
+++ b/tests/unittests/test_handler/test_handler_bootcmd.py
@@ -30,6 +30,7 @@ class FakeExtendedTempFile(object):
 class TestBootcmd(CiTestCase):
 
     with_logs = True
+    allowed_subp = ['/bin/sh']
 
     _etmpfile_path = ('cloudinit.config.cc_bootcmd.temp_utils.'
                       'ExtendedTemporaryFile')
diff --git a/tests/unittests/test_handler/test_handler_chef.py b/tests/unittests/test_handler/test_handler_chef.py
index f4bbd66..b16532e 100644
--- a/tests/unittests/test_handler/test_handler_chef.py
+++ b/tests/unittests/test_handler/test_handler_chef.py
@@ -36,13 +36,21 @@ class TestInstallChefOmnibus(HttprettyTestCase):
 
     @mock.patch("cloudinit.config.cc_chef.OMNIBUS_URL", OMNIBUS_URL_HTTP)
     def test_install_chef_from_omnibus_runs_chef_url_content(self):
-        """install_chef_from_omnibus runs downloaded OMNIBUS_URL as script."""
-        chef_outfile = self.tmp_path('chef.out', self.new_root)
-        response = '#!/bin/bash\necho "Hi Mom" > {0}'.format(chef_outfile)
+        """install_chef_from_omnibus calls subp_blob_in_tempfile."""
+        response = b'#!/bin/bash\necho "Hi Mom"'
         httpretty.register_uri(
             httpretty.GET, cc_chef.OMNIBUS_URL, body=response, status=200)
-        cc_chef.install_chef_from_omnibus()
-        self.assertEqual('Hi Mom\n', util.load_file(chef_outfile))
+        ret = (None, None)  # stdout, stderr but capture=False
+
+        with mock.patch("cloudinit.config.cc_chef.util.subp_blob_in_tempfile",
+                        return_value=ret) as m_subp_blob:
+            cc_chef.install_chef_from_omnibus()
+        # admittedly whitebox, but assuming subp_blob_in_tempfile works
+        # this should be fine.
+        self.assertEqual(
+            [mock.call(blob=response, args=[], basename='chef-omnibus-install',
+                       capture=False)],
+            m_subp_blob.call_args_list)
 
     @mock.patch('cloudinit.config.cc_chef.url_helper.readurl')
     @mock.patch('cloudinit.config.cc_chef.util.subp_blob_in_tempfile')
diff --git a/tests/unittests/test_handler/test_handler_resizefs.py b/tests/unittests/test_handler/test_handler_resizefs.py
index f92175f..feca56c 100644
--- a/tests/unittests/test_handler/test_handler_resizefs.py
+++ b/tests/unittests/test_handler/test_handler_resizefs.py
@@ -150,10 +150,12 @@ class TestResizefs(CiTestCase):
         self.assertEqual(('growfs', '-y', devpth),
                          _resize_ufs(mount_point, devpth))
 
+    @mock.patch('cloudinit.util.is_container', return_value=False)
     @mock.patch('cloudinit.util.get_mount_info')
     @mock.patch('cloudinit.util.get_device_info_from_zpool')
     @mock.patch('cloudinit.util.parse_mount')
-    def test_handle_zfs_root(self, mount_info, zpool_info, parse_mount):
+    def test_handle_zfs_root(self, mount_info, zpool_info, parse_mount,
+                             is_container):
         devpth = 'vmzroot/ROOT/freebsd'
         disk = 'gpt/system'
         fs_type = 'zfs'
@@ -354,8 +356,10 @@ class TestMaybeGetDevicePathAsWritableBlock(CiTestCase):
             ('btrfs', 'filesystem', 'resize', 'max', '/'),
             _resize_btrfs("/", "/dev/sda1"))
 
+    @mock.patch('cloudinit.util.is_container', return_value=True)
     @mock.patch('cloudinit.util.is_FreeBSD')
-    def test_maybe_get_writable_device_path_zfs_freebsd(self, freebsd):
+    def test_maybe_get_writable_device_path_zfs_freebsd(self, freebsd,
+                                                        m_is_container):
         freebsd.return_value = True
         info = 'dev=gpt/system mnt_point=/ path=/'
         devpth = maybe_get_writable_device_path('gpt/system', info, LOG)
diff --git a/tests/unittests/test_handler/test_schema.py b/tests/unittests/test_handler/test_schema.py
index fb266fa..1bad07f 100644
--- a/tests/unittests/test_handler/test_schema.py
+++ b/tests/unittests/test_handler/test_schema.py
@@ -4,7 +4,7 @@ from cloudinit.config.schema import (
     CLOUD_CONFIG_HEADER, SchemaValidationError, annotated_cloudconfig_file,
     get_schema_doc, get_schema, validate_cloudconfig_file,
     validate_cloudconfig_schema, main)
-from cloudinit.util import subp, write_file
+from cloudinit.util import write_file
 
 from cloudinit.tests.helpers import CiTestCase, mock, skipUnlessJsonSchema
 
@@ -406,8 +406,14 @@ class CloudTestsIntegrationTest(CiTestCase):
         integration_testdir = os.path.sep.join(
             [testsdir, 'cloud_tests', 'testcases'])
         errors = []
-        out, _ = subp(['find', integration_testdir, '-name', '*yaml'])
-        for filename in out.splitlines():
+
+        yaml_files = []
+        for root, _dirnames, filenames in os.walk(integration_testdir):
+            yaml_files.extend([os.path.join(root, f)
+                               for f in filenames if f.endswith(".yaml")])
+        self.assertTrue(len(yaml_files) > 0)
+
+        for filename in yaml_files:
             test_cfg = safe_load(open(filename))
             cloud_config = test_cfg.get('cloud_config')
             if cloud_config:
diff --git a/tests/unittests/test_net.py b/tests/unittests/test_net.py
index 58e5ea1..371b75d 100644
--- a/tests/unittests/test_net.py
+++ b/tests/unittests/test_net.py
@@ -1493,6 +1493,12 @@ def _setup_test(tmp_dir, mock_get_devicelist, mock_read_sys_net,
 
 class TestGenerateFallbackConfig(CiTestCase):
 
+    def setUp(self):
+        super(TestGenerateFallbackConfig, self).setUp()
+        self.add_patch(
+            "cloudinit.util.get_cmdline", "m_get_cmdline",
+            return_value="root=/dev/sda1")
+
     @mock.patch("cloudinit.net.sys_dev_path")
     @mock.patch("cloudinit.net.read_sys_net")
     @mock.patch("cloudinit.net.get_devicelist")
@@ -1728,12 +1734,13 @@ class TestSysConfigRendering(CiTestCase):
         if missing:
             raise AssertionError("Missing headers in: %s" % missing)
 
+    @mock.patch("cloudinit.net.util.get_cmdline", return_value="root=myroot")
     @mock.patch("cloudinit.net.sys_dev_path")
     @mock.patch("cloudinit.net.read_sys_net")
     @mock.patch("cloudinit.net.get_devicelist")
     def test_default_generation(self, mock_get_devicelist,
                                 mock_read_sys_net,
-                                mock_sys_dev_path):
+                                mock_sys_dev_path, m_get_cmdline):
         tmp_dir = self.tmp_dir()
         _setup_test(tmp_dir, mock_get_devicelist,
                     mock_read_sys_net, mock_sys_dev_path)
@@ -1963,12 +1970,13 @@ USERCTL=no
 
 class TestEniNetRendering(CiTestCase):
 
+    @mock.patch("cloudinit.net.util.get_cmdline", return_value="root=myroot")
     @mock.patch("cloudinit.net.sys_dev_path")
     @mock.patch("cloudinit.net.read_sys_net")
     @mock.patch("cloudinit.net.get_devicelist")
     def test_default_generation(self, mock_get_devicelist,
                                 mock_read_sys_net,
-                                mock_sys_dev_path):
+                                mock_sys_dev_path, m_get_cmdline):
         tmp_dir = self.tmp_dir()
         _setup_test(tmp_dir, mock_get_devicelist,
                     mock_read_sys_net, mock_sys_dev_path)
@@ -2016,6 +2024,7 @@ iface eth0 inet dhcp
 
 class TestNetplanNetRendering(CiTestCase):
 
+    @mock.patch("cloudinit.net.util.get_cmdline", return_value="root=myroot")
     @mock.patch("cloudinit.net.netplan._clean_default")
     @mock.patch("cloudinit.net.sys_dev_path")
     @mock.patch("cloudinit.net.read_sys_net")
@@ -2023,7 +2032,7 @@ class TestNetplanNetRendering(CiTestCase):
     def test_default_generation(self, mock_get_devicelist,
                                 mock_read_sys_net,
                                 mock_sys_dev_path,
-                                mock_clean_default):
+                                mock_clean_default, m_get_cmdline):
         tmp_dir = self.tmp_dir()
         _setup_test(tmp_dir, mock_get_devicelist,
                     mock_read_sys_net, mock_sys_dev_path)
diff --git a/tests/unittests/test_util.py b/tests/unittests/test_util.py
index 7a203ce..7d9be95 100644
--- a/tests/unittests/test_util.py
+++ b/tests/unittests/test_util.py
@@ -742,6 +742,7 @@ class TestReadSeeded(helpers.TestCase):
 
 class TestSubp(helpers.CiTestCase):
     with_logs = True
+    allowed_subp = True
 
     stdin2err = [BASH, '-c', 'cat >&2']
     stdin2out = ['cat']