openstack/nova
Code Issues Proposed changes

Merge "Added hacking rule for assertTrue/False(A in B)"

Browse Source
This commit is contained in:
Jenkins 2015-01-13 15:23:11 +00:00 committed by Gerrit Code Review
commit efe19fab90
11 changed files with 118 additions and 41 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
HACKING.rst
View File

@ -44,6 +44,8 @@ Nova Specific Commandments
- [N331] Change LOG.warn on LOG.warning.
- [N332] Check that the api_version decorator is the first decorator on a method
- [N333] Check for oslo library imports use the non-namespaced packages
- [N334] Change assertTrue/False(A in/not in B, message) to the more specific
assertIn/NotIn(A, B, message)
Creating Unit Tests
-------------------

32
nova/hacking/checks.py
View File

@ -54,6 +54,22 @@ asse_equal_end_with_none_re = re.compile(
r"assertEqual\(.*?,\s+None\)$")
asse_equal_start_with_none_re = re.compile(
r"assertEqual\(None,")
# NOTE(snikitin): Next two regexes weren't united to one for more readability.
# asse_true_false_with_in_or_not_in regex checks
# assertTrue/False(A in B) cases where B argument has no spaces
# asse_true_false_with_in_or_not_in_spaces regex checks cases
# where B argument has spaces and starts/ends with [, ', ".
# For example: [1, 2, 3], "some string", 'another string'.
# We have to separate these regexes to escape a false positives
# results. B argument should have spaces only if it starts
# with [, ", '. Otherwise checking of string
# "assertFalse(A in B and C in D)" will be false positives.
# In this case B argument is "B and C in D".
asse_true_false_with_in_or_not_in = re.compile(r"assert(True|False)\("
r"(\w|[][.'\"])+( not)? in (\w|[][.'\",])+(, .*)?\)")
asse_true_false_with_in_or_not_in_spaces = re.compile(r"assert(True|False)"
r"\((\w|[][.'\"])+( not)? in [\[|'|\"](\w|[][.'\", ])+"
r"[\[|'|\"](, .*)?\)")
conf_attribute_set_re = re.compile(r"CONF\.[a-z0-9_.]+\s*=\s*\w")
log_translation = re.compile(
r"(.)*LOG\.(audit|error|critical)\(\s*('|\")")
@ -447,6 +463,21 @@ def check_oslo_namespace_imports(logical_line, blank_before, filename):
yield(0, msg)
def assert_true_or_false_with_in(logical_line):
"""Check for assertTrue/False(A in B), assertTrue/False(A not in B),
assertTrue/False(A in B, message) or assertTrue/False(A not in B, message)
sentences.
N334
"""
res = (asse_true_false_with_in_or_not_in.search(logical_line) or
asse_true_false_with_in_or_not_in_spaces.search(logical_line))
if res:
yield (0, "N334: Use assertIn/NotIn(A, B) rather than "
"assertTrue/False(A in/not in B) when checking collection "
"contents.")
def factory(register):
register(import_no_db_in_virt)
register(no_db_session_in_public_api)
@ -468,3 +499,4 @@ def factory(register):
register(CheckForStrUnicodeExc)
register(CheckForTransAdd)
register(check_oslo_namespace_imports)
register(assert_true_or_false_with_in)

6
nova/tests/unit/api/ec2/test_api.py
View File

