From bfc1a64ac627f8b507d937bfb1059fa6c2a3a4dd Mon Sep 17 00:00:00 2001 From: Paras Babbar Date: Sun, 10 May 2020 17:22:48 -0400 Subject: [PATCH] Move the most common hacking rules from other projects This patch will copy the most common hacking rules used in diff. projects and add them to hacking itself. Currently, added assert_true_instance, assert_equal_type assert_raises_regexp, assert_true_or_false_with_in assert_equal_in Change-Id: I122d250cab90964c346e9d53046a97c25054bc00 --- HACKING.rst | 15 ++ hacking/checks/except_checks.py | 108 ++++++++ hacking/tests/checks/test_except_checks.py | 244 ++++++++++++++++++ lower-constraints.txt | 1 + .../migrating-most-common-hacking-checks.yaml | 19 ++ setup.cfg | 5 + test-requirements.txt | 1 + 7 files changed, 393 insertions(+) create mode 100644 hacking/tests/checks/test_except_checks.py create mode 100644 releasenotes/notes/migrating-most-common-hacking-checks.yaml diff --git a/HACKING.rst b/HACKING.rst index fa0cc018..ece20f3c 100644 --- a/HACKING.rst +++ b/HACKING.rst @@ -346,6 +346,21 @@ exception possible should be used. will not be required: ``new``, ``new_callable``, ``spec``, ``spec_set``, ``wraps`` +- [H211] Change assertTrue(isinstance(A, B)) by optimal assert like + assertIsInstance(A, B). + +- [H212] Change assertEqual(type(A), B) by optimal assert like + assertIsInstance(A, B) + +- [H213] Check for usage of deprecated assertRaisesRegexp + +- [H214] Change assertTrue/False(A in/not in B, message) to the more + specific assertIn/NotIn(A, B, message) + +- [H215] Change assertEqual(A in B, True), assertEqual(True, A in B), + assertEqual(A in B, False) or assertEqual(False, A in B) to the more + specific assertIn/NotIn(A, B) + OpenStack Trademark ------------------- diff --git a/hacking/checks/except_checks.py b/hacking/checks/except_checks.py index 029ae55d..a646de28 100644 --- a/hacking/checks/except_checks.py +++ b/hacking/checks/except_checks.py @@ -18,6 +18,37 @@ import re from hacking import core RE_ASSERT_RAISES_EXCEPTION = re.compile(r"self\.assertRaises\(Exception[,\)]") +RE_ASSERT_TRUE_INST = re.compile( + r"(.)*assertTrue\(isinstance\((\w|\.|\'|\"|\[|\])+, " + r"(\w|\.|\'|\"|\[|\])+\)\)") +RE_ASSERT_EQUAL_TYPE = re.compile( + r"(.)*assertEqual\(type\((\w|\.|\'|\"|\[|\])+\), " + r"(\w|\.|\'|\"|\[|\])+\)") +RE_ASSERT_EQUAL_IN_START_WITH_TRUE_OR_FALSE = re.compile( + r"assertEqual\(" + r"(True|False), (\w|[][.'\"])+ in (\w|[][.'\", ])+\)") +RE_ASSERT_RAISES_REGEXP = re.compile(r"assertRaisesRegexp\(") +# 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". +RE_ASSERT_TRUE_FALSE_WITH_IN_OR_NOT_IN = re.compile( + r"assert(True|False)\(" + r"(\w|[][.'\"])+( not)? in (\w|[][.'\",])+(, .*)?\)") +RE_ASSERT_TRUE_FALSE_WITH_IN_OR_NOT_IN_SPACES = re.compile( + r"assert(True|False)" + r"\((\w|[][.'\"])+( not)? in [\[|'|\"](\w|[][.'\", ])+" + r"[\[|'|\"](, .*)?\)") +RE_ASSERT_EQUAL_IN_END_WITH_TRUE_OR_FALSE = re.compile( + r"assertEqual\(" + r"(\w|[][.'\"])+ in (\w|[][.'\", ])+, (True|False)\)") @core.flake8ext @@ -80,6 +111,7 @@ class NoneArgChecker(ast.NodeVisitor): self.none_found will be True if any None arguments were found. ''' + def __init__(self, func_name, num_args=2): self.func_name = func_name self.num_args = num_args @@ -134,6 +166,7 @@ class AssertTrueFalseChecker(ast.NodeVisitor): :param ops: list of comparisons we want to look for (objects from the ast module) ''' + def __init__(self, method_names, ops): self.method_names = method_names self.ops = tuple(ops) @@ -214,3 +247,78 @@ def hacking_assert_greater_less(logical_line, noqa): checker.visit(ast.parse(logical_line)) if checker.error: yield start, 'H205: Use assert{Greater,Less}[Equal]' + + +@core.flake8ext +def hacking_assert_true_instance(logical_line): + """Check for assertTrue(isinstance(a, b)) sentences + + H211 + """ + if RE_ASSERT_TRUE_INST.match(logical_line): + yield ( + 0, + "H211: Use assert{Is,IsNot}instance") + + +@core.flake8ext +def hacking_assert_equal_type(logical_line): + """Check for assertEqual(type(A), B) sentences + + H212 + """ + if RE_ASSERT_EQUAL_TYPE.match(logical_line): + yield ( + 0, + "H212: Use assert{type(A),B} instance") + + +@core.flake8ext +def hacking_assert_raises_regexp(logical_line): + """Check for usage of deprecated assertRaisesRegexp + + H213 + """ + res = RE_ASSERT_RAISES_REGEXP.search(logical_line) + if res: + yield ( + 0, + "H213: assertRaisesRegex must be used instead " + "of assertRaisesRegexp") + + +@core.flake8ext +def hacking_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. + H214 + """ + res = (RE_ASSERT_TRUE_FALSE_WITH_IN_OR_NOT_IN.search(logical_line) + or RE_ASSERT_TRUE_FALSE_WITH_IN_OR_NOT_IN_SPACES.search + (logical_line)) + if res: + yield ( + 0, + "H214: Use assertIn/NotIn(A, B) rather than " + "assertTrue/False(A in/not in B) when checking collection " + "contents.") + + +@core.flake8ext +def hacking_assert_equal_in(logical_line): + """Check for assertEqual(A in B, True), assertEqual(True, A in B), + + assertEqual(A in B, False) or assertEqual(False, A in B) sentences + + H215 + """ + res = (RE_ASSERT_EQUAL_IN_START_WITH_TRUE_OR_FALSE.search(logical_line) or + RE_ASSERT_EQUAL_IN_END_WITH_TRUE_OR_FALSE.search(logical_line)) + if res: + yield ( + 0, + "H215: Use assertIn/NotIn(A, B) rather than " + "assertEqual(A in B, True/False) when checking collection " + "contents.") diff --git a/hacking/tests/checks/test_except_checks.py b/hacking/tests/checks/test_except_checks.py new file mode 100644 index 00000000..49a154ba --- /dev/null +++ b/hacking/tests/checks/test_except_checks.py @@ -0,0 +1,244 @@ +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or +# implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +import textwrap + +import mock +import pycodestyle + +from hacking.checks import except_checks +from hacking import tests + + +class ExceptChecksTest(tests.TestCase): + + def _assert_has_errors(self, code, checker, expected_errors=None, + filename=None): + actual_errors = [e[:3] for e in + self._run_check(code, checker, filename)] + self.assertEqual(expected_errors or [], actual_errors) + + # We are patching pycodestyle so that only the check under test is actually + # installed. + @mock.patch('pycodestyle._checks', + {'physical_line': {}, 'logical_line': {}, 'tree': {}}) + def _run_check(self, code, checker, filename=None): + pycodestyle.register_check(checker) + + lines = textwrap.dedent(code).lstrip().splitlines(True) + + checker = pycodestyle.Checker(filename=filename, lines=lines) + # NOTE(sdague): the standard reporter has printing to stdout + # as a normal part of check_all, which bleeds through to the + # test output stream in an unhelpful way. This blocks that printing. + with mock.patch('pycodestyle.StandardReport.get_file_results'): + checker.check_all() + checker.report._deferred_print.sort() + return checker.report._deferred_print + + def test_hacking_assert_true_instance(self): + self.assertEqual( + len(list(except_checks.hacking_assert_true_instance( + "self.assertTrue(isinstance(e, " + "exception.BuildAbortException))" + ))), + 1) + + self.assertEqual( + len(list(except_checks.hacking_assert_true_instance( + "self.assertTrue()" + ))), + 0) + + def test_hacking_assert_equal_in(self): + self.assertEqual( + len(list(except_checks.hacking_assert_equal_in( + "self.assertEqual(a in b, True)" + ))), + 1) + + self.assertEqual( + len(list(except_checks.hacking_assert_equal_in( + "self.assertEqual('str' in 'string', True)" + ))), + 1) + + self.assertEqual( + len(list(except_checks.hacking_assert_equal_in( + "self.assertEqual(any(a==1 for a in b), True)" + ))), + 0) + + self.assertEqual( + len(list(except_checks.hacking_assert_equal_in( + "self.assertEqual(a in b, True)" + ))), + 1) + + self.assertEqual( + len(list(except_checks.hacking_assert_equal_in( + "self.assertEqual('str' in 'string', True)" + ))), + 1) + + self.assertEqual(len(list(except_checks.hacking_assert_equal_in( + "self.assertEqual(any(a==1 for a in b, True))" + ))), + 0) + + self.assertEqual( + len(list(except_checks.hacking_assert_equal_in( + "self.assertEqual(a in b, False)" + ))), + 1) + + self.assertEqual( + len(list(except_checks.hacking_assert_equal_in( + "self.assertEqual('str' in 'string', False)" + ))), + 1) + + self.assertEqual( + len(list(except_checks.hacking_assert_equal_in( + "self.assertEqual(any(a==1 for a in b), False)" + ))), + 0) + + self.assertEqual( + len(list(except_checks.hacking_assert_equal_in( + "self.assertEqual(a in b, False)" + ))), + 1) + + self.assertEqual( + len(list(except_checks.hacking_assert_equal_in( + "self.assertEqual('str' in 'string', False)" + ))), + 1) + + self.assertEqual( + len(list(except_checks.hacking_assert_equal_in( + "self.assertEqual(any(a==1 for a in b, False))" + ))), + 0) + + def test_hacking_assert_true_or_false_with_in_or_not_in(self): + self.assertEqual( + len(list(except_checks.hacking_assert_true_or_false_with_in( + "self.assertTrue(A in B)" + ))), + 1) + + self.assertEqual( + len(list(except_checks.hacking_assert_true_or_false_with_in( + "self.assertFalse(A in B)" + ))), + 1) + + self.assertEqual( + len(list(except_checks.hacking_assert_true_or_false_with_in( + "self.assertTrue(A not in B)" + ))), + 1) + + self.assertEqual( + len(list(except_checks.hacking_assert_true_or_false_with_in( + "self.assertFalse(A not in B)" + ))), + 1) + + self.assertEqual( + len(list(except_checks.hacking_assert_true_or_false_with_in( + "self.assertTrue(A in B, 'some message')" + ))), + 1) + + self.assertEqual( + len(list(except_checks.hacking_assert_true_or_false_with_in( + "self.assertFalse(A in B, 'some message')" + ))), + 1) + + self.assertEqual( + len(list(except_checks.hacking_assert_true_or_false_with_in( + "self.assertTrue(A not in B, 'some message')" + ))), + 1) + + self.assertEqual( + len(list(except_checks.hacking_assert_true_or_false_with_in( + "self.assertFalse(A not in B, 'some message')" + ))), + 1) + + self.assertEqual( + len(list(except_checks.hacking_assert_true_or_false_with_in( + "self.assertTrue(A in 'some string with spaces')" + ))), + 1) + + self.assertEqual( + len(list(except_checks.hacking_assert_true_or_false_with_in( + "self.assertTrue(A in ['1', '2', '3'])" + ))), + 1) + + self.assertEqual(len( + list(except_checks.hacking_assert_true_or_false_with_in( + "self.assertTrue(A in [1, 2, 3])" + ))), + 1) + + self.assertEqual( + len(list(except_checks.hacking_assert_true_or_false_with_in( + "self.assertTrue(any(A > 5 for A in B))" + ))), + 0) + + self.assertEqual( + len(list(except_checks.hacking_assert_true_or_false_with_in( + "self.assertTrue(any(A > 5 for A in B), 'some message')" + ))), + 0) + + self.assertEqual( + len(list(except_checks.hacking_assert_true_or_false_with_in( + "self.assertFalse(some in list1 and some2 in list2)" + ))), + 0) + + def test_hacking_oslo_assert_raises_regexp(self): + code = """ + self.assertRaisesRegexp(ValueError, + "invalid literal for.*XYZ'$", + int, + 'XYZ') + """ + self._assert_has_errors( + code, + except_checks.hacking_assert_raises_regexp, expected_errors=[ + (1, 0, "H213") + ]) + + def test_hacking_assert_equal_type(self): + self.assertEqual( + len(list(except_checks.hacking_assert_equal_type( + "self.assertEqual(type(als['QuicAssist']), list)" + ))), + 1) + + self.assertEqual( + len(list(except_checks.hacking_assert_equal_type( + "self.assertTrue()" + ))), + 0) diff --git a/lower-constraints.txt b/lower-constraints.txt index 8f70e1a1..28cf9f3c 100644 --- a/lower-constraints.txt +++ b/lower-constraints.txt @@ -12,6 +12,7 @@ Jinja2==2.10 linecache2==1.0.0 MarkupSafe==1.0 mccabe==0.6.0 +mock==3.0.0 pycodestyle==2.4.0 pyflakes==2.1.1 Pygments==2.2.0 diff --git a/releasenotes/notes/migrating-most-common-hacking-checks.yaml b/releasenotes/notes/migrating-most-common-hacking-checks.yaml new file mode 100644 index 00000000..92539e20 --- /dev/null +++ b/releasenotes/notes/migrating-most-common-hacking-checks.yaml @@ -0,0 +1,19 @@ +--- +features: + - | + This release added new checks related to unittest module: + + * [H211] Change assertTrue(isinstance(A, B)) by optimal assert like + assertIsInstance(A, B). + + * [H212] Change assertEqual(type(A), B) by optimal assert like + assertIsInstance(A, B) + + * [H213] Check for usage of deprecated assertRaisesRegexp + + * [H214] Change assertTrue/False(A in/not in B, message) to the more + specific assertIn/NotIn(A, B, message) + + * [H215] Change assertEqual(A in B, True), assertEqual(True, A in B), + assertEqual(A in B, False) or assertEqual(False, A in B) to the more + specific assertIn/NotIn(A, B) \ No newline at end of file diff --git a/setup.cfg b/setup.cfg index e235ed2f..1bddf3eb 100644 --- a/setup.cfg +++ b/setup.cfg @@ -40,6 +40,11 @@ flake8.extension = H204 = hacking.checks.except_checks:hacking_assert_equal H205 = hacking.checks.except_checks:hacking_assert_greater_less H210 = hacking.checks.mock_checks:MockAutospecCheck + H211 = hacking.checks.except_checks:hacking_assert_true_instance + H212 = hacking.checks.except_checks:hacking_assert_equal_type + H213 = hacking.checks.except_checks:hacking_assert_raises_regexp + H214 = hacking.checks.except_checks:hacking_assert_true_or_false_with_in + H215 = hacking.checks.except_checks:hacking_assert_equal_in H231 = hacking.checks.python23:hacking_python3x_except_compatible H232 = hacking.checks.python23:hacking_python3x_octal_literals H233 = hacking.checks.python23:hacking_python3x_print_function diff --git a/test-requirements.txt b/test-requirements.txt index 203cea22..f5b78aa1 100644 --- a/test-requirements.txt +++ b/test-requirements.txt @@ -3,6 +3,7 @@ # process, which may cause wedges in the gate later. coverage!=4.4,>=4.0 # Apache-2.0 fixtures>=3.0.0 # Apache-2.0/BSD +mock>=3.0.0 # BSD python-subunit>=1.0.0 # Apache-2.0/BSD stestr>=2.0.0 # Apache-2.0 testscenarios>=0.4 # Apache-2.0/BSD