Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 33 additions & 2 deletions src/_pytest/config/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -915,6 +915,25 @@ def _preparse(self, args, addopts=True):
else:
raise

if ns.override_ini:
for ini_config in ns.override_ini:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: can we please move all the validation code to a separate function, and make that function reuse the work done by _validate_ini_value to avoid duplication?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe, but I would also rather like to keep this "minimal" / "a no brainer" patch for now.
Can be refactored in the end maybe then.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yup, please add a follow-up issue

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure.

try:
key, user_ini_value = ini_config.split("=", 1)
except ValueError:
raise UsageError("-o/--override-ini expects option=value style.")
else:
try:
_, type, _, choices = self._parser._inidict[key]
except KeyError:
raise UsageError(
"unknown configurations ini value: {!r}".format(key)
)
if type == "choice":
if user_ini_value not in choices:
raise UsageError(
"invalid value for {}: {!r}".format(key, user_ini_value)
)

def _checkversion(self):
import pytest

Expand Down Expand Up @@ -978,7 +997,7 @@ def getini(self, name: str):

def _getini(self, name: str) -> Any:
try:
description, type, default = self._parser._inidict[name]
description, type, default, choices = self._parser._inidict[name]
except KeyError:
raise ValueError("unknown configuration value: {!r}".format(name))
value = self._get_override_ini_value(name)
Expand All @@ -1003,6 +1022,8 @@ def _getini(self, name: str) -> Any:
return [t for t in map(lambda x: x.strip(), value.split("\n")) if t]
elif type == "bool":
return bool(_strtobool(value.strip()))
elif type == "choice":
return self._validate_ini_value(name, value)
else:
assert type is None
return value
Expand Down Expand Up @@ -1033,7 +1054,17 @@ def _get_override_ini_value(self, name: str) -> Optional[str]:
raise UsageError("-o/--override-ini expects option=value style.")
else:
if key == name:
value = user_ini_value
value = self._validate_ini_value(name, user_ini_value)
return value

def _validate_ini_value(self, name: str, value: str) -> str:
try:
_, type, _, choices = self._parser._inidict[name]
except KeyError:
raise UsageError("unknown configurations ini value: {!r}".format(name))
if type == "choice":
if value not in choices:
raise UsageError("invalid value for {}: {!r}".format(name, value))
return value

def getoption(self, name: str, default=notset, skip: bool = False):
Expand Down
24 changes: 15 additions & 9 deletions src/_pytest/config/argparsing.py
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,9 @@ def __init__(self, usage=None, processopt=None):
self._groups = [] # type: List[OptionGroup]
self._processopt = processopt
self._usage = usage
self._inidict = {} # type: Dict[str, Tuple[str, Optional[str], Any]]
self._inidict = (
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this have grown enough to deserve a namedtuple or attrs instance. What do you think?

{}
) # type: Dict[str, Tuple[str, Optional[str], Any, Optional[List]]]
self._ininames = [] # type: List[str]
self.extra_info = {} # type: Dict[str, Any]

Expand Down Expand Up @@ -65,9 +67,8 @@ def addoption(self, *opts, **attrs):
""" register a command line option.

:opts: option names, can be short or long options.
:attrs: same attributes which the ``add_option()`` function of the
`argparse library
<http://docs.python.org/2/library/argparse.html>`_
:attrs: same attributes which the ``add_argument()`` function of the
`argparse library <https://docs.python.org/3.7/library/argparse.html>`_
accepts.

After command line parsing options are available on the pytest config
Expand Down Expand Up @@ -127,19 +128,24 @@ def parse_known_and_unknown_args(
args = [str(x) if isinstance(x, py.path.local) else x for x in args]
return optparser.parse_known_args(args, namespace=namespace)

def addini(self, name, help, type=None, default=None):
def addini(self, name, help, type=None, default=None, choices=None):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's take this opportunity and make choices keyword-only

Suggested change
def addini(self, name, help, type=None, default=None, choices=None):
def addini(self, name, help, type=None, default=None, *, choices=None):

""" register an ini-file option.

:name: name of the ini-variable
:type: type of the variable, can be ``pathlist``, ``args``, ``linelist``
or ``bool``.
:type: type of the variable, can be ``pathlist``, ``args``, ``linelist``,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
:type: type of the variable, can be ``pathlist``, ``args``, ``linelist``,
:type: type of the variable, can be ``"pathlist"``, ``"args"``, ``"linelist"``,

