-
-
Notifications
You must be signed in to change notification settings - Fork 3.4k
fixtures: use a better data structure for storing FixtureDefs #14984
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?
Changes from all commits
854d6d1
53fc913
516aecd
c3e29c6
d938a56
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,2 @@ | ||
| Fixed a regression in 9.1.0 that made collection very slow (quadratic) when many fixtures are defined with the same name. | ||
| Then can particularly happen when many tests are defined in separate classes which inherit from a base class which defines fixtures (the fixtures are repeated per class). | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -473,9 +473,9 @@ class FuncFixtureInfo: | |
| # Note: can't include dynamic dependencies (`request.getfixturevalue` calls). | ||
| names_closure: list[str] | ||
| # A map from a fixture name in the transitive closure to the FixtureDefs | ||
| # matching the name which are applicable to this function. | ||
| # matching the name which are visible to this item. | ||
| # There may be multiple overriding fixtures with the same name. The | ||
| # sequence is ordered from furthest to closes to the function. | ||
| # sequence is ordered from furthest to closes to the item. | ||
| name2fixturedefs: dict[str, Sequence[FixtureDef[Any]]] | ||
|
|
||
| def prune_dependency_tree(self) -> None: | ||
|
|
@@ -726,7 +726,7 @@ def _get_active_fixturedef(self, argname: str) -> FixtureDef[object]: | |
| # No fixtures defined with this name. | ||
| if fixturedefs is None: | ||
| raise FixtureLookupError(argname, self) | ||
| # The are no fixtures with this name applicable for the function. | ||
| # The are no fixtures with this name visible for the item. | ||
| if not fixturedefs: | ||
| raise FixtureLookupError(argname, self) | ||
|
|
||
|
|
@@ -999,10 +999,8 @@ def formatrepr(self) -> FixtureLookupErrorRepr: | |
| available = set() | ||
| parent = self.request._pyfuncitem.parent | ||
| assert parent is not None | ||
| for name, fixturedefs in fm._arg2fixturedefs.items(): | ||
| faclist = list(fm._matchfactories(fixturedefs, parent)) | ||
| if faclist: | ||
| available.add(name) | ||
| for fixturedef in fm._get_all_fixture_defs_for_node(parent): | ||
| available.add(fixturedef.argname) | ||
| if self.argname in available: | ||
| msg = ( | ||
| f" recursive dependency involving fixture '{self.argname}' detected" | ||
|
|
@@ -1788,17 +1786,28 @@ def __init__(self, session: Session) -> None: | |
| self.session = session | ||
| self.config: Config = session.config | ||
| # Maps a fixture name (argname) to all of the FixtureDefs in the test | ||
| # suite/plugins defined with this name. Populated by parsefactories(). | ||
| # TODO: The order of the FixtureDefs list of each arg is significant, | ||
| # explain. | ||
| self._arg2fixturedefs: Final[dict[str, list[FixtureDef[Any]]]] = {} | ||
| # suite/plugins defined with this name. | ||
| # For each name, there is a mapping from a Node to the fixtures with the | ||
| # name registered under that Node. The node determines the FixtureDef's | ||
| # visibility (equal to the fixturedef.node). | ||
| # Populated by parsefactories(). | ||
| self._arg2node2fixturedefs: Final[ | ||
| dict[str, dict[nodes.Node, list[FixtureDef[Any]]]] | ||
| ] = {} | ||
| # Legacy fallback, for plugins still using the deprecated nodeid-based | ||
| # API without a node reference. | ||
| # Part of FIXTURE_NODEID_DEPRECATED deprecation. | ||
| self._arg2nodeid2fixturedefs: Final[ | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Instead of a separate datastructure, is it possible for us to get the nodeid from the session collection tree? Or perhaps the node might not exist at this point yet? |
||
| dict[str, dict[str, list[FixtureDef[Any]]]] | ||
| ] = {} | ||
| # A mapping from a node to a list of autouse fixture names it defines. | ||
| # The Session entry holds global usefixtures from config. | ||
| self._node_autousenames: Final[dict[nodes.Node, list[str]]] = { | ||
| session: list(self.config.getini("usefixtures")), | ||
| } | ||
| # Legacy fallback: nodeid string -> autouse names, for plugins still | ||
| # using the deprecated nodeid-based API without a node reference. | ||
| # Part of FIXTURE_NODEID_DEPRECATED deprecation. | ||
| self._nodeid_autousenames: Final[dict[str, list[str]]] = {} | ||
| # Pending conftest modules waiting to be parsed when their Directory is collected. | ||
| # Maps directory path -> conftest plugin module. | ||
|
|
@@ -1928,7 +1937,7 @@ def pytest_collection_finish(self) -> None: | |
| self._pending_conftests.clear() | ||
|
|
||
| def _getautousenames(self, node: nodes.Node) -> Iterator[str]: | ||
| """Return the names of autouse fixtures applicable to node.""" | ||
| """Return the names of autouse fixtures visible to node.""" | ||
| for parentnode in node.listchain(): | ||
| basenames = self._node_autousenames.get(parentnode) | ||
| if basenames: | ||
|
|
@@ -1939,7 +1948,7 @@ def _getautousenames(self, node: nodes.Node) -> Iterator[str]: | |
| yield from nodeid_basenames | ||
|
|
||
| def _getusefixturesnames(self, node: nodes.Item) -> Iterator[str]: | ||
| """Return the names of usefixtures fixtures applicable to node.""" | ||
| """Return the names of usefixtures fixtures visible to node.""" | ||
| for marker_node, mark in node.iter_markers_with_node(name="usefixtures"): | ||
| if not mark.args: | ||
| marker_node.warn( | ||
|
|
@@ -2095,24 +2104,16 @@ def _register_fixture( | |
| node=node, | ||
| ) | ||
|
|
||
| faclist = self._arg2fixturedefs.setdefault(name, []) | ||
| # Insert the fixturedef into the list while maintaining a partial order | ||
| # based on visibility: a fixturedef whose visibility is more specific | ||
| # sorts after a more general one, so that it takes precedence in the | ||
| # override chain (the last applicable fixturedef in the list is used | ||
| # first, see getfixturedefs). | ||
| # fixturedefs with the same visibility keep registration order, i.e. the | ||
| # last registered wins. | ||
| # The order between non-comparable fixturedefs doesn't matter since they | ||
| # cannot be visible together. | ||
| # The idea is that a fixture that is defined closer to the item should | ||
| # take precedence. | ||
| for i, existing in enumerate(faclist): | ||
| if is_visibility_more_specific(existing, fixture_def): | ||
| faclist.insert(i, fixture_def) | ||
| break | ||
| if node is not NOTSET: | ||
| node2fixturedefs = self._arg2node2fixturedefs.setdefault(name, {}) | ||
| node2fixturedefs.setdefault(node, []).insert(0, fixture_def) | ||
| elif nodeid is not NOTSET and nodeid is not None: | ||
| nodeid2fixturedefs = self._arg2nodeid2fixturedefs.setdefault(name, {}) | ||
| nodeid2fixturedefs.setdefault(nodeid, []).insert(0, fixture_def) | ||
| else: | ||
| faclist.append(fixture_def) | ||
| # Global plugin autouse fixtures go under Session. | ||
| node2fixturedefs = self._arg2node2fixturedefs.setdefault(name, {}) | ||
| node2fixturedefs.setdefault(self.session, []).insert(0, fixture_def) | ||
| if autouse: | ||
| if node is not NOTSET: | ||
| self._node_autousenames.setdefault(node, []).append(name) | ||
|
|
@@ -2338,41 +2339,64 @@ def parsefactories( | |
| nodeid=effective_nodeid, | ||
| ) | ||
|
|
||
| def _get_all_fixture_defs(self) -> Iterable[FixtureDef[Any]]: | ||
| """Get all FixtureDefs. | ||
|
|
||
| The order is not guaranteed. | ||
| """ | ||
| for node2fixturedefs in self._arg2node2fixturedefs.values(): | ||
| for fixturedefs in node2fixturedefs.values(): | ||
| yield from fixturedefs | ||
| for nodeid2fixturedefs in self._arg2nodeid2fixturedefs.values(): | ||
| for fixturedefs in nodeid2fixturedefs.values(): | ||
| yield from fixturedefs | ||
|
|
||
| def _get_all_fixture_defs_for_node( | ||
| self, node: nodes.Node | ||
| ) -> Iterable[FixtureDef[Any]]: | ||
| """Get all FixtureDefs visible to a node. | ||
|
|
||
| The order is not guaranteed. | ||
| """ | ||
| for node2fixturedefs in self._arg2node2fixturedefs.values(): | ||
| for parent in node.iter_parents(): | ||
| yield from node2fixturedefs.get(parent, ()) | ||
| for nodeid2fixturedefs in self._arg2nodeid2fixturedefs.values(): | ||
| for parent in node.iter_parents(): | ||
| yield from nodeid2fixturedefs.get(parent.nodeid, ()) | ||
|
|
||
| def getfixturedefs( | ||
| self, argname: str, node: nodes.Node | ||
| ) -> Sequence[FixtureDef[Any]] | None: | ||
| """Get FixtureDefs for a fixture name which are applicable | ||
| """Get FixtureDefs for a fixture name which are visible | ||
| to a given node. | ||
|
|
||
| Returns None if there are no fixtures at all defined with the given | ||
| name. (This is different from the case in which there are fixtures | ||
| with the given name, but none applicable to the node. In this case, | ||
| with the given name, but none visible to the node. In this case, | ||
| an empty result is returned). | ||
|
|
||
| The returned FixtureDefs are ordered from least specific (registered | ||
| higher in the collection tree) to most specific. For FixtureDefs | ||
| registered at the same Node, registered later => more specific. | ||
|
|
||
| :param argname: Name of the fixture to search for. | ||
| :param node: The requesting Node. | ||
| """ | ||
| try: | ||
| fixturedefs = self._arg2fixturedefs[argname] | ||
| except KeyError: | ||
| node2fixturedefs = self._arg2node2fixturedefs.get(argname, {}) | ||
| nodeid2fixturedefs = self._arg2nodeid2fixturedefs.get(argname, {}) | ||
| if not node2fixturedefs and not nodeid2fixturedefs: | ||
| return None | ||
| return tuple(self._matchfactories(fixturedefs, node)) | ||
|
|
||
| def _matchfactories( | ||
| self, fixturedefs: Iterable[FixtureDef[Any]], node: nodes.Node | ||
| ) -> Iterator[FixtureDef[Any]]: | ||
| # Collect parent nodes and their IDs for matching | ||
| parent_nodes = set(node.iter_parents()) | ||
| parentnodeids = {n.nodeid for n in parent_nodes} | ||
|
|
||
| for fixturedef in fixturedefs: | ||
| if fixturedef.node is not None: | ||
| # Node-based matching: check if fixture's node is a parent | ||
| if fixturedef.node in parent_nodes: | ||
| yield fixturedef | ||
| elif fixturedef.baseid in parentnodeids: | ||
| # Fallback to string-based matching for legacy/plugins | ||
| yield fixturedef | ||
| fixturedefs = [ | ||
| fixturedef | ||
| for parent in node.iter_parents() | ||
| for fixturedef in [ | ||
| *node2fixturedefs.get(parent, ()), | ||
| *nodeid2fixturedefs.get(parent.nodeid, ()), | ||
| ] | ||
|
Comment on lines
+2393
to
+2396
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We could use |
||
| ] | ||
| fixturedefs.reverse() | ||
| return fixturedefs | ||
|
|
||
|
|
||
| def show_fixtures_per_test(config: Config) -> int | ExitCode: | ||
|
|
@@ -2487,30 +2511,24 @@ def _showfixtures_main(config: Config, session: Session) -> None: | |
| verbose = config.get_verbosity() | ||
|
|
||
| fm = session._fixturemanager | ||
|
|
||
| available = [] | ||
| seen: set[tuple[str, str]] = set() | ||
|
|
||
| for argname, fixturedefs in fm._arg2fixturedefs.items(): | ||
| assert fixturedefs is not None | ||
| if not fixturedefs: | ||
| for fixturedef in fm._get_all_fixture_defs(): | ||
| loc = getlocation(fixturedef.func, invocation_dir) | ||
| if (fixturedef.argname, loc) in seen: | ||
| continue | ||
| for fixturedef in fixturedefs: | ||
| loc = getlocation(fixturedef.func, invocation_dir) | ||
| if (fixturedef.argname, loc) in seen: | ||
| continue | ||
| seen.add((fixturedef.argname, loc)) | ||
| available.append( | ||
| ( | ||
| len(fixturedef.baseid), | ||
| fixturedef.func.__module__, | ||
| _pretty_fixture_path(invocation_dir, fixturedef.func), | ||
| fixturedef.argname, | ||
| fixturedef, | ||
| ) | ||
| seen.add((fixturedef.argname, loc)) | ||
| available.append( | ||
| ( | ||
| len(fixturedef.baseid), | ||
| fixturedef.func.__module__, | ||
| _pretty_fixture_path(invocation_dir, fixturedef.func), | ||
| fixturedef.argname, | ||
| fixturedef, | ||
| ) | ||
|
|
||
| ) | ||
| available.sort() | ||
|
|
||
| currentmodule = None | ||
| for baseid, module, prettypath, argname, fixturedef in available: | ||
| if currentmodule != module: | ||
|
|
||
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.