Re: [PATCH 6/6] dts: add statefulness to TestPmdShell

[Luca Vizzarro <Luca.Vizzarro@arm.com>]

On 10/04/2024 08:41, Juraj Linkeš wrote:
>> <snip>
>>> @@ -723,7 +731,13 @@ def _start_application(self, get_privileged_command: Callable[[str], str] | None
>>>           if self._app_args.app_params is None:
>>>               self._app_args.app_params = TestPmdParameters()
>>>
>>> -        self.number_of_ports = len(self._app_args.ports) if self._app_args.ports is not None else 0
>>> +        assert isinstance(self._app_args.app_params, TestPmdParameters)
>>> +
>>
>> This is tricky because ideally we wouldn't have the assertion here,
>> but I understand why it is needed because Eal parameters have app args
>> which can be any instance of params. I'm not sure of the best way to
>> solve this, because making testpmd parameters extend from eal would
>> break the general scheme that you have in place, and having an
>> extension of EalParameters that enforces this app_args is
>> TestPmdParameters would solve the issues, but might be a little
>> clunky. Is there a way we can use a generic to get python to just
>> understand that, in this case, this will always be TestPmdParameters?
>> If not I might prefer making a private class where this is
>> TestPmdParameters, just because there aren't really any other
>> assertions that we use elsewhere and an unexpected exception from this
>> (even though I don't think that can happen) could cause people some
>> issues.
>>
>> It might be the case that an assertion is the easiest way to deal with
>> it though, what do you think?
>>
> 
> We could change the signature (just the type of app_args) of the init
> method - I think we should be able to create a type that's
> EalParameters with .app_params being TestPmdParameters or None. The
> init method would just call super().
> 
> Something like the above is basically necessary with inheritance where
> subclasses are all extensions (not just implementations) of the
> superclass (having differences in API).
> 

I believe this is indeed a tricky one. But, unfortunately, I am not 
understanding the solution that is being proposed. To me, it just feels 
like using a generic factory like:

   self.sut_node.create_interactive_shell(..)

is one of the reasons to bring in the majority of these complexities.

What do you mean by creating this new type that combines EalParams and 
TestPmdParams?