diff --git a/HACKING.rst b/HACKING.rst index bda11bcbd..9179bd7c0 100644 --- a/HACKING.rst +++ b/HACKING.rst @@ -51,3 +51,13 @@ in a way that protects against local environment. Test cases should use requests-mock to mock out HTTP interactions rather than using mock to mock out object access. + +Don't Use setUpClass +-------------------- + +setUpClass looks like it runs once for the class. In parallel test execution +environments though, it runs once per execution context. This makes reasoning +about when it is going to actually run and what is going to happen extremely +difficult and can produce hard to debug test issues. + +Don't ever use it. It makes baby pandas cry. diff --git a/openstack/_hacking.py b/openstack/_hacking.py new file mode 100644 index 000000000..94952630c --- /dev/null +++ b/openstack/_hacking.py @@ -0,0 +1,44 @@ +# Copyright (c) 2019, Red Hat, Inc. +# +# 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. + +import re + +""" +Guidelines for writing new hacking checks + + - Use only for openstacksdk specific tests. OpenStack general tests + should be submitted to the common 'hacking' module. + - Pick numbers in the range O3xx. 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 O3xx value. + - List the new rule in the top level HACKING.rst file + - Add test cases for each new rule to nova/tests/unit/test_hacking.py + +""" + +SETUPCLASS_RE = re.compile(r"def setUpClass\(") + + +def assert_no_setupclass(logical_line): + """Check for use of setUpClass + + O300 + """ + if SETUPCLASS_RE.match(logical_line): + yield (0, "O300: setUpClass not allowed") + + +def factory(register): + register(assert_no_setupclass) diff --git a/openstack/tests/functional/base.py b/openstack/tests/functional/base.py index 603125f21..43900c99a 100644 --- a/openstack/tests/functional/base.py +++ b/openstack/tests/functional/base.py @@ -45,14 +45,7 @@ FLAVOR_NAME = _get_resource_value('flavor_name', 'm1.small') class BaseFunctionalTest(base.TestCase): - @classmethod - def setUpClass(cls): - super(BaseFunctionalTest, cls).setUpClass() - # Defines default timeout for wait_for methods used - # in the functional tests - cls._wait_for_timeout = int(os.getenv( - 'OPENSTACKSDK_FUNC_TEST_TIMEOUT', - 300)) + _wait_for_timeout_key = '' def setUp(self): super(BaseFunctionalTest, self).setUp() @@ -70,6 +63,12 @@ class BaseFunctionalTest(base.TestCase): self.identity_version = \ self.operator_cloud.config.get_api_version('identity') + # Defines default timeout for wait_for methods used + # in the functional tests + self._wait_for_timeout = int( + os.getenv(self._wait_for_timeout_key, os.getenv( + 'OPENSTACKSDK_FUNC_TEST_TIMEOUT', 300))) + def _set_user_cloud(self, **kwargs): user_config = self.config.get_one( cloud=self._demo_name, **kwargs) diff --git a/openstack/tests/functional/block_storage/v2/base.py b/openstack/tests/functional/block_storage/v2/base.py index 5475d1e14..4d7f0c099 100644 --- a/openstack/tests/functional/block_storage/v2/base.py +++ b/openstack/tests/functional/block_storage/v2/base.py @@ -10,19 +10,12 @@ # License for the specific language governing permissions and limitations # under the License. -import os - from openstack.tests.functional import base class BaseBlockStorageTest(base.BaseFunctionalTest): - @classmethod - def setUpClass(cls): - super(BaseBlockStorageTest, cls).setUpClass() - cls._wait_for_timeout = int(os.getenv( - 'OPENSTACKSDK_FUNC_TEST_TIMEOUT_BLOCK_STORAGE', - cls._wait_for_timeout)) + _wait_for_timeout_key = 'OPENSTACKSDK_FUNC_TEST_TIMEOUT_BLOCK_STORAGE' def setUp(self): super(BaseBlockStorageTest, self).setUp() diff --git a/openstack/tests/functional/block_storage/v3/base.py b/openstack/tests/functional/block_storage/v3/base.py index 5f98a3cdc..adab8dba8 100644 --- a/openstack/tests/functional/block_storage/v3/base.py +++ b/openstack/tests/functional/block_storage/v3/base.py @@ -10,19 +10,12 @@ # License for the specific language governing permissions and limitations # under the License. -import os - from openstack.tests.functional import base class BaseBlockStorageTest(base.BaseFunctionalTest): - @classmethod - def setUpClass(cls): - super(BaseBlockStorageTest, cls).setUpClass() - cls._wait_for_timeout = int(os.getenv( - 'OPENSTACKSDK_FUNC_TEST_TIMEOUT_BLOCK_STORAGE', - cls._wait_for_timeout)) + _wait_for_timeout_key = 'OPENSTACKSDK_FUNC_TEST_TIMEOUT_BLOCK_STORAGE' def setUp(self): super(BaseBlockStorageTest, self).setUp() diff --git a/openstack/tests/functional/block_store/v2/test_stats.py b/openstack/tests/functional/block_store/v2/test_stats.py index 581f6e33e..ef64d49a7 100644 --- a/openstack/tests/functional/block_store/v2/test_stats.py +++ b/openstack/tests/functional/block_store/v2/test_stats.py @@ -17,12 +17,11 @@ from openstack.tests.functional import base class TestStats(base.BaseFunctionalTest): - @classmethod - def setUpClass(cls): - super(TestStats, cls).setUpClass() - sot = cls.conn.block_storage.backend_pools() + def setUp(self): + super(TestStats, self).setUp() + sot = self.conn.block_storage.backend_pools() for pool in sot: - assert isinstance(pool, _stats.Pools) + self.assertIsInstance(pool, _stats.Pools) def test_list(self): capList = ['volume_backend_name', 'storage_protocol', diff --git a/openstack/tests/functional/clustering/test_cluster.py b/openstack/tests/functional/clustering/test_cluster.py index e964d37d4..4f03e9f21 100644 --- a/openstack/tests/functional/clustering/test_cluster.py +++ b/openstack/tests/functional/clustering/test_cluster.py @@ -10,7 +10,6 @@ # License for the specific language governing permissions and limitations # under the License. -import os import time from openstack.clustering.v1 import cluster @@ -20,12 +19,7 @@ from openstack.tests.functional.network.v2 import test_network class TestCluster(base.BaseFunctionalTest): - @classmethod - def setUpClass(cls): - super(TestCluster, cls).setUpClass() - cls._wait_for_timeout = int(os.getenv( - 'OPENSTACKSDK_FUNC_TEST_TIMEOUT_CLUSTER', - cls._wait_for_timeout)) + _wait_for_timeout_key = 'OPENSTACKSDK_FUNC_TEST_TIMEOUT_CLUSTER' def setUp(self): super(TestCluster, self).setUp() diff --git a/openstack/tests/functional/compute/base.py b/openstack/tests/functional/compute/base.py index 12b86fad1..bfeabd521 100644 --- a/openstack/tests/functional/compute/base.py +++ b/openstack/tests/functional/compute/base.py @@ -10,16 +10,9 @@ # License for the specific language governing permissions and limitations # under the License. -import os - from openstack.tests.functional import base class BaseComputeTest(base.BaseFunctionalTest): - @classmethod - def setUpClass(cls): - super(BaseComputeTest, cls).setUpClass() - cls._wait_for_timeout = int(os.getenv( - 'OPENSTACKSDK_FUNC_TEST_TIMEOUT_COMPUTE', - cls._wait_for_timeout)) + _wait_for_timeout_key = 'OPENSTACKSDK_FUNC_TEST_TIMEOUT_COMPUTE' diff --git a/openstack/tests/functional/load_balancer/v2/test_load_balancer.py b/openstack/tests/functional/load_balancer/v2/test_load_balancer.py index a9b984653..daae3b619 100644 --- a/openstack/tests/functional/load_balancer/v2/test_load_balancer.py +++ b/openstack/tests/functional/load_balancer/v2/test_load_balancer.py @@ -10,8 +10,6 @@ # License for the specific language governing permissions and limitations # under the License. -import os - from openstack.load_balancer.v2 import flavor from openstack.load_balancer.v2 import flavor_profile from openstack.load_balancer.v2 import health_monitor @@ -56,12 +54,7 @@ class TestLoadBalancer(base.BaseFunctionalTest): FLAVOR_DATA = '{"loadbalancer_topology": "SINGLE"}' DESCRIPTION = 'Test description' - @classmethod - def setUpClass(cls): - super(TestLoadBalancer, cls).setUpClass() - cls._wait_for_timeout = int(os.getenv( - 'OPENSTACKSDK_FUNC_TEST_TIMEOUT_LOAD_BALANCER', - cls._wait_for_timeout)) + _wait_for_timeout_key = 'OPENSTACKSDK_FUNC_TEST_TIMEOUT_LOAD_BALANCER' # TODO(shade): Creating load balancers can be slow on some hosts due to # nova instance boot times (up to ten minutes). This used to diff --git a/openstack/tests/functional/orchestration/v1/test_stack.py b/openstack/tests/functional/orchestration/v1/test_stack.py index 67ef21537..afc08c523 100644 --- a/openstack/tests/functional/orchestration/v1/test_stack.py +++ b/openstack/tests/functional/orchestration/v1/test_stack.py @@ -10,7 +10,6 @@ # License for the specific language governing permissions and limitations # under the License. -import os import yaml from openstack import exceptions @@ -27,12 +26,7 @@ class TestStack(base.BaseFunctionalTest): subnet = None cidr = '10.99.99.0/16' - @classmethod - def setUpClass(cls): - super(TestStack, cls).setUpClass() - cls._wait_for_timeout = int(os.getenv( - 'OPENSTACKSDK_FUNC_TEST_TIMEOUT_ORCHESTRATION', - cls._wait_for_timeout)) + _wait_for_timeout_key = 'OPENSTACKSDK_FUNC_TEST_TIMEOUT_ORCHESTRATION' def setUp(self): super(TestStack, self).setUp() diff --git a/openstack/tests/unit/test_hacking.py b/openstack/tests/unit/test_hacking.py new file mode 100644 index 000000000..e6da4f58d --- /dev/null +++ b/openstack/tests/unit/test_hacking.py @@ -0,0 +1,60 @@ +# Copyright 2019 Red Hat, Inc. +# +# 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 openstack import _hacking +from openstack.tests.unit import base + + +class HackingTestCase(base.TestCase): + """This class tests the hacking checks in openstack._hacking.checks. + + It works by passing strings to the check methods like the pep8/flake8 + parser would. The parser loops over each line in the file and then passes + the parameters to the check method. The parameter names in the check method + dictate what type of object is passed to the check method. + + The parameter types are:: + + logical_line: A processed line with the following modifications: + - Multi-line statements converted to a single line. + - Stripped left and right. + - Contents of strings replaced with "xxx" of same length. + - Comments removed. + physical_line: Raw line of text from the input file. + lines: a list of the raw lines from the input file + tokens: the tokens that contribute to this logical line + line_number: line number in the input file + total_lines: number of lines in the input file + blank_lines: blank lines before this one + indent_char: indentation character in this file (" " or "\t") + indent_level: indentation (with tabs expanded to multiples of 8) + previous_indent_level: indentation on previous line + previous_logical: previous logical line + filename: Path of the file being run through pep8 + + When running a test on a check method the return will be False/None if + there is no violation in the sample input. If there is an error a tuple is + returned with a position in the line, and a message. So to check the result + just assertTrue if the check is expected to fail and assertFalse if it + should pass. + """ + def test_assert_no_setupclass(self): + self.assertEqual(len(list(_hacking.assert_no_setupclass( + "def setUpClass(cls)"))), 1) + + self.assertEqual(len(list(_hacking.assert_no_setupclass( + "# setUpClass is evil"))), 0) + + self.assertEqual(len(list(_hacking.assert_no_setupclass( + "def setUpClassyDrinkingLocation(cls)"))), 0) diff --git a/tox.ini b/tox.ini index 63be5d510..20e755222 100644 --- a/tox.ini +++ b/tox.ini @@ -39,18 +39,18 @@ commands = stestr --test-path ./openstack/tests/functional/{env:OPENSTACKSDK_TES stestr slowest [testenv:pep8] -usedevelop = False -skip_install = True deps = - -c{env:UPPER_CONSTRAINTS_FILE:https://git.openstack.org/cgit/openstack/requirements/plain/upper-constraints.txt} - doc8 - flake8 + {[testenv]deps} hacking + doc8 pygments readme commands = - doc8 doc/source flake8 + doc8 doc/source + +[hacking] +local-check-factory = openstack._hacking.factory [testenv:venv] commands = {posargs}