Type annotate fixtures.py & related #6736

bluetech · 2020-02-14T10:14:30Z

Extracted from #6717.

~~Depends/based on #6737.~~

bluetech · 2020-02-14T10:19:07Z

src/_pytest/fixtures.py

        fixturedefs = self._arg2fixturedefs.get(argname, None)
        if fixturedefs is None:
            # we arrive here because of a dynamic call to
            # getfixturevalue(argname) usage which was naturally
            # not known at parsing/collection time
+            assert self._pyfuncitem.parent is not None


This is immediately dereferenced below so should be fine.

bluetech · 2020-02-14T10:19:44Z

src/_pytest/fixtures.py

        """ Dynamically run a named fixture function.

        Declaring fixtures via function argument is recommended where possible.
        But if you can only decide whether to use another fixture at test
        setup time, you may use this function to retrieve it inside a fixture
        or test function body.
        """
-        return self._get_active_fixturedef(argname).cached_result[0]
+        fixturedef = self._get_active_fixturedef(argname)
+        assert fixturedef.cached_result is not None


Immediately dereferenced below.

bluetech · 2020-02-14T10:20:10Z

src/_pytest/fixtures.py

        while 1:
            fixturedef = getattr(current, "_fixturedef", None)
            if fixturedef is None:
                values.reverse()
                return values
            values.append(fixturedef)
+            assert isinstance(current, SubRequest)


This attribute is only set on SubRequest.

That would make a helpful in-code comment for future maintainers 😄

I don't know, once the assert is in, the implication is reversed -- the access is legal because of the assert, not the other way :)

bluetech · 2020-02-14T10:20:59Z

src/_pytest/fixtures.py

@@ -669,20 +718,21 @@ def _schedule_finalizers(self, fixturedef, subrequest):
        super()._schedule_finalizers(fixturedef, subrequest)


-scopes = "session package module class function".split()
+scopes = ["session", "package", "module", "class", "function"]  # type: List[_Scope]


Changed to make the type annotation work (_Scope is a Literal).

bluetech · 2020-02-14T10:22:41Z

src/_pytest/fixtures.py

    """Look up the index of ``scope`` and raise a descriptive value error
    if not defined.
    """
+    strscopes = scopes  # type: Sequence[str]


Just a hack to expand the type of scopes to allow checking against an str rather than _Scope.

bluetech · 2020-02-14T10:23:42Z

src/_pytest/fixtures.py

@@ -871,6 +927,7 @@ def finish(self, request):
                    exceptions.append(sys.exc_info())
            if exceptions:
                _, val, tb = exceptions[0]
+                assert val is not None


Dereferenced just below.

bluetech · 2020-02-14T10:25:23Z

src/_pytest/setuponly.py

    yield
    if request.config.option.setupshow:
        if hasattr(request, "param"):
            # Save the fixture parameter so ._show_fixture_action() can
            # display it now and during the teardown (in .finish()).
            if fixturedef.ids:
                if callable(fixturedef.ids):
-                    fixturedef.cached_param = fixturedef.ids(request.param)


Changed so I only need to ignore once.

Maybe a helper method FixtureDef.set_cached_param would be appropriate? Though I can see an argument that it's not worth the trouble when we have to maintain the current option for backwards-compatibility.

Yeah, I'm not sure it's worth it.

Zac-HD

Thanks @bluetech! A few questions and suggestions for further annotations below, but overall this looks great to me 😁

Zac-HD · 2020-02-27T01:32:24Z

src/_pytest/setuponly.py

    yield
    if request.config.option.setupshow:
        if hasattr(request, "param"):
            # Save the fixture parameter so ._show_fixture_action() can
            # display it now and during the teardown (in .finish()).
            if fixturedef.ids:
                if callable(fixturedef.ids):
-                    fixturedef.cached_param = fixturedef.ids(request.param)


Maybe a helper method FixtureDef.set_cached_param would be appropriate? Though I can see an argument that it's not worth the trouble when we have to maintain the current option for backwards-compatibility.

