Merge "Fix hacking checks"

tests/hacking/checks.py

@@ -68,11 +68,11 @@ re_objects_import = re.compile(r"^from rally.common import objects")
 def skip_ignored_lines(func):
 
     @functools.wraps(func)
-    def wrapper(logical_line, filename):
+    def wrapper(logical_line, physical_line, filename):
-        line = logical_line.strip()
+        line = physical_line.strip()
         if not line or line.startswith("#") or line.endswith("# noqa"):
             return
-        yield next(func(logical_line, filename))
+        yield next(func(logical_line, physical_line, filename))
 
     return wrapper
 
@@ -88,7 +88,7 @@ def _parse_assert_mock_str(line):
 
 
 @skip_ignored_lines
-def check_assert_methods_from_mock(logical_line, filename):
+def check_assert_methods_from_mock(logical_line, physical_line, filename):
     """Ensure that ``assert_*`` methods from ``mock`` library is used correctly
 
     N301 - base error number
@@ -134,7 +134,7 @@ def check_assert_methods_from_mock(logical_line, filename):
 
 
 @skip_ignored_lines
-def check_import_of_logging(logical_line, filename):
+def check_import_of_logging(logical_line, physical_line, filename):
     """Check correctness import of logging module
 
     N310
@@ -156,7 +156,7 @@ def check_import_of_logging(logical_line, filename):
 
 
 @skip_ignored_lines
-def no_translate_debug_logs(logical_line, filename):
+def no_translate_debug_logs(logical_line, physical_line, filename):
     """Check for "LOG.debug(_("
 
     As per our translation policy,
@@ -174,7 +174,7 @@ def no_translate_debug_logs(logical_line, filename):
 
 
 @skip_ignored_lines
-def no_use_conf_debug_check(logical_line, filename):
+def no_use_conf_debug_check(logical_line, physical_line, filename):
     """Check for "cfg.CONF.debug"
 
     Rally has two DEBUG level:
@@ -194,7 +194,7 @@ def no_use_conf_debug_check(logical_line, filename):
 
 
 @skip_ignored_lines
-def assert_true_instance(logical_line, filename):
+def assert_true_instance(logical_line, physical_line, filename):
     """Check for assertTrue(isinstance(a, b)) sentences
 
     N320
@@ -205,7 +205,7 @@ def assert_true_instance(logical_line, filename):
 
 
 @skip_ignored_lines
-def assert_equal_type(logical_line, filename):
+def assert_equal_type(logical_line, physical_line, filename):
     """Check for assertEqual(type(A), B) sentences
 
     N321
@@ -216,7 +216,7 @@ def assert_equal_type(logical_line, filename):
 
 
 @skip_ignored_lines
-def assert_equal_none(logical_line, filename):
+def assert_equal_none(logical_line, physical_line, filename):
     """Check for assertEqual(A, None) or assertEqual(None, A) sentences
 
     N322
@@ -230,7 +230,7 @@ def assert_equal_none(logical_line, filename):
 
 
 @skip_ignored_lines
-def assert_true_or_false_with_in(logical_line, filename):
+def assert_true_or_false_with_in(logical_line, physical_line, filename):
     """Check assertTrue/False(A in/not in B) with collection contents
 
     Check for assertTrue/False(A in B), assertTrue/False(A not in B),
@@ -248,7 +248,7 @@ def assert_true_or_false_with_in(logical_line, filename):
 
 
 @skip_ignored_lines
-def assert_equal_in(logical_line, filename):
+def assert_equal_in(logical_line, physical_line, filename):
     """Check assertEqual(A in/not in B, True/False) with collection contents
 
     Check for assertEqual(A in B, True/False), assertEqual(True/False, A in B),
@@ -266,7 +266,8 @@ def assert_equal_in(logical_line, filename):
 
 
 @skip_ignored_lines
-def check_no_direct_rally_objects_import(logical_line, filename):
+def check_no_direct_rally_objects_import(logical_line, physical_line,
+                                         filename):
     """Check if rally.common.objects are properly imported.
 
     If you import "from rally.common import objects" you are able to use
@@ -288,7 +289,7 @@ def check_no_direct_rally_objects_import(logical_line, filename):
 
 
 @skip_ignored_lines
-def check_no_oslo_deprecated_import(logical_line, filename):
+def check_no_oslo_deprecated_import(logical_line, physical_line, filename):
     """Check if oslo.foo packages are not imported instead of oslo_foo ones.
 
     Libraries from oslo.foo namespace are deprecated because of namespace
@@ -304,7 +305,7 @@ def check_no_oslo_deprecated_import(logical_line, filename):
 
 
 @skip_ignored_lines
-def check_quotes(logical_line, filename):
+def check_quotes(logical_line, physical_line, filename):
     """Check that single quotation marks are not used
 
     N350
@@ -357,7 +358,7 @@ def check_quotes(logical_line, filename):
 
 
 @skip_ignored_lines
