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

Cases depending on other cases getting a LazyValue is problematic #158

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

Cases depending on other cases getting a LazyValue is problematic #158

plammens opened this issue Dec 4, 2020 · 7 comments

Comments

@plammens
Copy link
Contributor

plammens commented Dec 4, 2020

Sorry for the issue spamming Sylvain.

Very briefly, if I have a case case_a that is parametrised with another case, case_b, case_a doesn't get an actual value, it gets a LazyValue. So when I do e.g. MagicMock(spec=value_of_case_b), it uses the attributes and methods of the LazyValue object, not the value itself. Is there a way around that? Are case functions supposed to be aware that they'll be getting a LazyValue? If so is it encouraged to explicitly call .valuegetter()?

I'm starting to think these LazyValues cause more problems than they are useful 🤔


Example:

import pytest_cases


def case_x():
    return 42


@pytest_cases.parametrize_with_cases("x", case_x)
def case_y(x):
    return 3*x + 2


@pytest_cases.parametrize_with_cases("y", case_y)
def test_foo(y):
    print(y)

This gives a TypeError when trying to compute 3*x + 2, since x is a LazyValue.

@smarie
Copy link
Owner

smarie commented Dec 4, 2020

Thanks @plammens for testing all these situations ! I really appreciate, this is great

Well, lazy_value is at the heart of pytest-cases because a case function is basically a parameter that is a function, and should only be called when a test node is run. My idea was that maybe it would be overkill to transform all case functions into fixtures, I have no idea how many fixtures pytest can handle. And pytest-cases' aim is definitely to encourage users to create many cases to increase the size/power of their test harness ! Powerful code requires powerful tests :)

So at least one workaround is probably to have your case require a fixture parametrized with the other case. This should work.

@plammens
Copy link
Contributor Author

plammens commented Dec 4, 2020

I see. So if I understand correctly, the case is getting a lazy value because it's parameterised with a case, and cases get collected at the test collection stage (hence the need for LazyValue). But a fixture would get the actual value because it is run "immediately" before the test node.

Is this more or less what's going on?

@plammens
Copy link
Contributor Author

plammens commented Dec 4, 2020

This makes me think, are cases parametrised with other cases even supported, or did I just dream that up?

If they are supported, I think an exception should be made for these, in that the LazyValue should be actually computed before passing it to the dependent case—you can't do anything useful at all with a lazy value if you need that value to construct the dependent test case. And if the dependent case function is being called, it means that the depended-on case function would have been called soon in any case, so you're not gaining anything by delaying the computation of the value. Otherwise I'd resort to calling .valuegetter() manually in any case.

@smarie
Copy link
Owner

smarie commented Dec 16, 2020

This makes me think, are cases parametrised with other cases even supported, or did I just dream that up?

They were not working correctly because of some premature optimization. In 3.0.0 I fixed it so they should be working fine, at the cost of some extra fixtures being created. I'll probably need to re-optimize it again: if a case is parameterized it does not always need to be turned into a fixture (as it is for now). But doing this properly, as you noted with nested lazy values, requires an extra care.

Let me know if 3.0.0 solves the issue, and I'll open another ticket for the internal optimization that needs to be done

@plammens
Copy link
Contributor Author

plammens commented Dec 17, 2020

