diff --git a/HACKING.rst b/HACKING.rst index 8db85525d38..6b9df25901c 100644 --- a/HACKING.rst +++ b/HACKING.rst @@ -17,6 +17,7 @@ Neutron Specific Commandments - [N325] Python 3: Do not use xrange. - [N326] Python 3: do not use basestring. - [N327] Python 3: do not use dict.iteritems. +- [N328] Detect wrong usage with assertEqual Creating Unit Tests ------------------- diff --git a/doc/source/devref/effective_neutron.rst b/doc/source/devref/effective_neutron.rst index 8b7981b118c..0d99e00356a 100644 --- a/doc/source/devref/effective_neutron.rst +++ b/doc/source/devref/effective_neutron.rst @@ -138,6 +138,7 @@ For anything more elaborate, please visit the testing section. reviewers to understand which one is the expected/observed value in non-trivial assertions. The expected and observed values are also labeled in the output when the assertion fails. +* Prefer specific assertions (assertTrue, assertFalse) over assertEqual(True/False, observed). * Don't write tests that don't test the intended code. This might seem silly but it's easy to do with a lot of mocks in place. Ensure that your tests break as expected before your code change. diff --git a/neutron/hacking/checks.py b/neutron/hacking/checks.py index cc6d6419b21..383c64e9f1e 100644 --- a/neutron/hacking/checks.py +++ b/neutron/hacking/checks.py @@ -174,6 +174,18 @@ def check_python3_no_iteritems(logical_line): yield(0, msg) +def check_asserttrue(logical_line, filename): + if 'neutron/tests/' in filename: + if re.search(r"assertEqual\(True,.*\)", logical_line): + msg = ("N328: Use assertTrue(observed) instead of" + "assertEqual(True, observed)") + yield (0, msg) + if re.search(r"assertEqual\(.*, True\)", logical_line): + msg = ("N328: Use assertTrue(observed) instead of" + "assertEqual(True, observed)") + yield (0, msg) + + def factory(register): register(validate_log_translations) register(use_jsonutils) @@ -184,3 +196,4 @@ def factory(register): register(check_python3_xrange) register(check_no_basestring) register(check_python3_no_iteritems) + register(check_asserttrue) diff --git a/neutron/tests/api/test_flavors_extensions.py b/neutron/tests/api/test_flavors_extensions.py index 8575c6f31d8..31e7898efa2 100644 --- a/neutron/tests/api/test_flavors_extensions.py +++ b/neutron/tests/api/test_flavors_extensions.py @@ -113,7 +113,7 @@ class TestFlavorsJson(base.BaseAdminNetworkTest): service_profile['description']) self.assertEqual(self.service_profile['metainfo'], service_profile['metainfo']) - self.assertEqual(True, service_profile['enabled']) + self.assertTrue(service_profile['enabled']) @test.attr(type='smoke') @test.idempotent_id('30abb445-0eea-472e-bd02-8649f54a5968') @@ -124,7 +124,7 @@ class TestFlavorsJson(base.BaseAdminNetworkTest): self.assertEqual(self.flavor['id'], flavor['id']) self.assertEqual(self.flavor['description'], flavor['description']) self.assertEqual(self.flavor['name'], flavor['name']) - self.assertEqual(True, flavor['enabled']) + self.assertTrue(flavor['enabled']) @test.attr(type='smoke') @test.idempotent_id('e2fb2f8c-45bf-429a-9f17-171c70444612') diff --git a/neutron/tests/functional/api/test_policies.py b/neutron/tests/functional/api/test_policies.py index c73400a269f..c5b70583355 100644 --- a/neutron/tests/functional/api/test_policies.py +++ b/neutron/tests/functional/api/test_policies.py @@ -71,8 +71,7 @@ class APIPolicyTestCase(base.BaseTestCase): tenant_context = context.Context('test_user', 'test_tenant_id', False) extension_manager.extend_resources(self.api_version, attributes.RESOURCE_ATTRIBUTE_MAP) - self.assertEqual(self._check_external_router_policy(admin_context), - True) + self.assertTrue(self._check_external_router_policy(admin_context)) self.assertEqual(self._check_external_router_policy(tenant_context), False) @@ -87,10 +86,8 @@ class APIPolicyTestCase(base.BaseTestCase): attributes.RESOURCE_ATTRIBUTE_MAP) admin_context = context.get_admin_context() tenant_context = context.Context('test_user', 'test_tenant_id', False) - self.assertEqual(self._check_external_router_policy(admin_context), - True) - self.assertEqual(self._check_external_router_policy(tenant_context), - True) + self.assertTrue(self._check_external_router_policy(admin_context)) + self.assertTrue(self._check_external_router_policy(tenant_context)) def tearDown(self): policy.reset() diff --git a/neutron/tests/unit/agent/l3/test_agent.py b/neutron/tests/unit/agent/l3/test_agent.py index a40a29dcc93..60812cf2079 100644 --- a/neutron/tests/unit/agent/l3/test_agent.py +++ b/neutron/tests/unit/agent/l3/test_agent.py @@ -204,7 +204,7 @@ class TestBasicRouterOperations(BasicRouterOperationsFramework): agent = l3_agent.L3NATAgentWithStateReport(host=HOSTNAME, conf=self.conf) - self.assertEqual(agent.agent_state['start_flag'], True) + self.assertTrue(agent.agent_state['start_flag']) use_call_arg = agent.use_call agent.after_start() report_state.assert_called_once_with(agent.context, diff --git a/neutron/tests/unit/api/v2/test_base.py b/neutron/tests/unit/api/v2/test_base.py index 1491dd7ed68..8507df9f91f 100644 --- a/neutron/tests/unit/api/v2/test_base.py +++ b/neutron/tests/unit/api/v2/test_base.py @@ -790,7 +790,7 @@ class JSONV2TestCase(APIv2TestBase, testlib_api.WebTestCase): self.assertIn('network', res) net = res['network'] self.assertEqual(net['id'], net_id) - self.assertEqual(net['admin_state_up'], True) + self.assertTrue(net['admin_state_up']) self.assertEqual(net['status'], "ACTIVE") def test_create_no_keystone_env(self): diff --git a/neutron/tests/unit/db/test_allowedaddresspairs_db.py b/neutron/tests/unit/db/test_allowedaddresspairs_db.py index af0ec336dc9..ecf2670f621 100644 --- a/neutron/tests/unit/db/test_allowedaddresspairs_db.py +++ b/neutron/tests/unit/db/test_allowedaddresspairs_db.py @@ -133,7 +133,7 @@ class TestAllowedAddressPairs(AllowedAddressPairDBTestCase): port_security_enabled=True, allowed_address_pairs=address_pairs) port = self.deserialize(self.fmt, res) - self.assertEqual(port['port'][psec.PORTSECURITY], True) + self.assertTrue(port['port'][psec.PORTSECURITY]) self.assertEqual(port['port'][addr_pair.ADDRESS_PAIRS], address_pairs) self._delete('ports', port['port']['id']) diff --git a/neutron/tests/unit/db/test_db_base_plugin_v2.py b/neutron/tests/unit/db/test_db_base_plugin_v2.py index 3192297333b..759eccffb6f 100644 --- a/neutron/tests/unit/db/test_db_base_plugin_v2.py +++ b/neutron/tests/unit/db/test_db_base_plugin_v2.py @@ -1303,7 +1303,7 @@ fixed_ips=ip_address%%3D%s&fixed_ips=ip_address%%3D%s&fixed_ips=subnet_id%%3D%s 'device_id': port['port']['device_id']}} req = self.new_update_request('ports', data, port['port']['id']) res = self.deserialize(self.fmt, req.get_response(self.api)) - self.assertEqual(res['port']['admin_state_up'], True) + self.assertTrue(res['port']['admin_state_up']) def test_update_device_id_null(self): with self.port() as port: @@ -2306,7 +2306,7 @@ class TestNetworksV2(NeutronDbPluginV2TestCase): ctx = context.Context('', '', is_admin=True) subnet_db = manager.NeutronManager.get_plugin().get_subnet( ctx, subnet['subnet']['id']) - self.assertEqual(subnet_db['shared'], True) + self.assertTrue(subnet_db['shared']) def test_update_network_set_not_shared_single_tenant(self): with self.network(shared=True) as network: diff --git a/neutron/tests/unit/extensions/test_external_net.py b/neutron/tests/unit/extensions/test_external_net.py index 8e887973b82..cf833b9d737 100644 --- a/neutron/tests/unit/extensions/test_external_net.py +++ b/neutron/tests/unit/extensions/test_external_net.py @@ -161,8 +161,7 @@ class ExtNetDBTestCase(test_db_base_plugin_v2.NeutronDbPluginV2TestCase): def test_create_external_network_admin_succeeds(self): with self.network(router__external=True) as ext_net: - self.assertEqual(ext_net['network'][external_net.EXTERNAL], - True) + self.assertTrue(ext_net['network'][external_net.EXTERNAL]) def test_delete_network_check_disassociated_floatingips(self): with mock.patch.object(manager.NeutronManager, diff --git a/neutron/tests/unit/extensions/test_l3.py b/neutron/tests/unit/extensions/test_l3.py index 19b7abac098..4be077e1e61 100644 --- a/neutron/tests/unit/extensions/test_l3.py +++ b/neutron/tests/unit/extensions/test_l3.py @@ -114,7 +114,7 @@ class L3NatExtensionTestCase(test_extensions_base.ExtensionTestCase): router = res['router'] self.assertEqual(router['id'], router_id) self.assertEqual(router['status'], "ACTIVE") - self.assertEqual(router['admin_state_up'], True) + self.assertTrue(router['admin_state_up']) def test_router_list(self): router_id = _uuid() diff --git a/neutron/tests/unit/extensions/test_portsecurity.py b/neutron/tests/unit/extensions/test_portsecurity.py index 76a269839ec..353477b6cd3 100644 --- a/neutron/tests/unit/extensions/test_portsecurity.py +++ b/neutron/tests/unit/extensions/test_portsecurity.py @@ -176,7 +176,7 @@ class TestPortSecurity(PortSecurityDBTestCase): def test_create_network_with_portsecurity_mac(self): res = self._create_network('json', 'net1', True) net = self.deserialize('json', res) - self.assertEqual(net['network'][psec.PORTSECURITY], True) + self.assertTrue(net['network'][psec.PORTSECURITY]) def test_create_network_with_portsecurity_false(self): res = self._create_network('json', 'net1', True, @@ -189,7 +189,7 @@ class TestPortSecurity(PortSecurityDBTestCase): res = self._create_network('json', 'net1', True, port_security_enabled='True') net = self.deserialize('json', res) - self.assertEqual(net['network'][psec.PORTSECURITY], True) + self.assertTrue(net['network'][psec.PORTSECURITY]) update_net = {'network': {psec.PORTSECURITY: False}} req = self.new_update_request('networks', update_net, net['network']['id']) @@ -203,7 +203,7 @@ class TestPortSecurity(PortSecurityDBTestCase): with self.network() as net: res = self._create_port('json', net['network']['id']) port = self.deserialize('json', res) - self.assertEqual(port['port'][psec.PORTSECURITY], True) + self.assertTrue(port['port'][psec.PORTSECURITY]) self._delete('ports', port['port']['id']) def test_create_port_passing_true(self): @@ -213,7 +213,7 @@ class TestPortSecurity(PortSecurityDBTestCase): net = self.deserialize('json', res) res = self._create_port('json', net['network']['id']) port = self.deserialize('json', res) - self.assertEqual(port['port'][psec.PORTSECURITY], True) + self.assertTrue(port['port'][psec.PORTSECURITY]) self._delete('ports', port['port']['id']) def test_create_port_on_port_security_false_network(self): @@ -235,7 +235,7 @@ class TestPortSecurity(PortSecurityDBTestCase): arg_list=('port_security_enabled',), port_security_enabled=True) port = self.deserialize('json', res) - self.assertEqual(port['port'][psec.PORTSECURITY], True) + self.assertTrue(port['port'][psec.PORTSECURITY]) self._delete('ports', port['port']['id']) def test_create_port_fails_with_secgroup_and_port_security_false(self): @@ -261,7 +261,7 @@ class TestPortSecurity(PortSecurityDBTestCase): with self.subnet(network=net): res = self._create_port('json', net['network']['id']) port = self.deserialize('json', res) - self.assertEqual(port['port'][psec.PORTSECURITY], True) + self.assertTrue(port['port'][psec.PORTSECURITY]) self.assertEqual(len(port['port'][ext_sg.SECURITYGROUPS]), 1) self._delete('ports', port['port']['id']) @@ -285,7 +285,7 @@ class TestPortSecurity(PortSecurityDBTestCase): port_security_enabled=True, security_groups=[security_group_id]) port = self.deserialize('json', res) - self.assertEqual(port['port'][psec.PORTSECURITY], True) + self.assertTrue(port['port'][psec.PORTSECURITY]) self.assertEqual(port['port']['security_groups'], [security_group_id]) self._delete('ports', port['port']['id']) @@ -307,7 +307,7 @@ class TestPortSecurity(PortSecurityDBTestCase): with self.subnet(network=net): res = self._create_port('json', net['network']['id']) port = self.deserialize('json', res) - self.assertEqual(port['port'][psec.PORTSECURITY], True) + self.assertTrue(port['port'][psec.PORTSECURITY]) update_port = {'port': {psec.PORTSECURITY: False}} req = self.new_update_request('ports', update_port, @@ -331,7 +331,7 @@ class TestPortSecurity(PortSecurityDBTestCase): arg_list=('port_security_enabled',), port_security_enabled=True) port = self.deserialize('json', res) - self.assertEqual(port['port'][psec.PORTSECURITY], True) + self.assertTrue(port['port'][psec.PORTSECURITY]) # remove security group on port update_port = {'port': {ext_sg.SECURITYGROUPS: None, @@ -352,7 +352,7 @@ class TestPortSecurity(PortSecurityDBTestCase): arg_list=('port_security_enabled',), port_security_enabled=True) port = self.deserialize('json', res) - self.assertEqual(port['port'][psec.PORTSECURITY], True) + self.assertTrue(port['port'][psec.PORTSECURITY]) # remove security group on port update_port = {'port': {ext_sg.SECURITYGROUPS: None, @@ -369,7 +369,7 @@ class TestPortSecurity(PortSecurityDBTestCase): port['port']['id']) port = self.deserialize('json', req.get_response(self.api)) - self.assertEqual(port['port'][psec.PORTSECURITY], True) + self.assertTrue(port['port'][psec.PORTSECURITY]) self.assertEqual(len(port['port'][ext_sg.SECURITYGROUPS]), 1) self._delete('ports', port['port']['id']) diff --git a/neutron/tests/unit/extensions/test_vlantransparent.py b/neutron/tests/unit/extensions/test_vlantransparent.py index 717d306b70c..ee48f0828b3 100644 --- a/neutron/tests/unit/extensions/test_vlantransparent.py +++ b/neutron/tests/unit/extensions/test_vlantransparent.py @@ -82,7 +82,7 @@ class VlanTransparentExtensionTestCase(test_db_base_plugin_v2.TestNetworksV2): res = self.deserialize(self.fmt, req.get_response(self.api)) self.assertEqual(net['network']['name'], res['network']['name']) - self.assertEqual(True, res['network'][vlt.VLANTRANSPARENT]) + self.assertTrue(res['network'][vlt.VLANTRANSPARENT]) def test_network_create_with_bad_vlan_transparent_attr(self): vlantrans = {'vlan_transparent': "abc"} diff --git a/neutron/tests/unit/hacking/test_checks.py b/neutron/tests/unit/hacking/test_checks.py index f3e98c92da3..d6e1d5f8db6 100644 --- a/neutron/tests/unit/hacking/test_checks.py +++ b/neutron/tests/unit/hacking/test_checks.py @@ -154,3 +154,26 @@ class HackingTestCase(base.BaseTestCase): f = checks.check_python3_no_iteritems self.assertLineFails(f, "d.iteritems()") self.assertLinePasses(f, "six.iteritems(d)") + + def test_asserttrue(self): + fail_code1 = """ + test_bool = True + self.assertEqual(True, test_bool) + """ + fail_code2 = """ + test_bool = True + self.assertEqual(test_bool, True) + """ + pass_code = """ + test_bool = True + self.assertTrue(test_bool) + """ + self.assertEqual( + 1, len(list(checks.check_asserttrue(fail_code1, + "neutron/tests/test_assert.py")))) + self.assertEqual( + 1, len(list(checks.check_asserttrue(fail_code2, + "neutron/tests/test_assert.py")))) + self.assertEqual( + 0, len(list(checks.check_asserttrue(pass_code, + "neutron/tests/test_assert.py")))) diff --git a/neutron/tests/unit/plugins/ml2/drivers/openvswitch/agent/test_ovs_neutron_agent.py b/neutron/tests/unit/plugins/ml2/drivers/openvswitch/agent/test_ovs_neutron_agent.py index 9cd455bba42..c3539c1c1c8 100644 --- a/neutron/tests/unit/plugins/ml2/drivers/openvswitch/agent/test_ovs_neutron_agent.py +++ b/neutron/tests/unit/plugins/ml2/drivers/openvswitch/agent/test_ovs_neutron_agent.py @@ -108,7 +108,7 @@ class CreateAgentConfigMap(ovs_test_base.OVSAgentConfigTestBase): cfg.CONF.set_override('enable_distributed_routing', True, group='AGENT') cfgmap = self.mod_agent.create_agent_config_map(cfg.CONF) - self.assertEqual(cfgmap['enable_distributed_routing'], True) + self.assertTrue(cfgmap['enable_distributed_routing']) class TestOvsNeutronAgent(object): diff --git a/neutron/tests/unit/test_auth.py b/neutron/tests/unit/test_auth.py index 74323f071b1..ad20a2321aa 100644 --- a/neutron/tests/unit/test_auth.py +++ b/neutron/tests/unit/test_auth.py @@ -74,7 +74,7 @@ class NeutronKeystoneContextTestCase(base.BaseTestCase): self.assertEqual(response.status, '200 OK') self.assertEqual(self.context.roles, ['role1', 'role2', 'role3', 'role4', 'role5', 'AdMiN']) - self.assertEqual(self.context.is_admin, True) + self.assertTrue(self.context.is_admin) def test_with_user_tenant_name(self): self.request.headers['X_PROJECT_ID'] = 'testtenantid' diff --git a/neutron/tests/unit/test_policy.py b/neutron/tests/unit/test_policy.py index ed230179fdf..019b9f92ec9 100644 --- a/neutron/tests/unit/test_policy.py +++ b/neutron/tests/unit/test_policy.py @@ -103,7 +103,7 @@ class PolicyTestCase(base.BaseTestCase): def test_enforce_good_action(self): action = "example:allowed" result = policy.enforce(self.context, action, self.target) - self.assertEqual(result, True) + self.assertTrue(result) @mock.patch.object(urlrequest, 'urlopen', return_value=six.StringIO("True")) @@ -111,7 +111,7 @@ class PolicyTestCase(base.BaseTestCase): action = "example:get_http" target = {} result = policy.enforce(self.context, action, target) - self.assertEqual(result, True) + self.assertTrue(result) def test_enforce_http_false(self): @@ -305,7 +305,7 @@ class NeutronPolicyTestCase(base.BaseTestCase): context, action, target) else: result = policy.enforce(context, action, target) - self.assertEqual(result, True) + self.assertTrue(result) def _test_nonadmin_action_on_attr(self, action, attr, value, exception=None, **kwargs): @@ -411,7 +411,7 @@ class NeutronPolicyTestCase(base.BaseTestCase): if kwargs: target.update(kwargs) result = policy.enforce(admin_context, action, target) - self.assertEqual(result, True) + self.assertTrue(result) def test_enforce_adminonly_attribute_create(self): self._test_enforce_adminonly_attribute('create_network') @@ -469,7 +469,7 @@ class NeutronPolicyTestCase(base.BaseTestCase): action = "create_" + FAKE_RESOURCE_NAME target = {'tenant_id': 'fake', 'attr': {'sub_attr_1': 'x'}} result = policy.enforce(self.context, action, target, None) - self.assertEqual(result, True) + self.assertTrue(result) def test_enforce_admin_only_subattribute(self): action = "create_" + FAKE_RESOURCE_NAME @@ -477,7 +477,7 @@ class NeutronPolicyTestCase(base.BaseTestCase): 'sub_attr_2': 'y'}} result = policy.enforce(context.get_admin_context(), action, target, None) - self.assertEqual(result, True) + self.assertTrue(result) def test_enforce_admin_only_subattribute_nonadminctx_returns_403(self): action = "create_" + FAKE_RESOURCE_NAME