Merge ~raharper/curtin:fix/unittest-prevent-subp-by-default into curtin:master

[Ryan Harper]

unittest: do not allow util.subp by default

Bring over our util.subp mock from cloud-init. Rework unittest to
enable subp as needed.

LP: #1873913

[Server Team CI bot (server-team-bot)]

PASSED: Continuous integration, rev:d68faec0fd44fd0d7f2b2d5ef32cb95d0056e188
https://jenkins.ubuntu.com/server/job/curtin-ci/44/
Executed test runs:
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-amd64/44/
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-arm64/44/
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-ppc64el/44/
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-s390x/44/

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/curtin-ci/44//rebuild

review: Approve (continuous-integration)

[Dan Watkins (oddbloke)]

Good thinking to pull this over, I have some comments inline.

review: Needs Fixing

[Ryan Harper (raharper)]

Let me drop the logging bits; I replied in line; I think the per-test enablement with a tearDown() is reasonable. Let me see if I can sub-class the main test class and put the subp_enabled ones within that; hoping to avoid duplicating code from one class to another when all we need is to enable/disable subp.

[Dan Watkins (oddbloke)]

On Mon, Apr 20, 2020 at 08:27:59PM -0000, Ryan Harper wrote:
> Let me drop the logging bits

Thanks!

> > diff --git a/tests/unittests/test_block_zfs.py b/tests/unittests/test_block_zfs.py
> > index 3508d4b..2be7c41 100644
> > --- a/tests/unittests/test_block_zfs.py
> > +++ b/tests/unittests/test_block_zfs.py
> > @@ -425,6 +425,9 @@ class TestAssertZfsSupported(CiTestCase):
> > def setUp(self):
> > super(TestAssertZfsSupported, self).setUp()
> >
> > + def tearDown(self):
> > + self.allowed_subp = False
>
> I think this is reasonable safe. It's off by default. The test-cases
> that know they need it set it at the start of their test_*; this just
> retains the default off unless you need it and I think help reduce the
> scope of when subp is enabled.

Yep, I think the case I was worried about isn't applicable here, so
let's stick with this.

[Dan Watkins (oddbloke)]

Sorry, noticed one more thing! Other than that, this looks good now, thanks for the refactors!

[Ryan Harper (raharper)]

Fixing

[Dan Watkins (oddbloke)]

Thanks!

review: Approve

[Server Team CI bot (server-team-bot)]

PASSED: Continuous integration, rev:86771d11f41cb2b7467acb342f0d5406afb1e4d9
https://jenkins.ubuntu.com/server/job/curtin-ci/49/
Executed test runs:
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-amd64/49/
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-arm64/49/
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-ppc64el/49/
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-s390x/49/

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/curtin-ci/49//rebuild

review: Approve (continuous-integration)

[Server Team CI bot (server-team-bot)]

Commit message lints:
- Line #2 has 15 too many characters. Line starts with: "Bring over our util.subp"...

review: Needs Fixing

[Server Team CI bot (server-team-bot)]

Autolanding: FAILED
More details in the following jenkins job:
https://jenkins.ubuntu.com/server/job/curtin-autoland-test/145/
Executed test runs:
    FAILURE: https://jenkins.ubuntu.com/server/job/curtin-autoland-test/nodes=metal-amd64/145/
    FAILURE: https://jenkins.ubuntu.com/server/job/curtin-autoland-test/nodes=metal-arm64/145/
    FAILURE: https://jenkins.ubuntu.com/server/job/curtin-autoland-test/nodes=metal-ppc64el/145/
    FAILURE: https://jenkins.ubuntu.com/server/job/curtin-autoland-test/nodes=metal-s390x/145/

review: Needs Fixing (continuous-integration)

[Server Team CI bot (server-team-bot)]

Autolanding: FAILED
More details in the following jenkins job:
https://jenkins.ubuntu.com/server/job/curtin-autoland-test/146/
Executed test runs:
    FAILURE: https://jenkins.ubuntu.com/server/job/curtin-autoland-test/nodes=metal-amd64/146/
    FAILURE: https://jenkins.ubuntu.com/server/job/curtin-autoland-test/nodes=metal-arm64/146/
    FAILURE: https://jenkins.ubuntu.com/server/job/curtin-autoland-test/nodes=metal-ppc64el/146/
    FAILURE: https://jenkins.ubuntu.com/server/job/curtin-autoland-test/nodes=metal-s390x/146/

review: Needs Fixing (continuous-integration)

[Server Team CI bot (server-team-bot)]

Autolanding: FAILED
More details in the following jenkins job:
https://jenkins.ubuntu.com/server/job/curtin-autoland-test/147/
Executed test runs:
    FAILURE: https://jenkins.ubuntu.com/server/job/curtin-autoland-test/nodes=metal-amd64/147/
    FAILURE: https://jenkins.ubuntu.com/server/job/curtin-autoland-test/nodes=metal-arm64/147/
    FAILURE: https://jenkins.ubuntu.com/server/job/curtin-autoland-test/nodes=metal-ppc64el/147/
    FAILURE: https://jenkins.ubuntu.com/server/job/curtin-autoland-test/nodes=metal-s390x/147/

review: Needs Fixing (continuous-integration)

[Server Team CI bot (server-team-bot)]

Autolanding: FAILED
More details in the following jenkins job:
https://jenkins.ubuntu.com/server/job/curtin-autoland-test/148/
Executed test runs:
    FAILURE: https://jenkins.ubuntu.com/server/job/curtin-autoland-test/nodes=metal-amd64/148/
    FAILURE: https://jenkins.ubuntu.com/server/job/curtin-autoland-test/nodes=metal-arm64/148/
    FAILURE: https://jenkins.ubuntu.com/server/job/curtin-autoland-test/nodes=metal-ppc64el/148/
    FAILURE: https://jenkins.ubuntu.com/server/job/curtin-autoland-test/nodes=metal-s390x/148/

review: Needs Fixing (continuous-integration)

[Server Team CI bot (server-team-bot)]

Autolanding: FAILED
More details in the following jenkins job:
https://jenkins.ubuntu.com/server/job/curtin-autoland-test/149/
Executed test runs:
    FAILURE: https://jenkins.ubuntu.com/server/job/curtin-autoland-test/nodes=metal-amd64/149/
    FAILURE: https://jenkins.ubuntu.com/server/job/curtin-autoland-test/nodes=metal-arm64/149/
    FAILURE: https://jenkins.ubuntu.com/server/job/curtin-autoland-test/nodes=metal-ppc64el/149/
    FAILURE: https://jenkins.ubuntu.com/server/job/curtin-autoland-test/nodes=metal-s390x/149/

review: Needs Fixing (continuous-integration)

56df421... by Ryan Harper on 2020-04-21

Fix new calls of util.subp and multipath.is_mpath_member

[Ryan Harper (raharper)]

wait for CI to say yes, then mark approve

[Server Team CI bot (server-team-bot)]

Autolanding: FAILED
More details in the following jenkins job:
https://jenkins.ubuntu.com/server/job/curtin-autoland-test/150/
Executed test runs:
    FAILURE: https://jenkins.ubuntu.com/server/job/curtin-autoland-test/nodes=metal-amd64/150/
    FAILURE: https://jenkins.ubuntu.com/server/job/curtin-autoland-test/nodes=metal-arm64/150/
    FAILURE: https://jenkins.ubuntu.com/server/job/curtin-autoland-test/nodes=metal-ppc64el/150/
    FAILURE: https://jenkins.ubuntu.com/server/job/curtin-autoland-test/nodes=metal-s390x/150/

review: Needs Fixing (continuous-integration)

[Server Team CI bot (server-team-bot)]

PASSED: Continuous integration, rev:f1e57f03529cd5a20a001685fe2275fb0501d96c
https://jenkins.ubuntu.com/server/job/curtin-ci/50/
Executed test runs:
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-amd64/50/
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-arm64/50/
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-ppc64el/50/
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-s390x/50/

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/curtin-ci/50//rebuild

