From 77f8033235f40c87d0cc593195bfb7650008acf4 Mon Sep 17 00:00:00 2001 From: Jiri Podivin Date: Wed, 17 Mar 2021 17:01:46 +0100 Subject: [PATCH] Refactoring and improvement of check_ironic_boot_config tests Tests now utilize mocks and cover larger portion of the module. Key tests were documented with additional docstrings and variables were renamed to be more obvious to future maintainers. The helper function was moved to fakes.py Signed-off-by: Jiri Podivin Change-Id: Ie50aae2bd9b9ea6ccc1ec92c20f1529f551297ef --- tripleo_validations/tests/fakes.py | 33 +++ .../library/test_check_ironic_boot_config.py | 236 +++++++++++++----- 2 files changed, 204 insertions(+), 65 deletions(-) diff --git a/tripleo_validations/tests/fakes.py b/tripleo_validations/tests/fakes.py index 6f3eae428..9e3d9a5b2 100644 --- a/tripleo_validations/tests/fakes.py +++ b/tripleo_validations/tests/fakes.py @@ -12,6 +12,13 @@ # License for the specific language governing permissions and limitations # under the License. # + +"""This sub module provides various data structures and functions +useful for automated testing. Additional helpers should be placed here +if at all possible. This should help with reduction of redundancy and +isolation of potentially problematic testing code. +""" + import sys try: from unittest import mock @@ -126,3 +133,29 @@ MOCK_NODES = [ MOCK_PROFILE_FLAVORS = { 'fooflavor': (MOCK_FLAVORS['ok'], 1), } + + +UUIDs = [ + '13c319a4-7704-4b44-bb2e-501951879f96', + '8201bb8e-be20-4a97-bcf4-91bcf7eeff86', + 'cc04effd-6bac-45ba-a0dc-83e6cd2c589d', + 'cbb12140-a088-4646-a873-73eeb055ccc2' +] + + +def node_helper(node_id, kernel_id, ram_id, arch=None, platform=None): + + node = { + "uuid": node_id, + "driver_info": { + "deploy_kernel": kernel_id, + "deploy_ramdisk": ram_id, + }, + "properties": {}, + "extra": {}, + } + if arch: + node["properties"]["cpu_arch"] = arch + if platform: + node["extra"]["tripleo_platform"] = platform + return node diff --git a/tripleo_validations/tests/library/test_check_ironic_boot_config.py b/tripleo_validations/tests/library/test_check_ironic_boot_config.py index 68a293050..2afb37214 100644 --- a/tripleo_validations/tests/library/test_check_ironic_boot_config.py +++ b/tripleo_validations/tests/library/test_check_ironic_boot_config.py @@ -13,85 +13,191 @@ # License for the specific language governing permissions and limitations # under the License. +"""Tests of the check_ironic_boot_config submodule. +The initial try/except block is a safeguard against Python version +incompatibility and general confusion it can cause. +But worry not, it's barely used these days. +""" try: from unittest import mock except ImportError: import mock -from tripleo_validations.tests import base -from tripleo_validations.tests import fakes - - +import tripleo_validations.tests.base as base +import tripleo_validations.tests.fakes as fakes import library.check_ironic_boot_config as validation -UUIDs = [ - '13c319a4-7704-4b44-bb2e-501951879f96', - '8201bb8e-be20-4a97-bcf4-91bcf7eeff86', - 'cc04effd-6bac-45ba-a0dc-83e6cd2c589d', - 'cbb12140-a088-4646-a873-73eeb055ccc2' -] +class TestCheckIronicBootConfigModule(base.TestCase): -class TestCheckIronicBootConfig(base.TestCase): + def setUp(self): + super(TestCheckIronicBootConfigModule, self).setUp() + self.module = validation - def _node_helper(self, n_id, k_id, r_id, arch=None, platform=None): - node = { - "uuid": n_id, - "driver_info": { - "deploy_kernel": k_id, - "deploy_ramdisk": r_id, - }, - "properties": {}, - "extra": {}, - } - if arch: - node["properties"]["cpu_arch"] = arch - if platform: - node["extra"]["tripleo_platform"] = platform - return node + def test_module_init(self): + module_attributes = dir(self.module) - def _do_positive_test_case(self, nodes): - res = validation.validate_boot_config(nodes) - self.assertEqual([], res) - - def _do_negative_test_case(self, nodes, fail_reason='too_diverse'): - with mock.patch( - 'library.check_ironic_boot_config._%s' % fail_reason) as e: - validation.validate_boot_config(nodes) - e.assert_called() - - def test_basic_functionality(self): - nodes = [ - self._node_helper(1, UUIDs[0], UUIDs[1], 'ppc64le', 'p9'), - self._node_helper(2, UUIDs[0], UUIDs[1], 'ppc64le', 'p9') + required_attributes = [ + 'DOCUMENTATION', + 'EXAMPLES' ] - self._do_positive_test_case(nodes) - nodes.append( - self._node_helper( - 3, 'file://k.img', 'file://r.img', 'ppc64le', 'p9') - ) - self._do_positive_test_case(nodes) + self.assertTrue(set(required_attributes).issubset(module_attributes)) - nodes.append( - self._node_helper(4, UUIDs[0], UUIDs[1], 'ppc64le') - ) - self._do_positive_test_case(nodes) + @mock.patch( + 'library.check_ironic_boot_config.yaml_safe_load', + return_value={'options': 'fizz'}) + @mock.patch( + 'library.check_ironic_boot_config.validate_boot_config', + return_value=None) + @mock.patch('library.check_ironic_boot_config.AnsibleModule') + def test_module_main_success(self, mock_module, + mock_validate_boot_config, + mock_yaml_safe_load): - nodes.append( - self._node_helper(5, UUIDs[2], UUIDs[3], 'ppc64le', 'p9'), - ) - self._do_negative_test_case(nodes) - nodes = nodes[:-1] + module_calls = [ + mock.call(argument_spec='fizz'), + mock.call().params.get('nodes'), + mock.call().exit_json() + ] - nodes.append( - self._node_helper( - 5, 'file://k2.img', 'file://r2.img', 'ppc64le', 'p9') - ) - self._do_negative_test_case(nodes) - nodes = nodes[:-1] + self.module.main() - nodes.append( - self._node_helper(5, 'not_uuid_or_path', 'not_uuid_or_path') - ) - self._do_negative_test_case(nodes, 'invalid_image_entry') + mock_validate_boot_config.assert_called_once() + mock_module.assert_has_calls(module_calls) + + @mock.patch( + 'library.check_ironic_boot_config.yaml_safe_load', + return_value={'options': 'fizz'}) + @mock.patch( + 'library.check_ironic_boot_config.validate_boot_config', + return_value=['foo', 'bar']) + @mock.patch('library.check_ironic_boot_config.AnsibleModule') + def test_module_main_fail(self, mock_module, + mock_validate_boot_config, + mock_yaml_safe_load): + + module_calls = [ + mock.call(argument_spec='fizz'), + mock.call().params.get('nodes'), + mock.call().fail_json('foobar') + ] + + self.module.main() + + mock_validate_boot_config.assert_called_once() + mock_module.assert_has_calls(module_calls) + + def test_too_diverse(self): + """Test if the function returns string without raising exception. + """ + + return_value = self.module._too_diverse( + 'foo', + [ + 'bar', + 'fizz', + 'buzz' + ], + '000') + + self.assertIsInstance(return_value, str) + + def test_invalid_image_entry(self): + """Test if the function returns string without raising exception. + """ + + return_value = self.module._invalid_image_entry( + 'foo', + [ + 'bar', + 'fizz', + 'buzz' + ], + '000') + + self.assertIsInstance(return_value, str) + + +class TestValidateBootConfig(base.TestCase): + """Tests for validate_boot_config function of the check_ironic_boot_config + submodule. Tests assert on returned value and calls made. + """ + + @mock.patch('library.check_ironic_boot_config._too_diverse') + @mock.patch('library.check_ironic_boot_config._invalid_image_entry') + def test_validate_boot_config_success(self, mock_image_entry_error, mock_diverse_error): + """As we are trying to verify functionality for multiple subsets + of various nodes, this test is slightly more complex. + List of nodes is sliced and individual slices are fed + to the validate_boot_config function we are testing. + However, this approach still doesn't test all the possibilities. + For example the order of original list is maintained, and number + of nodes is very, very limited. + Further improvement will require consultation. + """ + nodes = [ + fakes.node_helper(1, fakes.UUIDs[0], fakes.UUIDs[1], 'ppc64le', 'p9'), + fakes.node_helper(2, fakes.UUIDs[0], fakes.UUIDs[1], 'ppc64le', 'p9'), + fakes.node_helper(3, 'file://k.img', 'file://r.img', 'ppc64le', 'p9'), + fakes.node_helper(4, fakes.UUIDs[0], fakes.UUIDs[1], 'ppc64le') + ] + + for node_slice in [nodes[::index] for index in range(1, len(nodes))]: + errors = validation.validate_boot_config(node_slice) + + mock_diverse_error.assert_not_called() + mock_image_entry_error.assert_not_called() + + self.assertIsInstance(errors, list) + self.assertEqual(len(errors), 0) + + @mock.patch('library.check_ironic_boot_config._too_diverse') + def test_validate_boot_config_fail_too_diverse_uuid(self, mock_error): + nodes = [ + fakes.node_helper(1, fakes.UUIDs[0], fakes.UUIDs[1], 'ppc64le', 'p9'), + fakes.node_helper(2, fakes.UUIDs[0], fakes.UUIDs[1], 'ppc64le', 'p9'), + fakes.node_helper(3, 'file://k.img', 'file://r.img', 'ppc64le', 'p9'), + fakes.node_helper(4, fakes.UUIDs[0], fakes.UUIDs[1], 'ppc64le'), + fakes.node_helper(5, fakes.UUIDs[2], fakes.UUIDs[3], 'ppc64le', 'p9'), + ] + + validation.validate_boot_config(nodes) + mock_error.assert_called() + + @mock.patch('library.check_ironic_boot_config._too_diverse') + def test_validate_boot_config_fail_too_diverse_path(self, mock_error): + nodes = [ + fakes.node_helper(1, fakes.UUIDs[0], fakes.UUIDs[1], 'ppc64le', 'p9'), + fakes.node_helper(2, fakes.UUIDs[0], fakes.UUIDs[1], 'ppc64le', 'p9'), + fakes.node_helper(3, 'file://k.img', 'file://r.img', 'ppc64le', 'p9'), + fakes.node_helper(4, fakes.UUIDs[0], fakes.UUIDs[1], 'ppc64le'), + fakes.node_helper(5, 'file://k2.img', 'file://r2.img', 'ppc64le', 'p9') + ] + + calls = [ + mock.call('file-based', ('kernel', 'ppc64le', 'p9'), {'file://k.img', 'file://k2.img'}), + mock.call('file-based', ('ramdisk', 'ppc64le', 'p9'), {'file://r2.img', 'file://r.img'}) + ] + + validation.validate_boot_config(nodes) + mock_error.assert_has_calls(calls) + + @mock.patch('library.check_ironic_boot_config._invalid_image_entry') + def test_validate_boot_config_fail_invalid_image_entry(self, mock_error): + nodes = [ + fakes.node_helper(1, fakes.UUIDs[0], fakes.UUIDs[1], 'ppc64le', 'p9'), + fakes.node_helper(2, fakes.UUIDs[0], fakes.UUIDs[1], 'ppc64le', 'p9'), + fakes.node_helper(3, 'file://k.img', 'file://r.img', 'ppc64le', 'p9'), + fakes.node_helper(4, fakes.UUIDs[0], fakes.UUIDs[1], 'ppc64le'), + fakes.node_helper(5, 'not_uuid_or_path', 'not_uuid_or_path') + ] + + calls = [ + mock.call('kernel', 'not_uuid_or_path', 5), + mock.call('ramdisk', 'not_uuid_or_path', 5) + ] + + validation.validate_boot_config(nodes) + + mock_error.assert_has_calls(calls)