openstack
/
hacking
Code Issues Proposed changes

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
This commit is contained in:
Paras Babbar 2020-05-10 17:22:48 -04:00 committed by Paras Babbar
parent 93e6cd2904
commit bfc1a64ac6
7 changed files with 393 additions and 0 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

15
HACKING.rst
View File

@ -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
-------------------

108
hacking/checks/except_checks.py
View File

@ -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.")

244
hacking/tests/checks/test_except_checks.py Normal file
View File

@ -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)

1
lower-constraints.txt
View File

@ -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

19
releasenotes/notes/migrating-most-common-hacking-checks.yaml Normal file
View File

@ -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)

5
setup.cfg
View File

@ -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

1
test-requirements.txt
View File

@ -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