``bool``, or ``type``.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
``bool``, or ``type``.
``"bool"``, or ``"choice"``.

?

:default: default value if no ini-file option exists but is queried.

The value of ini-variables can be retrieved via a call to
:py:func:`config.getini(name) <_pytest.config.Config.getini>`.
"""
assert type in (None, "pathlist", "args", "linelist", "bool")
self._inidict[name] = (help, type, default)
# TODO: callable type?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure we need to make this a callable, after all the user can always call the callable before passing the choices to this function... or do you have a specific use case in mind?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was meant to work like type in argparse then - which is more universal than having choices (only).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I thought you meant choices being able to be a callable, my bad.

We can add type here no problem, following the same semantics that type and choices have in argparser. I will leave this up to you if you prefer to do this in this PR or do it separately.

if type == "choice":
if choices is None:
raise ValueError("need to pass choices with type=choice")
else:
assert type in (None, "pathlist", "args", "linelist", "bool")
self._inidict[name] = (help, type, default, choices)
self._ininames.append(name)


Expand Down
2 changes: 1 addition & 1 deletion src/_pytest/helpconfig.py
Original file line number Diff line number Diff line change
Expand Up @@ -152,7 +152,7 @@ def showhelp(config):
indent_len = 24 # based on argparse's max_help_position=24
indent = " " * indent_len
for name in config._parser._ininames:
help, type, default = config._parser._inidict[name]
help, type, default, choices = config._parser._inidict[name]
if type is None:
type = "string"
spec = "{} ({}):".format(name, type)
Expand Down
46 changes: 46 additions & 0 deletions testing/test_config.py
Original file line number Diff line number Diff line change
Expand Up @@ -365,6 +365,52 @@ def pytest_addoption(parser):
config = testdir.parseconfig()
assert config.getini("strip") is bool_val

def test_addini_choices(self, testdir):
testdir.makeconftest(
"""
def pytest_addoption(parser):
parser.addini("mychoice", "", type="choice", default="def", choices=["one", "two"])
"""
)
config = testdir.parseconfig()
assert config.getini("mychoice") == "def"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the user should explicitly state "def" in the choices list.


testdir.makeini(
"""
[pytest]
mychoice=invalid
"""
)
config = testdir.parseconfig()
with pytest.raises(UsageError, match="invalid value for mychoice: 'invalid'"):
config.getini("mychoice")

p1 = testdir.makepyfile(
"""
def test_pass(pytestconfig):
ini_val = pytestconfig.getini("mychoice")
print('\\nmychoice:%s\\n' % ini_val)"""
)
result = testdir.runpytest("-s", str(p1))
result.stdout.fnmatch_lines(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to be tested by the previous with pytest.raises already

["E *UsageError: invalid value for mychoice: 'invalid'", "*= 1 failed in *"]
)
assert result.ret == ExitCode.TESTS_FAILED

p1 = testdir.makepyfile(
Copy link
Member

@nicoddemus nicoddemus Oct 22, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I usually prefer to set the data in some module (say sys) and inspect it later:

        monkeypatch.setattr(sys, "MY_INI_VAL", None, raising=False)
        p1 = testdir.makepyfile(
            """
            import sys
            def test_pass(pytestconfig):
                ini_val = pytestconfig.getini("mychoice")
                sys.MY_INI_VAL = pytestconfig.getini("mychoice")
                print('\\nmychoice:%s\\n' % ini_val)"""
        )
        result = testdir.runpytest("-s", "-o", "mychoice=one", str(p1))
        assert sys.MY_INI_VAL == "one"
        assert result.ret == ExitCode.OK

But it is merely a comment in case you like this approach.

"""
def test_pass(pytestconfig):
ini_val = pytestconfig.getini("mychoice")
print('\\nmychoice:%s\\n' % ini_val)"""
)
result = testdir.runpytest("-s", "-o", "mychoice=one", str(p1))
result.stdout.fnmatch_lines(["mychoice:one"])
assert result.ret == ExitCode.OK

result = testdir.runpytest("-o", "mychoice=invalid2", str(p1))
result.stderr.fnmatch_lines(["ERROR: invalid value for mychoice: 'invalid2'"])
assert result.ret == ExitCode.USAGE_ERROR

def test_addinivalue_line_existing(self, testdir):
testdir.makeconftest(
"""
Expand Down