From 37093b2436c93d0dc3724107defc43de8856b7ac Mon Sep 17 00:00:00 2001 From: indicoliteplus Date: Wed, 14 Jun 2017 13:58:16 +0800 Subject: [PATCH] Revert "Using assertFalse(A) instead of assertEqual(False, A)" This is not correct, because assertEqual(False, A) is a stricter check than assertFalse(A). assertFalse(None) passes, but assertEqual(False, None) will fail. Therefore, this patch weakened our tests. Change-Id: I35c8978d8e189c894038b8d1e974938ffff71fcc --- HACKING.rst | 2 -- magnum/hacking/checks.py | 17 ----------------- .../swarm/test_swarm_python_client.py | 2 +- magnum/tests/unit/api/test_attr_validator.py | 2 +- magnum/tests/unit/common/test_profiler.py | 2 +- magnum/tests/unit/objects/test_objects.py | 4 +++- magnum/tests/unit/test_hacking.py | 16 ---------------- 7 files changed, 6 insertions(+), 39 deletions(-) diff --git a/HACKING.rst b/HACKING.rst index 64ab9dec88..da7989fb02 100644 --- a/HACKING.rst +++ b/HACKING.rst @@ -15,8 +15,6 @@ Magnum Specific Commandments - [M316] Change assertTrue(isinstance(A, B)) by optimal assert like assertIsInstance(A, B). - [M322] Method's default argument shouldn't be mutable. -- [M323] Change assertEqual(True, A) or assertEqual(False, A) by optimal assert - like assertTrue(A) or assertFalse(A) - [M336] Must use a dict comprehension instead of a dict constructor with a sequence of key-value pairs. - [M338] Use assertIn/NotIn(A, B) rather than assertEqual(A in B, True/False). diff --git a/magnum/hacking/checks.py b/magnum/hacking/checks.py index febc8e7334..25f15d38e3 100644 --- a/magnum/hacking/checks.py +++ b/magnum/hacking/checks.py @@ -38,10 +38,6 @@ assert_equal_in_end_with_true_or_false_re = re.compile( r"assertEqual\((\w|[][.'\"])+ in (\w|[][.'\", ])+, (True|False)\)") assert_equal_in_start_with_true_or_false_re = re.compile( r"assertEqual\((True|False), (\w|[][.'\"])+ in (\w|[][.'\", ])+\)") -assert_equal_with_true_re = re.compile( - r"assertEqual\(True,") -assert_equal_with_false_re = re.compile( - r"assertEqual\(False,") asse_equal_with_is_not_none_re = re.compile( r"assertEqual\(.*?\s+is+\s+not+\s+None\)$") assert_true_isinstance_re = re.compile( @@ -72,18 +68,6 @@ def no_mutable_default_args(logical_line): yield (0, msg) -def assert_equal_true_or_false(logical_line): - """Check for assertEqual(True, A) or assertEqual(False, A) sentences - - M323 - """ - res = (assert_equal_with_true_re.search(logical_line) or - assert_equal_with_false_re.search(logical_line)) - if res: - yield (0, "M323: assertEqual(True, A) or assertEqual(False, A) " - "sentences not allowed") - - def assert_equal_not_none(logical_line): """Check for assertEqual(A is not None) sentences M302""" msg = "M302: assertEqual(A is not None) sentences not allowed." @@ -181,7 +165,6 @@ def check_explicit_underscore_import(logical_line, filename): def factory(register): register(no_mutable_default_args) - register(assert_equal_true_or_false) register(assert_equal_not_none) register(assert_true_isinstance) register(assert_equal_in) diff --git a/magnum/tests/functional/swarm/test_swarm_python_client.py b/magnum/tests/functional/swarm/test_swarm_python_client.py index cc8448a4c5..c8b8b0ee20 100644 --- a/magnum/tests/functional/swarm/test_swarm_python_client.py +++ b/magnum/tests/functional/swarm/test_swarm_python_client.py @@ -137,7 +137,7 @@ class TestSwarmAPIs(ClusterTest): container=container_id) resp = self._container_operation(self.docker_client.inspect_container, container=container_id) - self.assertFalse(resp['State']['Running']) + self.assertEqual(False, resp['State']['Running']) self._container_operation(self.docker_client.remove_container, container=container_id) diff --git a/magnum/tests/unit/api/test_attr_validator.py b/magnum/tests/unit/api/test_attr_validator.py index 01294113ce..d6802682e0 100644 --- a/magnum/tests/unit/api/test_attr_validator.py +++ b/magnum/tests/unit/api/test_attr_validator.py @@ -46,7 +46,7 @@ class TestAttrValidator(base.BaseTestCase): mock_os_cli = mock.MagicMock() mock_os_cli.nova.return_value = mock_nova attr_validator.validate_flavor(mock_os_cli, None) - self.assertFalse(mock_nova.flavors.list.called) + self.assertEqual(False, mock_nova.flavors.list.called) def test_validate_flavor_with_invalid_flavor(self): mock_flavor = mock.MagicMock() diff --git a/magnum/tests/unit/common/test_profiler.py b/magnum/tests/unit/common/test_profiler.py index a21f9e5226..5fded8d011 100644 --- a/magnum/tests/unit/common/test_profiler.py +++ b/magnum/tests/unit/common/test_profiler.py @@ -79,4 +79,4 @@ class TestProfiler(base.TestCase): @mock.patch.object(conf, 'CONF', new=cfg.ConfigOpts()) def test_setup_profiler_without_osprofiler(self, mock_init): profiler.setup('foo', 'localhost') - self.assertFalse(mock_init.called) + self.assertEqual(False, mock_init.called) diff --git a/magnum/tests/unit/objects/test_objects.py b/magnum/tests/unit/objects/test_objects.py index 2d30c1196b..64d39219bf 100644 --- a/magnum/tests/unit/objects/test_objects.py +++ b/magnum/tests/unit/objects/test_objects.py @@ -427,7 +427,9 @@ class TestObjectSerializer(test_base.TestCase): primitive = obj.obj_to_primitive() result = ser.deserialize_entity(self.context, primitive) if backported_to is None: - self.assertFalse(mock_indirection_api.object_backport.called) + self.assertEqual( + False, + mock_indirection_api.object_backport.called) else: self.assertEqual('backported', result) mock_indirection_api.object_backport.assert_called_with( diff --git a/magnum/tests/unit/test_hacking.py b/magnum/tests/unit/test_hacking.py index 5c0ca9f027..3a877340f0 100644 --- a/magnum/tests/unit/test_hacking.py +++ b/magnum/tests/unit/test_hacking.py @@ -117,22 +117,6 @@ class HackingTestCase(base.TestCase): code = "self.assertEqual(False, any(a==1 for a in b))" self._assert_has_no_errors(code, check) - def test_assert_equal_true_or_false(self): - errors = [(1, 0, "M323")] - check = checks.assert_equal_true_or_false - - code = "self.assertEqual(True, A)" - self._assert_has_errors(code, check, errors) - - code = "self.assertEqual(False, A)" - self._assert_has_errors(code, check, errors) - - code = "self.assertTrue()" - self._assert_has_no_errors(code, check) - - code = "self.assertFalse()" - self._assert_has_no_errors(code, check) - def test_no_mutable_default_args(self): errors = [(1, 0, "M322")] check = checks.no_mutable_default_args