-
-
Notifications
You must be signed in to change notification settings - Fork 2.9k
Remove prune_dependency_tree
and reuse getfixtureclosure
logic
#11243
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
base: main
Are you sure you want to change the base?
Remove prune_dependency_tree
and reuse getfixtureclosure
logic
#11243
Conversation
ef380d7
to
b9aebdb
Compare
There was a problem hiding this 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?
src/_pytest/fixtures.py
Outdated
arg2fixturedefs: Dict[str, Sequence[FixtureDef[Any]]], | ||
ignore_args: Sequence[str] = (), | ||
) -> Tuple[Tuple[str, ...], List[str], Dict[str, Sequence[FixtureDef[Any]]]]: | ||
) -> List[str]: |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
b9aebdb
to
d508df7
Compare
@sadra-barikbin Can you please rebase this on latest main? |
b92c1a5
to
fc92f9f
Compare
7c61003
to
c72507c
Compare
src/_pytest/fixtures.py
Outdated
) | ||
|
||
|
||
class IdentityFixture(FixtureDef[FixtureValue]): |
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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.
a4fa65a
to
95f53e3
Compare
95f53e3
to
677f4e4
Compare
I rebased, hopefully correctly. |
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 inprune_dependency_tree
. They are the same except that the second one should also ignore dynamically-introduced parametrise args, besides the ones inignore_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 needis_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 inFixtureManager::getfixtureclosure
upon creatingFuncFixtureInfo
and once inprunc_dependency_tree
after callingpytest_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.