#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

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):