-def check_no_constructor_data_struct(logical_line, filename):
+def check_no_constructor_data_struct(logical_line, physical_line, filename):
     """Check that data structs (lists, dicts) are declared using literals
 
     N351
@@ -439,7 +440,7 @@ def check_dict_formatting_in_string(logical_line, tokens):
 
 
 @skip_ignored_lines
-def check_using_unicode(logical_line, filename):
+def check_using_unicode(logical_line, physical_line, filename):
     """Check crosspython unicode usage
 
     N353
@@ -465,7 +466,7 @@ def check_raises(physical_line, filename):
 
 
 @skip_ignored_lines
-def check_db_imports_in_cli(logical_line, filename):
+def check_db_imports_in_cli(logical_line, physical_line, filename):
     """Ensure that CLI modules do not use ``rally.common.db``
 
     N360
@@ -479,7 +480,7 @@ def check_db_imports_in_cli(logical_line, filename):
 
 
 @skip_ignored_lines
-def check_objects_imports_in_cli(logical_line, filename):
+def check_objects_imports_in_cli(logical_line, physical_line, filename):
     """Ensure that CLI modules do not use ``rally.common.objects``
 
     N361

tests/unit/test_hacking.py

@@ -12,12 +12,14 @@
 
 import tokenize
 
+import ddt
 import six
 
 from tests.hacking import checks
 from tests.unit import test
 
 
+@ddt.ddt
 class HackingTestCase(test.TestCase):
 
     def test__parse_assert_mock_str(self):
@@ -33,30 +35,35 @@ class HackingTestCase(test.TestCase):
         self.assertIsNone(method)
         self.assertIsNone(obj)
 
-    def test_skip_ignored_lines(self):
+    @ddt.data(
+        {"line": "fdafadfdas  # noqa", "result": []},
+        {"line": "  # fdafadfdas", "result": []},
+        {"line": "  ", "result": []},
+        {"line": "otherstuff", "result": [42]}
+    )
+    @ddt.unpack
+    def test_skip_ignored_lines(self, line, result):
 
         @checks.skip_ignored_lines
-        def any_gen(logical_line, file_name):
+        def any_gen(physical_line, logical_line, file_name):
             yield 42
 
-        self.assertEqual([], list(any_gen("fdafadfdas  # noqa", "f")))
+        self.assertEqual(result, list(any_gen(line, line, "f")))
-        self.assertEqual([], list(any_gen("  # fdafadfdas", "f")))
-        self.assertEqual([], list(any_gen("  ", "f")))
-        self.assertEqual(42, next(any_gen("otherstuff", "f")))
 
     def test_correct_usage_of_assert_from_mock(self):
         correct_method_names = ["assert_any_call", "assert_called_once_with",
                                 "assert_called_with", "assert_has_calls"]
         for name in correct_method_names:
+            line = "some_mock.%s(asd)" % name
             self.assertEqual(0, len(
                 list(checks.check_assert_methods_from_mock(
-                    "some_mock.%s(asd)" % name, "./tests/fake/test"))))
+                    line, line, "./tests/fake/test"))))
 
     def test_wrong_usage_of_broad_assert_from_mock(self):
         fake_method = "rtfm.assert_something()"
 
         actual_number, actual_msg = next(checks.check_assert_methods_from_mock(
-            fake_method, "./tests/fake/test"))
+            fake_method, fake_method, "./tests/fake/test"))
         self.assertEqual(4, actual_number)
         self.assertTrue(actual_msg.startswith("N301"))
 
@@ -64,7 +71,7 @@ class HackingTestCase(test.TestCase):
         fake_method = "rtfm.assert_called()"
 
         actual_number, actual_msg = next(checks.check_assert_methods_from_mock(
-            fake_method, "./tests/fake/test"))
+            fake_method, fake_method, "./tests/fake/test"))
         self.assertEqual(4, actual_number)
         self.assertTrue(actual_msg.startswith("N302"))
 
@@ -72,17 +79,17 @@ class HackingTestCase(test.TestCase):
         fake_method = "rtfm.assert_called_once()"
 
         actual_number, actual_msg = next(checks.check_assert_methods_from_mock(
-            fake_method, "./tests/fake/test"))
+            fake_method, fake_method, "./tests/fake/test"))
         self.assertEqual(4, actual_number)
         self.assertTrue(actual_msg.startswith("N303"))
 
     def _assert_good_samples(self, checker, samples, module_file="f"):
         for s in samples:
-            self.assertEqual([], list(checker(s, module_file)), s)
+            self.assertEqual([], list(checker(s, s, module_file)), s)
 
     def _assert_bad_samples(self, checker, samples, module_file="f"):
         for s in samples:
-            self.assertEqual(1, len(list(checker(s, module_file))), s)
+            self.assertEqual(1, len(list(checker(s, s, module_file))), s)
 
     def test_check_wrong_logging_import(self):
         bad_imports = ["from oslo_log import log",
@@ -92,18 +99,17 @@ class HackingTestCase(test.TestCase):
                         "from rally.common.logging",
                         "import rally.common.logging"]
 
-        for bad_import in bad_imports:
+        for bad in bad_imports:
-            checkres = checks.check_import_of_logging(bad_import, "fakefile")
+            checkres = checks.check_import_of_logging(bad, bad, "fakefile")
             self.assertIsNotNone(next(checkres))
 
