Remove double mocking

In py310 unittest.mock does not allow to mock the same function twice as
the second mocking will fail to autospec the Mock object created by the
first mocking.

This patch manually fixes the double mocking.

Fixed cases:
1) one of the mock was totally unnecessary so it was removed
2) the second mock specialized the behavior of the first generic mock.
   In this case the second mock is replaced with the configuration of
   the first mock
3) a test case with two test steps mocked the same function for each
   step with overlapping mocks. Here the overlap was removed to have
   the two mock exists independently

The get_connection injection in the libvirt functional test needed a
further tweak (yeah I know it has many already) to act like a single
mock (basically case #2) instead of a temporary re-mocking. Still the
globalness of the get_connection mocking warrant the special set / reset
logic there.

Change-Id: I3998d0d49583806ac1c3ae64f1b1fe343cefd20d

Author: gibizer

nova/test.py

@@ -355,7 +355,7 @@ def stub_out(self, old, new):
         self.useFixture(fixtures.MonkeyPatch(old, new))
 
     @staticmethod
-    def patch_exists(patched_path, result):
+    def patch_exists(patched_path, result, other=None):
         """Provide a static method version of patch_exists(), which if you
         haven't already imported nova.test can be slightly easier to
         use as a context manager within a test method via:
@@ -364,7 +364,7 @@ def test_something(self):
                 with self.patch_exists(path, True):
                     ...
         """
-        return patch_exists(patched_path, result)
+        return patch_exists(patched_path, result, other)
 
     @staticmethod
     def patch_open(patched_path, read_data):
@@ -848,10 +848,12 @@ def __repr__(self):
 
 
 @contextlib.contextmanager
-def patch_exists(patched_path, result):
+def patch_exists(patched_path, result, other=None):
     """Selectively patch os.path.exists() so that if it's called with
     patched_path, return result.  Calls with any other path are passed
-    through to the real os.path.exists() function.
+    through to the real os.path.exists() function if other is not provided.
+    If other is provided then that will be the result of the call on paths
+    other than patched_path.
 
     Either import and use as a decorator / context manager, or use the
     nova.TestCase.patch_exists() static method as a context manager.
@@ -885,7 +887,10 @@ def test_my_code(self, mock_exists):
     def fake_exists(path):
         if path == patched_path:
             return result
-        return real_exists(path)
+        elif other is not None:
+            return other
+        else:
+            return real_exists(path)
 
     with mock.patch.object(os.path, "exists") as mock_exists:
         mock_exists.side_effect = fake_exists

nova/tests/functional/libvirt/base.py

@@ -142,15 +142,15 @@ def _start_compute(hostname, host_info):
                     pci_info.get_pci_address_mac_mapping())
             # This is fun. Firstly we need to do a global'ish mock so we can
             # actually start the service.
-            with mock.patch('nova.virt.libvirt.host.Host.get_connection',
-                            return_value=fake_connection):
-                compute = self.start_service('compute', host=hostname)
-                # Once that's done, we need to tweak the compute "service" to
-                # make sure it returns unique objects. We do this inside the
-                # mock context to avoid a small window between the end of the
-                # context and the tweaking where get_connection would revert to
-                # being an autospec mock.
-                compute.driver._host.get_connection = lambda: fake_connection
+            orig_con = self.mock_conn.return_value
+            self.mock_conn.return_value = fake_connection
+            compute = self.start_service('compute', host=hostname)
+            # Once that's done, we need to tweak the compute "service" to
+            # make sure it returns unique objects.
+            compute.driver._host.get_connection = lambda: fake_connection
+            # Then we revert the local mock tweaking so the next compute can
+            # get its own
+            self.mock_conn.return_value = orig_con
             return compute
 
         # ensure we haven't already registered services with these hostnames

nova/tests/functional/libvirt/test_vtpm.py

@@ -128,7 +128,7 @@ def setUp(self):
         # the presence of users on the host, none of which makes sense here
         _p = mock.patch(
             'nova.virt.libvirt.driver.LibvirtDriver._check_vtpm_support')
-        self.mock_conn = _p.start()
+        _p.start()
         self.addCleanup(_p.stop)
 
         self.key_mgr = crypto._get_key_manager()

nova/tests/functional/regressions/test_bug_1781286.py

@@ -67,11 +67,11 @@ def test_server_create_reschedule_blocked_az_up_call(self):
         def wrap_bari(*args, **kwargs):
             # Poison the AZ query to blow up as if the cell conductor does not
             # have access to the API DB.
-            patcher = mock.patch('nova.objects.AggregateList.get_by_host',
-                                 side_effect=oslo_db_exc.CantStartEngineError)
-            patcher.start()
-            self.addCleanup(patcher.stop)
-            return original_bari(*args, **kwargs)
+            with mock.patch(
+                'nova.objects.AggregateList.get_by_host',
+                side_effect=oslo_db_exc.CantStartEngineError
+            ):
+                return original_bari(*args, **kwargs)
 
         self.stub_out('nova.compute.manager.ComputeManager.'
                       'build_and_run_instance', wrap_bari)
@@ -81,10 +81,6 @@ def wrap_bari(*args, **kwargs):
         # compute service we have to wait for the notification that the build
         # is complete and then stop the mock so we can use the API again.
         self.notifier.wait_for_versioned_notifications('instance.create.end')
-        # Note that we use stopall here because we actually called
-        # build_and_run_instance twice so we have more than one instance of
-        # the mock that needs to be stopped.
-        mock.patch.stopall()
         server = self._wait_for_state_change(server, 'ACTIVE')
         # We should have rescheduled and the instance AZ should be set from the
         # Selection object. Since neither compute host is in an AZ, the server
@@ -128,20 +124,20 @@ def test_migrate_reschedule_blocked_az_up_call(self):
         self.rescheduled = None
 
         def wrap_prep_resize(_self, *args, **kwargs):
-            # Poison the AZ query to blow up as if the cell conductor does not
-            # have access to the API DB.
-            patcher = mock.patch('nova.objects.AggregateList.get_by_host',
-                                 side_effect=oslo_db_exc.CantStartEngineError)
-            self.agg_mock = patcher.start()
-            self.addCleanup(patcher.stop)
-
             if self.rescheduled is None:
                 # Track the first host that we rescheduled from.
                 self.rescheduled = _self.host
                 # Trigger a reschedule.
                 raise exception.ComputeResourcesUnavailable(
                     reason='test_migrate_reschedule_blocked_az_up_call')
-            return original_prep_resize(_self, *args, **kwargs)
+            # Poison the AZ query to blow up as if the cell conductor does not
+            # have access to the API DB.
+            with mock.patch(
+                'nova.objects.AggregateList.get_by_host',
+                side_effect=oslo_db_exc.CantStartEngineError,
+            ) as agg_mock:
+                self.agg_mock = agg_mock
+                return original_prep_resize(_self, *args, **kwargs)
 
         self.stub_out('nova.compute.manager.ComputeManager._prep_resize',
                       wrap_prep_resize)