From 1dd12d8deb229216189b0a075052e104f73de111 Mon Sep 17 00:00:00 2001 From: Akihiro Motoki Date: Fri, 26 Feb 2021 18:29:02 +0900 Subject: [PATCH] test: Ensure to stop mock when create_mocks decorator exits During reviewing https://review.opendev.org/c/openstack/horizon/+/772603, we noticed that a method decorated by create_mocks is called multiple time in a single test. Previously create_mocks decorator does not stop mocking, so this means that multiple active mock can exist for one method. In general, it is not a good idea to mock a method multiple times at the same time. To cope with this situation, this commit ensures for create_mocks decorator to stop active mocks when exiting the decorator. This works for most cases, but it does not work only when mocking and assertions are handled by separate methods and test logic is placed between them. To cope with this, "stop_mock" optional argument is introduced. FYI: Details on why "stop_mock" is needed. I explored various ways but could not find a good way so far. create_mocks needs to be a decorator as it needs to refer an object reference (self). On the other hand, if we would like to merge the logic of mocking and assertions (for example, _stub_api_calls() and _check_api_calls() in openstack_dashboard/dashboards/project/overview/tests.py), a context manager would be good as we would like to call it inside a class. However, we cannot mix a decorator and a context manger as a decorator is executed when a context manager is iniitialized and stopping mock in the create_mocks decorator is done during the initialization of the context manager and methods are not mocked when test code is run. Change-Id: I9e37dc1eaa08adf36d11975fed6f5a0a90cdde52 --- .../dashboards/project/instances/tests.py | 5 ++++- .../dashboards/project/overview/tests.py | 9 ++++++--- openstack_dashboard/test/helpers.py | 13 +++++++++++-- 3 files changed, 21 insertions(+), 6 deletions(-) diff --git a/openstack_dashboard/dashboards/project/instances/tests.py b/openstack_dashboard/dashboards/project/instances/tests.py index 6ad04a529f..8fc35415b2 100644 --- a/openstack_dashboard/dashboards/project/instances/tests.py +++ b/openstack_dashboard/dashboards/project/instances/tests.py @@ -194,7 +194,10 @@ class InstanceTableTests(InstanceTestBase, InstanceTableTestMixin): 'servers_update_addresses', ), api.cinder: ('volume_list',), - }) + }, stop_mock=False) + # NOTE: _get_index() and _check_get_index() are used as pair + # and the test logic will be placed between these calls, + # so we cannot stop mocking when exiting this method. def _get_index(self, use_servers_update_address=True): servers = self.servers.list() self.mock_is_feature_available.return_value = True diff --git a/openstack_dashboard/dashboards/project/overview/tests.py b/openstack_dashboard/dashboards/project/overview/tests.py index d6dac2cdfb..0e07f28b4f 100644 --- a/openstack_dashboard/dashboards/project/overview/tests.py +++ b/openstack_dashboard/dashboards/project/overview/tests.py @@ -35,8 +35,12 @@ class UsageViewTests(test.TestCase): @test.create_mocks({ api.nova: ('usage_get',), - api.neutron: ('is_quotas_extension_supported',) - }) + api.neutron: ('is_quotas_extension_supported',), + usage.quotas: ('tenant_quota_usages',), + }, stop_mock=False) + # NOTE: _stub_api_calls() and _check_api_calls() are used as pair + # and the test logic will be placed between these calls, + # so we cannot stop mocking when exiting this method. def _stub_api_calls(self, nova_stu_enabled=True, stu_exception=False, overview_days_range=1, quota_usage_overrides=None, @@ -70,7 +74,6 @@ class UsageViewTests(test.TestCase): usages.add_quota(api.base.Quota(k, quota)) usages.tally(k, quota_usages[k]['used']) - @test.create_mocks({usage.quotas: ('tenant_quota_usages',)}) def _stub_tenant_quota_usages(self, overrides): usages_data = usage.quotas.QuotaUsage() self._add_quota_usages(usages_data, self.quota_usages.first(), diff --git a/openstack_dashboard/test/helpers.py b/openstack_dashboard/test/helpers.py index fc3c3576ec..42ef34fc73 100644 --- a/openstack_dashboard/test/helpers.py +++ b/openstack_dashboard/test/helpers.py @@ -54,7 +54,7 @@ IsA = horizon_helpers.IsA IsHttpRequest = horizon_helpers.IsHttpRequest -def create_mocks(target_methods): +def create_mocks(target_methods, stop_mock=True): """decorator to simplify setting up multiple mocks at once :param target_methods: a dict to define methods to be patched using mock. @@ -109,10 +109,14 @@ def create_mocks(target_methods): self.mock_cinder_tenant_absolute_limits.return_value = ... ... + :param stop_mock: If True (default), mocks started in this decorator will + be stopped. Set this to False only if you cannot stop mocks when exiting + this decorator. The default value, True, should work for most cases. """ def wrapper(function): @wraps(function) def wrapped(inst, *args, **kwargs): + patchers = [] for target, methods in target_methods.items(): for method in methods: if isinstance(method, str): @@ -122,8 +126,13 @@ def create_mocks(target_methods): method_mocked = method[0] attr_name = method[1] m = mock.patch.object(target, method_mocked) + patchers.append(m) setattr(inst, 'mock_%s' % attr_name, m.start()) - return function(inst, *args, **kwargs) + retval = function(inst, *args, **kwargs) + if stop_mock: + for m in patchers: + m.stop() + return retval return wrapped return wrapper