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

[Luca Vizzarro]

On 11/04/2024 11:30, Juraj Linkeš wrote:
> I've been thinking about these interactive shell constructors for some
> time and I think the factory pattern is not well suitable for this.
> Factories work well with classes with the same API (i.e.
> implementations of abstract classes that don't add anything extra),
> but are much less useful when dealing with classes with different
> behaviors, such as the interactive shells. We see this here, different
> apps are going to require different args and that alone kinda breaks
> the factory pattern. I think we'll need to either ditch these
> factories and instead just have methods that return the proper shell
> (and the methods would only exist in classes where they belong, e.g.
> testpmd only makes sense on an SUT). Or we could overload each factory
> (the support has only been added in 3.11 with @typing.overload, but is
> also available in typing_extensions, so we would be able to use it
> with the extra dependency) where different signatures would return
> different objects. In both cases the caller won't have to import the
> class and the method signature is going to be clearer.
> 
> We have this pattern with sut/tg nodes. I decided to move away from
> the node factory because it didn't add much and in fact the code was
> only clunkier. The interactive shell is not quite the same, as the
> shells are not standalone in the same way the nodes are (the shells
> are tied to nodes). Let me know what you think about all this - both
> Luca and Jeremy.

When writing this series, I went down the path of creating a 
`create_testpmd_shell` method at some point as a solution to these 
problems. Realising after that it may be too big of a change, and 
possibly best left to a discussion exactly like this one.

Generics used at this level may be a bit too much, especially for 
Python, as support is not *that* great. I am of the opinion that having 
a dedicated wrapper is easier for the developer and the user. Generics 
are not needed to this level anyways, as we have a limited selection of 
shells that are actually going to be used.

We can also swap the wrapping process to simplify things, instead of:

   shell = self.sut_node.create_interactive_shell(TestPmdShell, ..)

do:

   shell = TestPmdShell(self.sut_node, ..)

Let the Shell class ingest the node, and not the other way round.

The current approach appears to me to be top-down instead of bottom-up. 
We take the most abstracted part and we work our way down. But all we 
want is concreteness to the end user (developer).

> Let me illustrate this on the TestPmdShell __init__() method I had in mind:
> 
> def __init__(self, interactive_session: SSHClient,
>          logger: DTSLogger,
>          get_privileged_command: Callable[[str], str] | None,
>          app_args: EalTestPmdParams | None = None,
>          timeout: float = SETTINGS.timeout,
>      ) -> None:
>      super().__init__(interactive_session, logger, get_privileged_command)
>      self.state = TestPmdState()
> 
> Where EalTestPmdParams would be something that enforces that
> app_args.app_params is of the TestPmdParameters type.
> 
> But thinking more about this, we're probably better off switching the
> params composition. Instead of TestPmdParameters being part of
> EalParameters, we do it the other way around. This way the type of
> app_args could just be TestPmdParameters and the types should work.
> Or we pass the args separately, but that would likely require ditching
> the factories and replacing them with methods (or overloading them).
> 
> And hopefully the imports won't be impossible to solve. :-)

It is what I feared, and I think it may become even more convoluted. As 
you said, ditching the factories will simplify things and make it more 
straightforward. So, we wouldn't find ourselves in problems like these.

I don't have a strong preference in approach between:
* overloading node methods
* dedicated node methods
* let the shells ingest nodes instead

But if I were to give priority, I'd take it from last to first. Letting 
shells ingest nodes will decouple the situation adding an extra step of 
simplification. I may not see the full picture though. The two are 
reasonable but, having a dedicated node method will stop the requirement 
to import the shell we need, and it's pretty much equivalent... but 
overloading also is very new to Python, so I may prefer to stick to more 
established.

Letting TestPmdParams take EalParams, instead of the other way around, 
would naturally follow the bottom-up approach too. Allowing Params to 
arbitrarily append string arguments – as proposed, would also allow 
users to use a plain (EalParams + string). So sounds like a good 
approach overall.