Remove `prune_dependency_tree` and reuse `getfixtureclosure` logic by sadra-barikbin · Pull Request #11243 · pytest-dev/pytest (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation30 Commits19 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

To reuse getfixtureclosure in pruning dependency tree. In pytest, fixture closure of a test is computed twice if it's parametrized. The second computation takes place in prune_dependency_tree. They are the same except that the second one should also ignore dynamically-introduced parametrise args, besides the ones in ignore_args. The motivation behind unifying these two was that the code for computing the closure got larger in the 7th improvement of Some possible improvements in fixtures module #11234 so the duplicate code between these two had become significant. Maybe the bad news is that now I figured out that I need is_pseudo concept for unifying the two, to do the additional objective of the second computation.
To prune dependency tree only if dynamic parametrization has taken place. Currently for each parametrized test , directly or indirectly, dynamically or non-dynamically(using @pytest.mark.parametrize), computing fixture closure takes place twice. Once in FixtureManager::getfixtureclosure upon creating FuncFixtureInfo and once in prunc_dependency_tree after calling pytest_generate_tests hook. The second one is only necessary if direct dynamic parametrization has occurred for the test because this is the only parametrization that might shadow some fixtures in the fixture closure computed in the first call. Note that as differentiating between direct and indirect dynamic parametrization requires a dirty hack, the second fixture closure computation in this PR is done in the latter case as well.

Choose a reason for hiding this comment

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

Hi @sadra-barikbin thanks a lot for the PR, as always.

I left some minor comments, but I'm not sure overall it is an improvement, specially the "magic" needed around unwrap_metafunc_parametrize_and_possibly_prune_dependency_tree. The previous function was a bit convoluted, but at least it was direct and executed around a specific point.

Can you add more details to the PR, specially why you think prune_dependency_tree is problematic and how this improves and/or enables further improvements?

ignore_args: Sequence[str] = (),
) -> Tuple[Tuple[str, ...], List[str], Dict[str, Sequence[FixtureDef[Any]]]]:
) -> List[str]:

Choose a reason for hiding this comment

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

I would prefer to keep returning a brand new arg2fixturedefs, and let the caller merge it with other dict if they want, rather than passing in a dict and fill it inside. This makes it clear what's input/output.

Choose a reason for hiding this comment

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

But the second fixture closure computation needs arg2fixturedefs as input, otherwise it should compute it again which is expensive according to docstring:

........ we also populate arg2fixturedefs mapping

for the args missing therein so that the caller can reuse it and does

not have to re-discover fixturedefs again for each fixturename

(discovering matching fixtures for a given name/node is expensive).

Choose a reason for hiding this comment

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

How about this?

def getfixtureclosure( self, parentnode: nodes.Node, initialnames: Tuple[str, ...], arg2fixturedefs: Union[Dict[str, Sequence[FixtureDef[Any]]], None], ignore_args: Sequence[str] = (), ) -> Tuple[List[str], Dict[str, Sequence[FixtureDef[Any]]]]:

If arg2fixturedefs is none, we create one, populate it and return it. Otherwise we just use it and return it.

…dency_tree' into Improvement-remove-prune_dependency_tree

Now we precisely prune only when there's dynamic direct parametrization.

@@ -4534,8 +4534,178 @@ def test_fixt(custom):
assert result.ret == ExitCode.TESTS_FAILED


def test_fixture_info_after_dynamic_parametrize(pytester: Pytester) -> None:

Choose a reason for hiding this comment

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

Since these tests (except test_dont_recompute_dependency_tree_if_no_dynamic_parametrize) are applicable also to the current code in main, I'd like to add them there first (https://github.com/bluetech/pytest/commits/fixtures-tests). However, when I comment out the fixtureinfo.prune_dependency_tree() call, they all still pass. So I'm not sure if they're testing the right thing?

It would be great to add a docstring to each test explaining what is its purpose, i.e. if it fails I should the docstring should help me understand which mechanism probably failed.

Choose a reason for hiding this comment

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

Are you sure that test_fixture_info_after_dynamic_parametrize passes? The rest are to make sure the correct behavior isn't going to change but this one should fail, because the prune_dependency_tree takes place after fixtures.py::pytest_generate_tests's parametrization on fixture3 and its two generated items along with the two from dynamic parametrization on fixture2 make up four items causing the test to fail.
I added the test to main branch and it failed expectedly.
OK, I add docstring to the tests.

Choose a reason for hiding this comment

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

Update: test_reordering_after_dynamic_parametrize had a bug. I fix it. This should fail as well for the same reason as the test_fixture_info_after_dynamic_parametrize.

@@ -1102,6 +1074,26 @@ def __repr__(self) -> str:
)


class IdentityFixture(FixtureDef[FixtureValue]):

Choose a reason for hiding this comment

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

Since it's a special case of FixtureDef, should be IdentityFixtureDef.

Also, I think "Identity" is not what we want. It described what it does well enough, however when we check isinstance(fixturedefs[-1], IdentityFixture): we aren't checking if it's an "identity fixture", we check if it's a "pseudo fixture". Therefore I think we should call it PseudoFixtureDef.

Also, is it possible to change name2pseudofixturedef type from FixtureDef to PseudoFixtureDef?

Choose a reason for hiding this comment

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

~~How is to keep IdentityFixtureDef but do PseudoFixtureDef = IdentityFixtureDef in fixtures.py?~~

Choose a reason for hiding this comment

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

But we already have a PseudoFixtureDef representing request fixturedefs.

and fix a couple of Mypy issues

Remove prune_dependency_tree and reuse getfixtureclosure logic by sadra-barikbin · Pull Request #11243 · pytest-dev/pytest (original) (raw)

Conversation

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

........ we also populate arg2fixturedefs mapping

for the args missing therein so that the caller can reuse it and does

not have to re-discover fixturedefs again for each fixturename

(discovering matching fixtures for a given name/node is expensive).

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Remove `prune_dependency_tree` and reuse `getfixtureclosure` logic by sadra-barikbin · Pull Request #11243 · pytest-dev/pytest (original) (raw)