Merge "Fix issue with double mocking of utils.execute functions"

3 files changed, 134 insertions, 17 deletions

diff --git a/ironic/tests/base.py b/ironic/tests/base.py
index 9bca7a2ce..5a3851bd0 100644
--- a/ironic/tests/base.py
+++ b/ironic/tests/base.py
@@ -32,7 +32,6 @@ eventlet.monkey_patch(os=False)
 
 import fixtures  # noqa for I202 due to 'import eventlet' above
 from ironic_lib import utils
-import mock
 from oslo_concurrency import processutils
 from oslo_config import fixture as config_fixture
 from oslo_log import log as logging
@@ -103,22 +102,21 @@ class TestCase(oslo_test_base.BaseTestCase):
         for factory in driver_factory._INTERFACE_LOADERS.values():
             factory._extension_manager = None
 
-        # Block access to utils.execute() and related functions.
-        # 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!")
-
-            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)
+            # 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)
+            # subprocess.Popen is a class
+            self.patch(subprocess, 'Popen', DoNotCallPopen)
 
     def _set_config(self):
         self.cfg_fixture = self.useFixture(config_fixture.Config(CONF))
@@ -201,3 +199,35 @@ class TestCase(oslo_test_base.BaseTestCase):
         self.assertEqual(event_type, notif_args['event_type'].
                          to_event_type_field())
         self.assertEqual(level, notif_args['level'])
+
+
+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
diff --git a/ironic/tests/unit/drivers/modules/test_console_utils.py b/ironic/tests/unit/drivers/modules/test_console_utils.py
index d9f0e6719..a5ee94128 100644
--- a/ironic/tests/unit/drivers/modules/test_console_utils.py
+++ b/ironic/tests/unit/drivers/modules/test_console_utils.py
@@ -557,7 +557,7 @@ class ConsoleUtilsTestCase(db_base.DbTestCase):
 
         mock_stop.assert_called_once_with(self.info['uuid'])
         mock_dir_exists.assert_called_once_with()
-        mock_popen.assert_not_called()
+        self.assertEqual(0, mock_popen.call_count)
 
     @mock.patch.object(console_utils, '_stop_console', autospec=True)
     def test_stop_socat_console(self, mock_stop):
diff --git a/ironic/tests/unit/test_base.py b/ironic/tests/unit/test_base.py
new file mode 100644
index 000000000..338983516
--- /dev/null
+++ b/ironic/tests/unit/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
+
+from ironic_lib import utils
+import mock
+from oslo_concurrency import processutils
+
+from ironic.tests import base
+
+
+class BlockExecuteTestCase(base.TestCase):
+    """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.TestCase):
+    """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)