openstack/ironic-python-agent
Code Issues Proposed changes

Prevent tests' unmocked access to utils.execute()

Browse Source 
This change introduces a new base test class that mocks out
utils.execute and forces an exception if it gets called.
This has rooted out many tests that were doing this as a side effect of
calling other functions, doing things like modprobe and running iscsi
on the host's actual machine.

The tests are all now appropriately patched in places where this was
happening, and the new base class permanently prevents this from
accidentally happening again.

If you really want to call utils.execute() then you need to re-mock it
in your unit test.

Change-Id: Idf87d09a9c01a6bfe2767f8becabe65c02983518
This commit is contained in:
Julian Edwards 2017-04-03 15:32:44 +10:00
parent 4be702242e
commit f57cbccf8b
12 changed files with 113 additions and 47 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

43
ironic_python_agent/tests/unit/base.py Normal file
View File

@ -0,0 +1,43 @@
# Copyright 2017 Cisco Systems, Inc
#
# 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.
"""Common utilities and classes across all unit tests."""
import mock
from oslotest import base as test_base
from ironic_python_agent import utils
class IronicAgentTest(test_base.BaseTestCase):
"""Extends the base test to provide common features across agent tests."""
def setUp(self):
super(IronicAgentTest, self).setUp()
"""Add a blanket ban on running external processes via utils.execute().
`self` will grow 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.
self._exec_patch = mock.Mock()
self._exec_patch.side_effect = Exception(
"Don't call utils.execute in tests!")
self.patch(utils, 'execute', self._exec_patch)

29
ironic_python_agent/tests/unit/test_agent.py
View File

@ -20,7 +20,6 @@ import mock
from oslo_concurrency import processutils
from oslo_config import cfg
from oslo_serialization import jsonutils
from oslotest import base as test_base
import pkg_resources
from stevedore import extension
@ -31,6 +30,7 @@ from ironic_python_agent.extensions import base
from ironic_python_agent import hardware
from ironic_python_agent import inspector
from ironic_python_agent import netutils
from ironic_python_agent.tests.unit import base as ironic_agent_base
from ironic_python_agent import utils
EXPECTED_ERROR = RuntimeError('command execution failed')
@ -49,7 +49,7 @@ class FakeExtension(base.BaseAgentExtension):
pass
class TestHeartbeater(test_base.BaseTestCase):
class TestHeartbeater(ironic_agent_base.IronicAgentTest):
def setUp(self):
super(TestHeartbeater, self).setUp()
self.mock_agent = mock.Mock()
@ -129,7 +129,7 @@ class TestHeartbeater(test_base.BaseTestCase):
@mock.patch.object(hardware.GenericHardwareManager, '_wait_for_disks',
lambda self: None)
class TestBaseAgent(test_base.BaseTestCase):
class TestBaseAgent(ironic_agent_base.IronicAgentTest):
def setUp(self):
super(TestBaseAgent, self).setUp()
@ -171,6 +171,10 @@ class TestBaseAgent(test_base.BaseTestCase):
self.assertEqual(pkg_resources.get_distribution('ironic-python-agent')
.version, status.version)
@mock.patch.object(hardware, '_check_for_iscsi', mock.Mock())
@mock.patch(
'ironic_python_agent.hardware_managers.cna._detect_cna_card',
mock.Mock())
@mock.patch.object(agent.IronicPythonAgent,
'_wait_for_interface', autospec=True)
@mock.patch.object(hardware, 'dispatch_to_managers', autospec=True)
@ -203,6 +207,9 @@ class TestBaseAgent(test_base.BaseTestCase):
mock_dispatch.assert_called_once_with("list_hardware_info")
self.agent.heartbeater.start.assert_called_once_with()
@mock.patch.object(hardware, '_check_for_iscsi', mock.Mock())
@mock.patch('ironic_python_agent.hardware_managers.cna._detect_cna_card',
mock.Mock())
@mock.patch.object(agent.IronicPythonAgent,
'_wait_for_interface', autospec=True)
@mock.patch.object(inspector, 'inspect', autospec=True)
@ -249,6 +256,10 @@ class TestBaseAgent(test_base.BaseTestCase):
mock_dispatch.assert_called_once_with("list_hardware_info")
self.agent.heartbeater.start.assert_called_once_with()
@mock.patch.object(hardware, '_check_for_iscsi', mock.Mock())
@mock.patch(
'ironic_python_agent.hardware_managers.cna._detect_cna_card',
mock.Mock())
@mock.patch.object(agent.IronicPythonAgent,
'_wait_for_interface', autospec=True)
@mock.patch.object(inspector, 'inspect', autospec=True)
@ -298,6 +309,10 @@ class TestBaseAgent(test_base.BaseTestCase):
self.assertFalse(mock_wait.called)
self.assertFalse(mock_dispatch.called)
@mock.patch.object(hardware, '_check_for_iscsi', mock.Mock())
@mock.patch(
'ironic_python_agent.hardware_managers.cna._detect_cna_card',
mock.Mock())
@mock.patch.object(agent.IronicPythonAgent,
'_wait_for_interface', autospec=True)
@mock.patch.object(inspector, 'inspect', autospec=True)
@ -497,7 +512,7 @@ class TestBaseAgent(test_base.BaseTestCase):
@mock.patch.object(hardware.GenericHardwareManager, '_wait_for_disks',
lambda self: None)
class TestAgentStandalone(test_base.BaseTestCase):
class TestAgentStandalone(ironic_agent_base.IronicAgentTest):
def setUp(self):
super(TestAgentStandalone, self).setUp()
@ -515,6 +530,10 @@ class TestAgentStandalone(test_base.BaseTestCase):
'agent_ipmitool',
True)
@mock.patch.object(hardware, '_check_for_iscsi', mock.Mock())
@mock.patch(
'ironic_python_agent.hardware_managers.cna._detect_cna_card',
mock.Mock())
@mock.patch('wsgiref.simple_server.make_server', autospec=True)
@mock.patch.object(hardware.HardwareManager, 'list_hardware_info',
autospec=True)
@ -551,7 +570,7 @@ class TestAgentStandalone(test_base.BaseTestCase):
lambda self: None)
@mock.patch.object(socket, 'gethostbyname', autospec=True)
@mock.patch.object(utils, 'execute', autospec=True)
class TestAdvertiseAddress(test_base.BaseTestCase):
class TestAdvertiseAddress(ironic_agent_base.IronicAgentTest):
def setUp(self):
super(TestAdvertiseAddress, self).setUp()

4
ironic_python_agent/tests/unit/test_api.py
View File

@ -15,18 +15,18 @@
import time
import mock
from oslotest import base as test_base
import pecan
import pecan.testing
from ironic_python_agent import agent
from ironic_python_agent.extensions import base
from ironic_python_agent.tests.unit import base as ironic_agent_base
PATH_PREFIX = '/v1'
class TestIronicAPI(test_base.BaseTestCase):
class TestIronicAPI(ironic_agent_base.IronicAgentTest):
def setUp(self):
super(TestIronicAPI, self).setUp()

7
ironic_python_agent/tests/unit/test_encoding.py
View File

@ -12,9 +12,8 @@
# License for the specific language governing permissions and limitations
# under the License.
from oslotest import base as test_base
from ironic_python_agent import encoding
from ironic_python_agent.tests.unit import base
class SerializableTesting(encoding.Serializable):
@ -33,7 +32,7 @@ class SerializableComparableTesting(encoding.SerializableComparable):
self.jill = jill
class TestSerializable(test_base.BaseTestCase):
class TestSerializable(base.IronicAgentTest):
def test_baseclass_serialize(self):
obj = encoding.Serializable()
self.assertEqual({}, obj.serialize())
@ -44,7 +43,7 @@ class TestSerializable(test_base.BaseTestCase):
self.assertEqual(expected, obj.serialize())
class TestSerializableComparable(test_base.BaseTestCase):
class TestSerializableComparable(base.IronicAgentTest):
def test_childclass_equal(self):
obj1 = SerializableComparableTesting('hello', 'world')

5
ironic_python_agent/tests/unit/test_errors.py
View File

@ -12,9 +12,8 @@
# License for the specific language governing permissions and limitations
# under the License.
from oslotest import base as test_base
from ironic_python_agent import errors
from ironic_python_agent.tests.unit import base
DETAILS = 'details'
SAME_CL_DETAILS = 'same_as_class_details'
@ -32,7 +31,7 @@ class TestError(errors.RESTError):
super(TestError, self).__init__(details)
class TestErrors(test_base.BaseTestCase):
class TestErrors(base.IronicAgentTest):
def test_RESTError(self):
e = errors.RESTError()

15
ironic_python_agent/tests/unit/test_hardware.py
View File

@ -22,12 +22,12 @@ import netifaces
from oslo_concurrency import processutils
from oslo_config import cfg
from oslo_utils import units
from oslotest import base as test_base
import pyudev
from stevedore import extension
from ironic_python_agent import errors
from ironic_python_agent import hardware
from ironic_python_agent.tests.unit import base
from ironic_python_agent import utils
CONF = cfg.CONF
@ -257,7 +257,7 @@ class FakeHardwareManager(hardware.GenericHardwareManager):
return self._hardware_support
class TestHardwareManagerLoading(test_base.BaseTestCase):
class TestHardwareManagerLoading(base.IronicAgentTest):
def setUp(self):
super(TestHardwareManagerLoading, self).setUp()
# In order to use ExtensionManager.make_test_instance() without
@ -286,7 +286,7 @@ class TestHardwareManagerLoading(test_base.BaseTestCase):
@mock.patch.object(hardware, '_udev_settle', lambda *_: None)
class TestGenericHardwareManager(test_base.BaseTestCase):
class TestGenericHardwareManager(base.IronicAgentTest):
def setUp(self):
super(TestGenericHardwareManager, self).setUp()
self.hardware = hardware.GenericHardwareManager()
@ -784,6 +784,9 @@ class TestGenericHardwareManager(test_base.BaseTestCase):
self.hardware.get_boot_info.return_value = hardware.BootInfo(
current_boot_mode='bios', pxe_interface='boot:if')
self.hardware.get_bmc_address = mock.Mock()
self.hardware.get_system_vendor_info = mock.Mock()
hardware_info = self.hardware.list_hardware_info()
self.assertEqual(self.hardware.get_memory(), hardware_info['memory'])
self.assertEqual(self.hardware.get_cpus(), hardware_info['cpu'])
@ -1490,6 +1493,7 @@ class TestGenericHardwareManager(test_base.BaseTestCase):
self.assertEqual(2, mocked_root_dev.call_count)
mocked_sleep.assert_called_once_with(CONF.disk_wait_delay)
@mock.patch.object(hardware, '_check_for_iscsi', mock.Mock())
@mock.patch.object(hardware.GenericHardwareManager, 'list_block_devices',
autospec=True)
@mock.patch.object(time, 'sleep', autospec=True)
@ -1517,6 +1521,7 @@ class TestGenericHardwareManager(test_base.BaseTestCase):
mocked_root_dev.assert_called_with(mocked_block_dev.return_value)
self.assertEqual(10, mocked_root_dev.call_count)
@mock.patch.object(hardware, '_check_for_iscsi', mock.Mock())
@mock.patch.object(hardware.GenericHardwareManager, 'list_block_devices',
autospec=True)
@mock.patch.object(time, 'sleep', autospec=True)
@ -1539,6 +1544,7 @@ class TestGenericHardwareManager(test_base.BaseTestCase):
mocked_root_dev.assert_called_with(mocked_block_dev.return_value)
self.assertEqual(2, mocked_root_dev.call_count)
@mock.patch.object(hardware, '_check_for_iscsi', mock.Mock())
@mock.patch.object(hardware.GenericHardwareManager, 'list_block_devices',
autospec=True)
@mock.patch.object(time, 'sleep', autospec=True)
@ -1552,6 +1558,7 @@ class TestGenericHardwareManager(test_base.BaseTestCase):
mocked_sleep.assert_called_with(3)
@mock.patch.object(hardware, '_check_for_iscsi', mock.Mock())
@mock.patch.object(hardware.GenericHardwareManager, 'list_block_devices',
autospec=True)
@mock.patch.object(time, 'sleep', autospec=True)
@ -1611,7 +1618,7 @@ class TestGenericHardwareManager(test_base.BaseTestCase):
@mock.patch.object(os, 'listdir', lambda *_: [])
@mock.patch.object(utils, 'execute', autospec=True)
class TestModuleFunctions(test_base.BaseTestCase):
class TestModuleFunctions(base.IronicAgentTest):
@mock.patch.object(hardware, '_get_device_info',
lambda x, y, z: 'FooTastic')

22
ironic_python_agent/tests/unit/test_inspector.py
View File

@ -21,13 +21,13 @@ import time
import mock
from oslo_concurrency import processutils
from oslo_config import cfg
from oslotest import base as test_base
import requests
import stevedore
from ironic_python_agent import errors
from ironic_python_agent import hardware
from ironic_python_agent import inspector
from ironic_python_agent.tests.unit import base
from ironic_python_agent import utils
@ -46,7 +46,7 @@ class AcceptingFailure(mock.Mock):
failure, expect_error)
class TestMisc(test_base.BaseTestCase):
class TestMisc(base.IronicAgentTest):
def test_default_collector_loadable(self):
ext = inspector.extension_manager([inspector.DEFAULT_COLLECTOR])
self.assertIs(ext[inspector.DEFAULT_COLLECTOR].plugin,
@ -62,7 +62,7 @@ class TestMisc(test_base.BaseTestCase):
@mock.patch.object(inspector, 'setup_ipmi_credentials', autospec=True)
@mock.patch.object(inspector, 'call_inspector', new_callable=AcceptingFailure)
@mock.patch.object(stevedore, 'NamedExtensionManager', autospec=True)
class TestInspect(test_base.BaseTestCase):
class TestInspect(base.IronicAgentTest):
def setUp(self):
super(TestInspect, self).setUp()
CONF.set_override('inspection_callback_url', 'http://foo/bar',
@ -131,7 +131,7 @@ class TestInspect(test_base.BaseTestCase):
@mock.patch.object(requests, 'post', autospec=True)
class TestCallInspector(test_base.BaseTestCase):
class TestCallInspector(base.IronicAgentTest):
def setUp(self):
super(TestCallInspector, self).setUp()
CONF.set_override('inspection_callback_url', 'url',
@ -176,7 +176,7 @@ class TestCallInspector(test_base.BaseTestCase):
@mock.patch.object(utils, 'execute', autospec=True)
class TestSetupIpmiCredentials(test_base.BaseTestCase):
class TestSetupIpmiCredentials(base.IronicAgentTest):
def setUp(self):
super(TestSetupIpmiCredentials, self).setUp()
self.resp = {'ipmi_username': 'user',
@ -227,7 +227,7 @@ class TestSetupIpmiCredentials(test_base.BaseTestCase):
self.assertEqual(expected, mock_call.call_args_list)
class BaseDiscoverTest(test_base.BaseTestCase):
class BaseDiscoverTest(base.IronicAgentTest):
def setUp(self):
super(BaseDiscoverTest, self).setUp()
self.inventory = {
@ -302,7 +302,7 @@ class TestCollectDefault(BaseDiscoverTest):
@mock.patch.object(utils, 'collect_system_logs', autospec=True)
class TestCollectLogs(test_base.BaseTestCase):
class TestCollectLogs(base.IronicAgentTest):
def test(self, mock_collect):
data = {}
@ -320,7 +320,7 @@ class TestCollectLogs(test_base.BaseTestCase):
@mock.patch.object(utils, 'execute', autospec=True)
class TestCollectExtraHardware(test_base.BaseTestCase):
class TestCollectExtraHardware(base.IronicAgentTest):
def setUp(self):
super(TestCollectExtraHardware, self).setUp()
self.data = {}
@ -366,7 +366,7 @@ class TestCollectExtraHardware(test_base.BaseTestCase):
@mock.patch.object(os, 'listdir', autospec=True)
class TestCollectPciDevicesInfo(test_base.BaseTestCase):
class TestCollectPciDevicesInfo(base.IronicAgentTest):
def setUp(self):
super(TestCollectPciDevicesInfo, self).setUp()
self.data = {}
@ -421,7 +421,7 @@ class TestCollectPciDevicesInfo(test_base.BaseTestCase):
@mock.patch.object(utils, 'get_agent_params', lambda: {'BOOTIF': '01-cdef'})
@mock.patch.object(hardware, 'dispatch_to_managers', autospec=True)
class TestWaitForDhcp(test_base.BaseTestCase):
class TestWaitForDhcp(base.IronicAgentTest):
def setUp(self):
super(TestWaitForDhcp, self).setUp()
CONF.set_override('inspection_dhcp_wait_timeout',
@ -507,7 +507,7 @@ class TestWaitForDhcp(test_base.BaseTestCase):
self.assertFalse(mocked_dispatch.called)
class TestNormalizeMac(test_base.BaseTestCase):
class TestNormalizeMac(base.IronicAgentTest):
def test_correct_mac(self):
self.assertEqual('11:22:33:aa:bb:cc',
inspector._normalize_mac('11:22:33:aa:BB:cc'))

4
ironic_python_agent/tests/unit/test_ironic_api_client.py
View File

@ -15,11 +15,11 @@
import mock
from oslo_serialization import jsonutils
from oslo_service import loopingcall
from oslotest import base as test_base
from ironic_python_agent import errors
from ironic_python_agent import hardware
from ironic_python_agent import ironic_api_client
from ironic_python_agent.tests.unit import base
API_URL = 'http://agent-api.ironic.example.org/'
@ -32,7 +32,7 @@ class FakeResponse(object):
self.headers = headers or {}
class TestBaseIronicPythonAgent(test_base.BaseTestCase):
class TestBaseIronicPythonAgent(base.IronicAgentTest):
def setUp(self):
super(TestBaseIronicPythonAgent, self).setUp()
self.api_client = ironic_api_client.APIClient(API_URL)

11
ironic_python_agent/tests/unit/test_multi_hardware.py
View File

@ -15,11 +15,11 @@
import collections
import mock
from oslotest import base as test_base
from stevedore import extension
from ironic_python_agent import errors
from ironic_python_agent import hardware
from ironic_python_agent.tests.unit import base
def counted(fn):
@ -105,7 +105,7 @@ class FakeMainlineHardwareManager(hardware.HardwareManager):
return hardware.HardwareSupport.MAINLINE
class TestMultipleHardwareManagerLoading(test_base.BaseTestCase):
class TestMultipleHardwareManagerLoading(base.IronicAgentTest):
def setUp(self):
super(TestMultipleHardwareManagerLoading, self).setUp()
fake_ep = mock.Mock()
@ -120,14 +120,11 @@ class TestMultipleHardwareManagerLoading(test_base.BaseTestCase):
self.extension_mgr_patcher = mock.patch('stevedore.ExtensionManager',
autospec=True)
self.addCleanup(self.extension_mgr_patcher.stop)
self.mocked_extension_mgr = self.extension_mgr_patcher.start()
self.mocked_extension_mgr.return_value = self.fake_ext_mgr
hardware._global_managers = None
def tearDown(self):
super(TestMultipleHardwareManagerLoading, self).tearDown()
self.extension_mgr_patcher.stop()
def test_mainline_method_only(self):
hardware.dispatch_to_managers('specific_only')
@ -212,7 +209,7 @@ class TestMultipleHardwareManagerLoading(test_base.BaseTestCase):
'unexpected_fail')
class TestNoHardwareManagerLoading(test_base.BaseTestCase):
class TestNoHardwareManagerLoading(base.IronicAgentTest):
def setUp(self):
super(TestNoHardwareManagerLoading, self).setUp()
self.empty_ext_mgr = extension.ExtensionManager.make_test_instance([])

9
ironic_python_agent/tests/unit/test_multi_hardware_clean_steps.py
View File

@ -13,11 +13,11 @@
# limitations under the License.
import mock
from oslotest import base as test_base
from stevedore import extension
from ironic_python_agent.extensions import clean
from ironic_python_agent import hardware
from ironic_python_agent.tests.unit import base
def _build_clean_step(name, priority, reboot=False, abort=False):
@ -53,7 +53,7 @@ class ZFakeGenericHardwareManager(hardware.HardwareManager):
_build_clean_step('ZHigherPrio', 100)]
class TestMultipleHardwareManagerCleanSteps(test_base.BaseTestCase):
class TestMultipleHardwareManagerCleanSteps(base.IronicAgentTest):
def setUp(self):
super(TestMultipleHardwareManagerCleanSteps, self).setUp()
@ -73,14 +73,11 @@ class TestMultipleHardwareManagerCleanSteps(test_base.BaseTestCase):
self.extension_mgr_patcher = mock.patch('stevedore.ExtensionManager',
autospec=True)
self.addCleanup(self.extension_mgr_patcher.stop)
self.mocked_extension_mgr = self.extension_mgr_patcher.start()
self.mocked_extension_mgr.return_value = self.fake_ext_mgr
hardware._global_managers = None
def tearDown(self):
super(TestMultipleHardwareManagerCleanSteps, self).tearDown()
self.extension_mgr_patcher.stop()
def test_clean_step_ordering(self):
as_results = self.agent_extension.get_clean_steps(node={}, ports=[])
results = as_results.join().command_result

4
ironic_python_agent/tests/unit/test_netutils.py
View File

@ -16,9 +16,9 @@ import binascii
import mock
from oslo_config import cfg
from oslotest import base as test_base
from ironic_python_agent import netutils
from ironic_python_agent.tests.unit import base
# hexlify-ed output from LLDP packet
FAKE_LLDP_PACKET = binascii.unhexlify(
@ -37,7 +37,7 @@ def socket_socket_sig(family=None, type=None, proto=None):
pass
class TestNetutils(test_base.BaseTestCase):
class TestNetutils(base.IronicAgentTest):
def setUp(self):
super(TestNetutils, self).setUp()

7
ironic_python_agent/tests/unit/test_utils.py
View File

@ -31,9 +31,14 @@ import six
import testtools
from ironic_python_agent import errors
from ironic_python_agent.tests.unit import base as ironic_agent_base
from ironic_python_agent import utils
# Normally we'd use the IronicAgentTest base class which mocks out
# any use of utils.execute() to prevent accidental processes. However
# this test is checking the upcall to ironic_lib's execute(), so that
# is mocked out instead.
class ExecuteTestCase(test_base.BaseTestCase):
@mock.patch.object(ironic_utils, 'execute', autospec=True)
@ -43,7 +48,7 @@ class ExecuteTestCase(test_base.BaseTestCase):
check_exit_code=False)
class GetAgentParamsTestCase(test_base.BaseTestCase):
class GetAgentParamsTestCase(ironic_agent_base.IronicAgentTest):
@mock.patch('oslo_log.log.getLogger', autospec=True)
@mock.patch('six.moves.builtins.open', autospec=True)