Simplify and reduce the amount of patching required to unit test load…

…er modules

Fixes saltstack#156

Signed-off-by: Pedro Algarvio <palgarvio@vmware.com>

changelog/156.improvement.rst

@@ -0,0 +1 @@
+Simplify and reduce the amount of patching required to unit test loader modules

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)
 

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"