From 680d989e35d6df77133abada17ca06720e318d54 Mon Sep 17 00:00:00 2001 From: Boris Pavlovic Date: Wed, 6 Jan 2016 11:32:24 -0800 Subject: [PATCH] Fix hacking checks We should pass argument physical line otherwise our decorator for checking # noqa doesn't work always Change-Id: Iebe1872b5f8d037b2bef55766b93abe54e7f3b4a --- tests/hacking/checks.py | 39 +++++------ tests/unit/test_hacking.py | 135 +++++++++++++++++++++---------------- 2 files changed, 97 insertions(+), 77 deletions(-) diff --git a/tests/hacking/checks.py b/tests/hacking/checks.py index 5f1db629..56d29604 100644 --- a/tests/hacking/checks.py +++ b/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): - line = logical_line.strip() + def wrapper(logical_line, physical_line, filename): + 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 diff --git a/tests/unit/test_hacking.py b/tests/unit/test_hacking.py index 3ae338bf..c6079883 100644 --- a/tests/unit/test_hacking.py +++ b/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([], list(any_gen(" # fdafadfdas", "f"))) - self.assertEqual([], list(any_gen(" ", "f"))) - self.assertEqual(42, next(any_gen("otherstuff", "f"))) + self.assertEqual(result, list(any_gen(line, line, "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: - checkres = checks.check_import_of_logging(bad_import, "fakefile") + for bad in bad_imports: + 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: - checkres = checks.check_import_of_logging(good_import, - "fakefile") + for good in good_imports: + checkres = checks.check_import_of_logging(good, good, "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): - self.assertEqual(len(list(checks.assert_true_instance( - "self.assertTrue(isinstance(e, " - "exception.BuildAbortException))", "f"))), 1) + @ddt.data( + { + "line": "self.assertTrue(isinstance(e, exception.BuildAbortExc))", + "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_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) + len(list(checks.assert_equal_none(line, line, "f"))), result) 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')", - "fakefile") - self.assertIsNotNone(next(checkres)) - self.assertEqual([], list(checkres)) - - checkres = checks.check_using_unicode( - "text = process(unicode('sometext'))", "fakefile") + checkres = checks.check_using_unicode(line, line, "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", - "./rally/cli/filename")) + line, line, "./rally/cli/filename")) checkres = checks.check_db_imports_in_cli( - "from rally.common import db", - "./filename") + line, line, "./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", - "./rally/cli/filename")) + line, line, "./rally/cli/filename")) checkres = checks.check_objects_imports_in_cli( - "from rally.common import objects", - "./filename") + line, line, "./filename") self.assertRaises(StopIteration, next, checkres)