review: Approve (continuous-integration)

[Server Team CI bot (server-team-bot)]

PASSED: Continuous integration, rev:56df4215c9d7b4671f435ca84284f1bd52f89705
https://jenkins.ubuntu.com/server/job/curtin-ci/51/
Executed test runs:
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-amd64/51/
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-arm64/51/
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-ppc64el/51/
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-s390x/51/

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/curtin-ci/51//rebuild

review: Approve (continuous-integration)

diff --git a/tests/unittests/helpers.py b/tests/unittests/helpers.py
index 9514745..2f5e51a 100644
--- a/tests/unittests/helpers.py
+++ b/tests/unittests/helpers.py
@@ -1,6 +1,5 @@
 # This file is part of curtin. See LICENSE file for copyright and license info.
 
-import contextlib
 import imp
 import importlib
 import mock
@@ -10,9 +9,11 @@ import shutil
 import string
 import tempfile
 from unittest import TestCase, skipIf
-
+from contextlib import contextmanager
 from curtin import util
 
+_real_subp = util.subp
+
 
 def builtin_module_name():
     options = ('builtins', '__builtin__')
@@ -27,7 +28,7 @@ def builtin_module_name():
             return name
 
 
[e-mail]
+@contextmanager
 def simple_mocked_open(content=None):
     if not content:
         content = ''
@@ -54,6 +55,54 @@ def skipUnlessJsonSchema():
 class CiTestCase(TestCase):
     """Common testing class which all curtin unit tests subclass."""
 
+    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()
+        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:
+            cmd = args[0]
+
+        if not isinstance(cmd, str):
+            cmd = cmd[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):
+        util.subp = _real_subp
+        super(CiTestCase, self).tearDown()
+
     def add_patch(self, target, attr, **kwargs):
         """Patches specified target object and sets it as attr on test
         instance also schedules cleanup"""
diff --git a/tests/unittests/test_apt_custom_sources_list.py b/tests/unittests/test_apt_custom_sources_list.py
index fb6eb0c..bf004b1 100644
--- a/tests/unittests/test_apt_custom_sources_list.py
+++ b/tests/unittests/test_apt_custom_sources_list.py
@@ -93,7 +93,9 @@ class TestAptSourceConfigSourceList(CiTestCase):
     def setUp(self):
         super(TestAptSourceConfigSourceList, self).setUp()
         self.new_root = self.tmp_dir()
+        self.add_patch('curtin.util.subp', 'm_subp')
         # self.patchUtils(self.new_root)
+        self.m_subp.return_value = ("amd64", "")
 
     def _apt_source_list(self, cfg, expected):
         "_apt_source_list - Test rendering from template (generic)"
diff --git a/tests/unittests/test_apt_source.py b/tests/unittests/test_apt_source.py
index 353cdf8..6ae5579 100644
--- a/tests/unittests/test_apt_source.py
+++ b/tests/unittests/test_apt_source.py
@@ -75,6 +75,8 @@ class TestAptSourceConfig(CiTestCase):
         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('curtin.util.subp', 'm_subp')
+        self.m_subp.return_value = ('s390x', '')
 
     @staticmethod
     def _add_apt_sources(*args, **kwargs):
diff --git a/tests/unittests/test_block.py b/tests/unittests/test_block.py
index 707386f..dcdcf51 100644
--- a/tests/unittests/test_block.py
+++ b/tests/unittests/test_block.py
@@ -522,6 +522,12 @@ class TestPartTableSignature(CiTestCase):
     gpt_content_4k = b'\x00' * 0x800 + b'EFI PART' + b'\x00' * (0x800 - 8)
     null_content = b'\x00' * 0xf00
 
+    def setUp(self):
+        super(TestPartTableSignature, self).setUp()
+        self.add_patch('curtin.util.subp', 'm_subp')
+        self.m_subp.side_effect = iter([
+            util.ProcessExecutionError(stdout="", stderr="", exit_code=1)])
+
     def _test_util_load_file(self, content, device, read_len, offset, decode):
         return (bytes if not decode else str)(content[offset:offset+read_len])
 
