From e0d22ba4dea6d7b70faba14c6918fbc2151983a5 Mon Sep 17 00:00:00 2001 From: Balazs Gibizer Date: Fri, 12 Dec 2014 14:33:28 +0100 Subject: [PATCH] Reject non existent mock assert calls assert_called and assert_not_called are not asserting the state of the mock object but considered as mocked calls so mock will never raise exception but always executed successfully This change patches the Mock class during unit test to raise an exception if a function called on a mock object that's name starts with 'assert' and does not one of the supported Mock assert calls. This change also fix the unit test to call only the supported assert function on mock object. This change also removes hacking rule N327 as the mock change renders this hacking rule obsolete. Change-Id: Id75def84bd4bdc1c1fb06221b4223322a99251eb --- nova/hacking/checks.py | 12 ----------- nova/test.py | 24 +++++++++++++++++++++ nova/tests/unit/compute/test_compute.py | 2 +- nova/tests/unit/test_hacking.py | 12 ----------- nova/tests/unit/virt/libvirt/test_driver.py | 15 ++++++------- 5 files changed, 31 insertions(+), 34 deletions(-) diff --git a/nova/hacking/checks.py b/nova/hacking/checks.py index 795ed45278c0..7d6ef2c92836 100644 --- a/nova/hacking/checks.py +++ b/nova/hacking/checks.py @@ -379,17 +379,6 @@ def use_jsonutils(logical_line, filename): yield (pos, msg % {'fun': f[:-1]}) -def check_assert_called_once(logical_line, filename): - msg = ("N327: assert_called_once is a no-op. please use assert_called_" - "once_with to test with explicit parameters or an assertEqual with" - " call_count.") - - if 'nova/tests/' in filename: - pos = logical_line.find('.assert_called_once(') - if pos != -1: - yield (pos, msg) - - def check_api_version_decorator(logical_line, blank_before, filename): msg = ("N332: the api_version decorator must be the first decorator" " on a method.") @@ -474,7 +463,6 @@ def factory(register): register(no_mutable_default_args) register(check_explicit_underscore_import) register(use_jsonutils) - register(check_assert_called_once) register(check_api_version_decorator) register(CheckForStrUnicodeExc) register(CheckForTransAdd) diff --git a/nova/test.py b/nova/test.py index 53948adac98d..b6ae5ad5a43f 100644 --- a/nova/test.py +++ b/nova/test.py @@ -26,6 +26,7 @@ eventlet.monkey_patch(os=False) import copy import inspect +import mock import logging import os @@ -157,6 +158,29 @@ class skipXmlTest(skipIf): reason) +def _patch_mock_to_raise_for_invalid_assert_calls(): + def raise_for_invalid_assert_calls(wrapped): + def wrapper(_self, name): + valid_asserts = [ + 'assert_called_with', + 'assert_called_once_with', + 'assert_has_calls', + 'assert_any_calls'] + + if name.startswith('assert') and name not in valid_asserts: + raise AttributeError('%s is not a valid mock assert method' + % name) + + return wrapped(_self, name) + return wrapper + mock.Mock.__getattr__ = raise_for_invalid_assert_calls( + mock.Mock.__getattr__) + +# NOTE(gibi): needs to be called only once at import time +# to patch the mock lib +_patch_mock_to_raise_for_invalid_assert_calls() + + class TestCase(testtools.TestCase): """Test case base class for all unit tests. diff --git a/nova/tests/unit/compute/test_compute.py b/nova/tests/unit/compute/test_compute.py index 564811ccf6d8..83876be145d7 100644 --- a/nova/tests/unit/compute/test_compute.py +++ b/nova/tests/unit/compute/test_compute.py @@ -5017,7 +5017,7 @@ class ComputeTestCase(BaseTestCase): self.context, instance, bdms='fake_bdms') mock_terminate_vol_conn.assert_called_once_with(self.context, instance, 'fake_bdms') - mock_get_power_off_values.assert_caleld_once_with(self.context, + mock_get_power_off_values.assert_called_once_with(self.context, instance, clean_shutdown) self.assertEqual(migration.dest_compute, instance.host) self.compute.terminate_instance(self.context, instance, [], []) diff --git a/nova/tests/unit/test_hacking.py b/nova/tests/unit/test_hacking.py index b870f27df8cd..b245f3f1054c 100644 --- a/nova/tests/unit/test_hacking.py +++ b/nova/tests/unit/test_hacking.py @@ -291,18 +291,6 @@ class HackingTestCase(test.NoDBTestCase): self._run_check(code, checker, filename)] self.assertEqual(expected_errors or [], actual_errors) - def test_assert_called_once(self): - - checker = checks.check_assert_called_once - code = """ - mock = Mock() - mock.method(1, 2, 3, test='wow') - mock.method.assert_called_once() - """ - errors = [(3, 11, 'N327')] - self._assert_has_errors(code, checker, expected_errors=errors, - filename='nova/tests/test_assert.py') - def test_str_unicode_exception(self): checker = checks.CheckForStrUnicodeExc diff --git a/nova/tests/unit/virt/libvirt/test_driver.py b/nova/tests/unit/virt/libvirt/test_driver.py index 2192f7ad4182..04e4b3ae2edc 100644 --- a/nova/tests/unit/virt/libvirt/test_driver.py +++ b/nova/tests/unit/virt/libvirt/test_driver.py @@ -8513,7 +8513,7 @@ class LibvirtConnTestCase(test.NoDBTestCase): result = drvr._get_disk_over_committed_size_total() self.assertEqual(result, 10653532160) mock_list.assert_called_with() - mock_info.assert_called() + self.assertTrue(mock_info.called) @mock.patch.object(libvirt_driver.LibvirtDriver, "_list_instance_domains") @@ -9631,20 +9631,17 @@ Active: 8381604 kB dom_mock.ID.assert_called_once_with() lookup_mock.assert_called_once_with(instance['name']) - @mock.patch.object(fake_libvirt_utils, 'get_instance_path') @mock.patch.object(encodeutils, 'safe_decode') - def test_create_domain(self, mock_safe_decode, mock_get_inst_path): + def test_create_domain(self, mock_safe_decode): conn = libvirt_driver.LibvirtDriver(fake.FakeVirtAPI(), True) mock_domain = mock.MagicMock() mock_instance = mock.MagicMock() - mock_get_inst_path.return_value = '/tmp/' domain = conn._create_domain(domain=mock_domain, instance=mock_instance) self.assertEqual(mock_domain, domain) - mock_get_inst_path.assertHasCalls([mock.call(mock_instance)]) - mock_domain.createWithFlags.assertHasCalls([mock.call(0)]) + mock_domain.createWithFlags.assert_has_calls([mock.call(0)]) self.assertEqual(2, mock_safe_decode.call_count) @mock.patch('nova.virt.disk.api.clean_lxc_namespace') @@ -9682,7 +9679,7 @@ Active: 8381604 kB mock_instance, [], None) self.assertEqual('/dev/nbd0', inst_sys_meta['rootfs_device_name']) - mock_instance.save.assert_not_called() + self.assertFalse(mock_instance.called) mock_get_inst_path.assert_has_calls([mock.call(mock_instance)]) mock_ensure_tree.assert_has_calls([mock.call('/tmp/rootfs')]) conn.image_backend.image.assert_has_calls([mock.call(mock_instance, @@ -9745,7 +9742,7 @@ Active: 8381604 kB mock_instance, [], None) self.assertEqual('/dev/nbd0', inst_sys_meta['rootfs_device_name']) - mock_instance.save.assert_not_called() + self.assertFalse(mock_instance.called) mock_get_inst_path.assert_has_calls([mock.call(mock_instance)]) mock_ensure_tree.assert_has_calls([mock.call('/tmp/rootfs')]) conn.image_backend.image.assert_has_calls([mock.call(mock_instance, @@ -9794,7 +9791,7 @@ Active: 8381604 kB mock_instance, [], None) self.assertEqual('/dev/nbd0', inst_sys_meta['rootfs_device_name']) - mock_instance.save.assert_not_called() + self.assertFalse(mock_instance.called) mock_get_inst_path.assert_has_calls([mock.call(mock_instance)]) mock_ensure_tree.assert_has_calls([mock.call('/tmp/rootfs')]) conn.image_backend.image.assert_has_calls([mock.call(mock_instance,