Review: Needs Fixing
Good thinking to pull this over, I have some comments inline.
Diff comments:
> diff --git a/tests/unittests/helpers.py b/tests/unittests/helpers.py
> index 9514745..cc9b812 100644
> --- a/tests/unittests/helpers.py
> +++ b/tests/unittests/helpers.py
> @@ -54,6 +62,68 @@ def skipUnlessJsonSchema():
> class CiTestCase(TestCase):
> """Common testing class which all curtin unit tests subclass."""
>
> + with_logs = False
We should mention that we're bringing over the log functionality too, if that
was intentional. (If it wasn't intentional, then we should consider dropping
it. The version in cloud-init is actively used, so may accrue more
fixes/features which we would pick up if we pulled this in as needed. Or, put
another way, I'm in favour of JIT copy/paste. ;)
> + 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.with_logs:
> + # Create a log handler so unit tests can search expected logs.
> + self.logger = logging.getLogger()
> + self.logs = io.StringIO()
> + formatter = logging.Formatter('%(levelname)s: %(message)s')
> + handler = logging.StreamHandler(self.logs)
> + 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:
> + 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):
> + if self.with_logs:
> + # Remove the handler we setup
> + logging.getLogger().handlers = self.old_handlers
> + logging.getLogger().level = None
> + 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_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 fear that setting this in a single test method and unsetting here is setting
us up for some confusing class variable vs instance variable Teachable Moments
in the future. Might it be better to split the one test that needs this off
into a separate test class that can just set `allowed_subp = True` for all its
tests?
> +
> @mock.patch('curtin.block.zfs.get_supported_filesystems')
> @mock.patch('curtin.block.zfs.distro')
> @mock.patch('curtin.block.zfs.util')
> @@ -481,7 +484,7 @@ class TestAssertZfsSupported(CiTestCase):
> m_popen,
> ):
> """zfs_assert_supported raises RuntimeError modprobe zfs error"""
> -
> + self.allowed_subp = True
As an aside, one of the nice things about pytest fixtures is that we can easily
configure them for individual test methods as in
https://github.com/canonical/cloud-init/pull/304/files#diff-7300d81024a73a9f68c5afee8eceda36R16
> m_arch.return_value = 'amd64'
> m_release.return_value = {'codename': 'bionic'}
> m_supfs.return_value = ['ext4']
> diff --git a/tests/unittests/test_gpg.py b/tests/unittests/test_gpg.py
> index 2c83ae3..9514f67 100644
> --- a/tests/unittests/test_gpg.py
> +++ b/tests/unittests/test_gpg.py
> @@ -10,6 +10,9 @@ from .helpers import CiTestCase
>
> class TestCurtinGpg(CiTestCase):
>
> + def tearDown(self):
Same thought here as above; perhaps this indicates that we should be using a
separate class for these tests.
> + self.allowed_subp = False
> +
> @patch('curtin.util.subp')
> def test_export_armour(self, mock_subp):
> key = 'DEADBEEF'
--
https://code.launchpad.net/~raharper/curtin/+git/curtin/+merge/382604
Your team curtin developers is subscribed to branch curtin:master.
--
Mailing list: https://launchpad.net/~curtin-dev
Post to : [email protected]
Unsubscribe : https://launchpad.net/~curtin-dev
More help : https://help.launchpad.net/ListHelp
