From e97f70788c951b405dd6b24b2dcac769f868a886 Mon Sep 17 00:00:00 2001 From: Bertrand Lallau Date: Mon, 26 Oct 2015 08:05:14 +0100 Subject: [PATCH] Use assertIsNone instead of assertEqual(None, ***) Instead of using assertEqual(None, ***), developers should use assertIsNone(***) to have more clear messages in case of failure. Closes-Bug: #1510006 Change-Id: Ib3d09ed651877569a9b940da97662489885f18e9 --- HACKING.rst | 2 ++ magnum/hacking/checks.py | 18 ++++++++++++++++++ magnum/tests/unit/common/test_utils.py | 4 ++-- magnum/tests/unit/test_hacking.py | 10 ++++++++++ 4 files changed, 32 insertions(+), 2 deletions(-) diff --git a/HACKING.rst b/HACKING.rst index 4cd2ac5886..5c08ece7f1 100644 --- a/HACKING.rst +++ b/HACKING.rst @@ -9,4 +9,6 @@ Magnum Specific Commandments ---------------------------- - [M301] policy.enforce_wsgi decorator must be the first decorator on a method. +- [M318] Change assertEqual(A, None) or assertEqual(None, A) by optimal assert + like assertIsNone(A) - [M322] Method's default argument shouldn't be mutable. diff --git a/magnum/hacking/checks.py b/magnum/hacking/checks.py index 8b12d2b1ab..4443c28a18 100644 --- a/magnum/hacking/checks.py +++ b/magnum/hacking/checks.py @@ -33,6 +33,10 @@ Guidelines for writing new hacking checks enforce_re = re.compile(r"@policy.enforce_wsgi*") decorator_re = re.compile(r"@.*") mutable_default_args = re.compile(r"^\s*def .+\((.+=\{\}|.+=\[\])") +asse_equal_end_with_none_re = re.compile( + r"(.)*assertEqual\((\w|\.|\'|\"|\[|\])+, None\)") +asse_equal_start_with_none_re = re.compile( + r"(.)*assertEqual\(None, (\w|\.|\'|\"|\[|\])+\)") def check_policy_enforce_decorator(logical_line, @@ -45,6 +49,19 @@ def check_policy_enforce_decorator(logical_line, yield(0, msg) +def assert_equal_none(logical_line): + """Check for assertEqual(A, None) or assertEqual(None, A) sentences + + M318 + """ + msg = ("M318: assertEqual(A, None) or assertEqual(None, A) " + "sentences not allowed") + res = (asse_equal_start_with_none_re.match(logical_line) or + asse_equal_end_with_none_re.match(logical_line)) + if res: + yield (0, msg) + + def no_mutable_default_args(logical_line): msg = "M322: Method's default argument shouldn't be mutable!" if mutable_default_args.match(logical_line): @@ -54,3 +71,4 @@ def no_mutable_default_args(logical_line): def factory(register): register(check_policy_enforce_decorator) register(no_mutable_default_args) + register(assert_equal_none) diff --git a/magnum/tests/unit/common/test_utils.py b/magnum/tests/unit/common/test_utils.py index c29538e4f6..f0a48ccbb5 100644 --- a/magnum/tests/unit/common/test_utils.py +++ b/magnum/tests/unit/common/test_utils.py @@ -118,8 +118,8 @@ class UtilsTestCase(base.TestCase): utils.get_ip_version, 'x.x.x.x') def test_convert_to_list_dict(self): - self.assertEqual(None, utils.convert_to_list_dict(None, 'fred')) - self.assertEqual(None, utils.convert_to_list_dict('', 'fred')) + self.assertIsNone(utils.convert_to_list_dict(None, 'fred')) + self.assertIsNone(utils.convert_to_list_dict('', 'fred')) self.assertEqual([{'fred': 'list'}], utils.convert_to_list_dict('list', 'fred')) self.assertEqual([{'fred': 'first'}, {'fred': 'second'}], diff --git a/magnum/tests/unit/test_hacking.py b/magnum/tests/unit/test_hacking.py index f68812bb74..ef036ae842 100644 --- a/magnum/tests/unit/test_hacking.py +++ b/magnum/tests/unit/test_hacking.py @@ -86,6 +86,16 @@ class HackingTestCase(base.TestCase): self._assert_has_errors(code, checks.check_policy_enforce_decorator, expected_errors=[(2, 0, "M301")]) + def test_assert_equal_none(self): + self.assertEqual(len(list(checks.assert_equal_none( + "self.assertEqual(A, None)"))), 1) + + self.assertEqual(len(list(checks.assert_equal_none( + "self.assertEqual(None, A)"))), 1) + + self.assertEqual( + len(list(checks.assert_equal_none("self.assertIsNone()"))), 0) + def test_no_mutable_default_args(self): self.assertEqual(1, len(list(checks.no_mutable_default_args( "def get_info_from_bdm(virt_type, bdm, mapping=[])"))))