Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move fixtures.py::add_funcarg_pseudo_fixture_def to Metafunc.parametrize #11220

Conversation

sadra-barikbin
Copy link
Contributor

@sadra-barikbin sadra-barikbin commented Jul 17, 2023

  • To remove fixtures.py::add_funcargs_pseudo_fixture_def and add its logic i.e. registering funcargs as params and making corresponding fixturedefs, right to Metafunc.parametrize in which parametrization takes place.
  • To compute param indices smarter to have a better reordering experience. Moved to Resolve param indices using param values, not parameterset index #11257
  • To remove funcargs from metafunc attributes as we populate metafunc params and make pseudo fixturedefs simultaneously and there's no need to keep funcargs separately.

@sadra-barikbin sadra-barikbin changed the title "Move add_pseudo_funcarg_fixturedef to Metafunc.parametrize" Move fixtures.py::add_funcarg_pseudo_fixture_def to Metafunc.parametrize Jul 17, 2023
@sadra-barikbin sadra-barikbin marked this pull request as ready for review July 19, 2023 00:39
if name2pseudofixturedef is not None and argname in name2pseudofixturedef:
fixturedef = name2pseudofixturedef[argname]
else:
fixturedef = FixtureDef(
Copy link
Member

Choose a reason for hiding this comment

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

this particular usage indicates to me that we may want to investigate having a "constant" definitions that could be used to represent params both from parametrize, and also declaring fixtures that repressent sets of values

details are for a followup that may want to also integrate pytest-lazyfixtures

Copy link
Contributor Author

Choose a reason for hiding this comment

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

By "constant" you mean stateless? A fixturedef that just returns its input?

Copy link
Member

Choose a reason for hiding this comment

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

Exact

Copy link
Member

@bluetech bluetech left a comment

Choose a reason for hiding this comment

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

Hi @sadra-barikbin, I left some initial comments.

To remove fixtures.py::add_funcargs_pseudo_fixture_def and add its logic
To compute param indices smarter to have a better reordering experience

Are these two changes dependent on each other? It will help the review a lot if the two changes can be considered separately.

src/_pytest/python.py Show resolved Hide resolved
src/_pytest/fixtures.py Outdated Show resolved Hide resolved
src/_pytest/python.py Show resolved Hide resolved
src/_pytest/python.py Outdated Show resolved Hide resolved
src/_pytest/python.py Outdated Show resolved Hide resolved
src/_pytest/python.py Outdated Show resolved Hide resolved

argnames = ["A", "B", "C"]
parametersets = [("a1", "b1", "c1"), ("a1", "b2", "c1"), ("a1", "b3", "c2")]
result = [(0, 0, 0), (0, 1, 0), (0, 2, 1)]
Copy link
Member

Choose a reason for hiding this comment

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

I might be misunderstanding, but shouldn't the last triplet be (0, 2, 2) (i.e. the index of c2 should be 2 not 1)?

Copy link
Contributor Author

@sadra-barikbin sadra-barikbin Jul 29, 2023

Choose a reason for hiding this comment

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

It should be 1 as c2 is the second value in C unique values. In other words, its index is not determined by the index of the parameter set, but its place in the unique values of the arg.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Related to #11257

testing/python/metafunc.py Outdated Show resolved Hide resolved
@@ -1052,6 +973,9 @@ def __init__(
self.cached_result: Optional[_FixtureCachedResult[FixtureValue]] = None
self._finalizers: Final[List[Callable[[], object]]] = []

# Whether fixture is a pseudo-fixture made in direct parametrizations.
Copy link
Member

Choose a reason for hiding this comment

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

I understand this terminology is preexisting, but I don't like it much, "pseudo" is not very descriptive... Also, there's actually a PseudoFixtureDef already which is something else entirely and which only adds to the confusion :)

Anyway we can clean the terms later, you don't need to change it.

Copy link
Member

Choose a reason for hiding this comment

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

While we are at it, perhaps is_parametrized would be a good fit?

Copy link
Member

Choose a reason for hiding this comment

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

Also do we want this to be public, or prefix it with _ to be sure?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How is to introduce an IdentityFixture class and use isinstance instead of adding and checking is_pseudo?

class IdentityFixture(FixtureDef):
    def __init__(fixturemanager, argname, scope):
        super().__init__(fixturemanager, "", argname, lambda request: request.param, scope, None, _ispytest=True)

This also accounts for @RonnyPfannschmidt 's comment. Just we should remove @final from FixtureDef class.

Or simply introduce is_identity? @nicoddemus , Couldn't is_parametrized be confusing, since such a fixture is not really parametrized (using @pytest.fixture(params=) or @pytest.mark.parametrize(indirect=True) but the test is directly parametrized?

Copy link
Member

Choose a reason for hiding this comment

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

Hmm that's a good suggestion; my gut reaction is that I like this idea of having IdentityFixture (or some other name in case others have a suggestion). Let's see what others think.

Copy link
Member

Choose a reason for hiding this comment

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

i'd like to note that we do have a "spagetty-fied" pattern here about configuring a "dependency injection container" with either direct values or parameters to "factories"

if we introduce polymorphism, we might want to make "identity" and "from factory, possibly with parameters" siblings and check if we can get rid of those is_pseudo checks altogether

there is a rat-tail of discussions to be had around this, so we ought to consider it but without going down the rabbit hole too deep

Copy link
Contributor Author

@sadra-barikbin sadra-barikbin Jul 29, 2023

Choose a reason for hiding this comment

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

As params arg of this very fixturedef is None, it's automatically ruled out in the only place (fixtures.py::pytest_generate_tests) where I used is_pseudo. So the need for is_pseudo attribute is eliminated.

Copy link
Member

@bluetech bluetech left a comment

Choose a reason for hiding this comment

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

Thanks for splitting @sadra-barikbin. I left some more comments.

Overall the moving of add_funcarg_pseudo_fixture_def into Metafunc itself looks good to me, it's a good stepping-stone simplification.

I'd still like to understand the change in the issue519 test better.

Also, the removal of CallSpec2.funcargs is going to break some plugins, we'll need to analyze and mitigate and/or document it properly before merging. Since add_funcarg_pseudo_fixture_def always clears funcargs I'm not sure how useful it is, but maybe plugins hook to it before it is cleared.

src/_pytest/python.py Outdated Show resolved Hide resolved
src/_pytest/python.py Outdated Show resolved Hide resolved
src/_pytest/python.py Outdated Show resolved Hide resolved
src/_pytest/python.py Outdated Show resolved Hide resolved
name2pseudofixturedef = node.stash.setdefault(
name2pseudofixturedef_key, default
)
arg_values_types = self._resolve_arg_value_types(argnames, indirect)
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 it would be helpful for clarity if (in separate PR) we change _resolve_arg_value_types like so:

  • Rename to _resolve_arg_directness - "arg value type" is pretty generic/non-specific and doesn't say much.
  • Change the Literal return value from Literal['params', 'funcargs'] to Literal['direct', 'indirect'].

Then the check becomes if arg_directness[argname] == "direct": continue which is pretty self-explanatory I think.

Note: we can do it later, you don't have to do it.

testing/python/metafunc.py Outdated Show resolved Hide resolved
testing/python/metafunc.py Show resolved Hide resolved
testing/python/metafunc.py Outdated Show resolved Hide resolved
@@ -22,13 +22,13 @@ def checked_order():
assert order == [
("issue_519.py", "fix1", "arg1v1"),
("test_one[arg1v1-arg2v1]", "fix2", "arg2v1"),
("test_two[arg1v1-arg2v1]", "fix2", "arg2v1"),
Copy link
Member

Choose a reason for hiding this comment

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

As I said in https://github.com/pytest-dev/pytest/pull/11231/files#r1278270549 IMO this change makes sense (though @RonnyPfannschmidt may still disagree).

But I would like to understand what exactly in this PR is causing this change? @sadra-barikbin do you think you can explain it?

Copy link
Member

Choose a reason for hiding this comment

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

i left another comment, i think we can choose to opt for the new ordering if we ensure to add the tools i mentioned there eventually

Copy link
Contributor Author

@sadra-barikbin sadra-barikbin Aug 1, 2023

Choose a reason for hiding this comment

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

Current ordering stems from the way add_funcarg_pseudo_fixture_def assigns index to params. It assigns i to the index of all of the params in ith callspec of the metafunc. In our example, test_one and test_two, each have 4 callspecs and two params (arg1 and arg2). We assign index 0 to arg1 and arg2 of test_one[arg1v1,arg2v1] and also to those of test_two[arg1v1,arg2v1]. We assign index 1 to arg1 and arg2 of test_one[arg1v1,arg2v2] and also to those of test_two[arg1v1,arg2v2] and so on. As a result, ith callspecs of the two metafuncs would have common fixture keys. In our example, kth test_one pulls kth test_two towards itself, so we would have a one-two-one-two... pattern. Also test_one (also test_two ones) items would have no common fixture key because their index of arg1 and arg2 is 0,1,2 and 3.

The new way assigns i to the index of param j in all callspecs that use ith value of j. So the decision is made on a per-param basis. In our example, we would assign index 0 to arg1 of the items that use arg1v1 and index 1 to arg1 of the items that use arg1v2 and so on. As a result, if two callspecs have the ith value of the param j, they have a common fixture key.

The new way is improved in determining what ith value of the param is when there are multiple parametersets in #11257 .

Copy link
Member

Choose a reason for hiding this comment

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

After reading your comment and looking more closely at the issue_519.py file, I think I understood some things:


First, I'm really not used to the pytest_generate_tests hook, so I rewrote the test using pytest.mark.parametrize. I also changed some of the names to make things clearer to me:

Rewritten issue_519.py
import pprint
from typing import List
from typing import Tuple

import pytest


@pytest.fixture(scope="session")
def checked_order():
    order: List[Tuple[str, str, str]] = []

    yield order
    pprint.pprint(order)


@pytest.fixture(scope="module")
def fixmod(request, a, checked_order):
    checked_order.append((request.node.name, "fixmod", a))
    yield "fixmod-" + a


@pytest.fixture(scope="function")
def fixfun(request, fixmod, b, checked_order):
    checked_order.append((request.node.name, "fixfun", b))
    yield "fixfun-" + b + fixmod


@pytest.mark.parametrize("b", ["b1", "b2"], scope="function")
@pytest.mark.parametrize("a", ["a1", "a2"], scope="module")
def test_one(fixfun, request):
    print()
    # print(request._pyfuncitem.nodeid, request._pyfuncitem.callspec)


@pytest.mark.parametrize("b", ["b1", "b2"], scope="function")
@pytest.mark.parametrize("a", ["a1", "a2"], scope="module")
def test_two(fixfun, request):
    print()
    # print(request._pyfuncitem.nodeid, request._pyfuncitem.callspec)

This made me realize that the apparent niceness of the previous ordering, which orders test_one[arg1v1-arg2v1] next to test_two[arg1v1-arg2v1], thus saving a setup, is pretty arbitrary. This can be shown by making the values of the function-scoped parameter (arg2/b) different in test_one and test_two:

-@pytest.mark.parametrize("b", ["b1", "b2"], scope="function")
+@pytest.mark.parametrize("b", ["b11", "b12"], scope="function")
 @pytest.mark.parametrize("a", ["a1", "a2"], scope="module")
 def test_one(fixfun, request):
     print()
     # print(request._pyfuncitem.nodeid, request._pyfuncitem.callspec)
 
 
-@pytest.mark.parametrize("b", ["b1", "b2"], scope="function")
+@pytest.mark.parametrize("b", ["b21", "b22"], scope="function")
 @pytest.mark.parametrize("a", ["a1", "a2"], scope="module")
 def test_two(fixfun, request):
     print()

The ordering stays the same, which now doesn't make sense.


I also figured that the entire param_index thing was much more useful/natural in the pre-parametrize times, when only pytest_generate_tests/metafunc was used for parametrizing. With pytest_generate_tests you often use the same parameter set for different tests, as is done in the issue_519.py file. But with parametrize, which is mostly done for a single test-function only, it is much less common, I think.

@sadra-barikbin sadra-barikbin force-pushed the Improvement-move-add-pseudo-funcarg-to-metafunc-parametrize branch from 9ac1f5c to 40b2c09 Compare August 1, 2023 10:15
Copy link
Member

@bluetech bluetech left a comment

Choose a reason for hiding this comment

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

(I still need to grok https://github.com/pytest-dev/pytest/pull/11220/files#r1280394308 but some comments in the meantime)

src/_pytest/python.py Show resolved Hide resolved
src/_pytest/python.py Outdated Show resolved Hide resolved
@@ -1503,6 +1548,31 @@ def test_it(x): pass
result = pytester.runpytest()
assert result.ret == 0

def test_parametrize_module_level_test_with_class_scope(
Copy link
Member

Choose a reason for hiding this comment

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

Thanks, I think this test is good now.

I suggest adding a docstring:

Test that a class-scoped parametrization without a corresponding Class gets module scope, i.e. we only create a single FixtureDef for it per module.

Copy link
Contributor Author

@sadra-barikbin sadra-barikbin Aug 2, 2023

Choose a reason for hiding this comment

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

By the way, Is it ok that the scope attribute of the created fixturedef would be Module/Class depending on whether the parametrized module-level test functions come first or such a class-less test?

Copy link
Member

Choose a reason for hiding this comment

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

I'm pretty sure that the fixturedef.scope needs to be class in this case, for things like the "can't request lower scope from higher scope" check, introspection, and probably other stuff. It seems safer to me to keep the original scope and do the "find the actual scope" dance only where needed.

Copy link
Contributor Author

@sadra-barikbin sadra-barikbin Aug 2, 2023

Choose a reason for hiding this comment

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

I mean if we swap test_1 and test_2, the scope attribute of x fixturedef would be module as test_2 is collected earlier then. Is this OK?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

However, It does not affect reordering since we use callspec._arg2scope for creating fixture keys.

Copy link
Member

Choose a reason for hiding this comment

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

I mean if we swap test_1 and test_2, the scope attribute of x fixturedef would be module as test_2 is collected earlier then. Is this OK?

It's dubious, I'd say it's a bug (preexisting of course).

testing/python/metafunc.py Outdated Show resolved Hide resolved
testing/python/metafunc.py Show resolved Hide resolved
testing/python/metafunc.py Outdated Show resolved Hide resolved
argname=argname,
func=get_direct_param_fixture_func,
scope=scope_,
params=None,
Copy link
Member

Choose a reason for hiding this comment

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

This is now params=None, where before it was params=valuelist. This made me confused somewhat.

My mental model for before was the basically desugar this:

@pytest.mark.parametrize("x", [1])
def test_it(x): pass

to this:

@pytest.fixture(params=[1])
def x(request):
    return request.param

def test_it(x): pass

but with params=None that makes less sense now. And indeed, if we change to params=None in the current code, all tests still pass, i.e. it wasn't needed even before.

I guess then that the correct mental model for the desugaring is indirect parametrization of the x fixture? I.e. this:

@pytest.fixture()
def x(request):
    return request.param

@pytest.mark.parametrize("x", [1], indirect=["x"])
def test_it(x): pass

Ran out of time to look into it today but will appreciate your insights :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right. It is the test that holds param values in its callspec and the fixtures that it depends on, use them at test execution time. The only usage of fixturedef.params is in fixtures.py::pytest_generate_tests that the directly parametrized fixture hand the params to the test that uses it. By directly parametrized fixture, I mean this:

@pytest.fixture(params=[1])
def x(request):
    return request.param

@@ -22,13 +22,13 @@ def checked_order():
assert order == [
("issue_519.py", "fix1", "arg1v1"),
("test_one[arg1v1-arg2v1]", "fix2", "arg2v1"),
("test_two[arg1v1-arg2v1]", "fix2", "arg2v1"),
Copy link
Member

Choose a reason for hiding this comment

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

After reading your comment and looking more closely at the issue_519.py file, I think I understood some things:


First, I'm really not used to the pytest_generate_tests hook, so I rewrote the test using pytest.mark.parametrize. I also changed some of the names to make things clearer to me:

Rewritten issue_519.py
import pprint
from typing import List
from typing import Tuple

import pytest


@pytest.fixture(scope="session")
def checked_order():
    order: List[Tuple[str, str, str]] = []

    yield order
    pprint.pprint(order)


@pytest.fixture(scope="module")
def fixmod(request, a, checked_order):
    checked_order.append((request.node.name, "fixmod", a))
    yield "fixmod-" + a


@pytest.fixture(scope="function")
def fixfun(request, fixmod, b, checked_order):
    checked_order.append((request.node.name, "fixfun", b))
    yield "fixfun-" + b + fixmod


@pytest.mark.parametrize("b", ["b1", "b2"], scope="function")
@pytest.mark.parametrize("a", ["a1", "a2"], scope="module")
def test_one(fixfun, request):
    print()
    # print(request._pyfuncitem.nodeid, request._pyfuncitem.callspec)


@pytest.mark.parametrize("b", ["b1", "b2"], scope="function")
@pytest.mark.parametrize("a", ["a1", "a2"], scope="module")
def test_two(fixfun, request):
    print()
    # print(request._pyfuncitem.nodeid, request._pyfuncitem.callspec)

This made me realize that the apparent niceness of the previous ordering, which orders test_one[arg1v1-arg2v1] next to test_two[arg1v1-arg2v1], thus saving a setup, is pretty arbitrary. This can be shown by making the values of the function-scoped parameter (arg2/b) different in test_one and test_two:

-@pytest.mark.parametrize("b", ["b1", "b2"], scope="function")
+@pytest.mark.parametrize("b", ["b11", "b12"], scope="function")
 @pytest.mark.parametrize("a", ["a1", "a2"], scope="module")
 def test_one(fixfun, request):
     print()
     # print(request._pyfuncitem.nodeid, request._pyfuncitem.callspec)
 
 
-@pytest.mark.parametrize("b", ["b1", "b2"], scope="function")
+@pytest.mark.parametrize("b", ["b21", "b22"], scope="function")
 @pytest.mark.parametrize("a", ["a1", "a2"], scope="module")
 def test_two(fixfun, request):
     print()

The ordering stays the same, which now doesn't make sense.


I also figured that the entire param_index thing was much more useful/natural in the pre-parametrize times, when only pytest_generate_tests/metafunc was used for parametrizing. With pytest_generate_tests you often use the same parameter set for different tests, as is done in the issue_519.py file. But with parametrize, which is mostly done for a single test-function only, it is much less common, I think.

@sadra-barikbin
Copy link
Contributor Author

By pre-parametrize times, you mean such a use-case

def pytest_generate_tests(metafunc):
    metafunc.parametrize("arg", ["v1","v2","v3"])

in which user aims to parametrize all/many of the tests in a module/class with the same param? If yes, you say param_index is more meaningful in such case because for example arg_index=0 means the same thing (v1) for all the tests?

@bluetech
Copy link
Member

bluetech commented Aug 9, 2023

Yes exactly, my guess is that it was much more common for parametrizations with the same argname to have the exact same parameter sets because it was just difficult to do otherwise. From that POV the param_index concept made some sense. Nowadays with pytest.mark.parameterize, less so.

Copy link
Member

@bluetech bluetech left a comment

Choose a reason for hiding this comment

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

OK, I admit I am not fully 100% on all of the implications but we covered everything I could figure out.

One thing I'm not very happy about is that previously Metafunc was "pure" in the sense that it did not have external side-effects, but now metafunc.parametrize registers the pseudo-fixtures. I think it would be less bug-prone to have Metafunc only collect the fixturedefs but leave the actual registering of them to _genfunctions. But this is not a blocker, maybe something for later.

So LGTM, thanks @sadra-barikbin!

src/_pytest/python.py Outdated Show resolved Hide resolved
@bluetech bluetech enabled auto-merge (squash) August 9, 2023 16:55
@bluetech bluetech merged commit 09b7873 into pytest-dev:main Aug 9, 2023
23 checks passed
@sadra-barikbin
Copy link
Contributor Author

sadra-barikbin commented Aug 9, 2023

OK, I admit I am not fully 100% on all of the implications but we covered everything I could figure out.

One thing I'm not very happy about is that previously Metafunc was "pure" in the sense that it did not have external side-effects, but now metafunc.parametrize registers the pseudo-fixtures. I think it would be less bug-prone to have Metafunc only collect the fixturedefs but leave the actual registering of them to _genfunctions. But this is not a blocker, maybe something for later.

So LGTM, thanks @sadra-barikbin!

Regarding the external side effects, the good news is that it's welll-known; Only when the parametrization is direct and its scope is higher than function. (which stashes the new fixturedef to a class/module/package/session node). Otherwise, only the metafunc itself is changed which was also the case before this PR.

RonnyPfannschmidt pushed a commit that referenced this pull request Aug 11, 2024
In #11220, an unintended change in reordering was introduced by changing the way indices were assigned to direct params. This PR reverts that change and reduces #11220 changes to just refactors.

After this PR we could safely decide on the solutions discussed in #12008, i.e. #12082 or the one initially introduced in #11220 .

Fixes #12008

Co-authored-by: Bruno Oliveira <[email protected]>
Co-authored-by: Bruno Oliveira <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants