Feed a test only with initialnames, not the whole fixture closure

[sadra-barikbin]

To feed a test func only with its _fixtureinfo.initialnames which contains its argnames, usefixtures marks and autouse fixtures, not the whole fixture closure which might seem unexpected to the user.

[sadra-barikbin]

test / build (plugins) job fails on pytest_asyncio's test_sleep as the plugin uses funcargs["event_loop"] in a hook implementation while event_loop isn't requested, so it's not located in funcargs. pytest-asyncio manually adds event_loop fixture to the fixtureclosure of the test if it's async. Instead of funcargs["event_loop"], it could use _request.getfixturevalue() or keep it but add usefixtures("event_loop") mark to the test function in their current pytest_pycollect_makeitem hook implementation so that event_loop comes into funcargs.

[bluetech]

I am in favor of this change in principle, the idea being that a fixture should only be "available" to a test (here as item.funcargs["foo"]) if the test actually requests it itself, through a direct argname, usefixture or autouse. While transitive fixtures are "implementation details". If a test wants to access a fixture it did not request itself dynamically, should use request.getfixturevalue() instead.

The same argument also applies to item.fixturenames, which contains the full closure but IMO should only contain the initial names. But that is harder to change.

If we choose to do it, we ought to change the example in https://github.com/pytest-dev/pytest/blob/8032d212715108c5187e57b5fccdd2502e716410/doc/en/example/simple.rst#post-process-test-reports--failures to avoid item.fixturenames.

Even though funcargs isn't documented in the API Reference, it is mentioned in the docs and as such I'd say this is a breaking change and should go through a deprecation cycle (warn on access to a non-initial name) and only changed in the next major release (i.e. pytest 9). Overall I think it's not worth the effort but if you want to do it, 👍 from me.

[sadra-barikbin]

@bluetech, Complying with the deprecation cycle, now it raises a warning when user accesses a name in item.funcargs other than initialnames(argnames+usefixture+autouse). I also changed the rst file that you said. Does it look good?
Shall I create an issue for the rest of the job with milestone ==9 ?

[jgersti]

There are a few plugins, e.g. pytest-lazy-fixture, pytest-cases that manipulate item.funcargs to inject (lazy/transitive fixture) values. Will this change will eventually break them?

I am also slightly confused. What is the point of TopRequest._fillfixtures now, since it fills funcargs with all values for the whole fixture closure.

[RonnyPfannschmidt]

Well,those are hacks and they really need a better api from pytest,

As long as we keep the mess good things are blocked

Id like to push lazy fixture into pytest core and building blocks for cases as well

[jgersti]

Well,those are hacks and they really need a better api from pytest,

As long as we keep the mess good things are blocked

Id like to push lazy fixture into pytest core and building blocks for cases as well

If would be interested in helping to the best of my abilities, especially since the author of pytest-cases has not been active in a while. Is there a disussion thread or a roadmap/design considerations documents where i could add some notes?

[RonnyPfannschmidt]

Currently not

[bluetech]

Thanks @sadra-barikbin, I left some comments. Also, this also needs a changelog entry and a note in doc/en/deprecations.rst.

Before approving, we should check @jgersti comment regarding pytest-lazy-fixtures. Specifically how it uses funcargs and whether getfixturevalue is a viable replacement for it. See also @jgersti's post here: #11412

[jgersti]

Before approving, we should check @jgersti comment regarding pytest-lazy-fixtures. Specifically how it uses funcargs and whether getfixturevalue is a viable replacement for it. See also @jgersti's post here: #11412

While writing that post I checked the pytest-lazy-fixture code again and I think the code is fine as only iterators and __setitem__ are used. pytest-cases replaces the dict so would disable the deprecation.

[bluetech]

@jgersti Only had a quick look now at lazy-fixtures, I do think this will break (after the change is done fully):

https://github.com/TvoroG/pytest-lazy-fixture/blob/18ec85edb5e27c933733f748c685b2fd083198d7/pytest_lazyfixture.py#L50-L54

That's assuming is_lazy_fixture can be true for transitive fixtures.

[jgersti]

@jgersti Only had a quick look now at lazy-fixtures, I do think this will break (after the change is done fully):

https://github.com/TvoroG/pytest-lazy-fixture/blob/18ec85edb5e27c933733f748c685b2fd083198d7/pytest_lazyfixture.py#L50-L54

That's assuming is_lazy_fixture can be true for transitive fixtures.

If assignments into funcargs will still be allowed everything should continue to work as before. If only modification of existing entries is allowed, some minor patches are needed, but I think it should still continue to work because all entires for transitive fixtures in the dict are never read by pytest itself. If no modification of the dict will be allowed whatsoever the plugin will be unfixably broken since there is currently no other way to resolve the LazyFixture instances to the fixture values.

[bluetech]

If assignments into funcargs will still be allowed everything should continue to work as before.

