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

The following replacements were done in tests to have
clearer messages in case of failure:
- assertTrue(a in b) with assertIn(a, b)
- assertTrue(a not in b) with assertNotIn(a, b)
- assertFalse(a in b) with assertNotIn(a, b)

The error message would now be like:
   'abc' not in ['a', 'b', 'c']
rather than:
   'False is not True'.

Change-Id: I92d039731f17b731d302c35fb619300078b8a638

10 files changed, 116 insertions, 41 deletions

diff --git a/nova/hacking/checks.py b/nova/hacking/checks.py
index 1b9e9e38b6..7f7ff2a7e5 100644
--- a/nova/hacking/checks.py
+++ b/nova/hacking/checks.py
@@ -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*('|\")")
@@ -457,6 +473,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)
@@ -479,3 +510,4 @@ def factory(register):
     register(CheckForStrUnicodeExc)
     register(CheckForTransAdd)
     register(check_oslo_namespace_imports)
+    register(assert_true_or_false_with_in)
diff --git a/nova/tests/unit/api/ec2/test_api.py b/nova/tests/unit/api/ec2/test_api.py
index cc4a2adb75..5236eda71c 100644
--- a/nova/tests/unit/api/ec2/test_api.py
+++ b/nova/tests/unit/api/ec2/test_api.py
@@ -285,9 +285,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
diff --git a/nova/tests/unit/cmd/test_idmapshift.py b/nova/tests/unit/cmd/test_idmapshift.py
index 2f0fe06bc0..215a797b28 100644
--- a/nova/tests/unit/cmd/test_idmapshift.py
+++ b/nova/tests/unit/cmd/test_idmapshift.py
@@ -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):
diff --git a/nova/tests/unit/compute/test_compute_utils.py b/nova/tests/unit/compute/test_compute_utils.py
index 805b3e360b..c53254d635 100644
--- a/nova/tests/unit/compute/test_compute_utils.py
+++ b/nova/tests/unit/compute/test_compute_utils.py
@@ -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')
diff --git a/nova/tests/unit/test_hacking.py b/nova/tests/unit/test_hacking.py
index 84ca6362cc..36433f1aa1 100644
--- a/nova/tests/unit/test_hacking.py
+++ b/nova/tests/unit/test_hacking.py
@@ -143,6 +143,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)
diff --git a/nova/tests/unit/test_iptables_network.py b/nova/tests/unit/test_iptables_network.py
index 870d5a021f..5afdf55c1f 100644
--- a/nova/tests/unit/test_iptables_network.py
+++ b/nova/tests/unit/test_iptables_network.py
@@ -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")
diff --git a/nova/tests/unit/virt/libvirt/test_driver.py b/nova/tests/unit/virt/libvirt/test_driver.py
index cfaf3f3a66..00507ee1cd 100644
--- a/nova/tests/unit/virt/libvirt/test_driver.py
+++ b/nova/tests/unit/virt/libvirt/test_driver.py
@@ -10396,7 +10396,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)
 
diff --git a/nova/tests/unit/virt/libvirt/test_firewall.py b/nova/tests/unit/virt/libvirt/test_firewall.py
index 41b0917814..c71d29c434 100644
--- a/nova/tests/unit/virt/libvirt/test_firewall.py
+++ b/nova/tests/unit/virt/libvirt/test_firewall.py
@@ -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
diff --git a/nova/tests/unit/virt/libvirt/test_imagecache.py b/nova/tests/unit/virt/libvirt/test_imagecache.py
index f46f7f4fd2..5eb80b5681 100644
--- a/nova/tests/unit/virt/libvirt/test_imagecache.py
+++ b/nova/tests/unit/virt/libvirt/test_imagecache.py
@@ -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):
diff --git a/nova/tests/unit/virt/xenapi/test_xenapi.py b/nova/tests/unit/virt/xenapi/test_xenapi.py
index 4b7edd5c48..a927244f53 100644
--- a/nova/tests/unit/virt/xenapi/test_xenapi.py
+++ b/nova/tests/unit/virt/xenapi/test_xenapi.py
@@ -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: