From 03451c397f166b3b1999e49f190cd6880b5cf0a6 Mon Sep 17 00:00:00 2001 From: Ran Benita Date: Wed, 1 Apr 2020 18:22:15 +0300 Subject: [PATCH] Simplify positional arguments compatibility code in pytest.fixture() The dynamic scope feature added in 10bf6aac76d5060a0db4a94871d6dcf0a1 necessitated some wrangling of arguments in pytest.fixture(). In particular, it deprecated positional arguments in favor of keyword-only arguments, while keeping backward compatibility. The way it did this avoided some code duplication but ended up being quite hard to follow and to annotate with types. Replace it with some straightforward code, which is not very DRY but is simple and easy to remove when the time comes. --- src/_pytest/fixtures.py | 110 +++++++++++++++++-------------------- testing/python/fixtures.py | 39 ++++++++++++- 2 files changed, 87 insertions(+), 62 deletions(-) diff --git a/src/_pytest/fixtures.py b/src/_pytest/fixtures.py index 4c3d0d4bb..f547a7eab 100644 --- a/src/_pytest/fixtures.py +++ b/src/_pytest/fixtures.py @@ -1031,50 +1031,8 @@ class FixtureFunctionMarker: return function -FIXTURE_ARGS_ORDER = ("scope", "params", "autouse", "ids", "name") - - -def _parse_fixture_args(callable_or_scope, *args, **kwargs): - arguments = { - "scope": "function", - "params": None, - "autouse": False, - "ids": None, - "name": None, - } - kwargs = { - key: value for key, value in kwargs.items() if arguments.get(key) != value - } - - fixture_function = None - if isinstance(callable_or_scope, str): - args = list(args) - args.insert(0, callable_or_scope) - else: - fixture_function = callable_or_scope - - positionals = set() - for positional, argument_name in zip(args, FIXTURE_ARGS_ORDER): - arguments[argument_name] = positional - positionals.add(argument_name) - - duplicated_kwargs = {kwarg for kwarg in kwargs.keys() if kwarg in positionals} - if duplicated_kwargs: - raise TypeError( - "The fixture arguments are defined as positional and keyword: {}. " - "Use only keyword arguments.".format(", ".join(duplicated_kwargs)) - ) - - if positionals: - warnings.warn(FIXTURE_POSITIONAL_ARGUMENTS, stacklevel=2) - - arguments.update(kwargs) - - return fixture_function, arguments - - def fixture( - callable_or_scope=None, + fixture_function=None, *args, scope="function", params=None, @@ -1131,24 +1089,56 @@ def fixture( ``fixture_`` and then use ``@pytest.fixture(name='')``. """ + # Positional arguments backward compatibility. + # If a kwarg is equal to its default, assume it was not explicitly + # passed, i.e. not duplicated. The more correct way is to use a + # **kwargs and check `in`, but that obfuscates the function signature. + if isinstance(fixture_function, str): + # It's actually the first positional argument, scope. + args = (fixture_function, *args) + fixture_function = None + duplicated_args = [] + if len(args) > 0: + if scope == "function": + scope = args[0] + else: + duplicated_args.append("scope") + if len(args) > 1: + if params is None: + params = args[1] + else: + duplicated_args.append("params") + if len(args) > 2: + if autouse is False: + autouse = args[2] + else: + duplicated_args.append("autouse") + if len(args) > 3: + if ids is None: + ids = args[3] + else: + duplicated_args.append("ids") + if len(args) > 4: + if name is None: + name = args[4] + else: + duplicated_args.append("name") + if len(args) > 5: + raise TypeError( + "fixture() takes 5 positional arguments but {} were given".format(len(args)) + ) + if duplicated_args: + raise TypeError( + "The fixture arguments are defined as positional and keyword: {}. " + "Use only keyword arguments.".format(", ".join(duplicated_args)) + ) + if args: + warnings.warn(FIXTURE_POSITIONAL_ARGUMENTS, stacklevel=2) + # End backward compatiblity. + if params is not None: params = list(params) - fixture_function, arguments = _parse_fixture_args( - callable_or_scope, - *args, - scope=scope, - params=params, - autouse=autouse, - ids=ids, - name=name, - ) - scope = arguments.get("scope") - params = arguments.get("params") - autouse = arguments.get("autouse") - ids = arguments.get("ids") - name = arguments.get("name") - if fixture_function and params is None and autouse is False: # direct decoration return FixtureFunctionMarker(scope, params, autouse, name=name)( @@ -1159,7 +1149,7 @@ def fixture( def yield_fixture( - callable_or_scope=None, + fixture_function=None, *args, scope="function", params=None, @@ -1173,7 +1163,7 @@ def yield_fixture( Use :py:func:`pytest.fixture` directly instead. """ return fixture( - callable_or_scope, + fixture_function, *args, scope=scope, params=params, diff --git a/testing/python/fixtures.py b/testing/python/fixtures.py index bfbe35951..d9130d844 100644 --- a/testing/python/fixtures.py +++ b/testing/python/fixtures.py @@ -4155,7 +4155,7 @@ def test_fixture_named_request(testdir): ) -def test_fixture_duplicated_arguments(): +def test_fixture_duplicated_arguments() -> None: """Raise error if there are positional and keyword arguments for the same parameter (#1682).""" with pytest.raises(TypeError) as excinfo: @@ -4169,8 +4169,31 @@ def test_fixture_duplicated_arguments(): "Use only keyword arguments." ) + with pytest.raises(TypeError) as excinfo: -def test_fixture_with_positionals(): + @pytest.fixture( + "function", + ["p1"], + True, + ["id1"], + "name", + scope="session", + params=["p1"], + autouse=True, + ids=["id1"], + name="name", + ) + def arg2(request): + pass + + assert ( + str(excinfo.value) + == "The fixture arguments are defined as positional and keyword: scope, params, autouse, ids, name. " + "Use only keyword arguments." + ) + + +def test_fixture_with_positionals() -> None: """Raise warning, but the positionals should still works (#1682).""" from _pytest.deprecated import FIXTURE_POSITIONAL_ARGUMENTS @@ -4187,6 +4210,18 @@ def test_fixture_with_positionals(): assert fixture_with_positionals._pytestfixturefunction.autouse +def test_fixture_with_too_many_positionals() -> None: + with pytest.raises(TypeError) as excinfo: + + @pytest.fixture("function", [0], True, ["id"], "name", "extra") + def fixture_with_positionals(): + pass + + assert ( + str(excinfo.value) == "fixture() takes 5 positional arguments but 6 were given" + ) + + def test_indirect_fixture_does_not_break_scope(testdir): """Ensure that fixture scope is respected when using indirect fixtures (#570)""" testdir.makepyfile(