Skip to content
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

fixtures: more tweaks #11416

Merged

Conversation

bluetech
Copy link
Member

@bluetech bluetech commented Sep 8, 2023

A few more small fixture cleanups/improvements from looking at #11243.

The second commit is picked from #11243 authored by @sadra-barikbin. The idea is to make #11243 a bit smaller after it's rebased on top of this (which I'll be happy to do if @sadra-barikbin prefers).

The last commits are some doctest cleanups after checking its fixture handling. If we ever want to provide a proper interface for (non-Function) items to support fixtures, DoctestItem can be our model case.

bluetech and others added 6 commits September 8, 2023 15:53
Only used for containment checks so a Set is more appropriate than a
list.
Some code cleanups - no functional changes.
Since we already broke plugins using this (private) interface in this
version (pytest-play, pytest-wdl), might as well do a cleanup.
There used to be two callers to `_setup_fixtures()`, now there's only
one, so inline it and make `DoctestItem` more similar to `Function`.

(Eventually we may want to generalize `TopRequest` from taking
`Function` directly to some "fixture-supporting item", removing the
remaining `type: ignore` here and allowing plugins to do it in a stable
manner).
@bluetech
Copy link
Member Author

bluetech commented Sep 8, 2023

I also wrote another commit, making getfixtureclosure a bit faster by avoiding all of the repetitive work and linear searches it does. However I scrapped it because it will likely conflict further with @sadra-barikbin's plan to change getfixtureclosure from BFS to DFS (as stated in #11234). Here it is for reference:

patch
commit 95ca3bbe7f03380470092c1f3446b921cd93ddac
Author: Ran Benita <ran@unusedvar.com>
Date:   Tue Sep 5 00:30:36 2023 +0300

    fixtures: avoid linear searches in getfixtureclosure
    
    Previously did an inefficient fixpoint algorithm. Replace it with a work
    queue so we only check "new" argnames.

diff --git a/src/_pytest/fixtures.py b/src/_pytest/fixtures.py
index 80b965b81..ce006151c 100644
--- a/src/_pytest/fixtures.py
+++ b/src/_pytest/fixtures.py
@@ -1544,23 +1544,23 @@ class FixtureManager:
         # (discovering matching fixtures for a given name/node is expensive).
 
         parentid = parentnode.nodeid
-        fixturenames_closure = list(initialnames)
+        closure: dict[str, None] = dict.fromkeys(initialnames)
 
         arg2fixturedefs: Dict[str, Sequence[FixtureDef[Any]]] = {}
-        lastlen = -1
-        while lastlen != len(fixturenames_closure):
-            lastlen = len(fixturenames_closure)
-            for argname in fixturenames_closure:
-                if argname in ignore_args:
-                    continue
-                if argname in arg2fixturedefs:
-                    continue
-                fixturedefs = self.getfixturedefs(argname, parentid)
-                if fixturedefs:
-                    arg2fixturedefs[argname] = fixturedefs
-                    for arg in fixturedefs[-1].argnames:
-                        if arg not in fixturenames_closure:
-                            fixturenames_closure.append(arg)
+        work = deque(initialnames)
+        while work:
+            argname = work.popleft()
+            if argname in ignore_args:
+                continue
+            if argname in arg2fixturedefs:
+                continue
+            fixturedefs = self.getfixturedefs(argname, parentid)
+            if fixturedefs:
+                arg2fixturedefs[argname] = fixturedefs
+                for arg in fixturedefs[-1].argnames:
+                    if arg not in closure:
+                        closure[arg] = None
+                        work.append(arg)
 
         def sort_by_scope(arg_name: str) -> Scope:
             try:
@@ -1570,7 +1570,7 @@ class FixtureManager:
             else:
                 return fixturedefs[-1]._scope
 
-        fixturenames_closure.sort(key=sort_by_scope, reverse=True)
+        fixturenames_closure = sorted(closure, key=sort_by_scope, reverse=True)
         return fixturenames_closure, arg2fixturedefs
 
     def pytest_generate_tests(self, metafunc: "Metafunc") -> None:

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.

LGTM!

Copy link
Contributor

@sadra-barikbin sadra-barikbin left a comment

Choose a reason for hiding this comment

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

Thanks @bluetech. Looks good to me.

@RonnyPfannschmidt RonnyPfannschmidt merged commit dd7beb3 into pytest-dev:main Sep 8, 2023
@RonnyPfannschmidt
Copy link
Member

Love to see this land, it's just awesome to see all the collaboration on enhancing those details happen, thanks!

@bluetech bluetech deleted the fixtures-getfixtureclosure branch September 8, 2023 21:08
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