-        for bad_import in bad_imports:
+        for bad in bad_imports:
             checkres = checks.check_import_of_logging(
-                bad_import, "./rally/common/logging.py")
+                bad, bad, "./rally/common/logging.py")
             self.assertEqual([], list(checkres))
 
-        for good_import in good_imports:
+        for good in good_imports:
-            checkres = checks.check_import_of_logging(good_import,
+            checkres = checks.check_import_of_logging(good, good, "fakefile")
-                                                      "fakefile")
             self.assertEqual([], list(checkres))
 
     def test_no_translate_debug_logs(self):
@@ -123,31 +129,46 @@ class HackingTestCase(test.TestCase):
         good_samples = ["if logging.is_debug()"]
         self._assert_good_samples(checks.no_use_conf_debug_check, good_samples)
 
-    def test_assert_true_instance(self):
+    @ddt.data(
-        self.assertEqual(len(list(checks.assert_true_instance(
+        {
-            "self.assertTrue(isinstance(e, "
+            "line": "self.assertTrue(isinstance(e, exception.BuildAbortExc))",
-            "exception.BuildAbortException))", "f"))), 1)
+            "result": 1
+        },
+        {
+            "line": "self.assertTrue()",
+            "result": 0
+        }
+    )
+    @ddt.unpack
+    def test_assert_true_instance(self, line, result):
+        self.assertEqual(
+            result, len(list(checks.assert_true_instance(line, line, "f"))))
+
+    @ddt.data(
+        {
+            "line": "self.assertEqual(type(als['QuicAssist']), list)",
+            "result": 1
+        },
+        {
+            "line": "self.assertTrue()",
+            "result": 0
+        }
+    )
+    @ddt.unpack
+    def test_assert_equal_type(self, line, result):
+        self.assertEqual(
+            len(list(checks.assert_equal_type(line, line, "f"))), result)
+
+    @ddt.data(
+        {"line": "self.assertEqual(A, None)", "result": 1},
+        {"line": "self.assertEqual(None, A)", "result": 1},
+        {"line": "self.assertIsNone()", "result": 0}
+    )
+    @ddt.unpack
+    def test_assert_equal_none(self, line, result):
 
         self.assertEqual(
-            0,
+            len(list(checks.assert_equal_none(line, line, "f"))), result)
-            len(list(checks.assert_true_instance("self.assertTrue()", "f"))))
-
-    def test_assert_equal_type(self):
-        self.assertEqual(len(list(checks.assert_equal_type(
-            "self.assertEqual(type(als['QuicAssist']), list)", "f"))), 1)
-
-        self.assertEqual(
-            len(list(checks.assert_equal_type("self.assertTrue()", "f"))), 0)
-
-    def test_assert_equal_none(self):
-        self.assertEqual(len(list(checks.assert_equal_none(
-            "self.assertEqual(A, None)", "f"))), 1)
-
-        self.assertEqual(len(list(checks.assert_equal_none(
-            "self.assertEqual(None, A)", "f"))), 1)
-
-        self.assertEqual(
-            len(list(checks.assert_equal_none("self.assertIsNone()", "f"))), 0)
 
     def test_assert_true_or_false_with_in_or_not_in(self):
         good_lines = [
@@ -308,15 +329,13 @@ class HackingTestCase(test.TestCase):
                 [],
                 list(checks.check_dict_formatting_in_string(sample, tokens)))
 
-    def test_check_using_unicode(self):
+    @ddt.data(
+        "text = unicode('sometext')",
+        "text = process(unicode('sometext'))"
+    )
+    def test_check_using_unicode(self, line):
 
-        checkres = checks.check_using_unicode("text = unicode('sometext')",
+        checkres = checks.check_using_unicode(line, line, "fakefile")
-                                              "fakefile")
-        self.assertIsNotNone(next(checkres))
-        self.assertEqual([], list(checkres))
-
-        checkres = checks.check_using_unicode(
-            "text = process(unicode('sometext'))", "fakefile")
         self.assertIsNotNone(next(checkres))
         self.assertEqual([], list(checkres))
 
@@ -330,21 +349,21 @@ class HackingTestCase(test.TestCase):
         self.assertIsNone(checkres)
 
     def test_check_db_imports_of_cli(self):
+        line = "from rally.common import db"
+
         next(checks.check_db_imports_in_cli(
-            "from rally.common import db",
+            line, line, "./rally/cli/filename"))
-            "./rally/cli/filename"))
 
         checkres = checks.check_db_imports_in_cli(
-            "from rally.common import db",
+            line, line, "./filename")
-            "./filename")
         self.assertRaises(StopIteration, next, checkres)
 
     def test_check_objects_imports_of_cli(self):
+        line = "from rally.common import objects"
+
         next(checks.check_objects_imports_in_cli(
-            "from rally.common import objects",
+            line, line, "./rally/cli/filename"))
-            "./rally/cli/filename"))
 
         checkres = checks.check_objects_imports_in_cli(
-            "from rally.common import objects",
+            line, line, "./filename")
-            "./filename")
         self.assertRaises(StopIteration, next, checkres)