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)
