Skip to content

Commit 54c7c97

Browse files
gibizerSeanMooney
authored andcommitted
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. Conflicts: nova/tests/unit/api/openstack/compute/test_limits.py nova/tests/unit/virt/libvirt/volume/test_lightos.py nova/tests/unit/virt/test_block_device.py Change-Id: I3998d0d49583806ac1c3ae64f1b1fe343cefd20d (cherry picked from commit f8cf050) (cherry picked from commit b40bd1b)
1 parent a340630 commit 54c7c97

File tree

19 files changed

+527
-658
lines changed

19 files changed

+527
-658
lines changed

nova/test.py

Lines changed: 10 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -355,7 +355,7 @@ def stub_out(self, old, new):
355355
self.useFixture(fixtures.MonkeyPatch(old, new))
356356

357357
@staticmethod
358-
def patch_exists(patched_path, result):
358+
def patch_exists(patched_path, result, other=None):
359359
"""Provide a static method version of patch_exists(), which if you
360360
haven't already imported nova.test can be slightly easier to
361361
use as a context manager within a test method via:
@@ -364,7 +364,7 @@ def test_something(self):
364364
with self.patch_exists(path, True):
365365
...
366366
"""
367-
return patch_exists(patched_path, result)
367+
return patch_exists(patched_path, result, other)
368368

369369
@staticmethod
370370
def patch_open(patched_path, read_data):
@@ -845,10 +845,12 @@ def __repr__(self):
845845

846846

847847
@contextlib.contextmanager
848-
def patch_exists(patched_path, result):
848+
def patch_exists(patched_path, result, other=None):
849849
"""Selectively patch os.path.exists() so that if it's called with
850850
patched_path, return result. Calls with any other path are passed
851-
through to the real os.path.exists() function.
851+
through to the real os.path.exists() function if other is not provided.
852+
If other is provided then that will be the result of the call on paths
853+
other than patched_path.
852854
853855
Either import and use as a decorator / context manager, or use the
854856
nova.TestCase.patch_exists() static method as a context manager.
@@ -882,7 +884,10 @@ def test_my_code(self, mock_exists):
882884
def fake_exists(path):
883885
if path == patched_path:
884886
return result
885-
return real_exists(path)
887+
elif other is not None:
888+
return other
889+
else:
890+
return real_exists(path)
886891

887892
with mock.patch.object(os.path, "exists") as mock_exists:
888893
mock_exists.side_effect = fake_exists

nova/tests/functional/libvirt/base.py

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -142,15 +142,15 @@ def _start_compute(hostname, host_info):
142142
pci_info.get_pci_address_mac_mapping())
143143
# This is fun. Firstly we need to do a global'ish mock so we can
144144
# actually start the service.
145-
with mock.patch('nova.virt.libvirt.host.Host.get_connection',
146-
return_value=fake_connection):
147-
compute = self.start_service('compute', host=hostname)
148-
# Once that's done, we need to tweak the compute "service" to
149-
# make sure it returns unique objects. We do this inside the
150-
# mock context to avoid a small window between the end of the
151-
# context and the tweaking where get_connection would revert to
152-
# being an autospec mock.
153-
compute.driver._host.get_connection = lambda: fake_connection
145+
orig_con = self.mock_conn.return_value
146+
self.mock_conn.return_value = fake_connection
147+
compute = self.start_service('compute', host=hostname)
148+
# Once that's done, we need to tweak the compute "service" to
149+
# make sure it returns unique objects.
150+
compute.driver._host.get_connection = lambda: fake_connection
151+
# Then we revert the local mock tweaking so the next compute can
152+
# get its own
153+
self.mock_conn.return_value = orig_con
154154
return compute
155155

156156
# ensure we haven't already registered services with these hostnames

nova/tests/functional/libvirt/test_vtpm.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -128,7 +128,7 @@ def setUp(self):
128128
# the presence of users on the host, none of which makes sense here
129129
_p = mock.patch(
130130
'nova.virt.libvirt.driver.LibvirtDriver._check_vtpm_support')
131-
self.mock_conn = _p.start()
131+
_p.start()
132132
self.addCleanup(_p.stop)
133133

134134
self.key_mgr = crypto._get_key_manager()

nova/tests/functional/regressions/test_bug_1781286.py

Lines changed: 13 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,6 @@
1010
# License for the specific language governing permissions and limitations
1111
# under the License.
1212

13-
import fixtures
1413
import mock
1514
from oslo_db import exception as oslo_db_exc
1615

@@ -66,11 +65,11 @@ def test_server_create_reschedule_blocked_az_up_call(self):
6665
def wrap_bari(*args, **kwargs):
6766
# Poison the AZ query to blow up as if the cell conductor does not
6867
# have access to the API DB.
69-
self.useFixture(
70-
fixtures.MockPatch(
71-
'nova.objects.AggregateList.get_by_host',
72-
side_effect=oslo_db_exc.CantStartEngineError))
73-
return original_bari(*args, **kwargs)
68+
with mock.patch(
69+
'nova.objects.AggregateList.get_by_host',
70+
side_effect=oslo_db_exc.CantStartEngineError
71+
):
72+
return original_bari(*args, **kwargs)
7473

7574
self.stub_out('nova.compute.manager.ComputeManager.'
7675
'build_and_run_instance', wrap_bari)
@@ -80,10 +79,6 @@ def wrap_bari(*args, **kwargs):
8079
# compute service we have to wait for the notification that the build
8180
# is complete and then stop the mock so we can use the API again.
8281
self.notifier.wait_for_versioned_notifications('instance.create.end')
83-
# Note that we use stopall here because we actually called
84-
# build_and_run_instance twice so we have more than one instance of
85-
# the mock that needs to be stopped.
86-
mock.patch.stopall()
8782
server = self._wait_for_state_change(server, 'ACTIVE')
8883
# We should have rescheduled and the instance AZ should be set from the
8984
# Selection object. Since neither compute host is in an AZ, the server
@@ -126,19 +121,20 @@ def test_migrate_reschedule_blocked_az_up_call(self):
126121
self.rescheduled = None
127122

128123
def wrap_prep_resize(_self, *args, **kwargs):
129-
# Poison the AZ query to blow up as if the cell conductor does not
130-
# have access to the API DB.
131-
self.agg_mock = self.useFixture(
132-
fixtures.MockPatch(
133-
'nova.objects.AggregateList.get_by_host',
134-
side_effect=oslo_db_exc.CantStartEngineError)).mock
135124
if self.rescheduled is None:
136125
# Track the first host that we rescheduled from.
137126
self.rescheduled = _self.host
138127
# Trigger a reschedule.
139128
raise exception.ComputeResourcesUnavailable(
140129
reason='test_migrate_reschedule_blocked_az_up_call')
141-
return original_prep_resize(_self, *args, **kwargs)
130+
# Poison the AZ query to blow up as if the cell conductor does not
131+
# have access to the API DB.
132+
with mock.patch(
133+
'nova.objects.AggregateList.get_by_host',
134+
side_effect=oslo_db_exc.CantStartEngineError,
135+
) as agg_mock:
136+
self.agg_mock = agg_mock
137+
return original_prep_resize(_self, *args, **kwargs)
142138

143139
self.stub_out('nova.compute.manager.ComputeManager._prep_resize',
144140
wrap_prep_resize)

0 commit comments

Comments
 (0)