Zac-HD · 2020-02-27T01:35:40Z

src/_pytest/fixtures.py

@@ -111,7 +137,9 @@ def get_scope_node(node, scope):
    return node.getparent(cls)


-def add_funcarg_pseudo_fixture_def(collector, metafunc, fixturemanager):
+def add_funcarg_pseudo_fixture_def(
+    collector, metafunc: "Metafunc", fixturemanager: "FixtureManager"


Worth annotating collector? AFAICT it's an Optional[Type[nodes.Node]].

When I'm not sure of something, and too lazy to check/verify, I leave it unannotated. Usually some later PR adds it :)

src/_pytest/fixtures.py

Zac-HD · 2020-02-27T01:48:25Z

src/_pytest/fixtures.py

        while 1:
            fixturedef = getattr(current, "_fixturedef", None)
            if fixturedef is None:
                values.reverse()
                return values
            values.append(fixturedef)
+            assert isinstance(current, SubRequest)


That would make a helpful in-code comment for future maintainers 😄

src/_pytest/fixtures.py

Zac-HD · 2020-02-27T01:49:49Z

src/_pytest/fixtures.py

+        self,
+        request: "FixtureRequest",
+        scope: "_Scope",
+        param,


param: object, ?

IIRC, this one can also be NOTSET, and I'd like to distinguish it in the type annotation from just an object (although object subsumes everything) just for clarity.

I think there is another PR that does that although I sort of lost track myself already 👣

Zac-HD · 2020-02-27T04:22:13Z

src/_pytest/fixtures.py

@@ -805,7 +855,7 @@ def _teardown_yield_fixture(fixturefunc, it):
        )


-def _eval_scope_callable(scope_callable, fixture_name, config):
+def _eval_scope_callable(scope_callable, fixture_name: str, config: Config) -> str:


scope_callable: Callable[..., str] - speccing keyword arguments is pretty fiddly, but even without the arguments specified this could catch a fair few issues.

I prefer to leave this one for for later.

Zac-HD · 2020-02-27T04:29:38Z

src/_pytest/fixtures.py

@@ -1317,7 +1379,9 @@ def _getautousenames(self, nodeid):
                autousenames.extend(basenames)
        return autousenames

