From a49f2056b2500ea7e23fe1b5c30bf5b29cf8933f Mon Sep 17 00:00:00 2001 From: Monty Taylor Date: Fri, 1 Mar 2019 13:38:31 +0000 Subject: [PATCH] Get rid of setUpClass and block it for forever setUpClass is evil. It lures you in to a sense of complacency thinking that reality exists and makes sense. But it's all just a cruel joke designed to increase your personal karmic debt. In order that we might collectively one day reach nirvana, remove all senseless and misleading instances of setUpClass. Then add local hacking checks to prevent the introduction of such scourge ever again. Change-Id: Ifd43958bf47981aedad639bef61769ddb37618d3 --- HACKING.rst | 10 ++++ openstack/_hacking.py | 44 ++++++++++++++ openstack/tests/functional/base.py | 15 +++-- .../tests/functional/block_storage/v2/base.py | 9 +-- .../tests/functional/block_storage/v3/base.py | 9 +-- .../functional/block_store/v2/test_stats.py | 9 ++- .../functional/clustering/test_cluster.py | 8 +-- openstack/tests/functional/compute/base.py | 9 +-- .../load_balancer/v2/test_load_balancer.py | 9 +-- .../functional/orchestration/v1/test_stack.py | 8 +-- openstack/tests/unit/test_hacking.py | 60 +++++++++++++++++++ tox.ini | 12 ++-- 12 files changed, 137 insertions(+), 65 deletions(-) create mode 100644 openstack/_hacking.py create mode 100644 openstack/tests/unit/test_hacking.py 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}