From 2bfc92e58dcd2e6091dd3ca0276aa1240579207f Mon Sep 17 00:00:00 2001 From: Chris Julian Date: Wed, 30 Aug 2017 12:52:45 -0400 Subject: #4071 Mixin to prevent setting return_value after initializing certain Mock objects (#4963) * Addressing #4071 Wrote an ImmutableReturnMixin to prevent developers overriding return_value in certain Mock objects * Language * Loosening the assumption that underlying _mock objects need to be Immutable-like simplifies implementation * Addressing #4071 * Ensure side_effects and return_values are pushed down to the underlying _mock in FreezableMocks. And IDisplay mocks are no longer frozen in _create_get_utility_mock() * Edit a handful of tests to not override the mock_get_utility return_value * Brief explainer of FreezableMock.__setattr__ * Incorporating PR feedback and some compatibility * FreezableMock __getattr__ needs a shortcut in case of return_value or side_effect * Changing return_value only forbidden if set before freezing * Remove unnecessary else block * Expanded doc strings * Bring a couple new tests in line with patch_get_utility() norms --- .../certbot_apache/tests/configurator_test.py | 8 ++--- certbot/plugins/standalone_test.py | 6 ++-- certbot/tests/cert_manager_test.py | 11 ++----- certbot/tests/display/ops_test.py | 20 +++++++----- certbot/tests/main_test.py | 6 ++-- certbot/tests/util.py | 37 ++++++++++++++++++---- 6 files changed, 55 insertions(+), 33 deletions(-) diff --git a/certbot-apache/certbot_apache/tests/configurator_test.py b/certbot-apache/certbot_apache/tests/configurator_test.py index db04bfcd1..23242d091 100644 --- a/certbot-apache/certbot_apache/tests/configurator_test.py +++ b/certbot-apache/certbot_apache/tests/configurator_test.py @@ -121,7 +121,8 @@ class MultipleVhostsTest(util.ApacheTest): @certbot_util.patch_get_utility() def test_get_all_names(self, mock_getutility): - mock_getutility.notification = mock.MagicMock(return_value=True) + mock_utility = mock_getutility() + mock_utility.notification = mock.MagicMock(return_value=True) names = self.config.get_all_names() self.assertEqual(names, set( ["certbot.demo", "ocspvhost.com", "encryption-example.demo"] @@ -131,9 +132,8 @@ class MultipleVhostsTest(util.ApacheTest): @mock.patch("certbot_apache.configurator.socket.gethostbyaddr") def test_get_all_names_addrs(self, mock_gethost, mock_getutility): mock_gethost.side_effect = [("google.com", "", ""), socket.error] - notification = mock.Mock() - notification.notification = mock.Mock(return_value=True) - mock_getutility.return_value = notification + mock_utility = mock_getutility() + mock_utility.notification.return_value = True vhost = obj.VirtualHost( "fp", "ap", set([obj.Addr(("8.8.8.8", "443")), diff --git a/certbot/plugins/standalone_test.py b/certbot/plugins/standalone_test.py index 2a55c516f..1ae731e42 100644 --- a/certbot/plugins/standalone_test.py +++ b/certbot/plugins/standalone_test.py @@ -158,10 +158,11 @@ class AuthenticatorTest(unittest.TestCase): @test_util.patch_get_utility() def test_perform_eaddrinuse_retry(self, mock_get_utility): + mock_utility = mock_get_utility() errno = socket.errno.EADDRINUSE error = errors.StandaloneBindError(mock.MagicMock(errno=errno), -1) self.auth.servers.run.side_effect = [error] + 2 * [mock.MagicMock()] - mock_yesno = mock_get_utility.return_value.yesno + mock_yesno = mock_utility.yesno mock_yesno.return_value = True self.test_perform() @@ -169,7 +170,8 @@ class AuthenticatorTest(unittest.TestCase): @test_util.patch_get_utility() def test_perform_eaddrinuse_no_retry(self, mock_get_utility): - mock_yesno = mock_get_utility.return_value.yesno + mock_utility = mock_get_utility() + mock_yesno = mock_utility.yesno mock_yesno.return_value = False errno = socket.errno.EADDRINUSE diff --git a/certbot/tests/cert_manager_test.py b/certbot/tests/cert_manager_test.py index 6c43eb168..6585644cf 100644 --- a/certbot/tests/cert_manager_test.py +++ b/certbot/tests/cert_manager_test.py @@ -346,9 +346,8 @@ class RenameLineageTest(BaseCertManagerTest): self.assertRaises(errors.Error, self._call, self.config) mock_renewal_conf_files.return_value = ["one.conf"] - util_mock = mock.Mock() + util_mock = mock_get_utility() util_mock.menu.return_value = (display_util.CANCEL, 0) - mock_get_utility.return_value = util_mock self.assertRaises(errors.Error, self._call, self.config) util_mock.menu.return_value = (display_util.OK, -1) @@ -359,14 +358,11 @@ class RenameLineageTest(BaseCertManagerTest): self.config.certname = "one" self.config.new_certname = None - util_mock = mock.Mock() + util_mock = mock_get_utility() util_mock.input.return_value = (display_util.CANCEL, "name") - mock_get_utility.return_value = util_mock self.assertRaises(errors.Error, self._call, self.config) - util_mock = mock.Mock() util_mock.input.return_value = (display_util.OK, None) - mock_get_utility.return_value = util_mock self.assertRaises(errors.Error, self._call, self.config) @test_util.patch_get_utility() @@ -393,9 +389,8 @@ class RenameLineageTest(BaseCertManagerTest): def test_rename_cert_interactive_certname(self, mock_check, mock_get_utility): mock_check.return_value = True self.config.certname = None - util_mock = mock.Mock() + util_mock = mock_get_utility() util_mock.menu.return_value = (display_util.OK, 0) - mock_get_utility.return_value = util_mock self._call(self.config) from certbot import cert_manager updated_lineage = cert_manager.lineage_for_certname(self.config, self.config.new_certname) diff --git a/certbot/tests/display/ops_test.py b/certbot/tests/display/ops_test.py index 306b95841..cb0fb32e3 100644 --- a/certbot/tests/display/ops_test.py +++ b/certbot/tests/display/ops_test.py @@ -310,10 +310,11 @@ class ChooseNamesTest(unittest.TestCase): @test_util.patch_get_utility("certbot.display.ops.z_util") def test_choose_manually(self, mock_util): from certbot.display.ops import _choose_names_manually + utility_mock = mock_util() # No retry - mock_util().yesno.return_value = False + utility_mock.yesno.return_value = False # IDN and no retry - mock_util().input.return_value = (display_util.OK, + utility_mock.input.return_value = (display_util.OK, "uniçodé.com") self.assertEqual(_choose_names_manually(), []) # IDN exception with previous mocks @@ -324,7 +325,7 @@ class ChooseNamesTest(unittest.TestCase): mock_sli.side_effect = unicode_error self.assertEqual(_choose_names_manually(), []) # Valid domains - mock_util().input.return_value = (display_util.OK, + utility_mock.input.return_value = (display_util.OK, ("example.com," "under_score.example.com," "justtld," @@ -332,14 +333,17 @@ class ChooseNamesTest(unittest.TestCase): self.assertEqual(_choose_names_manually(), ["example.com", "under_score.example.com", "justtld", "valid.example.com"]) + + @test_util.patch_get_utility("certbot.display.ops.z_util") + def test_choose_manually_retry(self, mock_util): + from certbot.display.ops import _choose_names_manually + utility_mock = mock_util() # Three iterations - mock_util().input.return_value = (display_util.OK, + utility_mock.input.return_value = (display_util.OK, "uniçodé.com") - yn = mock.MagicMock() - yn.side_effect = [True, True, False] - mock_util().yesno = yn + utility_mock.yesno.side_effect = [True, True, False] _choose_names_manually() - self.assertEqual(mock_util().yesno.call_count, 3) + self.assertEqual(utility_mock.yesno.call_count, 3) class SuccessInstallationTest(unittest.TestCase): diff --git a/certbot/tests/main_test.py b/certbot/tests/main_test.py index 4abd344f2..51f0697f4 100644 --- a/certbot/tests/main_test.py +++ b/certbot/tests/main_test.py @@ -164,9 +164,7 @@ class CertonlyTest(unittest.TestCase): self.assertTrue(mock_report_cert.call_count == 2) # error in _ask_user_to_confirm_new_names - util_mock = mock.Mock() - util_mock.yesno.return_value = False - self.mock_get_utility.return_value = util_mock + self.mock_get_utility().yesno.return_value = False self.assertRaises(errors.ConfigurationError, self._call, ('certonly --webroot -d example.com -d test.com --cert-name example.com').split()) @@ -1115,7 +1113,7 @@ class UnregisterTest(unittest.TestCase): def test_abort_unregister(self): self.mocks['account'].AccountFileStorage.return_value = mock.Mock() - util_mock = self.mocks['get_utility'].return_value + util_mock = self.mocks['get_utility']() util_mock.yesno.return_value = False config = mock.Mock() diff --git a/certbot/tests/util.py b/certbot/tests/util.py index 4ecddc34f..698962516 100644 --- a/certbot/tests/util.py +++ b/certbot/tests/util.py @@ -173,8 +173,8 @@ class FreezableMock(object): """Mock object with the ability to freeze attributes. This class works like a regular mock.MagicMock object, except - attributes and behavior can be set and frozen so they cannot be - changed during tests. + attributes and behavior set before the object is frozen cannot + be changed during tests. If a func argument is provided to the constructor, this function is called first when an instance of FreezableMock is called, @@ -182,10 +182,12 @@ class FreezableMock(object): value of func is ignored. """ - def __init__(self, frozen=False, func=None): + def __init__(self, frozen=False, func=None, return_value=mock.sentinel.DEFAULT): self._frozen_set = set() if frozen else set(('freeze',)) self._func = func self._mock = mock.MagicMock() + if return_value != mock.sentinel.DEFAULT: + self.return_value = return_value self._frozen = frozen def freeze(self): @@ -203,17 +205,38 @@ class FreezableMock(object): return object.__getattribute__(self, name) except AttributeError: return False + elif name in ('return_value', 'side_effect',): + return getattr(object.__getattribute__(self, '_mock'), name) elif name == '_frozen_set' or name in self._frozen_set: return object.__getattribute__(self, name) else: return getattr(object.__getattribute__(self, '_mock'), name) def __setattr__(self, name, value): + """ Before it is frozen, attributes are set on the FreezableMock + instance and added to the _frozen_set. Attributes in the _frozen_set + cannot be changed after the FreezableMock is frozen. In this case, + they are set on the underlying _mock. + + In cases of return_value and side_effect, these attributes are always + passed through to the instance's _mock and added to the _frozen_set + before the object is frozen. + + """ if self._frozen: - return setattr(self._mock, name, value) - elif name != '_frozen_set': + if name in self._frozen_set: + raise AttributeError('Cannot change frozen attribute ' + name) + else: + return setattr(self._mock, name, value) + + if name != '_frozen_set': self._frozen_set.add(name) - return object.__setattr__(self, name, value) + + if name in ('return_value', 'side_effect'): + return setattr(self._mock, name, value) + + else: + return object.__setattr__(self, name, value) def _create_get_utility_mock(): @@ -223,7 +246,7 @@ def _create_get_utility_mock(): frozen_mock = FreezableMock(frozen=True, func=_assert_valid_call) setattr(display, name, frozen_mock) display.freeze() - return mock.MagicMock(return_value=display) + return FreezableMock(frozen=True, return_value=display) def _assert_valid_call(*args, **kwargs): -- cgit v1.2.3