From 2de00e070b91a27cb64c5657c21fae3db595abeb Mon Sep 17 00:00:00 2001 From: Andrey Kurilin Date: Tue, 16 Sep 2014 23:46:32 +0300 Subject: [PATCH] Add hacking checks Checking specific mistakes can be automated via hacking rules. This patch: * adds HACKING.rst - list of rules; * adds rally.hacking - module for implementation of rules; * edits tox.ini, so hacking rules will be checked during pep8 test; * adds 3 rules related to mistakes in `assert_*` methods from `mock` library. Also tests, which are failed due to these rules, are fixed. Closes-Bug: #1305991 Change-Id: I605dd5cfba4eb83d0735e4f9f3ab4e44e149d041 --- HACKING.rst | 13 +++ rally/hacking/__init__.py | 0 rally/hacking/checks.py | 83 +++++++++++++++++++ tests/benchmark/context/test_users.py | 2 +- tests/benchmark/runners/test_rps.py | 2 +- .../scenarios/designate/test_basic.py | 2 - .../scenarios/neutron/test_network.py | 29 ++++--- tests/benchmark/test_utils.py | 4 +- tests/cmd/commands/test_verify.py | 7 +- tests/cmd/test_cliutils.py | 2 +- tests/test_hacking.py | 62 ++++++++++++++ tox.ini | 1 + 12 files changed, 186 insertions(+), 21 deletions(-) create mode 100644 HACKING.rst create mode 100644 rally/hacking/__init__.py create mode 100644 rally/hacking/checks.py create mode 100644 tests/test_hacking.py diff --git a/HACKING.rst b/HACKING.rst new file mode 100644 index 0000000000..3ebf989e17 --- /dev/null +++ b/HACKING.rst @@ -0,0 +1,13 @@ +Nova Style Commandments +======================= + +- Step 1: Read the OpenStack Style Commandments + http://docs.openstack.org/developer/hacking/ +- Step 2: Read on + +Rally Specific Commandments +--------------------------- + +- [N301] Ensure that ``assert_*`` methods from ``mock`` library is used correctly +- [N302] Sub-error of N301, related to nonexistent "assert_called" +- [N303] Sub-error of N301, related to nonexistent "assert_called_once" diff --git a/rally/hacking/__init__.py b/rally/hacking/__init__.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/rally/hacking/checks.py b/rally/hacking/checks.py new file mode 100644 index 0000000000..3ed1611725 --- /dev/null +++ b/rally/hacking/checks.py @@ -0,0 +1,83 @@ +# 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. + +""" +Guidelines for writing new hacking checks + + - Use only for Rally specific tests. OpenStack general tests + should be submitted to the common 'hacking' module. + - Pick numbers in the range N3xx. Find the current test with + the highest allocated number and then pick the next value. + - Keep the test method code in the source file ordered based + on the N3xx value. + - List the new rule in the top level HACKING.rst file + - Add test cases for each new rule to tests/test_hacking.py + +""" + + +def _parse_assert_mock_str(line): + point = line.find('.assert_') + + if point != -1: + end_pos = line[point:].find('(') + point + return point, line[point + 1: end_pos], line[: point] + else: + return None, None, None + + +def check_assert_methods_from_mock(logical_line, filename): + """Ensure that ``assert_*`` methods from ``mock`` library is used correctly + + N301 - base error number + N302 - related to nonexistent "assert_called" + N303 - related to nonexistent "assert_called_once" + """ + + correct_names = ["assert_any_call", "assert_called_once_with", + "assert_called_with", "assert_has_calls"] + + if 'tests/' in filename: + pos, method_name, obj_name = _parse_assert_mock_str(logical_line) + + if pos: + if method_name not in correct_names: + error_number = "N301" + msg = ("%(error_number)s:'%(method)s' is not present in `mock`" + " library. %(custom_msg)s For more details, visit " + "http://www.voidspace.org.uk/python/mock/ .") + + if method_name == "assert_called": + error_number = "N302" + custom_msg = ("Maybe, you should try to use " + "'assertTrue(%s.called)' instead." % + obj_name) + elif method_name == "assert_called_once": + # For more details, see a bug in Rally: + # https://bugs.launchpad.net/rally/+bug/1305991 + error_number = "N303" + custom_msg = ("Maybe, you should try to use " + "'assertEqual(1, %(obj_name)s.call_count)' " + "or '%(obj_name)s.assert_called_once_with()'" + " instead." % {"obj_name": obj_name}) + else: + custom_msg = ("Correct 'assert_*' methods: '%s'." + % "', '".join(correct_names)) + + yield (pos, msg % { + "error_number": error_number, + "method": method_name, + "custom_msg": custom_msg}) + + +def factory(register): + register(check_assert_methods_from_mock) diff --git a/tests/benchmark/context/test_users.py b/tests/benchmark/context/test_users.py index 5bb25c1cc9..42487ebf7c 100644 --- a/tests/benchmark/context/test_users.py +++ b/tests/benchmark/context/test_users.py @@ -137,7 +137,7 @@ class UserGeneratorTestCase(test.TestCase): tenant2 = mock.MagicMock() args = (mock.MagicMock(), [tenant1, tenant2]) users.UserGenerator._delete_tenants(args) - self.keystone_wrapper.wrap.assert_called_once() + self.assertEqual(1, self.keystone_wrapper.wrap.call_count) self.wrapped_keystone.delete_project.assert_has_calls([ mock.call(tenant1["id"]), mock.call(tenant2["id"])]) diff --git a/tests/benchmark/runners/test_rps.py b/tests/benchmark/runners/test_rps.py index 5a63f13b20..c6b288bcfd 100644 --- a/tests/benchmark/runners/test_rps.py +++ b/tests/benchmark/runners/test_rps.py @@ -104,7 +104,7 @@ class RPSScenarioRunnerTestCase(test.TestCase): rps._worker_thread(mock_queue, args) - mock_queue.put.assert_called_once() + self.assertEqual(1, mock_queue.put.call_count) expected_calls = [mock.call(("some_args",))] self.assertEqual(expected_calls, diff --git a/tests/benchmark/scenarios/designate/test_basic.py b/tests/benchmark/scenarios/designate/test_basic.py index 5c9544f33a..a556766cdf 100644 --- a/tests/benchmark/scenarios/designate/test_basic.py +++ b/tests/benchmark/scenarios/designate/test_basic.py @@ -104,8 +104,6 @@ class DesignateBasicTestCase(test.TestCase): [mock.call(domain['id'], "321", atomic_action=False)] * records_per_domain) - mock_delete.assert_called_once() - @mock.patch(DESIGNATE_BASIC + "._list_records") def test_list_records(self, mock_list): scenario = basic.DesignateBasic() diff --git a/tests/benchmark/scenarios/neutron/test_network.py b/tests/benchmark/scenarios/neutron/test_network.py index b4f579607b..7dad693301 100644 --- a/tests/benchmark/scenarios/neutron/test_network.py +++ b/tests/benchmark/scenarios/neutron/test_network.py @@ -54,7 +54,7 @@ class NeutronNetworksTestCase(test.TestCase): network_create_args = {} neutron_scenario.create_and_delete_networks() mock_create.assert_called_once_with(network_create_args) - mock_delete.assert_called_once() + self.assertEqual(1, mock_delete.call_count) mock_create.reset_mock() mock_delete.reset_mock() @@ -64,7 +64,7 @@ class NeutronNetworksTestCase(test.TestCase): neutron_scenario.create_and_delete_networks( network_create_args=network_create_args) mock_create.assert_called_once_with(network_create_args) - mock_delete.assert_called_once() + self.assertEqual(1, mock_delete.call_count) @mock.patch(NEUTRON_NETWORKS + "._list_subnets") @mock.patch(NEUTRON_NETWORKS + "._create_subnet") @@ -143,7 +143,9 @@ class NeutronNetworksTestCase(test.TestCase): [mock.call({"network": {"id": "fake-id"}}, subnets_per_network, {})] * subnets_per_network) - mock_delete.assert_called_once() + self.assertEqual( + mock_delete.mock_calls, + [mock.call(mock_create_subnet())] * subnets_per_network) self.assertEqual(scenario.SUBNET_CIDR_START, "default_cidr") mock_create_network.reset_mock() @@ -158,11 +160,14 @@ class NeutronNetworksTestCase(test.TestCase): self.assertEqual(scenario.SUBNET_CIDR_START, "custom_cidr") mock_create_network.assert_called_once_with({}) self.assertEqual( - mock_create_subnet.mock_calls, - [mock.call({"network": {"id": "fake-id"}}, - subnets_per_network, - {"allocation_pools": []})] * subnets_per_network) - mock_delete.assert_called_once() + mock_create_subnet.mock_calls, + [mock.call({"network": {"id": "fake-id"}}, + subnets_per_network, + {"allocation_pools": []})] * subnets_per_network) + + self.assertEqual( + mock_delete.mock_calls, + [mock.call(mock_create_subnet())] * subnets_per_network) @mock.patch(NEUTRON_NETWORKS + "._list_routers") @mock.patch(NEUTRON_NETWORKS + "._create_router") @@ -303,11 +308,12 @@ class NeutronNetworksTestCase(test.TestCase): mock_create_network.assert_called_once_with({}) self.assertEqual(mock_create_port.mock_calls, [mock.call(net, {})] * ports_per_network) - mock_delete.assert_called_once() + self.assertEqual(mock_delete.mock_calls, + [mock.call(mock_create_port())] * ports_per_network) mock_create_network.reset_mock() mock_create_port.reset_mock() - mock_delete.reset() + mock_delete.reset_mock() # Custom options scenario.create_and_delete_ports( @@ -318,4 +324,5 @@ class NeutronNetworksTestCase(test.TestCase): self.assertEqual( mock_create_port.mock_calls, [mock.call(net, {"allocation_pools": []})] * ports_per_network) - mock_delete.assert_called_once() + self.assertEqual(mock_delete.mock_calls, + [mock.call(mock_create_port())] * ports_per_network) diff --git a/tests/benchmark/test_utils.py b/tests/benchmark/test_utils.py index 28c6cb31a6..003960c4fb 100644 --- a/tests/benchmark/test_utils.py +++ b/tests/benchmark/test_utils.py @@ -137,7 +137,7 @@ class BenchmarkUtilsTestCase(test.TestCase): service('glance-api')] ret = utils.check_service_status(client, 'nova-network') self.assertTrue(ret) - client.services.list.assert_called() + self.assertTrue(client.services.list.called) def test_check_service_status_fail(self): class service(): @@ -155,7 +155,7 @@ class BenchmarkUtilsTestCase(test.TestCase): service('glance-api')] ret = utils.check_service_status(client, 'nova-network') self.assertFalse(ret) - client.services.list.assert_called() + self.assertTrue(client.services.list.called) class WaitForTestCase(test.TestCase): diff --git a/tests/cmd/commands/test_verify.py b/tests/cmd/commands/test_verify.py index 01c89a74e7..bb5d1b0b29 100644 --- a/tests/cmd/commands/test_verify.py +++ b/tests/cmd/commands/test_verify.py @@ -103,7 +103,7 @@ class VerifyCommandsTestCase(test.TestCase): verifications = {'dummy': []} mock_db_verification_list.return_value = verifications self.verify.list() - mock_db_verification_list.assert_called_once() + mock_db_verification_list.assert_called_once_with() mock_print_list.assert_called_once_with(verifications, fields, sortby_index=fields.index( 'Created at')) @@ -199,13 +199,14 @@ class VerifyCommandsTestCase(test.TestCase): mock_open): mock_open.return_value = mock.MagicMock() verification_uuid = '7140dd59-3a7b-41fd-a3ef-5e3e615d7dfa' - results = {'data': {}} + fake_data = {} + results = {'data': fake_data} mock_db_result_get.return_value = results self.verify.results(verification_uuid, output_html=True, output_file='results') mock_db_result_get.assert_called_once_with(verification_uuid) - mock_json2html_main.assert_called_once() + mock_json2html_main.assert_called_once_with(fake_data) mock_open.assert_called_once_with('results', 'wb') fake_file = mock_open.return_value.__enter__.return_value fake_file.write.assert_called_once_with('') diff --git a/tests/cmd/test_cliutils.py b/tests/cmd/test_cliutils.py index 0a378b67a7..cde66a3e5c 100644 --- a/tests/cmd/test_cliutils.py +++ b/tests/cmd/test_cliutils.py @@ -172,6 +172,6 @@ class CliUtilsTestCase(test.TestCase): 'use': use.UseCommands, 'verify': verify.VerifyCommands} ret = cliutils.run(['rally', 'show', 'keypairs'], categories) - mock_validate_args.assert_called() + self.assertTrue(mock_validate_args.called) self._unregister_opts() self.assertEqual(ret, 1) diff --git a/tests/test_hacking.py b/tests/test_hacking.py new file mode 100644 index 0000000000..43759a81c3 --- /dev/null +++ b/tests/test_hacking.py @@ -0,0 +1,62 @@ +# 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. + +from rally.hacking import checks +from tests import test + + +class HackingTestCase(test.TestCase): + + def test__parse_assert_mock_str(self): + pos, method, obj = checks._parse_assert_mock_str( + "mock_clients.fake().quotas.delete.assert_called_once()") + self.assertEqual("assert_called_once", method) + self.assertEqual("mock_clients.fake().quotas.delete", obj) + + def test__parse_assert_mock_str_no_assert(self): + pos, method, obj = checks._parse_assert_mock_str( + "mock_clients.fake().quotas.delete.") + self.assertIsNone(pos) + self.assertIsNone(method) + self.assertIsNone(obj) + + 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: + self.assertEqual(0, len( + list(checks.check_assert_methods_from_mock( + 'some_mock.%s(asd)' % name, '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')) + self.assertEqual(4, actual_number) + self.assertTrue(actual_msg.startswith('N301')) + + def test_wrong_usage_of_assert_called_from_mock(self): + fake_method = 'rtfm.assert_called()' + + actual_number, actual_msg = next(checks.check_assert_methods_from_mock( + fake_method, 'tests/fake/test')) + self.assertEqual(4, actual_number) + self.assertTrue(actual_msg.startswith('N302')) + + def test_wrong_usage_of_assert_called_once_from_mock(self): + fake_method = 'rtfm.assert_called_once()' + + actual_number, actual_msg = next(checks.check_assert_methods_from_mock( + fake_method, 'tests/fake/test')) + self.assertEqual(4, actual_number) + self.assertTrue(actual_msg.startswith('N303')) diff --git a/tox.ini b/tox.ini index 1d2939e1ed..60a3992168 100644 --- a/tox.ini +++ b/tox.ini @@ -45,3 +45,4 @@ exclude=.venv,.git,.tox,dist,doc,*lib/python*,*egg,tools,*rally/verification/ver [hacking] import_exceptions = rally.openstack.common.gettextutils._ +local-check-factory = rally.hacking.checks.factory