openstack/magnum
Code Issues Proposed changes

Use assertIn and assertNotIn

Browse Source 
Tests should use:
  self.assertIn(value, list)
  self.assertNotIn(value, list)

instead of:
  self.assertTrue(value in list)
  self.assertFalse(value in list)

because assertIn and assertNotIn raise more meaningful errors:
  self.assertIn(3, [1, 2])
  >>> MismatchError: 3 not in [1, 2]

  self.assertTrue(3 in [1, 2])
  >>> AssertionError: False is not true

Closes-Bug: #1510007
Change-Id: If33252cc93c06a85e61871fb7b22b726f4a08500
This commit is contained in:
Bertrand Lallau 2015-10-26 08:25:07 +01:00
parent a930ac216b
commit 798c7e35ef
6 changed files with 71 additions and 14 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
HACKING.rst
View File

@ -18,3 +18,5 @@ Magnum Specific Commandments
assertIsNotNone(A).
- [M316] Change assertTrue(isinstance(A, B)) by optimal assert like
assertIsInstance(A, B).
- [M334] Change assertTrue/False(A in/not in B, message) to the more specific
assertIn/NotIn(A, B, message)

26
magnum/hacking/checks.py
View File

@ -33,9 +33,13 @@ Guidelines for writing new hacking checks
enforce_re = re.compile(r"@policy.enforce_wsgi*")
decorator_re = re.compile(r"@.*")
mutable_default_args = re.compile(r"^\s*def .+\((.+=\{\}|.+=\[\])")
asse_equal_end_with_none_re = re.compile(
assert_equal_in_end_with_true_or_false_re = re.compile(
r"assertEqual\((\w|[][.'\"])+ in (\w|[][.'\", ])+, (True|False)\)")
assert_equal_in_start_with_true_or_false_re = re.compile(
r"assertEqual\((True|False), (\w|[][.'\"])+ in (\w|[][.'\", ])+\)")
assert_equal_end_with_none_re = re.compile(
r"(.)*assertEqual\((\w|\.|\'|\"|\[|\])+, None\)")
asse_equal_start_with_none_re = re.compile(
assert_equal_start_with_none_re = re.compile(
r"(.)*assertEqual\(None, (\w|\.|\'|\"|\[|\])+\)")
assert_equal_with_true_re = re.compile(
r"assertEqual\(True,")
@ -65,8 +69,8 @@ def assert_equal_none(logical_line):
"""
msg = ("M318: assertEqual(A, None) or assertEqual(None, A) "
"sentences not allowed")
res = (asse_equal_start_with_none_re.match(logical_line) or
asse_equal_end_with_none_re.match(logical_line))
res = (assert_equal_start_with_none_re.match(logical_line) or
assert_equal_end_with_none_re.match(logical_line))
if res:
yield (0, msg)
@ -106,6 +110,19 @@ def assert_true_isinstance(logical_line):
yield (0, "M316: assertTrue(isinstance(a, b)) sentences not allowed")
def assert_equal_in(logical_line):
"""Check for assertEqual(True|False, A in B), assertEqual(A in B, True|False)
M338
"""
res = (assert_equal_in_start_with_true_or_false_re.search(logical_line) or
assert_equal_in_end_with_true_or_false_re.search(logical_line))
if res:
yield (0, "M338: Use assertIn/NotIn(A, B) rather than "
"assertEqual(A in B, True/False) when checking collection "
"contents.")
def factory(register):
register(check_policy_enforce_decorator)
register(no_mutable_default_args)
@ -113,3 +130,4 @@ def factory(register):
register(assert_equal_true_or_false)
register(assert_equal_not_none)
register(assert_true_isinstance)
register(assert_equal_in)

6
magnum/tests/functional/python_client_base.py
View File

@ -153,20 +153,20 @@ class BayTest(BaseMagnumClient):
def _test_baymodel_create_and_delete(self, delete=True):
baymodel = self._create_baymodel('testbay', coe=self.coe)
list = [item.uuid for item in self.cs.baymodels.list()]
self.assertTrue(baymodel.uuid in list)
self.assertIn(baymodel.uuid, list)
if not delete:
return baymodel
else:
self.cs.baymodels.delete(baymodel.uuid)
list = [item.uuid for item in self.cs.baymodels.list()]
self.assertTrue(baymodel.uuid not in list)
self.assertNotIn(baymodel.uuid, list)
def _test_bay_create_and_delete(self):
baymodel = self._test_baymodel_create_and_delete(delete=False)
bay = self._create_bay('testbay', baymodel.uuid)
list = [item.uuid for item in self.cs.bays.list()]
self.assertTrue(bay.uuid in list)
self.assertIn(bay.uuid, list)
try:
self.assertIn(self.cs.bays.get(bay.uuid).status,

2
magnum/tests/unit/common/test_context.py
View File

@ -47,7 +47,7 @@ class ContextTestCase(base.TestCase):
self.assertEqual("tenant1", ctx.project_name)
self.assertEqual("tenant-id1", ctx.project_id)
for role in ctx.roles:
self.assertTrue(role in ['admin', 'service'])
self.assertIn(role, ['admin', 'service'])
self.assertTrue(ctx.is_admin)
self.assertTrue(ctx.read_only)
self.assertTrue(ctx.show_deleted)

12
magnum/tests/unit/objects/test_objects.py
View File

@ -226,7 +226,7 @@ class _TestObject(object):
except NotImplementedError as ex:
raised = True
self.assertTrue(raised)
self.assertTrue('foobar' in str(ex))
self.assertIn('foobar', str(ex))
def test_loaded_in_primitive(self):
obj = MyObj(self.context)
@ -246,7 +246,7 @@ class _TestObject(object):
obj.foo = 123
self.assertEqual(set(['foo']), obj.obj_what_changed())
primitive = obj.obj_to_primitive()
self.assertTrue('magnum_object.changes' in primitive)
self.assertIn('magnum_object.changes', primitive)
obj2 = MyObj.obj_from_primitive(primitive)
self.assertEqual(set(['foo']), obj2.obj_what_changed())
obj2.obj_reset_changes()
@ -346,10 +346,10 @@ class _TestObject(object):
def test_contains(self):
obj = MyObj(self.context)
self.assertFalse('foo' in obj)
self.assertNotIn('foo', obj)
obj.foo = 1
self.assertTrue('foo' in obj)
self.assertFalse('does_not_exist' in obj)
self.assertIn('foo', obj)
self.assertNotIn('does_not_exist', obj)
def test_obj_attr_is_set(self):
obj = MyObj(self.context, foo=1)
@ -463,7 +463,7 @@ class TestObjectSerializer(test_base.TestCase):
ser = base.MagnumObjectSerializer()
obj = MyObj(self.context)
primitive = ser.serialize_entity(self.context, obj)
self.assertTrue('magnum_object.name' in primitive)
self.assertIn('magnum_object.name', primitive)
obj2 = ser.deserialize_entity(self.context, primitive)
self.assertIsInstance(obj2, MyObj)
self.assertEqual(self.context, obj2._context)

37
magnum/tests/unit/test_hacking.py
View File

@ -86,6 +86,43 @@ class HackingTestCase(base.TestCase):
self._assert_has_errors(code, checks.check_policy_enforce_decorator,
expected_errors=[(2, 0, "M301")])
def test_assert_equal_in(self):
self.assertEqual(1, len(list(checks.assert_equal_in(
"self.assertEqual(a in b, True)"))))
self.assertEqual(1, len(list(checks.assert_equal_in(
"self.assertEqual('str' in 'string', True)"))))
self.assertEqual(0, len(list(checks.assert_equal_in(
"self.assertEqual(any(a==1 for a in b), True)"))))
self.assertEqual(1, len(list(checks.assert_equal_in(
"self.assertEqual(True, a in b)"))))
self.assertEqual(1, len(list(checks.assert_equal_in(
"self.assertEqual(True, 'str' in 'string')"))))
self.assertEqual(0, len(list(checks.assert_equal_in(
"self.assertEqual(True, any(a==1 for a in b))"))))
self.assertEqual(1, len(list(checks.assert_equal_in(
"self.assertEqual(a in b, False)"))))
self.assertEqual(1, len(list(checks.assert_equal_in(
"self.assertEqual('str' in 'string', False)"))))
self.assertEqual(0, len(list(checks.assert_equal_in(
"self.assertEqual(any(a==1 for a in b), False)"))))
self.assertEqual(1, len(list(checks.assert_equal_in(
"self.assertEqual(False, a in b)"))))
self.assertEqual(1, len(list(checks.assert_equal_in(
"self.assertEqual(False, 'str' in 'string')"))))
self.assertEqual(0, len(list(checks.assert_equal_in(
"self.assertEqual(False, any(a==1 for a in b))"))))
def test_assert_equal_none(self):
self.assertEqual(len(list(checks.assert_equal_none(
"self.assertEqual(A, None)"))), 1)