From 6c2e267656bfd05390541e17dcab0a2d0b81085f Mon Sep 17 00:00:00 2001 From: Alberto Contreras Date: Fri, 22 Aug 2025 15:13:47 +0200 Subject: [PATCH 1/2] test: remove CiTestCase fully --- conftest.py | 9 +-- tests/unittests/helpers.py | 110 +-------------------------- tests/unittests/sources/test_init.py | 6 +- tests/unittests/test_conftest.py | 25 ------ 4 files changed, 6 insertions(+), 144 deletions(-) diff --git a/conftest.py b/conftest.py index 5d7aa5638d1..21e2dc2fdbe 100644 --- a/conftest.py +++ b/conftest.py @@ -95,8 +95,8 @@ def disable_subp_usage(request, fixture_utils): Note that this can only catch invocations where the ``subp`` module is imported and ``subp.subp(...)`` is called. ``from cloudinit.subp import - subp`` imports happen before the patching here (or the CiTestCase - monkey-patching) happens, so are left untouched. + subp`` imports happen before the patching here happens, so are left + untouched. While ``disable_subp_usage`` unconditionally patches ``cloudinit.subp.subp``, any test-local patching will override this @@ -128,11 +128,6 @@ def test_bash(self): def test_several_things(self): subp.subp(["bash"]) subp.subp(["whoami"]) - - This fixture (roughly) mirrors the functionality of - ``CiTestCase.allowed_subp``. N.B. While autouse fixtures do affect - non-pytest tests, CiTestCase's ``allowed_subp`` does take precedence (and - we have ``TestDisableSubpUsageInTestSubclass`` to confirm that). """ allow_subp_for = fixture_utils.closest_marker_args_or( request, "allow_subp_for", None diff --git a/tests/unittests/helpers.py b/tests/unittests/helpers.py index 7e9eda8c1b8..b107a75bce3 100644 --- a/tests/unittests/helpers.py +++ b/tests/unittests/helpers.py @@ -2,25 +2,20 @@ # pylint: disable=attribute-defined-outside-init import copy -import functools -import io -import logging import os import random import shutil import string -import tempfile import time import unittest from contextlib import contextmanager -from typing import ClassVar, List, Union from unittest import mock from unittest.util import strclass from urllib.parse import urlsplit, urlunsplit import responses -from cloudinit import distros, helpers, settings, subp, util +from cloudinit import distros, helpers, settings, util from cloudinit.config.schema import ( SchemaValidationError, validate_cloudconfig_schema, @@ -30,8 +25,6 @@ from tests.helpers import cloud_init_project_dir from tests.hypothesis_jsonschema import HAS_HYPOTHESIS_JSONSCHEMA -_real_subp = subp.subp - # Used for skipping tests SkipTest = unittest.SkipTest skipIf = unittest.skipIf @@ -164,107 +157,6 @@ def add_patch(self, target, attr, *args, **kwargs): setattr(self, attr, p) -class CiTestCase(TestCase): - """This is the preferred test case base class unless user - needs other test case classes below.""" - - # Subclass overrides for specific test behavior - # Whether or not a unit test needs logfile setup - with_logs = False - allowed_subp: ClassVar[Union[List, bool]] = 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] - self.old_level = logging.root.level - self.logger.level = logging.DEBUG - if self.allowed_subp is True: - subp.subp = _real_subp - else: - subp.subp = self._fake_subp - - def _fake_subp(self, *args, **kwargs): - if "args" in kwargs: - cmd = kwargs["args"] - else: - if not args: - raise TypeError( - "subp() missing 1 required positional argument: 'args'" - ) - 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 RuntimeError( - "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().setLevel(self.old_level) - subp.subp = _real_subp - super(CiTestCase, self).tearDown() - - def tmp_dir(self, dir=None, cleanup=True): - # return a full path to a temporary directory that will be cleaned up. - if dir is None: - tmpd = tempfile.mkdtemp(prefix="ci-%s." % self.__class__.__name__) - else: - tmpd = tempfile.mkdtemp(dir=dir) - self.addCleanup( - functools.partial(shutil.rmtree, tmpd, ignore_errors=True) - ) - return tmpd - - def tmp_path(self, path, dir=None): - # return an absolute path to 'path' under dir. - # if dir is None, one will be created with tmp_dir() - # the file is not created or modified. - if dir is None: - dir = self.tmp_dir() - return os.path.normpath(os.path.abspath(os.path.join(dir, path))) - - @classmethod - def random_string(cls, length=8): - return random_string(length) - - def replicate_test_root(example_root, target_root): real_root = resourceLocation() real_root = os.path.join(real_root, "roots", example_root) diff --git a/tests/unittests/sources/test_init.py b/tests/unittests/sources/test_init.py index 05bebd6e0d9..f4a6cc6b1e7 100644 --- a/tests/unittests/sources/test_init.py +++ b/tests/unittests/sources/test_init.py @@ -24,7 +24,7 @@ redact_sensitive_keys, ) from cloudinit.user_data import UserDataProcessor -from tests.unittests.helpers import CiTestCase, assert_count_equal, mock +from tests.unittests.helpers import assert_count_equal, mock class DataSourceTestSubclassNet(DataSource): @@ -933,7 +933,7 @@ def fake_get_data(): ) in caplog.record_tuples -class TestRedactSensitiveData(CiTestCase): +class TestRedactSensitiveData: def test_redact_sensitive_data_noop_when_no_sensitive_keys_present(self): """When sensitive_keys is absent or empty from metadata do nothing.""" md = {"my": "data"} @@ -962,7 +962,7 @@ def test_redact_sensitive_data_does_redacts_with_default_string(self): assert secure_md == redact_sensitive_keys(md) -class TestCanonicalCloudID(CiTestCase): +class TestCanonicalCloudID: def test_cloud_id_returns_platform_on_unknowns(self): """When region and cloud_name are unknown, return platform.""" assert "platform" == canonical_cloud_id( diff --git a/tests/unittests/test_conftest.py b/tests/unittests/test_conftest.py index d1a4be23f1c..bc2db355f16 100644 --- a/tests/unittests/test_conftest.py +++ b/tests/unittests/test_conftest.py @@ -2,7 +2,6 @@ from cloudinit import subp from conftest import UnexpectedSubpError -from tests.unittests.helpers import CiTestCase class TestDisableSubpUsage: @@ -51,27 +50,3 @@ def test_subp_usage_can_be_conditionally_reenabled_for_multiple_cmds(self): def test_both_marks_raise_an_error(self): with pytest.raises(UnexpectedSubpError, match="marked both"): subp.subp(["sh"]) - - -class TestDisableSubpUsageInTestSubclass(CiTestCase): - """Test that disable_subp_usage doesn't impact CiTestCase's subp logic. - - Once the rest of the CiTestCase tests are removed, this class - should be removed as well. - """ - - def test_using_subp_raises_exception(self): - with pytest.raises(Exception): - subp.subp(["some", "args"]) - - def test_typeerrors_on_incorrect_usage(self): - with pytest.raises(TypeError): - subp.subp() - - def test_subp_usage_can_be_reenabled(self): - _old_allowed_subp = self.allow_subp - self.allowed_subp = True - try: - subp.subp(["sh", "-c", "true"]) - finally: - self.allowed_subp = _old_allowed_subp From ac19095ec45ef03af12bdf808734cc3b4eebfb23 Mon Sep 17 00:00:00 2001 From: Alberto Contreras Date: Tue, 26 Aug 2025 12:32:20 +0200 Subject: [PATCH 2/2] pr fixes --- conftest.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/conftest.py b/conftest.py index 21e2dc2fdbe..52d8d5466da 100644 --- a/conftest.py +++ b/conftest.py @@ -95,8 +95,8 @@ def disable_subp_usage(request, fixture_utils): Note that this can only catch invocations where the ``subp`` module is imported and ``subp.subp(...)`` is called. ``from cloudinit.subp import - subp`` imports happen before the patching here happens, so are left - untouched. + subp`` is left untouched because those imports happen before the patching + happens here. While ``disable_subp_usage`` unconditionally patches ``cloudinit.subp.subp``, any test-local patching will override this