From 5062bee0519aeccf7af9c71b9d027b8d891355ae Mon Sep 17 00:00:00 2001 From: James Falcon Date: Mon, 11 Mar 2024 19:13:02 -0500 Subject: [PATCH] refactor: don't import subp function directly (#5065) When subp function is imported directly, it cannot be easily mocked in unit tests. Update imports and tests accordingly. --- cloudinit/config/cc_ansible.py | 6 ++--- cloudinit/config/cc_ubuntu_autoinstall.py | 5 ++-- tests/unittests/config/test_cc_ansible.py | 27 ++++++++++--------- .../config/test_cc_ubuntu_autoinstall.py | 2 +- 4 files changed, 21 insertions(+), 19 deletions(-) diff --git a/cloudinit/config/cc_ansible.py b/cloudinit/config/cc_ansible.py index 659420aff30..2878a68ac42 100644 --- a/cloudinit/config/cc_ansible.py +++ b/cloudinit/config/cc_ansible.py @@ -9,12 +9,12 @@ from textwrap import dedent from typing import Optional +from cloudinit import subp from cloudinit.cloud import Cloud from cloudinit.config import Config from cloudinit.config.schema import MetaSchema, get_meta_doc from cloudinit.distros import ALL_DISTROS, Distro from cloudinit.settings import PER_INSTANCE -from cloudinit.subp import subp, which from cloudinit.util import Version, get_cfg_by_path meta: MetaSchema = { @@ -100,7 +100,7 @@ def do_as(self, command: list, **kwargs): return self.distro.do_as(command, self.run_user, **kwargs) def subp(self, command, **kwargs): - return subp(command, update_env=self.env, **kwargs) + return subp.subp(command, update_env=self.env, **kwargs) @abc.abstractmethod def is_installed(self): @@ -165,7 +165,7 @@ def install(self, pkg_name: str): self.distro.install_packages([pkg_name]) def is_installed(self) -> bool: - return bool(which("ansible")) + return bool(subp.which("ansible")) def handle(name: str, cfg: Config, cloud: Cloud, args: list) -> None: diff --git a/cloudinit/config/cc_ubuntu_autoinstall.py b/cloudinit/config/cc_ubuntu_autoinstall.py index c75f7a979f9..ff5286370db 100644 --- a/cloudinit/config/cc_ubuntu_autoinstall.py +++ b/cloudinit/config/cc_ubuntu_autoinstall.py @@ -6,7 +6,7 @@ import re from textwrap import dedent -from cloudinit import util +from cloudinit import subp, util from cloudinit.cloud import Cloud from cloudinit.config import Config from cloudinit.config.schema import ( @@ -16,7 +16,6 @@ get_meta_doc, ) from cloudinit.settings import PER_ONCE -from cloudinit.subp import subp LOG = logging.getLogger(__name__) @@ -85,7 +84,7 @@ def handle(name: str, cfg: Config, cloud: Cloud, args: list) -> None: return util.wait_for_snap_seeded(cloud) - snap_list, _ = subp(["snap", "list"]) + snap_list, _ = subp.subp(["snap", "list"]) installer_present = None for snap_name in LIVE_INSTALLER_SNAPS: if re.search(snap_name, snap_list): diff --git a/tests/unittests/config/test_cc_ansible.py b/tests/unittests/config/test_cc_ansible.py index 685dbd70ff9..271d9d037ec 100644 --- a/tests/unittests/config/test_cc_ansible.py +++ b/tests/unittests/config/test_cc_ansible.py @@ -287,8 +287,8 @@ def test_filter_args(self): ), ) def test_required_keys(self, cfg, exception, mocker): - mocker.patch(M_PATH + "subp", return_value=("", "")) - mocker.patch(M_PATH + "which", return_value=True) + mocker.patch(M_PATH + "subp.subp", return_value=("", "")) + mocker.patch(M_PATH + "subp.which", return_value=True) mocker.patch(M_PATH + "AnsiblePull.check_deps") mocker.patch( M_PATH + "AnsiblePull.get_version", @@ -319,28 +319,30 @@ def test_required_keys(self, cfg, exception, mocker): ["python3-pip"] ) - @mock.patch(M_PATH + "which", return_value=False) + @mock.patch(M_PATH + "subp.which", return_value=False) def test_deps_not_installed(self, m_which): """assert exception raised if package not installed""" with raises(ValueError): cc_ansible.AnsiblePullDistro(get_cloud().distro).check_deps() - @mock.patch(M_PATH + "which", return_value=True) + @mock.patch(M_PATH + "subp.which", return_value=True) def test_deps(self, m_which): """assert exception not raised if package installed""" cc_ansible.AnsiblePullDistro(get_cloud().distro).check_deps() - @mock.patch(M_PATH + "subp", return_value=("stdout", "stderr")) - @mock.patch(M_PATH + "which", return_value=False) + @mock.patch(M_PATH + "subp.subp", return_value=("stdout", "stderr")) + @mock.patch(M_PATH + "subp.which", return_value=False) def test_pip_bootstrap(self, m_which, m_subp): distro = get_cloud(mocked_distro=True).distro with mock.patch("builtins.__import__", side_effect=ImportError): cc_ansible.AnsiblePullPip(distro, "ansible").install("") distro.install_packages.assert_called_once() - @mock.patch(M_PATH + "which", return_value=True) - @mock.patch(M_PATH + "subp", return_value=("stdout", "stderr")) - @mock.patch("cloudinit.distros.subp", return_value=("stdout", "stderr")) + @mock.patch(M_PATH + "subp.which", return_value=True) + @mock.patch(M_PATH + "subp.subp", return_value=("stdout", "stderr")) + @mock.patch( + "cloudinit.distros.subp.subp", return_value=("stdout", "stderr") + ) @mark.parametrize( ("cfg", "expected"), ( @@ -406,7 +408,8 @@ def test_do_not_run(self, m_validate): assert not m_validate.called @mock.patch( - "cloudinit.config.cc_ansible.subp", side_effect=[(distro_version, "")] + "cloudinit.config.cc_ansible.subp.subp", + side_effect=[(distro_version, "")], ) def test_parse_version_distro(self, m_subp): """Verify that the expected version is returned""" @@ -424,8 +427,8 @@ def test_parse_version_pip(self, m_subp): expected = util.Version(2, 13, 2) assert received == expected - @mock.patch(M_PATH + "subp", return_value=("stdout", "stderr")) - @mock.patch(M_PATH + "which", return_value=True) + @mock.patch(M_PATH + "subp.subp", return_value=("stdout", "stderr")) + @mock.patch(M_PATH + "subp.which", return_value=True) def test_ansible_env_var(self, m_which, m_subp): cc_ansible.handle("", CFG_FULL_PULL, get_cloud(), []) diff --git a/tests/unittests/config/test_cc_ubuntu_autoinstall.py b/tests/unittests/config/test_cc_ubuntu_autoinstall.py index 4b56669091d..1a492ad0f4c 100644 --- a/tests/unittests/config/test_cc_ubuntu_autoinstall.py +++ b/tests/unittests/config/test_cc_ubuntu_autoinstall.py @@ -66,7 +66,7 @@ def test_runtime_validation_errors(self, src_cfg, error_msg): @mock.patch(MODPATH + "util.wait_for_snap_seeded") -@mock.patch(MODPATH + "subp") +@mock.patch(MODPATH + "subp.subp") class TestHandleAutoinstall: """Test cc_ubuntu_autoinstall handling of config."""