Skip to content

Conversation

sadra-barikbin
Copy link
Contributor

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

  • 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.

@bluetech bluetech self-requested a review July 23, 2023 10:08
@bluetech bluetech mentioned this pull request Jul 27, 2023
@bluetech bluetech force-pushed the Improvement-remove-prune_dependency_tree branch from ef380d7 to b9aebdb Compare July 27, 2023 07:22
Copy link
Member

@nicoddemus nicoddemus 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 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?

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

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.

Copy link
Contributor Author

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).

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 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.

@bluetech
Copy link
Member

@sadra-barikbin Can you please rebase this on latest main?

@sadra-barikbin sadra-barikbin force-pushed the Improvement-remove-prune_dependency_tree branch from b92c1a5 to fc92f9f Compare August 10, 2023 08:49
@bluetech bluetech mentioned this pull request Sep 8, 2023
@sadra-barikbin sadra-barikbin force-pushed the Improvement-remove-prune_dependency_tree branch from 7c61003 to c72507c Compare September 8, 2023 21:31
)


class IdentityFixture(FixtureDef[FixtureValue]):
Copy link
Member

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?

Copy link
Contributor Author

@sadra-barikbin sadra-barikbin Sep 16, 2023

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?

Copy link
Contributor Author

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.

@sadra-barikbin sadra-barikbin force-pushed the Improvement-remove-prune_dependency_tree branch from a4fa65a to 95f53e3 Compare December 15, 2023 18:43
@bluetech bluetech force-pushed the Improvement-remove-prune_dependency_tree branch from 95f53e3 to 677f4e4 Compare October 8, 2025 06:34
@bluetech
Copy link
Member

bluetech commented Oct 8, 2025

I rebased, hopefully correctly.

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