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

Indirect parametrization of fixture with multiple params with parametrize_with_cases #150

Closed
plammens opened this issue Dec 1, 2020 · 7 comments

Comments

@plammens
Copy link
Contributor

plammens commented Dec 1, 2020

I know this might not have been intended to be possible in the first place, but fixtures that take multiple parameters can't be indirectly parametrized with parametrize_with_cases:

import sys

import pytest_cases


@pytest_cases.fixture()
@pytest_cases.parametrize("param2", "abc")
@pytest_cases.parametrize("param1", [0, 1, 2])
def my_fixture(param1, param2):
    return param1, param2


@pytest_cases.parametrize_with_cases("my_fixture", cases=lambda: (42, "z"), indirect=True)
def test_foo(my_fixture):
    print(f" {my_fixture}", file=sys.stderr, end="")

This raises a ValueError before the fixture is invoked. (BTW this does work if the fixture has only a single parameter.)

This seems to be because the single `LazyValue` given by the case is iterpreted as if it were the tuple of parameters:

# populate the parameters
if len(params_names_or_name_combinations) == 1:
_params = [request.param] # remove the simplification
else:
_params = request.param
for p_names, fixture_param_value in zip(params_names_or_name_combinations, _params):

(in the above passage, request.param is a LazyValue).

Maybe a way to provide support for this would be to select the parameter of the fixture we want to parametrize in the parametrization target, e.g. with double colon notation as in "my_fixture::param1":

import sys

import pytest_cases


@pytest_cases.fixture()
@pytest_cases.parametrize("param2", "abc")
@pytest_cases.parametrize("param1", [0, 1, 2])
def my_fixture(param1, param2):
    return param1, param2


@pytest_cases.parametrize_with_cases("my_fixture::param2", cases=lambda: "z", indirect=True)
@pytest_cases.parametrize_with_cases("my_fixture::param1", cases=lambda: 42, indirect=True)
def test_foo(my_fixture):
    print(f" {my_fixture}", file=sys.stderr, end="")

This would run test_foo(42, "z") as the only invocation of test_foo.

I stumbled upon this while thinking of a workaround for #149 (fixtures with function scope do recompute values at every invocation, so I was thinking to use a fixture and indirectly parametrize that with the cases).

@smarie
Copy link
Owner

smarie commented Dec 1, 2020

Thanks @plammens ! In all honesty I do not like the indirect parameter at all. I do not think that is intuitive to understand, and it makes things extremely ugly. If a fixture is to be parametrized in two different ways for two different tests, I would create two fixture relying on the same code, or on the same root fixture.

Maybe I should raise a NotImplementedError when indirect=True is used ? But on the other hand, as you point out, it works when the fixture has a single parameter, so that would penalize those users relying on this (if any). Puzzling...

Feel free to dig in the code to see how to handle this - but I am afraid that this would require some very ugly hack, and there are already a few in pytest-cases :) for example a double-parametrized fixture is actually a single-parametrized fixture where I make myself the cartesian product of parameters, and implement it with a wrapper of the user-provided function...

@plammens
Copy link
Contributor Author

plammens commented Dec 1, 2020

Thanks @plammens ! In all honesty I do not like the indirect parameter at all. I do not think that is intuitive to understand, and it makes things extremely ugly. If a fixture is to be parametrized in two different ways for two different tests, I would create two fixture relying on the same code, or on the same root fixture.

Feel free to dig in the code to see how to handle this - but I am afraid that this would require some very ugly hack, and there are already a few in pytest-cases :) for example a double-parametrized fixture is actually a single-parametrized fixture where I make myself the cartesian product of parameters, and implement it with a wrapper of the user-provided function...

Yeah it makes sense it's not worth adding another hack—pytest in general is already full of hacks... Plus as you said there are other ways to achieve this (although making a new fixture just to get a specific subset of a broader fixture also feels a bit unsatisfying to me).

I agree that indirect is not the best of things, but I think it is useful if you view it as a way to use a specific set of "configurations" for a fixture that is unique to the test, while the "default" fixture provides generic instances. To clarify what I mean, here is how I used it in a personal project I've recently started (forgive me the ugly naming conventions).

Maybe I should raise a NotImplementedError when indirect=True is used ? But on the other hand, as you point out, it works when the fixture has a single parameter, so that would penalize those users relying on this (if any). Puzzling...

Yeah I think explicitly raising an error is a very good idea. I would say that probably there are very few people relying on it working for single parameter fixtures (although for better or for worse I myself recently used this "feature" 😅), and it isn't explicitly documented how indirect is handled: the documentation for pytest_cases.parametrize just says that the kwargs are passed on to the vanilla pytest.mark.parametrize, but it isn't clear what happens with indirect since pytest_cases changes a how parametrized fixtures work (i.e. normal parameters instead of request.param).

Maybe irrelevant, but it's possible to work around this by using the vanilla pytest.mark.fixture that uses request.param, and then make cases that explicitly generate tuples of arguments to pass as the request.param. Ugliness on top of ugliness! 😆

@plammens
Copy link
Contributor Author

plammens commented Dec 1, 2020

By the way, I've been looking around and you've got some really awesome libraries/packages! I'm repeatedly finding that you have already written something that makes my life easier in certain recurring programming scenarios. I originally discovered you through the python-ideas mailing list, and now I've recently started using valid8 and pytest-cases.

Huge thanks for putting the time into these and distributing them for free, @smarie!

@smarie
Copy link
Owner

smarie commented Dec 2, 2020

Thanks @plammens for the kind feedback ! Yes, in general I hate low-level coding so I prefer to spend months solving things once and for all (or until someone else provides a better lib of course), so that I can later focus on more interesting things.

If you're curiours, you should probably look into pyfields too, it relies on valid8 but makes it usable for classes in a very similar fashion than attrs or dataclasses.

Back to the topic at hand now : your explanation on indirect definitely makes sense. And I'm happy to see that it actually works with cases :) I'll better leave this without explicit ValueError for now, as it makes no big harm. Let's wait for more user feedback on this topic.

Note that you can transform any case function into a fixture by having it require a fixture as argument. So you could in your example have your "hand" cases require the dummy player fixture, so that the "hand" case becomes a fully configured player. But then I'm sure we can find another situation in which you wont like this anymore, for example combining subset of cases for the "hand" and for some random other aspect of the player.

I need some aspirine now :D

@smarie
Copy link
Owner

smarie commented Jan 25, 2021

Proposal to close this: I'll issue a warning when indirect=True and fixture refs are present in the parametrize.

@codectl
Copy link

codectl commented Aug 8, 2022

I got here due to the following error:

.../pytest_cases/fixture_parametrize_plus.py:831: in _parametrize_plus
    raise ValueError("Setting `indirect=True` is not yet supported when at least a `fixure_ref` is present in "
E   ValueError: Setting `indirect=True` is not yet supported when at least a `fixure_ref` is present in the `argvalues`.

Is this not supported?

@pytest_cases.parametrize("subscribe", [fixture_ref("folder")], indirect=True)

@smarie
Copy link
Owner

smarie commented Sep 12, 2022

@rena2damas no this is not supported. Usually from my experience you can always find an (elegant) way to use pytest and pytest-cases without using indirect.

If a fixture is to be parametrized in two different ways for two different tests, I would suggest to create two fixture relying on the same code, or depending on the same root fixture. That way no need for indirect.

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

No branches or pull requests

3 participants