topotato: __init__ rather than from_parent

pytest's switch in object construction was intended as a caller-site
change, not constructor implementation (though the latter needs to be
"cooperative" and pass kwargs).

Move to implementing `__init__` appropriately; this clears up a bunch of
pylint/mypy workarounds too.

Signed-off-by: David Lamparter <[email protected]>

topotato/assertions.py

@@ -157,21 +157,20 @@ class AssertKernelRoutes(TimedMixin, TopotatoAssertion):
     af: ClassVar[Union[Literal[4], Literal[6]]]
     default_delay = 0.1
 
-    # pylint does not understand that from_parent is our __init__
     _rtr: str
     _routes: dict
     _local: bool
 
-    # pylint: disable=arguments-differ,too-many-arguments,protected-access
-    @classmethod
-    def from_parent(cls, parent, name, rtr, routes, *, local=False, **kwargs):
-        name = "%s:%s/routes-v%d" % (name, rtr, cls.af)
-        self = super().from_parent(parent, name=name, **kwargs)
+    posargs = ["rtr", "routes"]
+
+    # pylint: disable=too-many-arguments
+    def __init__(self, *, name, rtr, routes, local=False, **kwargs):
+        name = "%s:%s/routes-v%d" % (name, rtr, self.af)
+        super().__init__(name=name, **kwargs)
 
         self._rtr = rtr
         self._routes = routes
         self._local = local
-        return self
 
     def __call__(self):
         router = self.instance.routers[self._rtr]
@@ -224,19 +223,19 @@ class AssertVtysh(TimedMixin, TopotatoAssertion):
 
     commands: OrderedDict
 
-    # pylint does not understand that from_parent is our __init__
     _rtr: str
     _daemon: str
     _command: str
     _compare: Optional[str]
 
     default_delay = 0.1
 
-    # pylint: disable=arguments-differ,too-many-arguments,protected-access
-    @classmethod
-    def from_parent(
-        cls,
-        parent,
+    posargs = ["rtr", "daemon", "command", "compare"]
+
+    # pylint: disable=too-many-arguments
+    def __init__(
+        self,
+        *,
         name,
         rtr,
         daemon,
@@ -254,16 +253,15 @@ def from_parent(
             name,
             rtr.name,
             daemon,
-            cls._nodename,
+            self._nodename,
             command_cleaned,
         )
-        self = super().from_parent(parent, name=name, **kwargs)
+        super().__init__(name=name, **kwargs)
 
         self._rtr = rtr
         self._daemon = daemon
-        self._command = cls._cmdprefix + command
+        self._command = self._cmdprefix + command
         self._compare = compare
-        return self
 
     def __call__(self):
         router = self.instance.routers[self._rtr.name]
@@ -317,19 +315,19 @@ class ReconfigureFRR(AssertVtysh):
 
 
 class AssertPacket(TimedMixin, TopotatoAssertion):
-    # pylint does not understand that from_parent is our __init__
     _link: str
     _pkt: Any
     _argtypes: List[Type[Packet]]
     _expect_pkt: bool
 
     matched: Optional[Any]
 
-    # pylint: disable=arguments-differ,protected-access,too-many-arguments
-    @classmethod
-    def from_parent(cls, parent, name, link, pkt, expect_pkt=True, **kwargs) -> "AssertPacket":  # type: ignore
+    posargs = ["link", "pkt", "expect_pkt"]
+
+    # pylint: disable=too-many-arguments
+    def __init__(self, *, name, link, pkt, expect_pkt=True, **kwargs):
         name = "%s:%s/packet" % (name, link)
-        self = cast(AssertPacket, super().from_parent(parent, name=name, **kwargs))
+        super().__init__(name=name, **kwargs)
 
         self._link = link
         self._pkt = pkt
@@ -351,8 +349,6 @@ def from_parent(cls, parent, name, link, pkt, expect_pkt=True, **kwargs) -> "Ass
                 )
             self._argtypes.append(argtype)
 
-        return self
-
     def __call__(self):
         for element in self.timeline.run_timing(self._timing):
             if not isinstance(element, TimedScapy):
@@ -391,25 +387,24 @@ def __call__(self):
 
 
 class AssertLog(TimedMixin, TopotatoAssertion):
-    # pylint does not understand that from_parent is our __init__
     _rtr: str
     _daemon: str
     _pkt: Any
     _msg = Union[re.Pattern, str]
 
     matched: Optional[Any]
 
+    posargs = ["rtr", "daemon", "msg"]
+
     # pylint: disable=arguments-differ,protected-access,too-many-arguments
-    @classmethod
-    def from_parent(cls, parent, name, rtr, daemon, msg, **kwargs) -> "AssertLog":  # type: ignore
+    def __init__(self, *, name, rtr, daemon, msg, **kwargs):
         name = "%s:%s/%s/log" % (name, rtr.name, daemon)
-        self = cast(AssertLog, super().from_parent(parent, name=name, **kwargs))
+        super().__init__(name=name, **kwargs)
 
         self._rtr = rtr
         self._daemon = daemon
         self._msg = msg
         self.matched = None
-        return self
 
     @skiptrace
     def __call__(self):
@@ -438,41 +433,24 @@ def __call__(self):
 
 
 class Delay(TimedMixin, TopotatoAssertion):
-    # pylint: disable=arguments-differ,protected-access,too-many-arguments
-    @classmethod
-    def from_parent(cls, parent, name, **kwargs) -> "Delay":  # type: ignore
-        name = "%s" % (name,)
-        self = cast(Delay, super().from_parent(parent, name=name, **kwargs))
-
-        return self
-
     @skiptrace
     def __call__(self):
         for _ in self.timeline.run_timing(self._timing):
             pass
 
 
 class _DaemonControl(TopotatoModifier):
-    # pylint does not understand that from_parent is our __init__
     _rtr: str
     _daemon: str
 
     op_name: ClassVar[str]
+    posargs = ["rtr", "daemon"]
 
-    # pylint: disable=arguments-differ,protected-access
-    @classmethod
-    def from_parent(cls, parent, name, rtr, daemon, **kwargs) -> "DaemonControl":  # type: ignore
-        self = cast(
-            _DaemonControl,
-            super().from_parent(
-                parent,
-                name="%s:%s/%s/%s" % (name, rtr.name, daemon, cls.op_name),
-                **kwargs,
-            ),
-        )
+    def __init__(self, *, name, rtr, daemon, **kwargs):
+        name = "%s:%s/%s/%s" % (name, rtr.name, daemon, self.op_name)
+        super().__init__(name=name, **kwargs)
         self._rtr = rtr
         self._daemon = daemon
-        return self
 
     def do(self, router):
         pass
@@ -504,22 +482,22 @@ class ModifyLinkStatus(TopotatoModifier):
     _iface: str
     _state: bool
 
-    # pylint: disable=arguments-differ,protected-access,too-many-arguments
-    @classmethod
-    def from_parent(cls, parent, name, rtr, iface, state, **kwargs):
+    posargs = ["rtr", "iface", "state"]
+
+    # pylint: disable=too-many-arguments
+    def __init__(self, *, name, rtr, iface, state, **kwargs):
         name = "%s:%s/link[%s (%s) -> %s]" % (
             name,
             rtr.name,
             iface.ifname,
             iface.other.endpoint.name,
             "UP" if state else "DOWN",
         )
-        self = super().from_parent(parent, name=name, **kwargs)
+        super().__init__(name=name, **kwargs)
 
         self._rtr = rtr
         self._iface = iface
         self._state = state
-        return self
 
     def __call__(self):
         router = self.instance.routers[self._rtr.name]
@@ -542,20 +520,17 @@ class Action(TopotatoModifier):
         _rtr: str
         _cmdobj: "BackgroundCommand"
 
-        # pylint: disable=arguments-differ,protected-access
-        @classmethod
-        def from_parent(cls, parent, name, cmdobj, **kwargs):
+        def __init__(self, *, name, cmdobj, **kwargs):
             name = '%s:%s/exec["%s" (%s)]' % (
                 name,
                 cmdobj._rtr.name,
                 cmdobj._cmd,
-                cls.__name__,
+                self.__class__.__name__,
             )
-            self = super().from_parent(parent, name=name, **kwargs)
+            super().__init__(name=name, **kwargs)
 
             self._rtr = cmdobj._rtr
             self._cmdobj = cmdobj
-            return self
 
     class Start(Action):
         # pylint: disable=consider-using-with
@@ -586,8 +561,8 @@ def __call__(self):
 
     @skiptrace
     def start(self):
-        yield from self.Start.make(self)
+        yield from self.Start.make(cmdobj=self)
 
     @skiptrace
     def wait(self):
-        yield from self.Wait.make(self)
+        yield from self.Wait.make(cmdobj=self)

topotato/base.py

@@ -196,32 +196,37 @@ class TopotatoItem(nodes.Item):
     this test item is skipped.
     """
 
-    # pylint: disable=protected-access
-    @classmethod
-    def from_parent(
-        cls: Type["TopotatoItem"], parent: nodes.Node, *args, **kw
-    ) -> "TopotatoItem":
-        """
-        pytest's replacement for the constructor.  Supposedly less fragile.
-
-        Do not call this directly, use :py:meth:`make`.
-        """
-
-        name = kw.pop("name")
+    posargs: ClassVar[List[str]] = []
 
+    def __init__(self, *, name: str, parent: nodes.Node, codeloc=None, **kw):
         nodeid = None
         child_sep = getattr(parent, "nodeid_children_sep", None)
         if child_sep:
             nodeid = parent.nodeid + child_sep + name
-        self: TopotatoItem = cast(
-            "TopotatoItem", super().from_parent(parent, name=name, nodeid=nodeid, **kw)
-        )
+
+        super().__init__(parent=parent, name=name, nodeid=nodeid, **kw)
+        self.skipchecks = []
+        self._codeloc = codeloc
 
         tparent = self.getparent(TopotatoClass)
         assert tparent is not None
-
         self._obj = tparent.obj
-        return self
+
+    @classmethod
+    def from_parent(
+        cls: Type["TopotatoItem"], parent: nodes.Node, *args, **kw
+    ) -> "TopotatoItem":
+        if len(args) > len(cls.posargs):
+            raise TopotatoUnhandledArgs(f"too many positional args for {cls.__name__}")
+
+        for i, arg in enumerate(args):
+            if cls.posargs[i] in kw:
+                raise TopotatoUnhandledArgs(
+                    f"duplicate argument {cls.posargs[i]!r} for {cls.__name__}"
+                )
+            kw[cls.posargs[i]] = arg
+
+        return super().from_parent(parent=parent, **kw)
 
     @classmethod
     @GeneratorWrapper.apply
@@ -269,9 +274,9 @@ def _make(
         cls: Type["TopotatoItem"], namesuffix, codeloc, *args, **kwargs
     ) -> Generator[Optional["TopotatoItem"], Tuple["TopotatoClass", str], ItemGroup]:
         parent, _ = yield None
-        self = cls.from_parent(parent, namesuffix, *args, **kwargs)
-        self.skipchecks = []
-        self._codeloc = codeloc
+        self = cls.from_parent(
+            parent, name=namesuffix, codeloc=codeloc, *args, **kwargs
+        )
         yield self
 
         return ItemGroup([self])
@@ -285,6 +290,7 @@ def pytest_pycollect_makeitem(collector, name, obj):
         pytest hook-in that makes all the other topotato objects appear.
         """
         if hasattr(obj, "_topotato_makeitem"):
+            # pylint: disable=protected-access
             if inspect.ismethod(obj._topotato_makeitem):
                 _logger.debug("_topotato_makeitem(%r, %r, %r)", collector, name, obj)
                 return obj._topotato_makeitem(collector, name, obj)
@@ -462,11 +468,8 @@ class InstanceStartup(TopotatoItem):
 
     commands: OrderedDict
 
-    # pylint: disable=arguments-differ
-    @classmethod
-    def from_parent(cls, parent):
-        self = super().from_parent(parent, name="startup")
-        return self
+    def __init__(self, **kwargs):
+        super().__init__(name="startup", **kwargs)
 
     def reportinfo(self):
         tcls = self.getparent(TopotatoClass)
@@ -510,11 +513,8 @@ class InstanceShutdown(TopotatoItem):
     orderly fashion (otherwise you get truncated pcap files.)
     """
 
-    # pylint: disable=arguments-differ
-    @classmethod
-    def from_parent(cls, parent):
-        self = super().from_parent(parent, name="shutdown")
-        return self
+    def __init__(self, **kwargs):
+        super().__init__(name="shutdown", **kwargs)
 
     def reportinfo(self):
         tcls = self.getparent(TopotatoClass)