Thanks! Now I'm getting an error though, and I'm not able to make a minimal reproducible example :(

My case code looks like this:

class DummyPlayerCases:
    ...

    @staticmethod
    @pytest_cases.case()
    @pytest_cases.parametrize_with_cases("hand", cases=PlayerHandCases.case_single_card)
    def case_single_card(hand):
        return DummyPlayerCases.__make_player(hand)

and the test function:

@pytest_cases.parametrize_with_cases("card", cases=card_cases.CardCases)
@pytest_cases.parametrize_with_cases(
    "player", cases=player_cases.DummyPlayerCases.case_single_card
)
def test_cardSteps_correspondsToReality(player: RoundPlayer, card: cards.Card):
    ...

I'm getting

file C:\Users\Paolo\Code\JB-PycharmProjects\LoveLetter\tests\test_loveletter\unit\test_player_cases.py, line 48: source code not available
E       fixture 'case_single_card_hand' not found
>       available fixtures: cache, capfd, capfdbinary, caplog, capsys, capsysbinary, current_player, current_player_, discard_card, doctest_namespace, first_player, game_round, last_player, monkeypatch, new_round, ordered_pair, other_multistep_cancel, other_multistep_card_nocancel, pytestconfig, record_property, record_testsuite_property, record_xml_attribute, recwarn, set_random_state, single_card, started_round, target_card_cancel, test_baron_equalOpponent_noneEliminated_card, test_baron_strongerOpponent_selfEliminated_card1_card2, test_baron_weakerOpponent_opponentEliminated_card1_card2, test_cardSteps_correspondsToReality_card, test_cardSteps_correspondsToReality_player, test_cardTypeOrder_increasingPair_asExpected_card1_card2, test_prince_againstNonPrincess_dealsCard_target, test_prince_emptyDeck_dealsSetAsideCard_target, test_targetCard_againstImmunePlayer_raises_card, test_targetCard_allOpponentsImmune_canChooseNone_card, test_targetCard_chooseSelf_raises_card, tmp_path, tmp_path_factory, tmpdir, tmpdir_factory
>       use 'pytest --fixtures [testpath]' for help on them.

Could you give me a pointer perhaps as to where to put breakpoints for debugging, @smarie?

Running pytest --fixtures says that case_single_card_hand has been defined from fixture_core1_unions.py.

@smarie
Copy link
Owner

smarie commented Dec 18, 2020

I'm not able to make a minimal reproducible example :(

ouch. this will be hard then, as I won't be able to help. I'll try random examples this afternoon

Could you give me a pointer perhaps as to where to put breakpoints for debugging, @smarie?

@parametrize_with_cases is the place where you might wish to put a breakpoint. Applied on a function test_foo(a) it does three things:

    1. first it collects the list of case functions to use. Class methods that are not staticmethod nor classmethod are partialized here so as to remove 'self'.
    1. then for all cases functions, if they are parametrized or require a fixture, it generates a fixture around them and registers it in the module where @parametrize_with_cases is defined, and create a fixture_ref. Otherwise it creates a lazy_value.
    1. All fixture refs and lazy values are passed to a @parametrize for function test_foo. This, when seeing that there is at least a fixture reference in the argvalues, creates a fixture union named test_foo_a replacing the parameter and registers it on the module. It replaces test_foo with a wrapper requiring this fixture and injecting it in place of the parameter

Note that the intermediate fixture created at step 3 would not be if we fix #170 . Some (but not all) intermediate fixtures created at step 2 would also not be if we fix #169 .

So, now in your situation, I think that the problem happend in the first @parametrize_with_cases (on case_single_card) at the step 3 : a fixture union case_single_card_hand is created in the module containing the case, to replace the parameter hand. But this fixture is not visible from the place where the test_cardSteps_correspond_to_reality lives. Note that there would also be a similar error if PlayerHandCases.case_single_card was parametrized or requires a fixture: at step 2 the created fixture would live in the module where case_single_card lives.

So for now the only workaround I see is that you put all of your code in the same module. Ugly, but will work. I reopen this issue so that we can try to properly fix it in the future version of the engine (after #170 ).

@smarie smarie reopened this Dec 18, 2020
@plammens
Copy link
Contributor Author

So, now in your situation, I think that the problem happend in the first @parametrize_with_cases (on case_single_card) at the step 3 : a fixture union case_single_card_hand is created in the module containing the case, to replace the parameter hand. But this fixture is not visible from the place where the test_cardSteps_correspond_to_reality lives.

Thanks, this was the key to the issue! Now I was able to make a MRE, I was trying to do that in a single module but that wouldn't have reproduced it due to the nature of the error :) I will open a separate issue since it is kind of unrelated to this one.

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

2 participants