Skip to content

PyCollector._genfunctions: use already created fixtureinfo #6636

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

Merged
merged 1 commit into from
Feb 1, 2020

Conversation

blueyed
Copy link
Contributor

@blueyed blueyed commented Jan 30, 2020

@blueyed
Copy link
Contributor Author

blueyed commented Jan 30, 2020

NOTE: I've not looked too closely at this, just noticed it via pdb'ing that it got called twice.

@nicoddemus
Copy link
Member

I had checked it before approval, but decided to do a double check:

They seem to be identical calls:

pytest/src/_pytest/python.py

Lines 1396 to 1398 in 1dc265e

fixtureinfo = self.session._fixturemanager.getfixtureinfo(
self, self.obj, self.cls, funcargs=True
)

and:

fixtureinfo = fm.getfixtureinfo(definition, funcobj, cls)

(funcargs is True by default)

FuncFixtureInfo is mostly a data class:

@attr.s(slots=True)
class FuncFixtureInfo:
# original function argument names
argnames = attr.ib(type=tuple)
# argnames that function immediately requires. These include argnames +
# fixture names specified via usefixtures and via autouse=True in fixture
# definitions.
initialnames = attr.ib(type=tuple)
names_closure = attr.ib() # List[str]
name2fixturedefs = attr.ib() # List[str, List[FixtureDef]]
def prune_dependency_tree(self):
"""Recompute names_closure from initialnames and name2fixturedefs
Can only reduce names_closure, which means that the new closure will
always be a subset of the old one. The order is preserved.
This method is needed because direct parametrization may shadow some
of the fixtures that were included in the originally built dependency
tree. In this way the dependency tree can get pruned, and the closure
of argnames may get reduced.
"""
closure = set()
working_set = set(self.initialnames)
while working_set:
argname = working_set.pop()
# argname may be smth not included in the original names_closure,
# in which case we ignore it. This currently happens with pseudo
# FixtureDefs which wrap 'get_direct_param_fixture_func(request)'.
# So they introduce the new dependency 'request' which might have
# been missing in the original tree (closure).
if argname not in closure and argname in self.names_closure:
closure.add(argname)
if argname in self.name2fixturedefs:
working_set.update(self.name2fixturedefs[argname][-1].argnames)
self.names_closure[:] = sorted(closure, key=self.names_closure.index)

and prune_dependency_tree is called here:

fixtureinfo.prune_dependency_tree()

So I think it is OK, althought the prune_dependency_tree code could be improved (I'm not sure I like the same object being changed over like this).

@blueyed
Copy link
Contributor Author

blueyed commented Jan 30, 2020

@nicoddemus
Thanks for looking into / confirming it.
Let's wait for another approval though.

Copy link
Member

@RonnyPfannschmidt RonnyPfannschmidt left a comment

Choose a reason for hiding this comment

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

at first glance this looks good to me,
seems like i missed this opportunity when adding function-definition first

Copy link
Member

@RonnyPfannschmidt RonnyPfannschmidt left a comment

Choose a reason for hiding this comment

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

based on the looks this is fine

@blueyed blueyed merged commit a9c5d31 into pytest-dev:master Feb 1, 2020
@blueyed blueyed deleted the fixtureinfo branch February 1, 2020 05:27
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.

3 participants