curtin-dev team mailing list archive

[Dan Watkins <daniel.watkins@xxxxxxxxxxxxx>]

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.