From 38fad07c0a01072ee4e17815f6ad0e5c2ce1f64a Mon Sep 17 00:00:00 2001 From: Zane Bitter Date: Tue, 7 Aug 2018 16:38:03 -0400 Subject: [PATCH] Unit tests: Fix mock errors with too few side effects When a mock hasn't been provided with a long-enough list of side-effects for the number of times it is called, it raises StopIteration (instead of something sensible like AssertionError). Prior to Python 3.7, this exception could bubble up until it just stopped a task. (In Python 3.7 it will eventually be caught and turned into a RuntimeError.) To ensure that any errors of this sort are not handled silently, and to enable us to test for them prior to using Python 3.7, catch any StopIteration errors coming from plugin-provided non-generator functions and convert them to RuntimeError exceptions. This reveals many errors in the unit tests, many introduced in the process of converting from mox to mock, which are also fixed by this patch. Change-Id: I5a1eff6b704dff7c17edcbbe58cdbc380ae6abc9 Story: #2003412 Task: 24553 --- heat/engine/resource.py | 7 +- heat/tests/aws/test_security_group.py | 52 +++---- heat/tests/aws/test_volume.py | 21 ++- .../openstack/cinder/test_volume_utils.py | 3 +- heat/tests/openstack/heat/test_swiftsignal.py | 5 +- .../neutron/test_neutron_loadbalancer.py | 1 + heat/tests/openstack/nova/test_server.py | 138 +++++++++--------- heat/tests/test_resource.py | 9 +- heat/tests/test_vpc.py | 32 ++-- 9 files changed, 143 insertions(+), 125 deletions(-) diff --git a/heat/engine/resource.py b/heat/engine/resource.py index d0dd29fd24..9db6e974bb 100644 --- a/heat/engine/resource.py +++ b/heat/engine/resource.py @@ -976,7 +976,10 @@ class Resource(status.ResourceStatus): handler = getattr(self, 'handle_%s' % handler_action, None) if callable(handler): - handler_data = handler(*args) + try: + handler_data = handler(*args) + except StopIteration: + raise RuntimeError('Plugin method raised StopIteration') yield if callable(check): try: @@ -990,6 +993,8 @@ class Resource(status.ResourceStatus): break else: yield + except StopIteration: + raise RuntimeError('Plugin method raised StopIteration') except Exception: raise except: # noqa diff --git a/heat/tests/aws/test_security_group.py b/heat/tests/aws/test_security_group.py index 72eb205a0e..8c1b755e9f 100644 --- a/heat/tests/aws/test_security_group.py +++ b/heat/tests/aws/test_security_group.py @@ -463,6 +463,33 @@ Resources: # delete old not needed rules self.stubout_neutron_get_security_group() + stack = self.create_stack(self.test_template_neutron) + sg = stack['the_sg'] + self.assertResourceState(sg, 'aaaa') + + # make updated template + props = copy.deepcopy(sg.properties.data) + props['SecurityGroupIngress'] = [ + {'IpProtocol': 'tcp', + 'FromPort': '80', + 'ToPort': '80', + 'CidrIp': '0.0.0.0/0'}, + {'IpProtocol': 'tcp', + 'FromPort': '443', + 'ToPort': '443', + 'CidrIp': '0.0.0.0/0'}, + {'IpProtocol': 'tcp', + 'SourceSecurityGroupId': 'zzzz'}, + ] + props['SecurityGroupEgress'] = [ + {'IpProtocol': 'tcp', + 'FromPort': '22', + 'ToPort': '22', + 'CidrIp': '0.0.0.0/0'}, + {'SourceSecurityGroupName': 'xxxx'}, + ] + after = rsrc_defn.ResourceDefinition(sg.name, sg.type(), props) + # create missing rules self.m_csgr.side_effect = [ { @@ -501,32 +528,7 @@ Resources: } } ] - stack = self.create_stack(self.test_template_neutron) - sg = stack['the_sg'] - self.assertResourceState(sg, 'aaaa') - # make updated template - props = copy.deepcopy(sg.properties.data) - props['SecurityGroupIngress'] = [ - {'IpProtocol': 'tcp', - 'FromPort': '80', - 'ToPort': '80', - 'CidrIp': '0.0.0.0/0'}, - {'IpProtocol': 'tcp', - 'FromPort': '443', - 'ToPort': '443', - 'CidrIp': '0.0.0.0/0'}, - {'IpProtocol': 'tcp', - 'SourceSecurityGroupId': 'zzzz'}, - ] - props['SecurityGroupEgress'] = [ - {'IpProtocol': 'tcp', - 'FromPort': '22', - 'ToPort': '22', - 'CidrIp': '0.0.0.0/0'}, - {'SourceSecurityGroupName': 'xxxx'}, - ] - after = rsrc_defn.ResourceDefinition(sg.name, sg.type(), props) scheduler.TaskRunner(sg.update, after)() self.assertEqual((sg.UPDATE, sg.COMPLETE), sg.state) diff --git a/heat/tests/aws/test_volume.py b/heat/tests/aws/test_volume.py index 978747f8bb..8657437f3b 100644 --- a/heat/tests/aws/test_volume.py +++ b/heat/tests/aws/test_volume.py @@ -226,14 +226,13 @@ class VolumeTest(vt_base.VolumeTestCase): # delete script fva = vt_base.FakeVolume('in-use') - self.fc.volumes.get_server_volume.return_value = fva self.cinder_fc.volumes.get.side_effect = [ fva, vt_base.FakeVolume('detaching', id=fva.id), vt_base.FakeVolume('available', id=fva.id) ] self.fc.volumes.delete_server_volume.return_value = None self.fc.volumes.get_server_volume.side_effect = [ - fva, fakes_nova.fake_exception] + fva, fva, fakes_nova.fake_exception()] scheduler.TaskRunner(rsrc.delete)() self.fc.volumes.get_server_volume.assert_called_with(u'WikiDatabase', @@ -261,7 +260,7 @@ class VolumeTest(vt_base.VolumeTestCase): # delete script fva = vt_base.FakeVolume('in-use') self.fc.volumes.get_server_volume.side_effect = [ - fva, fva, fakes_nova.fake_exception] + fva, fva, fakes_nova.fake_exception()] self.cinder_fc.volumes.get.side_effect = [ fva, vt_base.FakeVolume('available', id=fva.id)] @@ -322,8 +321,8 @@ class VolumeTest(vt_base.VolumeTestCase): rsrc = self.create_attachment(self.t, stack, 'MountPoint') # delete script - self.cinder_fc.volumes.get.return_value = fva - exc = fakes_nova.fake_exception + self.cinder_fc.volumes.get.side_effect = [fva] + exc = fakes_nova.fake_exception() self.fc.volumes.get_server_volume.side_effect = exc scheduler.TaskRunner(rsrc.delete)() @@ -348,7 +347,7 @@ class VolumeTest(vt_base.VolumeTestCase): # delete script self.fc.volumes.get_server_volume.side_effect = [ - fva, fva, fakes_nova.fake_exception] + fva, fva, fakes_nova.fake_exception()] self.cinder_fc.volumes.get.side_effect = [ fva, vt_base.FakeVolume('in-use', id=fva.id), vt_base.FakeVolume('detaching', id=fva.id), @@ -542,6 +541,11 @@ class VolumeTest(vt_base.VolumeTestCase): rsrc = self.create_volume(self.t, stack, 'DataVolume') + self.cinder_fc.volumes.get.side_effect = [ + fv, + vt_base.FakeVolume('available'), + cinder_exp.NotFound('Not found') + ] scheduler.TaskRunner(rsrc.destroy)() self.m_backups.create.assert_called_once_with(fv.id) @@ -595,6 +599,11 @@ class VolumeTest(vt_base.VolumeTestCase): self.assertIn('Went to status error due to "Unknown"', six.text_type(ex)) + self.cinder_fc.volumes.get.side_effect = [ + fva, + cinder_exp.NotFound('Not found') + ] + scheduler.TaskRunner(rsrc.destroy)() def test_create_from_snapshot(self): diff --git a/heat/tests/openstack/cinder/test_volume_utils.py b/heat/tests/openstack/cinder/test_volume_utils.py index 5e3bb05b9e..7bcf4ff22b 100644 --- a/heat/tests/openstack/cinder/test_volume_utils.py +++ b/heat/tests/openstack/cinder/test_volume_utils.py @@ -61,7 +61,8 @@ class VolumeTestCase(common.HeatTestCase): result = [fva] for m in extra_create_server_volume_mocks: result.append(m) - self.fc.volumes.create_server_volume.side_effect = result + prev = self.fc.volumes.create_server_volume.side_effect or [] + self.fc.volumes.create_server_volume.side_effect = list(prev) + result fv_ready = FakeVolume(final_status, id=fva.id) return fv_ready diff --git a/heat/tests/openstack/heat/test_swiftsignal.py b/heat/tests/openstack/heat/test_swiftsignal.py index b688dea058..aa3a89f270 100644 --- a/heat/tests/openstack/heat/test_swiftsignal.py +++ b/heat/tests/openstack/heat/test_swiftsignal.py @@ -913,9 +913,10 @@ class SwiftSignalTest(common.HeatTestCase): mock_name.return_value = obj_name mock_swift_object.get_container.return_value = cont_index(obj_name, 2) mock_swift_object.get_object.side_effect = ( - (obj_header, ''), swiftclient_client.ClientException( - "Object %s not found" % obj_name, http_status=404) + "Object %s not found" % obj_name, http_status=404), + (obj_header, '{"id": 1}'), + (obj_header, '{"id": 2}'), ) st.create() diff --git a/heat/tests/openstack/neutron/test_neutron_loadbalancer.py b/heat/tests/openstack/neutron/test_neutron_loadbalancer.py index 739c21b606..da5658d062 100644 --- a/heat/tests/openstack/neutron/test_neutron_loadbalancer.py +++ b/heat/tests/openstack/neutron/test_neutron_loadbalancer.py @@ -368,6 +368,7 @@ class PoolTest(common.HeatTestCase): self.mock_create_vip.return_value = {'vip': {'id': 'xyz'}} self.mock_show.side_effect = [ {'pool': {'status': 'PENDING_CREATE'}}, + {'pool': {'status': 'ACTIVE'}}, {'pool': {'status': 'ACTIVE'}}] self.mock_show_vip.side_effect = [ {'vip': {'status': 'PENDING_CREATE'}}, diff --git a/heat/tests/openstack/nova/test_server.py b/heat/tests/openstack/nova/test_server.py index 2731d301cd..1109c886ea 100644 --- a/heat/tests/openstack/nova/test_server.py +++ b/heat/tests/openstack/nova/test_server.py @@ -229,6 +229,19 @@ def create_fake_iface(port=None, net=None, mac=None, ip=None, subnet=None): return fake_interface(port, net, mac, ip, subnet) +class ServerStatus(object): + def __init__(self, server, statuses): + self._server = server + self._status = iter(statuses) + + def __call__(self, server_id): + try: + self._server.status = next(self._status) + except StopIteration: + raise AssertionError('Unexpected call to servers.get()') + return self._server + + class ServersTest(common.HeatTestCase): def setUp(self): super(ServersTest, self).setUp() @@ -1696,12 +1709,12 @@ class ServersTest(common.HeatTestCase): # this makes sure the auto increment worked on server creation self.assertGreater(server.id, 0) - def make_soft_delete(*args): - return_server.status = "SOFT_DELETED" - return return_server self.patchobject(self.fc.servers, 'get', - side_effect=[return_server, return_server, - make_soft_delete()]) + side_effect=ServerStatus(return_server, + [return_server.status, + return_server.status, + "SOFT_DELETED", + "DELETED"])) scheduler.TaskRunner(server.delete)() self.assertEqual((server.DELETE, server.COMPLETE), server.state) @@ -2090,23 +2103,22 @@ class ServersTest(common.HeatTestCase): update_props['flavor'] = 'm1.small' update_template = server.t.freeze(properties=update_props) - def set_status(status): - return_server.status = status - return return_server - self.patchobject(self.fc.servers, 'get', - side_effect=[set_status('ACTIVE'), - set_status('RESIZE'), - set_status('VERIFY_RESIZE'), - set_status('VERIFY_RESIZE'), - set_status('ACTIVE')]) + side_effect=ServerStatus(return_server, + ['ACTIVE', + 'RESIZE', + 'VERIFY_RESIZE', + 'VERIFY_RESIZE', + 'ACTIVE'])) mock_post = self.patchobject(self.fc.client, 'post_servers_1234_action', return_value=(202, None)) scheduler.TaskRunner(server.update, update_template)() self.assertEqual((server.UPDATE, server.COMPLETE), server.state) - mock_post.called_once_with(body={'resize': {'flavorRef': 2}}) - mock_post.called_once_with(body={'confirmResize': None}) + mock_post.assert_has_calls([ + mock.call(body={'resize': {'flavorRef': '2'}}), + mock.call(body={'confirmResize': None}), + ]) def test_server_update_server_flavor_failed(self): """Check raising exception due to resize call failing. @@ -2122,13 +2134,9 @@ class ServersTest(common.HeatTestCase): update_props['flavor'] = 'm1.small' update_template = server.t.freeze(properties=update_props) - def set_status(status): - return_server.status = status - return return_server - self.patchobject(self.fc.servers, 'get', - side_effect=[set_status('RESIZE'), - set_status('ERROR')]) + side_effect=ServerStatus(return_server, + ['RESIZE', 'ERROR'])) mock_post = self.patchobject(self.fc.client, 'post_servers_1234_action', return_value=(202, None)) @@ -2138,7 +2146,7 @@ class ServersTest(common.HeatTestCase): "Error: resources.srv_update2: Resizing to '2' failed, " "status 'ERROR'", six.text_type(error)) self.assertEqual((server.UPDATE, server.FAILED), server.state) - mock_post.called_once_with(body={'resize': {'flavorRef': 2}}) + mock_post.assert_called_once_with(body={'resize': {'flavorRef': '2'}}) def test_server_update_flavor_resize_has_not_started(self): """Test update of server flavor if server resize has not started. @@ -2161,17 +2169,14 @@ class ServersTest(common.HeatTestCase): # define status transition when server resize # ACTIVE(initial) -> ACTIVE -> RESIZE -> VERIFY_RESIZE - def set_status(status): - server.status = status - return server - self.patchobject(self.fc.servers, 'get', - side_effect=[set_status('ACTIVE'), - set_status('ACTIVE'), - set_status('RESIZE'), - set_status('VERIFY_RESIZE'), - set_status('VERIFY_RESIZE'), - set_status('ACTIVE')]) + side_effect=ServerStatus(server, + ['ACTIVE', + 'ACTIVE', + 'RESIZE', + 'VERIFY_RESIZE', + 'VERIFY_RESIZE', + 'ACTIVE'])) mock_post = self.patchobject(self.fc.client, 'post_servers_1234_action', @@ -2180,8 +2185,10 @@ class ServersTest(common.HeatTestCase): scheduler.TaskRunner(server_resource.update, update_template)() self.assertEqual((server_resource.UPDATE, server_resource.COMPLETE), server_resource.state) - mock_post.called_once_with(body={'resize': {'flavorRef': 2}}) - mock_post.called_once_with(body={'confirmResize': None}) + mock_post.assert_has_calls([ + mock.call(body={'resize': {'flavorRef': '2'}}), + mock.call(body={'confirmResize': None}), + ]) @mock.patch.object(servers.Server, 'prepare_for_replace') def test_server_update_server_flavor_replace(self, mock_replace): @@ -2384,13 +2391,9 @@ class ServersTest(common.HeatTestCase): server.reparse() mock_rebuild = self.patchobject(self.fc.servers, 'rebuild') - def set_status(status): - return_server.status = status - return return_server - self.patchobject(self.fc.servers, 'get', - side_effect=[set_status('REBUILD'), - set_status('ERROR')]) + side_effect=ServerStatus(return_server, + ['REBUILD', 'ERROR'])) updater = scheduler.TaskRunner(server.update, update_template) error = self.assertRaises(exception.ResourceFailure, updater) self.assertEqual( @@ -2463,15 +2466,12 @@ class ServersTest(common.HeatTestCase): server.resource_id = '1234' server.state_set(state[0], state[1]) - def set_status(status): - return_server.status = status - return return_server - self.patchobject(return_server, 'suspend') self.patchobject(self.fc.servers, 'get', - side_effect=[set_status('ACTIVE'), - set_status('ACTIVE'), - set_status('SUSPENDED')]) + side_effect=ServerStatus(return_server, + ['ACTIVE', + 'ACTIVE', + 'SUSPENDED'])) scheduler.TaskRunner(server.suspend)() self.assertEqual((server.SUSPEND, server.COMPLETE), server.state) @@ -2495,15 +2495,12 @@ class ServersTest(common.HeatTestCase): 'srv_susp_uk') server.resource_id = '1234' - def set_status(status): - return_server.status = status - return return_server - self.patchobject(return_server, 'suspend') self.patchobject(self.fc.servers, 'get', - side_effect=[set_status('ACTIVE'), - set_status('ACTIVE'), - set_status('TRANSMOGRIFIED')]) + side_effect=ServerStatus(return_server, + ['ACTIVE', + 'ACTIVE', + 'TRANSMOGRIFIED'])) ex = self.assertRaises(exception.ResourceFailure, scheduler.TaskRunner(server.suspend)) self.assertIsInstance(ex.exc, exception.ResourceUnknownStatus) @@ -2520,15 +2517,12 @@ class ServersTest(common.HeatTestCase): server.resource_id = '1234' server.state_set(state[0], state[1]) - def set_status(status): - return_server.status = status - return return_server - self.patchobject(return_server, 'resume') self.patchobject(self.fc.servers, 'get', - side_effect=[set_status('SUSPENDED'), - set_status('SUSPENDED'), - set_status('ACTIVE')]) + side_effect=ServerStatus(return_server, + ['SUSPENDED', + 'SUSPENDED', + 'ACTIVE'])) scheduler.TaskRunner(server.resume)() self.assertEqual((server.RESUME, server.COMPLETE), server.state) @@ -2616,13 +2610,10 @@ class ServersTest(common.HeatTestCase): 'srv_sts_bld') server.resource_id = '1234' - def set_status(status): - return_server.status = status - return return_server - self.patchobject(self.fc.servers, 'get', - side_effect=[set_status(uncommon_status), - set_status('ACTIVE')]) + side_effect=ServerStatus(return_server, + [uncommon_status, + 'ACTIVE'])) scheduler.TaskRunner(server.create)() self.assertEqual((server.CREATE, server.COMPLETE), server.state) @@ -3903,10 +3894,9 @@ class ServersTest(common.HeatTestCase): self.assertEqual((server.UPDATE, server.COMPLETE), server.state) self.assertEqual(1, mock_detach.call_count) self.assertEqual(1, mock_attach.call_count) - mock_attach.called_once_with( - {'port_id': None, - 'net_id': auto_allocate_net, - 'fip': None}) + mock_attach.assert_called_once_with(None, + [auto_allocate_net], + None) else: self.assertRaises(exception.ResourceFailure, updater) self.assertEqual(0, mock_detach.call_count) @@ -4293,10 +4283,12 @@ class ServersTest(common.HeatTestCase): mock_create = self.patchobject(self.fc.servers, 'create', return_value=return_server) self.patchobject(self.fc.servers, 'get', - side_effect=[return_server, None]) + return_value=return_server) self.patchobject(neutron.NeutronClientPlugin, 'find_resourceid_by_name_or_id', return_value='aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa') + self.patchobject(return_server, 'get', return_value=None) + scheduler.TaskRunner(stack.create)() self.assertEqual(1, mock_create.call_count) self.assertEqual((stack.CREATE, stack.COMPLETE), stack.state) diff --git a/heat/tests/test_resource.py b/heat/tests/test_resource.py index 9c3736a97a..53336337e8 100644 --- a/heat/tests/test_resource.py +++ b/heat/tests/test_resource.py @@ -1055,10 +1055,8 @@ class ResourceTest(common.HeatTestCase): {'Foo': 'abc'}) res = generic_rsrc.ResourceWithProps('test_resource', tmpl, self.stack) - generic_rsrc.ResourceWithProps.handle_create = mock.Mock() - generic_rsrc.ResourceWithProps.handle_delete = mock.Mock() m_v.side_effect = [True, exception.StackValidationFailed()] - generic_rsrc.ResourceWithProps.handle_create.side_effect = [ + create_excs = [ exception.ResourceInError(resource_name='test_resource', resource_status='ERROR', resource_type='GenericResourceType', @@ -1071,8 +1069,11 @@ class ResourceTest(common.HeatTestCase): status_reason='just because'), None ] + self.patchobject(generic_rsrc.ResourceWithProps, 'handle_create', + side_effect=create_excs) - generic_rsrc.ResourceWithProps.handle_delete.return_value = None + self.patchobject(generic_rsrc.ResourceWithProps, 'handle_delete', + return_value=None) m_re.return_value = 0.01 scheduler.TaskRunner(res.create)() self.assertEqual(2, m_re.call_count) diff --git a/heat/tests/test_vpc.py b/heat/tests/test_vpc.py index 3d17bd06df..373f8e165b 100644 --- a/heat/tests/test_vpc.py +++ b/heat/tests/test_vpc.py @@ -95,7 +95,7 @@ class VPCTestBase(common.HeatTestCase): "tenant_id": "c1210485b2424d48804aad5d39c61b8f", "id": "aaaa" }}] - for i in range(3): + for i in range(7): show_network_returns.append( {"network": { "status": "ACTIVE", @@ -355,7 +355,7 @@ Resources: self.mockclient.show_network.assert_called_with('aaaa') self.mockclient.list_routers.assert_called_with(name=self.vpc_name) - self.assertEqual(2, self.mockclient.list_routers.call_count) + self.assertEqual(5, self.mockclient.list_routers.call_count) self.assertEqual(7, self.mockclient.show_network.call_count) @@ -719,14 +719,16 @@ Resources: self.assertEqual(2, self.mockclient.create_router.call_count) self.mockclient.create_router.assert_called_with( {'router': {'name': self.rt_name}}) - self.mockclient.add_interface_router.assert_called_once_with( - u'bbbb', {'subnet_id': 'cccc'}) + self.mockclient.add_interface_router.assert_has_calls([ + mock.call('bbbb', {'subnet_id': u'cccc'}), + mock.call('ffff', {'subnet_id': u'cccc'}), + ]) self.mockclient.list_networks.assert_called_once_with( **{'router:external': True}) gateway = stack['the_gateway'] self.assertResourceState(gateway, gateway.physical_resource_name()) - self.mockclient.add_gateway_router.assert_called_once_with( + self.mockclient.add_gateway_router.assert_called_with( 'ffff', {'network_id': '0389f747-7785-4757-b7bb-2ab07e4b09c3'}) attachment = stack['the_attachment'] @@ -736,9 +738,10 @@ Resources: self.assertEqual([route_table], list(attachment._vpc_route_tables())) stack.delete() - self.mockclient.remove_interface_router.assert_called_with( - 'ffff', - {'subnet_id': u'cccc'}) + self.mockclient.remove_interface_router.assert_has_calls([ + mock.call('ffff', {'subnet_id': u'cccc'}), + mock.call('bbbb', {'subnet_id': u'cccc'}), + ]) self.mockclient.remove_gateway_router.assert_called_with('ffff') self.assertEqual(2, self.mockclient.remove_gateway_router.call_count) self.assertEqual(2, self.mockclient.show_subnet.call_count) @@ -783,8 +786,10 @@ Resources: stack = self.create_stack(self.test_template) self.mockclient.create_router.assert_called_with( {'router': {'name': self.rt_name}}) - self.mockclient.add_interface_router.assert_called_once_with( - u'bbbb', {'subnet_id': 'cccc'}) + self.mockclient.add_interface_router.assert_has_calls([ + mock.call('bbbb', {'subnet_id': u'cccc'}), + mock.call('ffff', {'subnet_id': u'cccc'}), + ]) route_table = stack['the_route_table'] self.assertResourceState(route_table, 'ffff') @@ -800,9 +805,10 @@ Resources: scheduler.TaskRunner(route_table.delete)() stack.delete() - self.mockclient.remove_interface_router.assert_called_with( - 'ffff', - {'subnet_id': u'cccc'}) + self.mockclient.remove_interface_router.assert_has_calls([ + mock.call('ffff', {'subnet_id': u'cccc'}), + mock.call('bbbb', {'subnet_id': u'cccc'}), + ]) self.mockclient.remove_gateway_router.assert_called_once_with('ffff') self.assertEqual(2, self.mockclient.show_subnet.call_count) self.mockclient.show_subnet.assert_called_with('cccc')