It will, at least this PR won't change that. But can you explain how it wouldn't break? The code I linked loops over all of item.funcargs, which currently includes transitive fixtures. Are you saying the loop would be OK also if only requested fixtures are included?

[jgersti]

While looking through pytest i found only one location where item.funcargs is read and that was to fill the initial test function call.
Additionally the lines you hightlighted can be removed without impacting the testsuite of the plugin.

[bluetech]

Left a few comments.

The branch has some conflicts and needs a rebase. Note: prefer to rebase than to merge main, the merging makes my head spin :)

We need to think what to do about other forms of access to the dict, like iterating, setting and deleting. Out of 670 plugins I have checked out locally, 42 use funcargs, including some popular plugins like pytest-benchmark. From a quick look, it seems like at least some of them really do want to check if a fixture is in the full closure, not just in the directly-requested set. Needs an audit of the usages...

[bluetech]

I am worried about the performance implication of this - initialnames can get lengthy with a lot of autouse fixtures and this is an O(N) search. The easiest thing is to save the initialnames as a set instead of a tuple, though this has a performance cost as well.

Maybe can do something like this: Change fillfixtures to create two dicts, one with the initialnames only and one with the rest, then use a ChainMap instead of a Dict. This is just a thought, not sure how it would work out in practice.

[sadra-barikbin]

How is to add a flag attribute to DeprecatingFuncArgs in order to raise the warning once hence looking self.initialnames up only once? Like what it is like now.

[bluetech]

Haven't looked at the code yet, but from the description: for good-behaving test suites (hopefully the majority) the warning should never be emitted, in which case it will still be costly. So I don't think it's a good solution for the performance issue (though only warning once might be a good idea regardless).

[sadra-barikbin]

Left a few comments.

The branch has some conflicts and needs a rebase. Note: prefer to rebase than to merge main, the merging makes my head spin :)

We need to think what to do about other forms of access to the dict, like iterating, setting and deleting. Out of 670 plugins I have checked out locally, 42 use funcargs, including some popular plugins like pytest-benchmark. From a quick look, it seems like at least some of them really do want to check if a fixture is in the full closure, not just in the directly-requested set. Needs an audit of the usages...

Generally speaking, we could provide the user with following solutions, given his/her requirements:

Requirement	Solution
Delete a transitive fixture from funcargs	There's no such fixture there already
Setting a fixture in funcargs	It's possible to do so as before
Checking if a transitive fixture is in the closure	Use item.fixturenames

[sadra-barikbin]

@bluetech , CI fails due to a reason unknown to me. It complains about the hypothesis's deadline for testing/python/metafunc.py::TestMetafunc::test_idval_hypothesis which this PR has seemingly nothing to do with.

[bluetech]

Yep it's unrelated, see #11825 (comment)

[bluetech]

For reference, here are the usages of funcargs I've found in my "plugin corpus" (I tried to remove irrelevant matches):

Details

pytest-docker-tools/pytest_docker_tools/plugin.py
31:    if "request" not in item.funcargs:
34:    for name, fixturedef in item.funcargs["request"]._fixture_defs.items():

pytest-monitor/pytest_monitor/pytest_monitor.py
199:            funcargs = pyfuncitem.funcargs
200:            testargs = {arg: funcargs[arg] for arg in pyfuncitem._fixtureinfo.argnames}

pytest-aio/pytest_aio/plugin.py
51:    backend: Tuple[str, Dict] = pyfuncitem.funcargs.get("aiolib")  # type: ignore
68:        funcargs = pyfuncitem.funcargs
69:        testargs = {arg: funcargs[arg] for arg in pyfuncitem._fixtureinfo.argnames}

pytest_marker_bugzilla/pytest_marker_bugzilla.py
152:        bugs = item.funcargs["bugs"]
229:                item.funcargs["bugs"] = cache[bugs]

pytest-benchmark/src/pytest_benchmark/plugin.py
475:    fixture = hasattr(item, 'funcargs') and item.funcargs.get('benchmark')

pytest-twisted/pytest_twisted.py
410:        for name, value in pyfuncitem.funcargs.items()

pytest-play/pytest_play/plugin.py
104:        self.funcargs = {}
107:                                              cls=None, funcargs=False)

pytest-flakefinder/pytest_flakefinder.py
88:                    # without this, funcargs ends up being None

