openstack/rally
Code Issues Proposed changes

Add hacking checks

Browse Source 
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
This commit is contained in:
Andrey Kurilin 2014-09-16 23:46:32 +03:00
parent 130b355061
commit 2de00e070b
12 changed files with 186 additions and 21 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

13
HACKING.rst Normal file
View File

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

0
rally/hacking/__init__.py Normal file
View File

83
rally/hacking/checks.py Normal file
View File

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

2
tests/benchmark/context/test_users.py
View File

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

2
tests/benchmark/runners/test_rps.py
View File

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

2
tests/benchmark/scenarios/designate/test_basic.py
View File

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

21
tests/benchmark/scenarios/neutron/test_network.py
View File

@ -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()
@ -162,7 +164,10 @@ class NeutronNetworksTestCase(test.TestCase):
[mock.call({"network": {"id": "fake-id"}},
subnets_per_network,
{"allocation_pools": []})] * subnets_per_network)
mock_delete.assert_called_once()
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)

4
tests/benchmark/test_utils.py
View File

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

7
tests/cmd/commands/test_verify.py
View File

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

2
tests/cmd/test_cliutils.py
View File

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

62
tests/test_hacking.py Normal file
View File

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

1
tox.ini
View File

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