Fix issue with double mocking of utils.execute functions

An issue was discovered if we mock an already mock-ed function. This
was happening in our execute() detection code. Change it to not use a
mock and instead a function.

Add test cases to ensure it works as expected.

Change-Id: If58071b41c7e69bc182ea033bf647924f996f1f3

ironic_lib/tests/base.py

@@ -17,7 +17,6 @@
 
 import subprocess
 
-import mock
 from oslo_concurrency import processutils
 from oslotest import base as test_base
 
@@ -31,29 +30,57 @@ class IronicLibTestCase(test_base.BaseTestCase):
     processutils.execute() and similar functions.
     """
 
+    # By default block execution of utils.execute() and related functions.
     block_execute = True
 
     def setUp(self):
         super(IronicLibTestCase, self).setUp()
-        """Add a blanket ban on running external processes via utils.execute().
 
-        `self` will create a property called _exec_patch which is the Mock that
-        replaces utils.execute.
-
-        If the mock is called, an exception is raised to warn the tester.
-        """
-        # NOTE(bigjools): Not using a decorator on tests because I don't
-        # want to force every test method to accept a new arg. Instead, they
-        # can override or examine this self._exec_patch Mock as needed.
+        # Ban running external processes via 'execute' like functions. If the
+        # patched function is called, an exception is raised to warn the
+        # tester.
         if self.block_execute:
-            self._exec_patch = mock.Mock()
-            self._exec_patch.side_effect = Exception(
-                "Don't call ironic_lib.utils.execute() / "
-                "processutils.execute() or similar functions in tests!")
+            # NOTE(jlvillal): Intentionally not using mock as if you mock a
+            # mock it causes things to not work correctly. As doing an
+            # autospec=True causes strangeness. By using a simple function we
+            # can then mock it without issue.
+            self.patch(processutils, 'execute', do_not_call)
+            self.patch(subprocess, 'call', do_not_call)
+            self.patch(subprocess, 'check_call', do_not_call)
+            self.patch(subprocess, 'check_output', do_not_call)
+            self.patch(utils, 'execute', do_not_call)
 
-            self.patch(processutils, 'execute', self._exec_patch)
-            self.patch(subprocess, 'Popen', self._exec_patch)
-            self.patch(subprocess, 'call', self._exec_patch)
-            self.patch(subprocess, 'check_call', self._exec_patch)
-            self.patch(subprocess, 'check_output', self._exec_patch)
-            self.patch(utils, 'execute', self._exec_patch)
+            # subprocess.Popen is a class
+            self.patch(subprocess, 'Popen', DoNotCallPopen)
+
+
+def do_not_call(*args, **kwargs):
+    """Helper function to raise an exception if it is called"""
+    raise Exception(
+        "Don't call ironic_lib.utils.execute() / "
+        "processutils.execute() or similar functions in tests!")
+
+
+class DoNotCallPopen(object):
+    """Helper class to mimic subprocess.popen()
+
+    It's job is to raise an exception if it is called. We create stub functions
+    so mocks that use autospec=True will work.
+    """
+    def __init__(self, *args, **kwargs):
+        do_not_call(*args, **kwargs)
+
+    def communicate(input=None):
+        pass
+
+    def kill():
+        pass
+
+    def poll():
+        pass
+
+    def terminate():
+        pass
+
+    def wait():
+        pass

ironic_lib/tests/test_base.py

@@ -0,0 +1,87 @@
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#    http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
+# implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+import subprocess
+
+import mock
+from oslo_concurrency import processutils
+
+from ironic_lib.tests import base
+from ironic_lib import utils
+
+
+class BlockExecuteTestCase(base.IronicLibTestCase):
+    """Test to ensure we block access to the 'execute' type functions"""
+
+    def test_exception_raised_for_execute(self):
+        execute_functions = (processutils.execute, subprocess.Popen,
+                             subprocess.call, subprocess.check_call,
+                             subprocess.check_output, utils.execute)
+
+        for function_name in execute_functions:
+            exc = self.assertRaises(
+                Exception,
+                function_name,
+                ["echo", "%s" % function_name])  # noqa
+            # Have to use 'noqa' as we are raising plain Exception and we will
+            # get H202 error in 'pep8' check.
+
+            self.assertEqual(
+                "Don't call ironic_lib.utils.execute() / "
+                "processutils.execute() or similar functions in tests!",
+                "%s" % exc)
+
+    @mock.patch.object(utils, "execute", autospec=True)
+    def test_can_mock_execute(self, mock_exec):
+        # NOTE(jlvillal): We had discovered an issue where mocking wasn't
+        # working because we had used a mock to block access to the execute
+        # functions. This caused us to "mock a mock" and didn't work correctly.
+        # We want to make sure that we can mock our execute functions even with
+        # our "block execute" code.
+        utils.execute("ls")
+        utils.execute("echo")
+        self.assertEqual(2, mock_exec.call_count)
+
+    @mock.patch.object(processutils, "execute", autospec=True)
+    def test_exception_raised_for_execute_parent_mocked(self, mock_exec):
+        # Make sure that even if we mock the parent execute function, that we
+        # still get an exception for a child. So in this case
+        # ironic_lib.utils.execute() calls processutils.execute(). Make sure an
+        # exception is raised even though we mocked processutils.execute()
+        exc = self.assertRaises(
+            Exception,
+            utils.execute,
+            "ls")  # noqa
+        # Have to use 'noqa' as we are raising plain Exception and we will get
+        # H202 error in 'pep8' check.
+
+        self.assertEqual(
+            "Don't call ironic_lib.utils.execute() / "
+            "processutils.execute() or similar functions in tests!",
+            "%s" % exc)
+
+
+class DontBlockExecuteTestCase(base.IronicLibTestCase):
+    """Ensure we can turn off blocking access to 'execute' type functions"""
+
+    # Don't block the execute function
+    block_execute = False
+
+    @mock.patch.object(processutils, "execute", autospec=True)
+    def test_no_exception_raised_for_execute(self, mock_exec):
+        # Make sure we can call ironic_lib.utils.execute() even though we
+        # didn't mock it. We do mock processutils.execute() so we don't
+        # actually execute anything.
+        utils.execute("ls")
+        utils.execute("echo")
+        self.assertEqual(2, mock_exec.call_count)