From 7c7bdf457417cdc238d1e0911d7b4af8133cb208 Mon Sep 17 00:00:00 2001 From: Sharad Nair <134932980+SharadNair7@users.noreply.github.com> Date: Sat, 11 Nov 2023 21:38:18 +0530 Subject: [PATCH] Sanitize ini-options default handling #11282 (#11594) Fixes #11282 --- changelog/11282.breaking.rst | 11 ++++++ src/_pytest/config/__init__.py | 27 +++++++++++--- src/_pytest/config/argparsing.py | 30 ++++++++++++++- testing/test_config.py | 64 ++++++++++++++++++++++++++++++++ 4 files changed, 126 insertions(+), 6 deletions(-) create mode 100644 changelog/11282.breaking.rst diff --git a/changelog/11282.breaking.rst b/changelog/11282.breaking.rst new file mode 100644 index 00000000000..cee9788ef1c --- /dev/null +++ b/changelog/11282.breaking.rst @@ -0,0 +1,11 @@ +Sanitized the handling of the ``default`` parameter when defining configuration options. + +Previously if ``default`` was not supplied for :meth:`parser.addini ` and the configuration option value was not defined in a test session, then calls to :func:`config.getini ` returned an *empty list* or an *empty string* depending on whether ``type`` was supplied or not respectively, which is clearly incorrect. Also, ``None`` was not honored even if ``default=None`` was used explicitly while defining the option. + +Now the behavior of :meth:`parser.addini ` is as follows: + +* If ``default`` is NOT passed but ``type`` is provided, then a type-specific default will be returned. For example ``type=bool`` will return ``False``, ``type=str`` will return ``""``, etc. +* If ``default=None`` is passed and the option is not defined in a test session, then ``None`` will be returned, regardless of the ``type``. +* If neither ``default`` nor ``type`` are provided, assume ``type=str`` and return ``""`` as default (this is as per previous behavior). + +The team decided to not introduce a deprecation period for this change, as doing so would be complicated both in terms of communicating this to the community as well as implementing it, and also because the team believes this change should not break existing plugins except in rare cases. diff --git a/src/_pytest/config/__init__.py b/src/_pytest/config/__init__.py index 447ebc42abb..03f69ed31e3 100644 --- a/src/_pytest/config/__init__.py +++ b/src/_pytest/config/__init__.py @@ -1495,6 +1495,27 @@ def addinivalue_line(self, name: str, line: str) -> None: def getini(self, name: str): """Return configuration value from an :ref:`ini file `. + If a configuration value is not defined in an + :ref:`ini file `, then the ``default`` value provided while + registering the configuration through + :func:`parser.addini ` will be returned. + Please note that you can even provide ``None`` as a valid + default value. + + If ``default`` is not provided while registering using + :func:`parser.addini `, then a default value + based on the ``type`` parameter passed to + :func:`parser.addini ` will be returned. + The default values based on ``type`` are: + ``paths``, ``pathlist``, ``args`` and ``linelist`` : empty list ``[]`` + ``bool`` : ``False`` + ``string`` : empty string ``""`` + + If neither the ``default`` nor the ``type`` parameter is passed + while registering the configuration through + :func:`parser.addini `, then the configuration + is treated as a string and a default empty string '' is returned. + If the specified name hasn't been registered through a prior :func:`parser.addini ` call (usually from a plugin), a ValueError is raised. @@ -1521,11 +1542,7 @@ def _getini(self, name: str): try: value = self.inicfg[name] except KeyError: - if default is not None: - return default - if type is None: - return "" - return [] + return default else: value = override_value # Coerce the values based on types. diff --git a/src/_pytest/config/argparsing.py b/src/_pytest/config/argparsing.py index e345de01639..461b8afdff9 100644 --- a/src/_pytest/config/argparsing.py +++ b/src/_pytest/config/argparsing.py @@ -27,6 +27,14 @@ FILE_OR_DIR = "file_or_dir" +class NotSet: + def __repr__(self) -> str: + return "" + + +NOT_SET = NotSet() + + @final class Parser: """Parser for command line arguments and ini-file values. @@ -176,7 +184,7 @@ def addini( type: Optional[ Literal["string", "paths", "pathlist", "args", "linelist", "bool"] ] = None, - default: Any = None, + default: Any = NOT_SET, ) -> None: """Register an ini-file option. @@ -203,10 +211,30 @@ def addini( :py:func:`config.getini(name) `. """ assert type in (None, "string", "paths", "pathlist", "args", "linelist", "bool") + if default is NOT_SET: + default = get_ini_default_for_type(type) + self._inidict[name] = (help, type, default) self._ininames.append(name) +def get_ini_default_for_type( + type: Optional[Literal["string", "paths", "pathlist", "args", "linelist", "bool"]] +) -> Any: + """ + Used by addini to get the default value for a given ini-option type, when + default is not supplied. + """ + if type is None: + return "" + elif type in ("paths", "pathlist", "args", "linelist"): + return [] + elif type == "bool": + return False + else: + return "" + + class ArgumentError(Exception): """Raised if an Argument instance is created with invalid or inconsistent arguments.""" diff --git a/testing/test_config.py b/testing/test_config.py index ded30790188..b7c61feea98 100644 --- a/testing/test_config.py +++ b/testing/test_config.py @@ -5,6 +5,7 @@ import sys import textwrap from pathlib import Path +from typing import Any from typing import Dict from typing import List from typing import Sequence @@ -21,6 +22,7 @@ from _pytest.config import ConftestImportFailure from _pytest.config import ExitCode from _pytest.config import parse_warning_filter +from _pytest.config.argparsing import get_ini_default_for_type from _pytest.config.exceptions import UsageError from _pytest.config.findpaths import determine_setup from _pytest.config.findpaths import get_common_ancestor @@ -857,6 +859,68 @@ def pytest_addoption(parser): assert len(values) == 2 assert values == ["456", "123"] + def test_addini_default_values(self, pytester: Pytester) -> None: + """Tests the default values for configuration based on + config type + """ + + pytester.makeconftest( + """ + def pytest_addoption(parser): + parser.addini("linelist1", "", type="linelist") + parser.addini("paths1", "", type="paths") + parser.addini("pathlist1", "", type="pathlist") + parser.addini("args1", "", type="args") + parser.addini("bool1", "", type="bool") + parser.addini("string1", "", type="string") + parser.addini("none_1", "", type="linelist", default=None) + parser.addini("none_2", "", default=None) + parser.addini("no_type", "") + """ + ) + + config = pytester.parseconfig() + # default for linelist, paths, pathlist and args is [] + value = config.getini("linelist1") + assert value == [] + value = config.getini("paths1") + assert value == [] + value = config.getini("pathlist1") + assert value == [] + value = config.getini("args1") + assert value == [] + # default for bool is False + value = config.getini("bool1") + assert value is False + # default for string is "" + value = config.getini("string1") + assert value == "" + # should return None if None is explicity set as default value + # irrespective of the type argument + value = config.getini("none_1") + assert value is None + value = config.getini("none_2") + assert value is None + # in case no type is provided and no default set + # treat it as string and default value will be "" + value = config.getini("no_type") + assert value == "" + + @pytest.mark.parametrize( + "type, expected", + [ + pytest.param(None, "", id="None"), + pytest.param("string", "", id="string"), + pytest.param("paths", [], id="paths"), + pytest.param("pathlist", [], id="pathlist"), + pytest.param("args", [], id="args"), + pytest.param("linelist", [], id="linelist"), + pytest.param("bool", False, id="bool"), + ], + ) + def test_get_ini_default_for_type(self, type: Any, expected: Any) -> None: + assert get_ini_default_for_type(type) == expected + def test_confcutdir_check_isdir(self, pytester: Pytester) -> None: """Give an error if --confcutdir is not a valid directory (#2078)""" exp_match = r"^--confcutdir must be a directory, given: "