diff --git a/changelog/156.improvement.rst b/changelog/156.improvement.rst new file mode 100644 index 00000000..e027ed42 --- /dev/null +++ b/changelog/156.improvement.rst @@ -0,0 +1 @@ +Simplify and reduce the amount of patching required to unit test loader modules diff --git a/src/saltfactories/utils/loader.py b/src/saltfactories/utils/loader.py index 64ef5a08..f7046091 100644 --- a/src/saltfactories/utils/loader.py +++ b/src/saltfactories/utils/loader.py @@ -100,22 +100,12 @@ def start(self): f"not {type(globals_to_mock)}" ) raise pytest.UsageError(msg) - for key in self.salt_module_dunders: - if not hasattr(module, key): - # Set the dunder name as an attribute on the module if not present - setattr(module, key, {}) - # Remove the added attribute after the test finishes - self.addfinalizer(delattr, module, key) # Patch sys.modules as the first step - self._patch_sys_modules(globals_to_mock) + if "sys.modules" in globals_to_mock: + self._patch_sys_modules(globals_to_mock) # Now patch the module globals - # We actually want to grab a copy of the module globals so that if mocking - # multiple modules, and at least one of the modules has a function to path, - # the patch only happens on the module it's supposed to patch and not all of them. - # It's not a deepcopy because we want to maintain the reference to the salt dunders - # added in the start of this function self._patch_module_globals(module, globals_to_mock, module_globals.copy()) def stop(self): @@ -138,8 +128,6 @@ def addfinalizer(self, func, *args, **kwargs): self._finalizers.append((func, args, kwargs)) def _patch_sys_modules(self, mocks): - if "sys.modules" not in mocks: - return sys_modules = mocks["sys.modules"] if not isinstance(sys_modules, dict): msg = f"'sys.modules' must be a dictionary not: {type(sys_modules)}" @@ -163,22 +151,11 @@ def _patch_module_globals(self, module, mocks, module_globals): if key not in allowed_salt_dunders: msg = f"Don't know how to handle {key!r}. Passed loader module dict: {self.setup_loader_modules}" raise pytest.UsageError(msg) - if key in salt_dunder_dicts and not hasattr(module, key): - # Add the key as a dictionary attribute to the module so it can be patched by `patch.dict`' - setattr(module, key, {}) - # Remove the added attribute after the test finishes - self.addfinalizer(delattr, module, key) - - if not hasattr(module, key): - # Set the key as an attribute so it can be patched - setattr(module, key, None) - # Remove the added attribute after the test finishes - self.addfinalizer(delattr, module, key) module_globals[key] = mocks[key] # Patch the module! log.log(LOGGING_TRACE_LEVEL, "Patching globals for %s; globals: %s", module, module_globals) - patcher = patch.multiple(module, **module_globals) + patcher = patch.multiple(module, create=True, **module_globals) patcher.start() self.addfinalizer(patcher.stop) diff --git a/tests/functional/loader/test_fixture_deps.py b/tests/functional/loader/test_fixture_deps.py index dc671aa6..ccba447c 100644 --- a/tests/functional/loader/test_fixture_deps.py +++ b/tests/functional/loader/test_fixture_deps.py @@ -1,3 +1,4 @@ +# pylint: disable=no-member import logging from unittest.mock import patch @@ -7,10 +8,11 @@ try: saltfactories.__salt__ # pylint: disable=pointless-statement - HAS_SALT_DUNDER = True + HAS_SALT_DUNDER = True # pragma: no cover except AttributeError: HAS_SALT_DUNDER = False + log = logging.getLogger(__name__) @@ -21,19 +23,15 @@ def _confirm_saltfactories_does_not_have_salt_dunders(): ), "Weirdly, the saltfactories module has a __salt__ dunder defined. That's a bug!" -def confirm_saltfactories_does_not_have_salt_dunders_after_setup_loader_mock_terminates( - setup_loader_mock, -): - yield - with pytest.raises(AttributeError): - assert isinstance(saltfactories.__salt__, dict) # pylint: disable=no-member - - @pytest.fixture def pre_loader_modules_patched_fixture(): with pytest.raises(AttributeError): - assert isinstance(saltfactories.__salt__, dict) # pylint: disable=no-member - return False + assert isinstance(saltfactories.__salt__, dict) + try: + yield False + finally: + with pytest.raises(AttributeError): + assert isinstance(saltfactories.__salt__, dict) @pytest.fixture @@ -47,14 +45,24 @@ def configure_loader_modules(pre_loader_modules_patched_fixture): @pytest.fixture def _fixture_that_needs_loader_modules_patched(): - assert saltfactories.__salt__["foo"] is False # pylint: disable=no-member - with patch.dict(saltfactories.__salt__, {"foo": True}): # pylint: disable=no-member - assert saltfactories.__salt__["foo"] is True # pylint: disable=no-member - yield - assert saltfactories.__salt__["foo"] is False # pylint: disable=no-member + assert saltfactories.__salt__["foo"] is False + try: + with patch.dict(saltfactories.__salt__, {"foo": True}): + assert saltfactories.__salt__["foo"] is True + yield + finally: + assert saltfactories.__salt__["foo"] is False @pytest.mark.usefixtures("_fixture_that_needs_loader_modules_patched") -def test_fixture_deps(): - assert saltfactories.__salt__["foo"] is True # pylint: disable=no-member - assert saltfactories.__salt__["test.echo"]("foo") == "foo" # pylint: disable=no-member +@pytest.mark.parametrize( + "retval", + # These values are equal and only serve the purpose of running the same test + # twice. If patching fails to cleanup the added module globals, the second + # time the test runs the _fixture_that_needs_loader_modules_patched will + # fail it's assertions + [True, True], +) +def test_fixture_deps(retval): + assert saltfactories.__salt__["foo"] is retval + assert saltfactories.__salt__["test.echo"]("foo") == "foo"