Re: [RFC PATCH v2] dts: skip test cases based on capabilities

["Juraj Linkeš" <juraj.linkes@pantheon.tech>]

On 22. 5. 2024 16:58, Luca Vizzarro wrote:
> Hi Juraj,
> 
> Here's my review. Excuse me for the unordinary format, but I thought
> it would have just been easier to convey my suggestions through code.
> Apart from the smaller suggestions, the most important one I think is
> that we should make sure to enforce type checking (and hinting).
> Overall I like your approach, but I think it'd be better to initialise
> all the required variables per test case, so we can access them
> directly without doing checks everytime. The easiest approach I can see
> to do this though, is to decorate all the test cases, for example
> through @test. It'd be a somewhat important change as it changes the
> test writing API, but it brings some improvements while making the
> system more resilient.
> 

The format is a bit wonky, but I was able to figure it out. I answered 
some of the questions I found.

I like the idea of decorating the test cases. I was thinking of 
something similar in the past and now it makes sense to implement it. 
I'll definitely use some (or all :-)) of the suggestions.

> The comments in the code are part of the review and may refer to
> either your code or mine. The diff is in working order, so you could
> test the functionality if you wished.
> 
> Best regards,
> Luca
> 
> ---
> diff --git a/dts/framework/remote_session/__init__.py 
> b/dts/framework/remote_session/__init__.py
> index f18a9f2259..d4dfed3a58 100644
> --- a/dts/framework/remote_session/__init__.py
> +++ b/dts/framework/remote_session/__init__.py
> @@ -22,6 +22,9 @@
>   from .python_shell import PythonShell
>   from .remote_session import CommandResult, RemoteSession
>   from .ssh_session import SSHSession
> +
> +# in my testpmd params series these imports are removed as they promote 
> eager module loading,
> +# significantly increasing the chances of circular dependencies
>   from .testpmd_shell import NicCapability, TestPmdShell
> 
> 
> diff --git a/dts/framework/remote_session/testpmd_shell.py 
> b/dts/framework/remote_session/testpmd_shell.py
> index f6783af621..2b87e2e5c8 100644
> --- a/dts/framework/remote_session/testpmd_shell.py
> +++ b/dts/framework/remote_session/testpmd_shell.py
> @@ -16,7 +16,6 @@
>   """
> 
>   import time
> -from collections.abc import MutableSet
>   from enum import Enum, auto
>   from functools import partial
>   from pathlib import PurePath
> @@ -232,9 +231,8 @@ def close(self) -> None:
>           self.send_command("quit", "")
>           return super().close()
> 
> -    def get_capas_rxq(
> -        self, supported_capabilities: MutableSet, 
> unsupported_capabilities: MutableSet
> -    ) -> None:
> +    # the built-in `set` is a mutable set. Is there an advantage to 
> using MutableSet?

 From what I can tell, it's best practice to be as broad as possible 
with input types. set is just one class, MutableSet could be any class 
that's a mutable set.

> +    def get_capas_rxq(self, supported_capabilities: set, 
> unsupported_capabilities: set) -> None:
>           """Get all rxq capabilities and divide them into supported and 
> unsupported.
> 
>           Args:
> @@ -243,6 +241,7 @@ def get_capas_rxq(
>                   not supported will be stored.
>           """
>           self._logger.debug("Getting rxq capabilities.")
> +        # this assumes that the used ports are all the same. Could this 
> be of concern?

Our current assumption is one NIC per one execution, so this should be 
safe, at least for now.

>           command = "show rxq info 0 0"
>           rxq_info = self.send_command(command)
>           for line in rxq_info.split("\n"):
> @@ -270,4 +269,6 @@ class NicCapability(Enum):
>       `unsupported_capabilities` based on their support.
>       """
> 
> +    # partial is just a high-order function that pre-fills the 
> arguments... but we have no arguments
> +    # here? Was this intentional?

It's necessary because of the interaction between Enums and functions. 
Without partial, accessing NicCapability.scattered_rx returns the 
function instead of the enum.

>       scattered_rx = partial(TestPmdShell.get_capas_rxq)