python-pytest-cases/src/pytest_cases/plugin.py
69:    # now item.funcargs exists so we can handle it
70:    if hasattr(item, "funcargs"):
71:        item.funcargs = {argname: get_lazy_args(argvalue, item)
72:                         for argname, argvalue in item.funcargs.items()}
1031:            print("\n".join(["%s[%s]: funcargs=%s, params=%s" % (get_pytest_nodeid(self.metafunc),
1032:                                                                 c.id, c.funcargs, c.params)
1104:            if fixture not in c.params and fixture not in c.funcargs:
1129:            if fixture_name not in c.params and fixture_name not in c.funcargs:
1153:    #         if fixture_name in c.params or fixture_name in c.funcargs or n.requires(fixture_name):
1226:#             if fixture_name not in c.params and fixture_name not in c.funcargs:
1256:#             if fixture_name not in c.params and fixture_name not in c.funcargs:

python-pytest-cases/src/pytest_cases/common_pytest.py
693:            self.required_fixtures = tuple(f for f in self.fixturenames if f not in self._calls[0].funcargs)

pytest-tornado/pytest_tornado/plugin.py
91:        io_loop = pyfuncitem.funcargs.get('io_loop')
94:        funcargs = dict((arg, pyfuncitem.funcargs[arg])
98:            future = tornado.gen.convert_yielded(coroutine(**funcargs))
101:            future = coroutine(**funcargs)

pytest-android/src/pytest_android/hooks.py
19:        for k, v in node.funcargs.items():

pytest-wdl/pytest_wdl/loader.py
102:        self.funcargs = {}

pytest-testrail-client/pytest_testrail_client/pytest_testrail_client.py
493:    for key, value in request.node.funcargs.items():

pytest-golden/pytest_golden/plugin.py
384:    fixt = item.funcargs.get(FIXTURE_NAME)

pytest-xlog/src/pytest_xlog/plugin.py
104:                if params == list(item.funcargs.values()):

pytest-github/pytest_github/plugin.py
291:        issue_urls = item.funcargs["github_issues"]
325:        if marker is not None and hasattr(item, 'funcargs'):
340:            item.funcargs["github_issues"] = issue_urls

pytest-airflow/pytest_airflow/plugin.py
519:        funcargs = pyfuncitem.funcargs
525:                testkwargs[arg] = funcargs[arg]

pytest-lazy-fixture/pytest_lazyfixture.py
36:                elif param not in item.funcargs:
37:                    item.funcargs[param] = request.getfixturevalue(param)
51:    if hasattr(item, 'funcargs'):
52:        for arg, val in item.funcargs.items():
54:                item.funcargs[arg] = item._request.getfixturevalue(val.name)
74:    normalize_metafunc_calls(metafunc, 'funcargs')
120:                                  if fname not in callspec.params and fname not in callspec.funcargs]

pytest-leaks/pytest_leaks/plugin.py
154:                item.funcargs = None

pytest-failed-screenshot/pytest_failed_screenshot.py
44:        for value in item.funcargs.values():

pytest-aws/conftest.py
286:def get_metadata_from_funcargs(funcargs):
288:    for k in funcargs:
289:        if isinstance(funcargs[k], dict):
290:            metadata = {**metadata, **extract_metadata(funcargs[k])}
342:        metadata = get_metadata_from_funcargs(item.funcargs)

pytest-curio/pytest_curio/plugin.py
27:        kernel = pyfuncitem.funcargs['kernel']
28:        funcargs = pyfuncitem.funcargs
29:        testargs = {arg: funcargs[arg] for arg in pyfuncitem._fixtureinfo.argnames}

pytest-tornasync/src/pytest_tornasync/plugin.py
41:    funcargs = pyfuncitem.funcargs
42:    testargs = {arg: funcargs[arg] for arg in pyfuncitem._fixtureinfo.argnames}
49:        loop = funcargs["io_loop"]

pytest-molecule/src/pytest_molecule/__init__.py
169:        self.funcargs = {}

pytest-sanic/pytest_sanic/plugin.py
71:        loop = pyfuncitem.funcargs[LOOP_KEY]
72:        funcargs = pyfuncitem.funcargs
75:            testargs[arg] = funcargs[arg]

pytest-yield/pytest_yield/plugin.py
204:            item.funcargs = None
243:        funcargs = pyfuncitem.funcargs
246:            testargs[arg] = funcargs[arg]

pytest-scenario/pytest_scenario/plugin.py
92:                    item.funcargs[argname] = item._request.getfuncargvalue(func)

pytest-plugins/pytest-webdriver/pytest_webdriver.py
97:    if not hasattr(item, 'funcargs') or not 'webdriver' in item.funcargs:
104:        item.funcargs['webdriver'].get_screenshot_as_file(fname)

pytest-trello/pytest_trello/plugin.py
143:        cards = item.funcargs.get('cards', [])
243:        cards = item.funcargs["cards"]
272:            item.funcargs["cards"] = TrelloCardList(self.api, *cards, **marker.kwargs)

pytest-reraise/pytest_reraise/reraise.py
129:    if hasattr(item, "funcargs") and "reraise" in item.funcargs:
130:        reraise = item.funcargs["reraise"]

pigeonhole/pigeonhole/plugin.py
81:            report.pigeonhole_index = item.funcargs.get(self._fixture_name, NOT_APPLICABLE)

pytest-ansible/src/pytest_ansible/molecule.py
135:        self.funcargs = {}

pytest-count/pytest_count.py
56:            extra = ' (%s)' % item.funcargs['tmpdir']