From 17f64db38c2b12f71d5f14a98c6a155737b2b7f2 Mon Sep 17 00:00:00 2001 From: Matthew Treinish Date: Mon, 6 Feb 2023 16:29:32 -0500 Subject: [PATCH 1/3] Fix ResourceWarning from SubunitTestRunner._list() This commit fixes the ResourceWarning emitted by stestr's SubunitRunner._list method caused by a leaking file descriptor when that method exits. The root cause of this was this method was calling fdopen() internally to open a new descriptor to the user specified result stream and never closing it when the method returns. The issue was that the return from that method had a reference to that fd and closing it would have resulted in potentially unexpected behavior. However, this code is not needed, it was just ported from subunit's SubunitTestRunner class when this code was forked and rewritten to use unittest instead of testtools. The subunit code is used in a broader context and the fdopen might be needed there. But for stestr, we always use stdout for the result stream as this only gets called internally the worker processes to run tests. If we used something other than stdout the result stream would not get sent to the parent process. Since we're alwwys using stdout we don't need an fdopen call because we never will need a fresh fd for it. This commit removes the legacy baggage causing this ResourceWarning and just interacts with the stream directly avoiding an additional open that never gets closed. Related to #320 --- stestr/subunit_runner/run.py | 10 +--------- 1 file changed, 1 insertion(+), 9 deletions(-) diff --git a/stestr/subunit_runner/run.py b/stestr/subunit_runner/run.py index 9fab3e6..1b9b920 100644 --- a/stestr/subunit_runner/run.py +++ b/stestr/subunit_runner/run.py @@ -14,7 +14,6 @@ # under the License. from functools import partial -import os import sys from subunit import StreamResultToBytes @@ -71,14 +70,7 @@ def list(self, test, loader=None): def _list(self, test): test_ids, errors = program.list_test(test) - try: - fileno = self.stream.fileno() - except Exception: - fileno = None - if fileno is not None: - stream = os.fdopen(fileno, "wb", 0) - else: - stream = self.stream + stream = self.stream result = StreamResultToBytes(stream) for test_id in test_ids: result.status(test_id=test_id, test_status="exists") From 23910a855bcdc425fbcacfba64b596748c06c8a4 Mon Sep 17 00:00:00 2001 From: Matthew Treinish Date: Mon, 6 Feb 2023 16:42:31 -0500 Subject: [PATCH 2/3] Pin black to 2022 version Black only provides backwards compatibility guarantees for each year's versions. The major version is the year which will not make any breaking format changes in that release. Since we're in 2023 now black has released a 23.x.y release which causes incompatible changes with our previously used 22.x.y version. In the short term to avoid failures caused by these unrelated changes this commit pins our version to 22.x.y. In the future we can upgrade this version and update the formatting with the new rules at that time. --- test-requirements.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test-requirements.txt b/test-requirements.txt index 8d2231d..8073abf 100644 --- a/test-requirements.txt +++ b/test-requirements.txt @@ -7,4 +7,4 @@ sphinx>2.1.0 # BSD coverage>=4.0 # Apache-2.0 ddt>=1.0.1 # MIT doc8>=0.8.0 # Apache-2.0 -black>=22.8.0 +black~=22.0 From 8d260ed1c31f2cba66cea9579c4d7c21e2ed4ae8 Mon Sep 17 00:00:00 2001 From: Nicolas Simonds <0xDEC0DE@users.noreply.github.com> Date: Fri, 10 Mar 2023 07:18:56 -0800 Subject: [PATCH 3/3] Add support for storing stestr configs in pyproject.toml (#341) * Add support for storing stestr configs in pyproject.toml - Lightly refactor `config_file.TestrConf` to improve code reuse, and make it easier to invoke and use - Add/update tests where needed - Update documentation Fixes: Issue #340 * Add missing tests, update docs Attempt to address the review feedback. This also modifies `TestrConf.load_from_file` so that failure to load a user-specified config file is an error. This should make it adhere to the Principle of Least Astonishment. Since this changes docuemnted behavior, also update the docs to stop mentioning it. --------- Co-authored-by: Nicolas Simonds --- doc/source/MANUAL.rst | 44 +++++++++++++++-- requirements.txt | 1 + stestr/commands/list.py | 10 +--- stestr/commands/run.py | 9 +--- stestr/config_file.py | 84 +++++++++++++++++++++++--------- stestr/tests/test_config_file.py | 67 +++++++++++++++++++++++-- 6 files changed, 170 insertions(+), 45 deletions(-) diff --git a/doc/source/MANUAL.rst b/doc/source/MANUAL.rst index e9decfc..fae7c1a 100644 --- a/doc/source/MANUAL.rst +++ b/doc/source/MANUAL.rst @@ -84,24 +84,58 @@ CLI. This way you can run stestr directly without having to write a config file and manually specify the test_path like above with the ``--test-path``/``-t`` CLI argument. +.. _tox: + Tox ''' If you are also using `tox `__ with your project then it is not necessary to create separate stestr config file, instead you can embed the necessary configuration in the existing ``tox.ini`` file with -an ``stestr`` section. For example a full configuration section would be:: +an ``[stestr]`` section. For example a full configuration section would be:: [stestr] test_path=./project/tests top_dir=./ group_regex=([^\.]*\.)* +Any configuration directives outside the ``[stestr]`` section will be ignored. It's important to note that if either the ``--config``/``-c`` CLI argument is -specified and pointing to an existing file or the default location -``.stestr.conf`` file is present then any configuration in the ``tox.ini`` will -be ignored. Configuration embedded in a ``tox.ini`` will only be used if other -configuration files are not present. +specified, or the default location ``.stestr.conf`` file is present +then any configuration in the ``tox.ini`` will be ignored. Configuration +embedded in a ``tox.ini`` will only be used if other configuration +files are not present. + +pyproject.toml +'''''''''''''' + +Similarly, if your project is using ``pyproject.toml``, you may forego the +config file, and instead create a ``[tool.stestr]`` section with the desired +configuration options. For example:: + + [tool.stestr] + test_path = "./project/tests" + top_dir = "./" + group_regex = "([^\.]*\.)*" + +The same caveats apply as the :ref:`tox` with regards to CLI arguments. + +Configuration file precedence +''''''''''''''''''''''''''''' + +The order in which configuration files are read is as follows: + +* Any file specified with the ``--config``/``-c`` CLI argument +* The ``.stestr.conf`` file +* The ``[tool.stestr]`` section in a ``pyproject.toml`` file +* The ``[stestr]`` section in a ``tox.ini`` file + +Also of note is that files specified with ``--config-file``/``-c`` +may be either ``.ini`` or TOML format. If providing configs in +``.ini`` format, they **must** be in a ``[DEFAULT]`` section. If +providing configs in TOML format, the configuration directives +**must** be located in a ``[tool.stestr]`` section, and the filename +**must** have a ``.toml`` extension. Running tests ------------- diff --git a/requirements.txt b/requirements.txt index 55149fe..91bb1f3 100644 --- a/requirements.txt +++ b/requirements.txt @@ -8,3 +8,4 @@ fixtures>=3.0.0 # Apache-2.0/BSD testtools>=2.2.0 # MIT PyYAML>=3.10.0 # MIT voluptuous>=0.8.9 # BSD License +tomlkit>=0.11.6 # MIT diff --git a/stestr/commands/list.py b/stestr/commands/list.py index 8697ebe..39a32cb 100644 --- a/stestr/commands/list.py +++ b/stestr/commands/list.py @@ -13,7 +13,6 @@ """List the tests from a project and show them.""" from io import BytesIO -import os import sys from cliff import command @@ -90,7 +89,7 @@ def take_action(self, parsed_args): def list_command( - config=".stestr.conf", + config=config_file.TestrConf.DEFAULT_CONFIG_FILENAME, repo_url=None, test_path=None, top_dir=None, @@ -133,12 +132,7 @@ def list_command( """ ids = None - if config and os.path.isfile(config): - conf = config_file.TestrConf(config) - elif os.path.isfile("tox.ini"): - conf = config_file.TestrConf("tox.ini", section="stestr") - else: - conf = config_file.TestrConf(config) + conf = config_file.TestrConf.load_from_file(config) cmd = conf.get_run_command( regexes=filters, repo_url=repo_url, diff --git a/stestr/commands/run.py b/stestr/commands/run.py index 4cfac09..4afb8b5 100644 --- a/stestr/commands/run.py +++ b/stestr/commands/run.py @@ -368,7 +368,7 @@ def gather_errors(test_dict): def run_command( - config=".stestr.conf", + config=config_file.TestrConf.DEFAULT_CONFIG_FILENAME, repo_url=None, test_path=None, top_dir=None, @@ -644,12 +644,7 @@ def run_tests(): # that are both failing and listed. ids = list_ids.intersection(ids) - if config and os.path.isfile(config): - conf = config_file.TestrConf(config) - elif os.path.isfile("tox.ini"): - conf = config_file.TestrConf("tox.ini", section="stestr") - else: - conf = config_file.TestrConf(config) + conf = config_file.TestrConf.load_from_file(config) if not analyze_isolation: cmd = conf.get_run_command( ids, diff --git a/stestr/config_file.py b/stestr/config_file.py index 2587b6a..6a66503 100644 --- a/stestr/config_file.py +++ b/stestr/config_file.py @@ -15,6 +15,7 @@ import sys import configparser +import tomlkit from stestr.repository import util from stestr import test_processor @@ -27,17 +28,66 @@ class TestrConf: of a tox.ini file the stestr section in the tox.ini file :param str config_file: The path to the config file to use - :param str section: The section to use for the stestr config. By default - this is DEFATULT. + :param str section: The section to use for the stestr config. By default, + this is DEFAULT. """ + DEFAULT_CONFIG_FILENAME = ".stestr.conf" _escape_trailing_backslash_re = re.compile(r"(?<=[^\\])\\$") + # Set sensible config defaults here, so that override methods are kept DRY + test_path = None + top_dir = None + parallel_class = False + group_regex = None def __init__(self, config_file, section="DEFAULT"): - self.parser = configparser.ConfigParser() - self.parser.read(config_file) - self.config_file = config_file + self.config_file = str(config_file) self.section = section + if self.config_file.lower().endswith(".toml"): + self._load_from_toml() + else: + self._load_from_configparser() + + def _load_from_configparser(self): + parser = configparser.ConfigParser() + parser.read(self.config_file) + self.test_path = parser.get(self.section, "test_path", fallback=self.test_path) + self.top_dir = parser.get(self.section, "top_dir", fallback=self.top_dir) + self.parallel_class = parser.getboolean( + self.section, "parallel_class", fallback=self.parallel_class + ) + self.group_regex = parser.get( + self.section, "group_regex", fallback=self.group_regex + ) + + def _load_from_toml(self): + with open(self.config_file) as f: + doc = tomlkit.load(f) + root = doc["tool"]["stestr"] + self.test_path = root.get("test_path", self.test_path) + self.top_dir = root.get("top_dir", self.top_dir) + self.parallel_class = root.get("parallel_class", self.parallel_class) + self.group_regex = root.get("group_regex", self.group_regex) + + @classmethod + def load_from_file(cls, config): + """Load user-specified values from the various config files. + + ConfigParser (.ini) and TOML are supported. + If a config file is specified, it is used, and fails on errors. + If no config file is specified, the order of precedence is as follows: + - .stestr.conf + - pyproject.toml with a valid [tool.stestr] section + - tox.ini with a valid [stestr] section + + :param str config: The pathname of the config file to use + """ + if os.path.isfile(config) or config != cls.DEFAULT_CONFIG_FILENAME: + return cls(config) + try: + return cls("pyproject.toml") + except (FileNotFoundError, KeyError): + return cls("tox.ini", section="stestr") def _sanitize_path(self, path): if os.sep == "\\": @@ -114,22 +164,18 @@ def get_run_command( :rtype: test_processor.TestProcessorFixture """ - if not test_path and self.parser.has_option(self.section, "test_path"): - test_path = self.parser.get(self.section, "test_path") - elif not test_path: + if not test_path and not self.test_path: sys.exit( "No test_path can be found in either the command line " "options nor in the specified config file {}. Please " "specify a test path either in the config file or via " "the --test-path argument".format(self.config_file) ) - if not top_dir and self.parser.has_option(self.section, "top_dir"): - top_dir = self.parser.get(self.section, "top_dir") - elif not top_dir: + if not top_dir and not self.top_dir: top_dir = "./" - test_path = self._sanitize_path(test_path) - top_dir = self._sanitize_path(top_dir) + test_path = self._sanitize_path(test_path or self.test_path) + top_dir = self._sanitize_path(top_dir or self.top_dir) stestr_python = sys.executable # let's try to be explicit, even if it means a longer set of ifs @@ -160,16 +206,10 @@ def get_run_command( idoption = "--load-list $IDFILE" # If the command contains $IDOPTION read that command from config # Use a group regex if one is defined - if parallel_class: - group_regex = r"([^\.]*\.)*" - if ( - not group_regex - and self.parser.has_option(self.section, "parallel_class") - and self.parser.getboolean(self.section, "parallel_class") - ): + if parallel_class or self.parallel_class: group_regex = r"([^\.]*\.)*" - if not group_regex and self.parser.has_option(self.section, "group_regex"): - group_regex = self.parser.get(self.section, "group_regex") + if not group_regex and self.group_regex: + group_regex = self.group_regex if group_regex: def group_callback(test_id, regex=re.compile(group_regex)): diff --git a/stestr/tests/test_config_file.py b/stestr/tests/test_config_file.py index 452ff31..a1a1e9d 100644 --- a/stestr/tests/test_config_file.py +++ b/stestr/tests/test_config_file.py @@ -9,10 +9,12 @@ # 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 os +import random from unittest import mock import ddt +import fixtures from stestr import config_file from stestr.tests import base @@ -24,7 +26,6 @@ class TestTestrConf(base.TestCase): def setUp(self, mock_ConfigParser): super().setUp() self._testr_conf = config_file.TestrConf(mock.sentinel.config_file) - self._testr_conf.parser = mock.Mock() @mock.patch.object(config_file.util, "get_repo_open") @mock.patch.object(config_file.test_processor, "TestProcessorFixture") @@ -130,7 +131,8 @@ def test_get_run_command_parallel_class(self): self._check_get_run_command(parallel_class=True) def test_get_run_command_nogroup_regex_noparallel_class(self): - self._testr_conf.parser.has_option.return_value = False + self._testr_conf.parallel_class = False + self._testr_conf.group_regex = "" self._check_get_run_command(group_regex="", expected_group_callback=None) @ddt.data((".\\", ".\\\\"), ("a\\b\\", "a\\b\\\\"), ("a\\b", "a\\b")) @@ -139,3 +141,62 @@ def test_get_run_command_nogroup_regex_noparallel_class(self): def test_sanitize_dir_win32(self, path, expected): sanitized = self._testr_conf._sanitize_path(path) self.assertEqual(expected, sanitized) + + @mock.patch("os.path.isfile") + @mock.patch("stestr.config_file.TestrConf.__init__") + def test_load_from_file_user_specified(self, initializer, isfile): + # Test that user-specified config files are "one-and-done" + initializer.return_value = None + isfile.return_value = True + config_file.TestrConf.load_from_file("user.conf") + initializer.assert_called_once_with("user.conf") + + @mock.patch("os.path.isfile") + @mock.patch("stestr.config_file.TestrConf.__init__") + def test_load_from_file_user_specified_fails(self, initializer, isfile): + # Test that user-specified config files that do not exist gives up + # immediately + initializer.return_value = None + initializer.side_effect = FileNotFoundError + isfile.return_value = False + self.assertRaises( + FileNotFoundError, config_file.TestrConf.load_from_file, "user.conf" + ) + isfile.assert_called_once_with("user.conf") + initializer.assert_called_once_with("user.conf") + + @mock.patch("os.path.isfile") + @mock.patch("stestr.config_file.TestrConf.__init__") + def test_load_from_file_toml_has_precedence(self, initializer, isfile): + # Test that tox.ini is ignored if a pyproject.toml config exists + initializer.return_value = None + isfile.return_value = False + config_file.TestrConf.load_from_file(".stestr.conf") + isfile.assert_called_once_with(".stestr.conf") + initializer.assert_called_once_with("pyproject.toml") + + @mock.patch("os.path.isfile") + @mock.patch("stestr.config_file.TestrConf.__init__") + def test_load_from_file_ini_fallback(self, initializer, isfile): + initializer.return_value = None + # The only difference between "no config file" and "nothing defined + # in the config file" is the type of exception thrown; we'll make + # sure that, in aggregate, we test for both conditions + exc = random.choice([FileNotFoundError, KeyError]) + initializer.side_effect = (exc, None) + isfile.return_value = False + config_file.TestrConf.load_from_file(".stestr.conf") + isfile.assert_called_once_with(".stestr.conf") + initializer.assert_has_calls( + [mock.call("pyproject.toml"), mock.call("tox.ini", section="stestr")] + ) + + @mock.patch.object(config_file.tomlkit, "load") + def test_toml_load(self, mock_toml): + tmpdir = self.useFixture(fixtures.TempDir()).path + file_path = os.path.join(tmpdir, "myfile.toml") + with open(file_path, "w"): + pass + self._testr_conf = config_file.TestrConf(file_path) + self._check_get_run_command() + mock_toml.return_value.__getitem__.assert_called_once_with("tool")