@@ -577,15 +583,13 @@ class TestPartTableSignature(CiTestCase):
                 (self.assertTrue if expected else self.assertFalse)(
                     block.check_efi_signature(self.blockdev))
 
-    @mock.patch('curtin.block.util.subp')
-    def test_check_vtoc_signature_finds_vtoc_returns_true(self, mock_subp):
-        mock_subp.return_value = ("vtoc.....ok", "")
+    def test_check_vtoc_signature_finds_vtoc_returns_true(self):
+        self.m_subp.side_effect = iter([("vtoc.....ok", "")])
         self.assertTrue(block.check_vtoc_signature(self.blockdev))
 
-    @mock.patch('curtin.block.util.subp')
-    def test_check_vtoc_signature_returns_false_with_no_sig(self, mock_subp):
-        mock_subp.side_effect = [
-            util.ProcessExecutionError(stdout="", stderr="", exit_code=1)]
+    def test_check_vtoc_signature_returns_false_with_no_sig(self):
+        self.m_subp.side_effect = iter([
+            util.ProcessExecutionError(stdout="", stderr="", exit_code=1)])
         self.assertFalse(block.check_vtoc_signature(self.blockdev))
 
 
diff --git a/tests/unittests/test_block_zfs.py b/tests/unittests/test_block_zfs.py
index 3508d4b..e392000 100644
--- a/tests/unittests/test_block_zfs.py
+++ b/tests/unittests/test_block_zfs.py
@@ -468,6 +468,30 @@ class TestAssertZfsSupported(CiTestCase):
             with self.assertRaises(RuntimeError):
                 zfs.zfs_assert_supported()
 
+    @mock.patch('curtin.block.zfs.get_supported_filesystems')
+    @mock.patch('curtin.block.zfs.util.lsb_release')
+    @mock.patch('curtin.block.zfs.util.get_platform_arch')
+    @mock.patch('curtin.block.zfs.util')
+    def test_zfs_assert_supported_raises_exc_on_missing_binaries(self,
+                                                                 mock_util,
+                                                                 m_arch,
+                                                                 m_release,
+                                                                 m_supfs):
+        """zfs_assert_supported raises RuntimeError if no zpool or zfs tools"""
+        mock_util.get_platform_arch.return_value = 'amd64'
+        mock_util.lsb_release.return_value = {'codename': 'bionic'}
+        mock_util.subp.return_value = ("", "")
+        m_supfs.return_value = ['zfs']
+        mock_util.which.return_value = None
+
+        with self.assertRaises(RuntimeError):
+            zfs.zfs_assert_supported()
+
+
+class TestAssertZfsSupportedSubp(TestAssertZfsSupported):
+
+    allowed_subp = True
+
     @mock.patch('curtin.block.zfs.util.subprocess.Popen')
     @mock.patch('curtin.block.zfs.util.is_kmod_loaded')
     @mock.patch('curtin.block.zfs.get_supported_filesystems')
@@ -481,7 +505,6 @@ class TestAssertZfsSupported(CiTestCase):
                                                                m_popen,
                                                                ):
         """zfs_assert_supported raises RuntimeError modprobe zfs error"""
-
         m_arch.return_value = 'amd64'
         m_release.return_value = {'codename': 'bionic'}
         m_supfs.return_value = ['ext4']
@@ -497,25 +520,6 @@ class TestAssertZfsSupported(CiTestCase):
         with self.assertRaises(RuntimeError):
             zfs.zfs_assert_supported()
 
