Adds unit tests for hacking checks
Adds unit tests for Patrole hacking checks. Also cleans up
existing Patrole hacking checks for code maintainability.
This commit also modifies the P100 hacking check to work
with arbitrarily many decorators, so that rbac_rule_validation
decorator can be sandwiched between any number of decorators
in any order; the only requirement is that it appear before
each test.
Change-Id: Ic02c9278e5293311dd6f7b02790a256d391098f7
Closes-Bug: #1708794
diff --git a/HACKING.rst b/HACKING.rst
index 5281b53..c5ff7c7 100644
--- a/HACKING.rst
+++ b/HACKING.rst
@@ -32,8 +32,7 @@
The following are Patrole's specific Commandments:
- [P100] The ``rbac_rule_validation.action`` decorator must be applied to
- an RBAC test (the check fails if the decorator is not one of the
- two decorators directly above the function declaration)
+ an RBAC test
- [P101] RBAC test filenames must end with "_rbac.py"; for example,
test_servers_rbac.py, not test_servers.py
- [P102] RBAC test class names must end in 'RbacTest'
diff --git a/patrole_tempest_plugin/hacking/checks.py b/patrole_tempest_plugin/hacking/checks.py
index a3ef01f..eb73ef1 100644
--- a/patrole_tempest_plugin/hacking/checks.py
+++ b/patrole_tempest_plugin/hacking/checks.py
@@ -31,14 +31,13 @@
RAND_NAME_HYPHEN_RE = re.compile(r".*rand_name\(.+[\-\_][\"\']\)")
MUTABLE_DEFAULT_ARGS = re.compile(r"^\s*def .+\((.+=\{\}|.+=\[\])")
TESTTOOLS_SKIP_DECORATOR = re.compile(r'\s*@testtools\.skip\((.*)\)')
-TEST_METHOD = re.compile(r"^ def test_.+")
CLASS = re.compile(r"^class .+")
RBAC_CLASS_NAME_RE = re.compile(r'class .+RbacTest')
RULE_VALIDATION_DECORATOR = re.compile(
- r'\s*@.*rbac_rule_validation.action\((.*)\)')
+ r'\s*@rbac_rule_validation.action\(.*')
IDEMPOTENT_ID_DECORATOR = re.compile(r'\s*@decorators\.idempotent_id\((.*)\)')
-previous_decorator = None
+have_rbac_decorator = False
def import_no_clients_in_api_tests(physical_line, filename):
@@ -144,8 +143,7 @@
yield (0, msg)
-def no_rbac_rule_validation_decorator(physical_line, filename,
- previous_logical):
+def no_rbac_rule_validation_decorator(physical_line, filename):
"""Check that each test has the ``rbac_rule_validation.action`` decorator.
Checks whether the test function has "@rbac_rule_validation.action"
@@ -157,22 +155,24 @@
P100
"""
- global previous_decorator
+ global have_rbac_decorator
- if "patrole_tempest_plugin/tests/api" in filename:
+ if ("patrole_tempest_plugin/tests/api" in filename or
+ "patrole_tempest_plugin/tests/scenario" in filename):
- if IDEMPOTENT_ID_DECORATOR.match(physical_line):
- previous_decorator = previous_logical
+ if RULE_VALIDATION_DECORATOR.match(physical_line):
+ have_rbac_decorator = True
return
- if TEST_METHOD.match(physical_line):
- if not RULE_VALIDATION_DECORATOR.match(previous_logical) and \
- not RULE_VALIDATION_DECORATOR.match(previous_decorator):
- return (0, "Must use rbac_rule_validation.action "
- "decorator for API and scenario tests")
+ if TEST_DEFINITION.match(physical_line):
+ if not have_rbac_decorator:
+ return (0, "Must use rbac_rule_validation.action "
+ "decorator for API and scenario tests")
+
+ have_rbac_decorator = False
-def no_rbac_suffix_in_test_filename(physical_line, filename, previous_logical):
+def no_rbac_suffix_in_test_filename(filename):
"""Check that RBAC filenames end with "_rbac" suffix.
P101
@@ -186,8 +186,7 @@
return 0, "RBAC test filenames must end in _rbac suffix"
-def no_rbac_test_suffix_in_test_class_name(physical_line, filename,
- previous_logical):
+def no_rbac_test_suffix_in_test_class_name(physical_line, filename):
"""Check that RBAC class names end with "RbacTest"
P102
@@ -202,7 +201,7 @@
return 0, "RBAC test class names must end in 'RbacTest'"
-def no_client_alias_in_test_cases(filename, logical_line):
+def no_client_alias_in_test_cases(logical_line, filename):
"""Check that test cases don't use "self.client" to define a client.
P103
diff --git a/patrole_tempest_plugin/tests/unit/test_hacking.py b/patrole_tempest_plugin/tests/unit/test_hacking.py
new file mode 100644
index 0000000..021602b
--- /dev/null
+++ b/patrole_tempest_plugin/tests/unit/test_hacking.py
@@ -0,0 +1,258 @@
+# Copyright 2017 AT&T Corporation.
+# All Rights Reserved.
+#
+# 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 tempest.tests import base
+
+from patrole_tempest_plugin.hacking import checks
+
+
+class RBACHackingTestCase(base.TestCase):
+
+ def test_import_no_clients_in_api_tests(self):
+ for client in checks.PYTHON_CLIENTS:
+ import_string = "import " + client + "client"
+ self.assertTrue(checks.import_no_clients_in_api_tests(
+ import_string,
+ "./patrole_tempest_plugin/tests/api/fake_test_rbac.py"))
+ self.assertFalse(checks.import_no_clients_in_api_tests(
+ import_string,
+ "./patrole_tempest_plugin/tests/scenario/fake_test.py"))
+ self.assertFalse(checks.import_no_clients_in_api_tests(
+ import_string,
+ "./patrole_tempest_plugin/tests/unit/fake_test.py"))
+ self.assertFalse(checks.import_no_clients_in_api_tests(
+ import_string,
+ "./patrole_tempest_plugin/fake_test.py"))
+
+ def test_no_setup_teardown_class_for_tests(self):
+ self.assertTrue(checks.no_setup_teardown_class_for_tests(
+ " def setUpClass(cls):",
+ "./patrole_tempest_plugin/tests/fake_test.py"))
+ self.assertIsNone(checks.no_setup_teardown_class_for_tests(
+ " def setUpClass(cls): # noqa",
+ "./patrole_tempest_plugin/tests/fake_test.py"))
+ self.assertTrue(checks.no_setup_teardown_class_for_tests(
+ " def setUpClass(cls):",
+ "./patrole_tempest_plugin/tests/api/fake_test_rbac.py"))
+ self.assertTrue(checks.no_setup_teardown_class_for_tests(
+ " def setUpClass(cls):",
+ "./patrole_tempest_plugin/tests/scenario/fake_test.py"))
+ self.assertTrue(checks.no_setup_teardown_class_for_tests(
+ " def tearDownClass(cls):",
+ "./patrole_tempest_plugin/tests/fake_test.py"))
+ self.assertIsNone(checks.no_setup_teardown_class_for_tests(
+ " def tearDownClass(cls): # noqa",
+ "./patrole_tempest_plugin/tests/fake_test.py"))
+ self.assertTrue(checks.no_setup_teardown_class_for_tests(
+ " def tearDownClass(cls):",
+ "./patrole_tempest_plugin/tests/api/fake_test_rbac.py"))
+ self.assertTrue(checks.no_setup_teardown_class_for_tests(
+ " def tearDownClass(cls):",
+ "./patrole_tempest_plugin/tests/scenario/fake_test.py"))
+
+ def test_no_vi_headers(self):
+ self.assertTrue(checks.no_vi_headers(
+ "# vim: tabstop=4", 1, range(250)))
+ self.assertTrue(checks.no_vi_headers(
+ "# vim: tabstop=4", 249, range(250)))
+
+ def test_service_tags_not_in_module_path(self):
+ self.assertTrue(checks.service_tags_not_in_module_path(
+ "@test.services('volume')",
+ "./patrole_tempest_plugin/tests/api/volume/fake_test_rbac.py"))
+ self.assertFalse(checks.service_tags_not_in_module_path(
+ "@test.services('image')",
+ "./patrole_tempest_plugin/tests/api/volume/fake_test_rbac.py"))
+
+ def test_no_hyphen_at_end_of_rand_name(self):
+ self.assertIsNone(checks.no_hyphen_at_end_of_rand_name(
+ "data_utils.rand_name('test')",
+ "./patrole_tempest_plugin/tests/fake_test.py"))
+ self.assertIsNone(checks.no_hyphen_at_end_of_rand_name(
+ "data_utils.rand_name('test')",
+ "./patrole_tempest_plugin/tests/api/compute/fake_test_rbac.py"))
+ self.assertIsNone(checks.no_hyphen_at_end_of_rand_name(
+ "data_utils.rand_name('test')",
+ "./patrole_tempest_plugin/tests/scenario/fake_test.py"))
+ self.assertIsNone(checks.no_hyphen_at_end_of_rand_name(
+ "data_utils.rand_name('test')",
+ "./patrole_tempest_plugin/tests/unit/fake_test.py"))
+ self.assertTrue(checks.no_hyphen_at_end_of_rand_name(
+ "data_utils.rand_name('test-')",
+ "./patrole_tempest_plugin/tests/fake_test.py"))
+ self.assertTrue(checks.no_hyphen_at_end_of_rand_name(
+ "data_utils.rand_name('test-')",
+ "./patrole_tempest_plugin/tests/api/compute/fake_test_rbac.py"))
+ self.assertTrue(checks.no_hyphen_at_end_of_rand_name(
+ "data_utils.rand_name('test-')",
+ "./patrole_tempest_plugin/tests/scenario/fake_test.py"))
+ self.assertTrue(checks.no_hyphen_at_end_of_rand_name(
+ "data_utils.rand_name('test-')",
+ "./patrole_tempest_plugin/tests/unit/fake_test.py"))
+
+ def test_no_mutable_default_args(self):
+ self.assertEqual(0, len(list(checks.no_mutable_default_args(
+ " def test_function(test_param_1, test_param_2"))))
+ self.assertEqual(1, len(list(checks.no_mutable_default_args(
+ " def test_function(test_param_1, test_param_2={}"))))
+
+ def test_no_testtools_skip_decorator(self):
+ self.assertEqual(1, len(list(checks.no_testtools_skip_decorator(
+ " @testtools.skip('Bug')"))))
+ self.assertEqual(0, len(list(checks.no_testtools_skip_decorator(
+ " @testtools.skipTest('reason')"))))
+ self.assertEqual(0, len(list(checks.no_testtools_skip_decorator(
+ " @testtools.skipUnless(reason, 'message')"))))
+ self.assertEqual(0, len(list(checks.no_testtools_skip_decorator(
+ " @testtools.skipIf(reason, 'message')"))))
+
+ def test_use_rand_uuid_instead_of_uuid4(self):
+ self.assertTrue(checks.use_rand_uuid_instead_of_uuid4(
+ "new_uuid = uuid.uuid4()",
+ "./patrole_tempest_plugin/tests/fake_test.py"))
+ self.assertTrue(checks.use_rand_uuid_instead_of_uuid4(
+ "new_hex_uuid = uuid.uuid4().hex",
+ "./patrole_tempest_plugin/tests/fake_test.py"))
+ self.assertIsNotNone(checks.use_rand_uuid_instead_of_uuid4(
+ "new_uuid = data_utils.rand_uuid()",
+ "./patrole_tempest_plugin/tests/fake_test.py"))
+ self.assertIsNotNone(checks.use_rand_uuid_instead_of_uuid4(
+ "new_hex_uuid = data_utils.rand_uuid_hex()",
+ "./patrole_tempest_plugin/tests/fake_test.py"))
+
+ def _test_no_rbac_rule_validation_decorator(
+ self, filename, with_other_decorators=True,
+ with_rbac_decorator=True, expected_success=True):
+ other_decorators = [
+ "@decorators.idempotent_id(123)",
+ "@decorators.attr(type=['slow'])",
+ "@test.requires_ext(extension='ext', service='svc')"
+ ]
+
+ if with_other_decorators:
+ # Include multiple decorators to verify that this check works with
+ # arbitrarily many decorators. These insert decorators above the
+ # rbac_rule_validation decorator.
+ for decorator in other_decorators:
+ self.assertIsNone(checks.no_rbac_rule_validation_decorator(
+ " %s" % decorator, filename))
+ if with_rbac_decorator:
+ self.assertIsNone(checks.no_rbac_rule_validation_decorator(
+ " @rbac_rule_validation.action('rule')",
+ filename))
+ if with_other_decorators:
+ # Include multiple decorators to verify that this check works with
+ # arbitrarily many decorators. These insert decorators between
+ # the test and the @rbac_rule_validation decorator.
+ for decorator in other_decorators:
+ self.assertIsNone(checks.no_rbac_rule_validation_decorator(
+ " %s" % decorator, filename))
+ final_result = checks.no_rbac_rule_validation_decorator(
+ " def test_rbac_test",
+ filename)
+ if expected_success:
+ self.assertIsNone(final_result)
+ else:
+ self.assertIsInstance(final_result, tuple)
+ self.assertFalse(final_result[0])
+
+ def test_no_rbac_rule_validation_decorator(self):
+ self._test_no_rbac_rule_validation_decorator(
+ "./patrole_tempest_plugin/tests/api/fake_test_rbac.py")
+ self._test_no_rbac_rule_validation_decorator(
+ "./patrole_tempest_plugin/tests/api/fake_test_rbac.py",
+ False)
+ self._test_no_rbac_rule_validation_decorator(
+ "./patrole_tempest_plugin/tests/api/fake_test_rbac.py",
+ with_other_decorators=True, with_rbac_decorator=False,
+ expected_success=False)
+ self._test_no_rbac_rule_validation_decorator(
+ "./patrole_tempest_plugin/tests/api/fake_test_rbac.py",
+ with_other_decorators=False, with_rbac_decorator=False,
+ expected_success=False)
+
+ self._test_no_rbac_rule_validation_decorator(
+ "./patrole_tempest_plugin/tests/scenario/fake_test.py")
+ self._test_no_rbac_rule_validation_decorator(
+ "./patrole_tempest_plugin/tests/scenario/fake_test.py",
+ False)
+ self._test_no_rbac_rule_validation_decorator(
+ "./patrole_tempest_plugin/tests/scenario/fake_test.py",
+ with_other_decorators=True, with_rbac_decorator=False,
+ expected_success=False)
+ self._test_no_rbac_rule_validation_decorator(
+ "./patrole_tempest_plugin/tests/scenario/fake_test.py",
+ with_other_decorators=False, with_rbac_decorator=False,
+ expected_success=False)
+
+ def test_no_rbac_suffix_in_test_filename(self):
+ self.assertFalse(checks.no_rbac_suffix_in_test_filename(
+ "./patrole_tempest_plugin/tests/fake_test.py"))
+ self.assertFalse(checks.no_rbac_suffix_in_test_filename(
+ "./patrole_tempest_plugin/tests/scenario/fake_test.py"))
+ self.assertFalse(checks.no_rbac_suffix_in_test_filename(
+ "./patrole_tempest_plugin/tests/unit/fake_test.py"))
+ self.assertFalse(checks.no_rbac_suffix_in_test_filename(
+ "./patrole_tempest_plugin/tests/api/fake_rbac_base.py"))
+ self.assertFalse(checks.no_rbac_suffix_in_test_filename(
+ "./patrole_tempest_plugin/tests/api/fake_test_rbac.py"))
+ self.assertTrue(checks.no_rbac_suffix_in_test_filename(
+ "./patrole_tempest_plugin/tests/api/fake_test.py"))
+
+ def test_no_rbac_test_suffix_in_test_class_name(self):
+ self.assertFalse(checks.no_rbac_test_suffix_in_test_class_name(
+ "class FakeTest",
+ "./patrole_tempest_plugin/tests/fake_test.py"))
+ self.assertFalse(checks.no_rbac_test_suffix_in_test_class_name(
+ "class FakeTest",
+ "./patrole_tempest_plugin/tests/scenario/fake_test.py"))
+ self.assertFalse(checks.no_rbac_test_suffix_in_test_class_name(
+ "class FakeTest",
+ "./patrole_tempest_plugin/tests/unit/fake_test.py"))
+ self.assertFalse(checks.no_rbac_test_suffix_in_test_class_name(
+ "class FakeTest",
+ "./patrole_tempest_plugin/tests/api/fake_rbac_base.py"))
+ self.assertFalse(checks.no_rbac_test_suffix_in_test_class_name(
+ "class FakeRbacTest",
+ "./patrole_tempest_plugin/tests/api/fake_test_rbac.py"))
+ self.assertTrue(checks.no_rbac_test_suffix_in_test_class_name(
+ "class FakeTest",
+ "./patrole_tempest_plugin/tests/api/fake_test_rbac.py"))
+
+ def test_no_client_alias_in_test_cases(self):
+ self.assertFalse(checks.no_client_alias_in_test_cases(
+ " self.client",
+ "./patrole_tempest_plugin/tests/fake_test.py"))
+ self.assertFalse(checks.no_client_alias_in_test_cases(
+ " cls.client",
+ "./patrole_tempest_plugin/tests/fake_test.py"))
+ self.assertFalse(checks.no_client_alias_in_test_cases(
+ " self.client",
+ "./patrole_tempest_plugin/tests/unit/fake_test.py"))
+ self.assertFalse(checks.no_client_alias_in_test_cases(
+ " cls.client",
+ "./patrole_tempest_plugin/tests/unit/fake_test.py"))
+ self.assertFalse(checks.no_client_alias_in_test_cases(
+ " self.client",
+ "./patrole_tempest_plugin/tests/scenario/fake_test.py"))
+ self.assertFalse(checks.no_client_alias_in_test_cases(
+ " cls.client",
+ "./patrole_tempest_plugin/tests/scenario/fake_test.py"))
+ self.assertTrue(checks.no_client_alias_in_test_cases(
+ " self.client",
+ "./patrole_tempest_plugin/tests/api/fake_test_rbac.py"))
+ self.assertTrue(checks.no_client_alias_in_test_cases(
+ " cls.client",
+ "./patrole_tempest_plugin/tests/api/fake_test_rbac.py"))