-    def getfixtureclosure(self, fixturenames, parentnode, ignore_args=()):
+    def getfixtureclosure(
+        self, fixturenames: Tuple[str, ...], parentnode, ignore_args: Sequence[str] = ()


parentnode: nodes.Node

I'm not sure, I prefer to leave this one.

bluetech

Thanks for the review @Zac-HD. I applied (some) of your suggestions. Also rebased.

bluetech · 2020-02-28T11:16:15Z

src/_pytest/fixtures.py

@@ -111,7 +137,9 @@ def get_scope_node(node, scope):
    return node.getparent(cls)


-def add_funcarg_pseudo_fixture_def(collector, metafunc, fixturemanager):
+def add_funcarg_pseudo_fixture_def(
+    collector, metafunc: "Metafunc", fixturemanager: "FixtureManager"


When I'm not sure of something, and too lazy to check/verify, I leave it unannotated. Usually some later PR adds it :)

src/_pytest/fixtures.py

bluetech · 2020-02-28T11:20:47Z

src/_pytest/fixtures.py

        while 1:
            fixturedef = getattr(current, "_fixturedef", None)
            if fixturedef is None:
                values.reverse()
                return values
            values.append(fixturedef)
+            assert isinstance(current, SubRequest)


I don't know, once the assert is in, the implication is reversed -- the access is legal because of the assert, not the other way :)

src/_pytest/fixtures.py

bluetech · 2020-02-28T11:24:39Z

src/_pytest/fixtures.py

+        self,
+        request: "FixtureRequest",
+        scope: "_Scope",
+        param,


IIRC, this one can also be NOTSET, and I'd like to distinguish it in the type annotation from just an object (although object subsumes everything) just for clarity.

I think there is another PR that does that although I sort of lost track myself already 👣

bluetech · 2020-02-28T11:25:44Z

src/_pytest/fixtures.py

@@ -805,7 +855,7 @@ def _teardown_yield_fixture(fixturefunc, it):
        )


-def _eval_scope_callable(scope_callable, fixture_name, config):
+def _eval_scope_callable(scope_callable, fixture_name: str, config: Config) -> str:


I prefer to leave this one for for later.

bluetech · 2020-02-28T11:26:21Z

src/_pytest/fixtures.py

@@ -1317,7 +1379,9 @@ def _getautousenames(self, nodeid):
                autousenames.extend(basenames)
        return autousenames

-    def getfixtureclosure(self, fixturenames, parentnode, ignore_args=()):
+    def getfixtureclosure(
+        self, fixturenames: Tuple[str, ...], parentnode, ignore_args: Sequence[str] = ()


I'm not sure, I prefer to leave this one.

bluetech · 2020-02-28T11:26:46Z

src/_pytest/setuponly.py

    yield
    if request.config.option.setupshow:
        if hasattr(request, "param"):
            # Save the fixture parameter so ._show_fixture_action() can
            # display it now and during the teardown (in .finish()).
            if fixturedef.ids:
                if callable(fixturedef.ids):
-                    fixturedef.cached_param = fixturedef.ids(request.param)


Yeah, I'm not sure it's worth it.

bluetech force-pushed the fixtures-annotate branch from afad3d8 to 9bda23f Compare February 14, 2020 10:16

bluetech commented Feb 14, 2020

View reviewed changes

bluetech changed the title ~~Type annotate fixtures.py & related~~ WIP: Type annotate fixtures.py & related Feb 14, 2020

bluetech force-pushed the fixtures-annotate branch from 9bda23f to 3856200 Compare February 14, 2020 12:23

bluetech changed the title ~~WIP: Type annotate fixtures.py & related~~ Type annotate fixtures.py & related Feb 14, 2020

bluetech force-pushed the fixtures-annotate branch from 3856200 to c0c4ca9 Compare February 15, 2020 09:36

Zac-HD reviewed Feb 27, 2020

View reviewed changes

Type annotate fixtures.py & related

cba62b6

bluetech force-pushed the fixtures-annotate branch from c0c4ca9 to cba62b6 Compare February 28, 2020 11:30

bluetech commented Feb 28, 2020

View reviewed changes

bluetech closed this Feb 28, 2020

bluetech deleted the fixtures-annotate branch February 28, 2020 12:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Type annotate fixtures.py & related #6736

Type annotate fixtures.py & related #6736

bluetech commented Feb 14, 2020 •

edited

bluetech Feb 14, 2020

bluetech Feb 14, 2020

bluetech Feb 14, 2020

Zac-HD Feb 27, 2020

bluetech Feb 28, 2020

bluetech Feb 14, 2020

bluetech Feb 14, 2020

bluetech Feb 14, 2020

bluetech Feb 14, 2020

Zac-HD Feb 27, 2020

bluetech Feb 28, 2020

Zac-HD left a comment

Zac-HD Feb 27, 2020

Zac-HD Feb 27, 2020

bluetech Feb 28, 2020

Zac-HD Feb 27, 2020

Zac-HD Feb 27, 2020

bluetech Feb 28, 2020

Zac-HD Feb 27, 2020

bluetech Feb 28, 2020

Zac-HD Feb 27, 2020

bluetech Feb 28, 2020

bluetech left a comment

bluetech Feb 28, 2020

bluetech Feb 28, 2020

bluetech Feb 28, 2020

bluetech Feb 28, 2020

bluetech Feb 28, 2020

bluetech Feb 28, 2020

Type annotate fixtures.py & related #6736

Type annotate fixtures.py & related #6736

Conversation

bluetech commented Feb 14, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zac-HD left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bluetech left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bluetech commented Feb 14, 2020 •

edited