-    @mock.patch('curtin.block.zfs.get_supported_filesystems')
-    @mock.patch('curtin.block.zfs.util.lsb_release')
-    @mock.patch('curtin.block.zfs.util.get_platform_arch')
-    @mock.patch('curtin.block.zfs.util')
-    def test_zfs_assert_supported_raises_exc_on_missing_binaries(self,
-                                                                 mock_util,
-                                                                 m_arch,
-                                                                 m_release,
-                                                                 m_supfs):
-        """zfs_assert_supported raises RuntimeError if no zpool or zfs tools"""
-        mock_util.get_platform_arch.return_value = 'amd64'
-        mock_util.lsb_release.return_value = {'codename': 'bionic'}
-        mock_util.subp.return_value = ("", "")
-        m_supfs.return_value = ['zfs']
-        mock_util.which.return_value = None
-
-        with self.assertRaises(RuntimeError):
-            zfs.zfs_assert_supported()
-
 
 class TestZfsSupported(CiTestCase):
 
diff --git a/tests/unittests/test_clear_holders.py b/tests/unittests/test_clear_holders.py
index f73b49d..82b481a 100644
--- a/tests/unittests/test_clear_holders.py
+++ b/tests/unittests/test_clear_holders.py
@@ -701,13 +701,14 @@ class TestClearHolders(CiTestCase):
         mock_gen_holders_tree.return_value = self.example_holders_trees[1][1]
         clear_holders.assert_clear(device)
 
+    @mock.patch('curtin.block.clear_holders.udev')
     @mock.patch('curtin.block.clear_holders.multipath')
     @mock.patch('curtin.block.clear_holders.lvm')
     @mock.patch('curtin.block.clear_holders.zfs')
     @mock.patch('curtin.block.clear_holders.mdadm')
     @mock.patch('curtin.block.clear_holders.util')
     def test_start_clear_holders_deps(self, mock_util, mock_mdadm, mock_zfs,
-                                      mock_lvm, mock_mp):
+                                      mock_lvm, mock_mp, mock_udev):
         mock_zfs.zfs_supported.return_value = True
         clear_holders.start_clear_holders_deps()
         mock_mdadm.mdadm_assemble.assert_called_with(
@@ -715,13 +716,15 @@ class TestClearHolders(CiTestCase):
         mock_util.load_kernel_module.assert_has_calls([
                 mock.call('bcache')])
 
+    @mock.patch('curtin.block.clear_holders.udev')
     @mock.patch('curtin.block.clear_holders.multipath')
     @mock.patch('curtin.block.clear_holders.lvm')
     @mock.patch('curtin.block.clear_holders.zfs')
     @mock.patch('curtin.block.clear_holders.mdadm')
     @mock.patch('curtin.block.clear_holders.util')
     def test_start_clear_holders_deps_nozfs(self, mock_util, mock_mdadm,
-                                            mock_zfs, mock_lvm, mock_mp):
+                                            mock_zfs, mock_lvm, mock_mp,
+                                            mock_udev):
         """test that we skip zfs modprobe on unsupported platforms"""
         mock_zfs.zfs_supported.return_value = False
         clear_holders.start_clear_holders_deps()
diff --git a/tests/unittests/test_commands_block_meta.py b/tests/unittests/test_commands_block_meta.py
index aac1a9e..6da6412 100644
--- a/tests/unittests/test_commands_block_meta.py
+++ b/tests/unittests/test_commands_block_meta.py
@@ -19,6 +19,8 @@ class TestGetPathToStorageVolume(CiTestCase):
         self.add_patch(basepath + 'os.path.exists', 'm_exists')
         self.add_patch(basepath + 'block.lookup_disk', 'm_lookup')
         self.add_patch(basepath + 'devsync', 'm_devsync')
+        self.add_patch(basepath + 'util.subp', 'm_subp')
+        self.add_patch(basepath + 'multipath.is_mpath_member', 'm_mp')
 
     def test_block_lookup_called_with_disk_wwn(self):
         volume = 'mydisk'
@@ -93,6 +95,8 @@ class TestGetPathToStorageVolume(CiTestCase):
             ValueError('Error'), ValueError('Error')])
         # no path
         self.m_exists.return_value = False
+        # not multipath
+        self.m_mp.return_value = False
 
         with self.assertRaises(ValueError):
             block_meta.get_path_to_storage_volume(volume, s_cfg)