@ -284,9 +284,9 @@ class ApiEc2TestCase(test.TestCase):
# Any request should be fine
self.ec2.get_all_instances()
self.assertTrue(self.ec2.APIVersion in self.http.getresponsebody(),
'The version in the xmlns of the response does '
'not match the API version given in the request.')
self.assertIn(self.ec2.APIVersion, self.http.getresponsebody(),
'The version in the xmlns of the response does '
'not match the API version given in the request.')
def test_describe_instances(self):
"""Test that, after creating a user and a project, the describe

2
nova/tests/unit/cmd/test_idmapshift.py
View File

@ -75,7 +75,7 @@ class FindTargetIDTestCase(BaseTestCase):
def test_find_target_id_updates_memo(self):
memo = dict()
idmapshift.find_target_id(0, self.uid_maps, idmapshift.NOBODY_ID, memo)
self.assertTrue(0 in memo)
self.assertIn(0, memo)
self.assertEqual(10000, memo[0])
def test_find_target_guest_id_greater_than_count(self):

13
nova/tests/unit/compute/test_compute_utils.py
View File

@ -487,8 +487,8 @@ class UsageInfoTestCase(test.TestCase):
'state', 'state_description',
'bandwidth', 'audit_period_beginning',
'audit_period_ending', 'image_meta'):
self.assertTrue(attr in payload,
msg="Key %s not in payload" % attr)
self.assertIn(attr, payload,
"Key %s not in payload" % attr)
self.assertEqual(payload['image_meta'],
{'md_key1': 'val1', 'md_key2': 'val2'})
image_ref_url = "%s/images/1" % glance.generate_glance_url()
@ -528,8 +528,7 @@ class UsageInfoTestCase(test.TestCase):
'state', 'state_description',
'bandwidth', 'audit_period_beginning',
'audit_period_ending', 'image_meta'):
self.assertTrue(attr in payload,
msg="Key %s not in payload" % attr)
self.assertIn(attr, payload, "Key %s not in payload" % attr)
self.assertEqual(payload['image_meta'],
{'md_key1': 'val1', 'md_key2': 'val2'})
image_ref_url = "%s/images/1" % glance.generate_glance_url()
@ -559,8 +558,7 @@ class UsageInfoTestCase(test.TestCase):
'state', 'state_description',
'bandwidth', 'audit_period_beginning',
'audit_period_ending', 'image_meta'):
self.assertTrue(attr in payload,
msg="Key %s not in payload" % attr)
self.assertIn(attr, payload, "Key %s not in payload" % attr)
self.assertEqual(payload['image_meta'], {})
image_ref_url = "%s/images/1" % glance.generate_glance_url()
self.assertEqual(payload['image_ref_url'], image_ref_url)
@ -595,8 +593,7 @@ class UsageInfoTestCase(test.TestCase):
self.assertEqual(str(payload['instance_flavor_id']), str(flavor_id))
for attr in ('display_name', 'created_at', 'launched_at',
'state', 'state_description', 'image_meta'):
self.assertTrue(attr in payload,
msg="Key %s not in payload" % attr)
self.assertIn(attr, payload, "Key %s not in payload" % attr)
self.assertEqual(payload['image_meta'],
{'md_key1': 'val1', 'md_key2': 'val2'})
self.assertEqual(payload['image_name'], 'fake_name')

48
nova/tests/unit/test_hacking.py
View File

@ -131,6 +131,54 @@ class HackingTestCase(test.NoDBTestCase):
self.assertEqual(
len(list(checks.assert_equal_none("self.assertIsNone()"))), 0)
def test_assert_true_or_false_with_in_or_not_in(self):
self.assertEqual(len(list(checks.assert_equal_none(
"self.assertEqual(A, None)"))), 1)
self.assertEqual(len(list(checks.assert_true_or_false_with_in(
"self.assertTrue(A in B)"))), 1)
self.assertEqual(len(list(checks.assert_true_or_false_with_in(
"self.assertFalse(A in B)"))), 1)
self.assertEqual(len(list(checks.assert_true_or_false_with_in(
"self.assertTrue(A not in B)"))), 1)
self.assertEqual(len(list(checks.assert_true_or_false_with_in(
"self.assertFalse(A not in B)"))), 1)
self.assertEqual(len(list(checks.assert_true_or_false_with_in(
"self.assertTrue(A in B, 'some message')"))), 1)
self.assertEqual(len(list(checks.assert_true_or_false_with_in(
"self.assertFalse(A in B, 'some message')"))), 1)
self.assertEqual(len(list(checks.assert_true_or_false_with_in(
"self.assertTrue(A not in B, 'some message')"))), 1)
self.assertEqual(len(list(checks.assert_true_or_false_with_in(
"self.assertFalse(A not in B, 'some message')"))), 1)
self.assertEqual(len(list(checks.assert_true_or_false_with_in(
"self.assertTrue(A in 'some string with spaces')"))), 1)
self.assertEqual(len(list(checks.assert_true_or_false_with_in(
"self.assertTrue(A in 'some string with spaces')"))), 1)
self.assertEqual(len(list(checks.assert_true_or_false_with_in(
"self.assertTrue(A in ['1', '2', '3'])"))), 1)
self.assertEqual(len(list(checks.assert_true_or_false_with_in(
"self.assertTrue(A in [1, 2, 3])"))), 1)
self.assertEqual(len(list(checks.assert_true_or_false_with_in(
"self.assertTrue(any(A > 5 for A in B))"))), 0)
self.assertEqual(len(list(checks.assert_true_or_false_with_in(
"self.assertTrue(any(A > 5 for A in B), 'some message')"))), 0)
self.assertEqual(len(list(checks.assert_true_or_false_with_in(
"self.assertFalse(some in list1 and some2 in list2)"))), 0)
def test_no_translate_debug_logs(self):
self.assertEqual(len(list(checks.no_translate_debug_logs(
"LOG.debug(_('foo'))", "nova/scheduler/foo.py"))), 1)

20
nova/tests/unit/test_iptables_network.py
View File

@ -163,14 +163,13 @@ class IptablesManagerTestCase(test.NoDBTestCase):
':%s-snat - [0:0]' % (self.binary_name),
':%s-PREROUTING - [0:0]' % (self.binary_name),
':%s-POSTROUTING - [0:0]' % (self.binary_name)]:
self.assertTrue(line in new_lines, "One of our chains went"
self.assertIn(line, new_lines, "One of our chains went"
" missing.")
seen_lines = set()
for line in new_lines:
line = line.strip()
self.assertTrue(line not in seen_lines,
"Duplicate line: %s" % line)
self.assertNotIn(line, seen_lines, "Duplicate line: %s" % line)
seen_lines.add(line)
last_postrouting_line = ''
@ -179,7 +178,7 @@ class IptablesManagerTestCase(test.NoDBTestCase):
if line.startswith('[0:0] -A POSTROUTING'):
last_postrouting_line = line
self.assertTrue('-j nova-postrouting-bottom' in last_postrouting_line,
self.assertIn('-j nova-postrouting-bottom', last_postrouting_line,
"Last POSTROUTING rule does not jump to "
"nova-postouting-bottom: %s" % last_postrouting_line)
@ -198,22 +197,21 @@ class IptablesManagerTestCase(test.NoDBTestCase):
':%s-INPUT - [0:0]' % (self.binary_name),
':%s-local - [0:0]' % (self.binary_name),
':%s-OUTPUT - [0:0]' % (self.binary_name)]:
self.assertTrue(line in new_lines, "One of our chains went"
self.assertIn(line, new_lines, "One of our chains went"
" missing.")
seen_lines = set()
for line in new_lines:
line = line.strip()
self.assertTrue(line not in seen_lines,
"Duplicate line: %s" % line)
self.assertNotIn(line, seen_lines, "Duplicate line: %s" % line)
seen_lines.add(line)
for chain in ['FORWARD', 'OUTPUT']:
for line in new_lines:
if line.startswith('[0:0] -A %s' % chain):
self.assertTrue('-j nova-filter-top' in line,
"First %s rule does not "
"jump to nova-filter-top" % chain)
self.assertIn('-j nova-filter-top', line,
"First %s rule does not "
"jump to nova-filter-top" % chain)
break
self.assertTrue('[0:0] -A nova-filter-top '
@ -233,7 +231,7 @@ class IptablesManagerTestCase(test.NoDBTestCase):
for line in ['*filter',
'COMMIT']:
self.assertTrue(line in new_lines, "One of iptables key lines "
self.assertIn(line, new_lines, "One of iptables key lines "
"went missing.")
self.assertTrue(len(new_lines) > 4, "No iptables rules added")

2
nova/tests/unit/virt/libvirt/test_driver.py
View File

@ -10402,7 +10402,7 @@ Active: 8381604 kB
conn.post_live_migration_at_destination(
self.context, instance, network_info, True,
block_device_info=block_device_info)
self.assertTrue('fake' in self.resultXML)
self.assertIn('fake', self.resultXML)
self.assertTrue(
block_device_info['block_device_mapping'][0].save.called)

24
nova/tests/unit/virt/libvirt/test_firewall.py
View File

@ -286,8 +286,8 @@ class IptablesFirewallTestCase(test.NoDBTestCase):
self.in_rules)
for rule in in_rules:
if 'nova' not in rule:
self.assertTrue(rule in self.out_rules,
'Rule went missing: %s' % rule)
self.assertIn(rule, self.out_rules,
'Rule went missing: %s' % rule)
instance_chain = None
for rule in self.out_rules:
@ -573,9 +573,9 @@ class NWFilterTestCase(test.NoDBTestCase):
self.recursive_depends[name] = []
for f in dom.getElementsByTagName('filterref'):
ref = f.getAttribute('filter')
self.assertTrue(ref in self.defined_filters,
('%s referenced filter that does ' +
'not yet exist: %s') % (name, ref))
self.assertIn(ref, self.defined_filters,
('%s referenced filter that does ' +
'not yet exist: %s') % (name, ref))
dependencies = [ref] + self.recursive_depends[ref]
self.recursive_depends[name] += dependencies
@ -598,14 +598,14 @@ class NWFilterTestCase(test.NoDBTestCase):
else:
required_not_list.append('allow-dhcp-server')
for required in requiredlist:
self.assertTrue(required in
self.recursive_depends[instance_filter],
"Instance's filter does not include %s" %
required)
self.assertIn(required,
self.recursive_depends[instance_filter],
"Instance's filter does not include %s" %
required)
for required_not in required_not_list:
self.assertFalse(required_not in
self.recursive_depends[instance_filter],
"Instance filter includes %s" % required_not)
self.assertNotIn(required_not,
self.recursive_depends[instance_filter],
"Instance filter includes %s" % required_not)
network_info = _fake_network_info(self.stubs, 1)
# since there is one (network_info) there is one vif

6
nova/tests/unit/virt/libvirt/test_imagecache.py
View File

@ -180,7 +180,7 @@ class ImageCacheManagerTestCase(test.NoDBTestCase):
self.assertNotIn(unexpected, image_cache_manager.originals)
self.assertEqual(1, len(image_cache_manager.back_swap_images))
self.assertTrue('swap_1000' in image_cache_manager.back_swap_images)
self.assertIn('swap_1000', image_cache_manager.back_swap_images)
def test_list_backing_images_small(self):
self.stubs.Set(os, 'listdir',
@ -875,8 +875,8 @@ class ImageCacheManagerTestCase(test.NoDBTestCase):
image_cache_manager._age_and_verify_swap_images(None, '/tmp_age_test')
self.assertEqual(1, len(expected_exist))
self.assertEqual(1, len(expected_remove))
self.assertTrue('swap_128' in expected_exist)
self.assertTrue('swap_256' in expected_remove)
self.assertIn('swap_128', expected_exist)
self.assertIn('swap_256', expected_remove)
class VerifyChecksumTestCase(test.NoDBTestCase):

4
nova/tests/unit/virt/xenapi/test_xenapi.py
View File

@ -2654,8 +2654,8 @@ class XenAPIDom0IptablesFirewallTestCase(stubs.XenAPITestBase):
self._in_rules)
for rule in in_rules:
if 'nova' not in rule:
self.assertTrue(rule in self._out_rules,
'Rule went missing: %s' % rule)
self.assertIn(rule, self._out_rules,
'Rule went missing: %s' % rule)
instance_chain = None
for rule in self._out_rules: