curtin-dev team mailing list archive

[Ryan Harper <ryan.harper@xxxxxxxxxxxxx>]

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.

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

It creates some noise in the test run so I'm inclined to drop it; I don't have the time to sort whatever issue it is.

> +    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 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.

> +
>      @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

Yep, that is nice.

>          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):

Yeah; I think it's reasonable safe since it keeps things off by default which is what the class-level value is by default.

> +        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.