diff --git a/tests/unittests/test_commands_collect_logs.py b/tests/unittests/test_commands_collect_logs.py
index 1feba18..b5df902 100644
--- a/tests/unittests/test_commands_collect_logs.py
+++ b/tests/unittests/test_commands_collect_logs.py
@@ -288,6 +288,7 @@ class TestCreateTar(CiTestCase):
 
         Configured log_file or post_files which don't exist are ignored.
         """
+        self.add_patch('curtin.util.subp', 'mock_subp')
         tarfile = self.tmp_path('my.tar', _dir=self.new_root)
         log1 = self.tmp_path('some.log', _dir=self.new_root)
         write_file(log1, 'log content')
@@ -314,6 +315,7 @@ class TestCreateTar(CiTestCase):
 
     def test_create_log_tarfile_redacts_maas_credentials(self):
         """create_log_tarfile redacts sensitive maas credentials configured."""
+        self.add_patch('curtin.util.subp', 'mock_subp')
         tarfile = self.tmp_path('my.tar', _dir=self.new_root)
         self.add_patch(
             'curtin.commands.collect_logs._redact_sensitive_information',
diff --git a/tests/unittests/test_gpg.py b/tests/unittests/test_gpg.py
index 2c83ae3..42ff8c4 100644
--- a/tests/unittests/test_gpg.py
+++ b/tests/unittests/test_gpg.py
@@ -48,50 +48,6 @@ class TestCurtinGpg(CiTestCase):
                                       "--recv", key], capture=True,
                                      retries=None)
 
-    @patch('time.sleep')
-    @patch('curtin.util._subp')
-    def test_recv_key_retry_raises(self, mock_under_subp, mock_sleep):
-        key = 'DEADBEEF'
-        keyserver = 'keyserver.ubuntu.com'
-        retries = (1, 2, 5, 10)
-        nr_calls = 5
-        mock_under_subp.side_effect = iter([
-            util.ProcessExecutionError()] * nr_calls)
-
-        with self.assertRaises(ValueError):
-            gpg.recv_key(key, keyserver, retries=retries)
-
-        print("_subp calls: %s" % mock_under_subp.call_args_list)
-        print("sleep calls: %s" % mock_sleep.call_args_list)
-        expected_calls = nr_calls * [
-            call(["gpg", "--keyserver", keyserver, "--recv", key],
-                 capture=True)]
-        mock_under_subp.assert_has_calls(expected_calls)
-
-        expected_calls = [call(1), call(2), call(5), call(10)]
-        mock_sleep.assert_has_calls(expected_calls)
-
-    @patch('time.sleep')
-    @patch('curtin.util._subp')
-    def test_recv_key_retry_works(self, mock_under_subp, mock_sleep):
-        key = 'DEADBEEF'
-        keyserver = 'keyserver.ubuntu.com'
-        nr_calls = 2
-        mock_under_subp.side_effect = iter([
-            util.ProcessExecutionError(),  # 1
-            ("", ""),
-        ])
-
-        gpg.recv_key(key, keyserver, retries=[1])
-
-        print("_subp calls: %s" % mock_under_subp.call_args_list)
-        print("sleep calls: %s" % mock_sleep.call_args_list)
-        expected_calls = nr_calls * [
-            call(["gpg", "--keyserver", keyserver, "--recv", key],
-                 capture=True)]
-        mock_under_subp.assert_has_calls(expected_calls)
-        mock_sleep.assert_has_calls([call(1)])
-
     @patch('curtin.util.subp')
     def test_delete_key(self, mock_subp):
         key = 'DEADBEEF'
@@ -160,4 +116,54 @@ class TestCurtinGpg(CiTestCase):
             call(key, keyserver=keyserver, retries=None)])
         mock_del.assert_has_calls([call(key)])
 
+
+class TestCurtinGpgSubp(TestCurtinGpg):
+
+    allowed_subp = True
+
+    @patch('time.sleep')
+    @patch('curtin.util._subp')
+    def test_recv_key_retry_raises(self, mock_under_subp, mock_sleep):
+        key = 'DEADBEEF'
+        keyserver = 'keyserver.ubuntu.com'
+        retries = (1, 2, 5, 10)
+        nr_calls = 5
+        mock_under_subp.side_effect = iter([
+            util.ProcessExecutionError()] * nr_calls)
+
+        with self.assertRaises(ValueError):
+            gpg.recv_key(key, keyserver, retries=retries)
+
+        print("_subp calls: %s" % mock_under_subp.call_args_list)
+        print("sleep calls: %s" % mock_sleep.call_args_list)
+        expected_calls = nr_calls * [
+            call(["gpg", "--keyserver", keyserver, "--recv", key],
+                 capture=True)]
+        mock_under_subp.assert_has_calls(expected_calls)
+
+        expected_calls = [call(1), call(2), call(5), call(10)]
+        mock_sleep.assert_has_calls(expected_calls)
+
+    @patch('time.sleep')
+    @patch('curtin.util._subp')
+    def test_recv_key_retry_works(self, mock_under_subp, mock_sleep):
+        key = 'DEADBEEF'
+        keyserver = 'keyserver.ubuntu.com'
+        nr_calls = 2
+        mock_under_subp.side_effect = iter([
+            util.ProcessExecutionError(),  # 1
+            ("", ""),
+        ])
+
+        gpg.recv_key(key, keyserver, retries=[1])
+
+        print("_subp calls: %s" % mock_under_subp.call_args_list)
+        print("sleep calls: %s" % mock_sleep.call_args_list)
+        expected_calls = nr_calls * [
+            call(["gpg", "--keyserver", keyserver, "--recv", key],
+                 capture=True)]
+        mock_under_subp.assert_has_calls(expected_calls)
+        mock_sleep.assert_has_calls([call(1)])
+
+
 # vi: ts=4 expandtab syntax=python
diff --git a/tests/unittests/test_util.py b/tests/unittests/test_util.py
index 438f7ee..0377357 100644
--- a/tests/unittests/test_util.py
+++ b/tests/unittests/test_util.py
@@ -113,6 +113,7 @@ class TestSubp(CiTestCase):
     utf8_invalid = b'ab\xaadef'
     utf8_valid = b'start \xc3\xa9 end'
     utf8_valid_2 = b'd\xc3\xa9j\xc8\xa7'
+    allowed_subp = True
 
     try:
         decode_type = unicode
@@ -552,7 +553,7 @@ class TestTargetPath(CiTestCase):
                          paths.target_path("/target/", "///my/path/"))
 
 
-class TestRunInChroot(CiTestCase):
+class TestRunInChrootTestSubp(CiTestCase):
     """Test the legacy 'RunInChroot'.
 
     The test works by mocking ChrootableTarget's __enter__ to do nothing.
@@ -560,6 +561,7 @@ class TestRunInChroot(CiTestCase):
       a.) RunInChroot is a subclass of ChrootableTarget
       b.) ChrootableTarget's __exit__ only un-does work that its __enter__
           did.  Meaning for our mocked case, it does nothing."""
+    allowed_subp = True
 
     @mock.patch.object(util.ChrootableTarget, "__enter__", new=lambda a: a)
     def test_run_in_chroot_with_target_slash(self):
@@ -567,6 +569,16 @@ class TestRunInChroot(CiTestCase):
             out, err = i(['echo', 'HI MOM'], capture=True)
         self.assertEqual('HI MOM\n', out)
 
+
+class TestRunInChrootTest(CiTestCase):
+    """Test the legacy 'RunInChroot'.
+
+    The test works by mocking ChrootableTarget's __enter__ to do nothing.
+    The assumptions made are:
+      a.) RunInChroot is a subclass of ChrootableTarget
+      b.) ChrootableTarget's __exit__ only un-does work that its __enter__
+          did.  Meaning for our mocked case, it does nothing."""
+
     @mock.patch.object(util.ChrootableTarget, "__enter__", new=lambda a: a)
     @mock.patch("curtin.util.subp")
     def test_run_in_chroot_with_target(self, m_subp):